min/max-width/height, replaced elements #25207

SimonSapin · 2019-12-08T00:26:56Z

No description provided.

highfive · 2019-12-08T00:27:01Z

Heads up! This PR modifies the following files:

@jgraham: tests/wpt/metadata-layout-2020/css/CSS2/box-display/containing-block-008.xht.ini, tests/wpt/metadata-layout-2020/css/CSS2/box-display/display-change-001.xht.ini, tests/wpt/metadata-layout-2020/css/CSS2/box-display/containing-block-009.xht.ini, tests/wpt/metadata-layout-2020/css/CSS2/box-display/containing-block-028.xht.ini, tests/wpt/metadata-layout-2020/css/CSS2/box-display/containing-block-027.xht.ini and 1 more
@emilio: components/style/values/computed/length.rs, components/style/values/generics/length.rs

nox

Lots of nits but otherwise nothing to say.

nox · 2019-12-09T11:31:42Z

components/style/values/generics/length.rs

+
+    /// Convert
+    #[cfg(not(feature = "gecko"))]
+    pub fn to_option(self) -> Option<LengthPercentage> {


Nit: Just name that length, like Result::ok?

This is subtle but I mean this method not as "if this enum is the LengthPercentage variant, access its field" but rather "losslessly convert all the information in this enum". If/when we add support for e.g. min-width: min-content https://drafts.csswg.org/css-sizing/#min-size-properties this enum will have additional variants like it does today for Gecko, this method will need to be removed, and the callers should be modified to handle those variants separately.

nox · 2019-12-09T11:31:42Z

components/layout_2020/sizing.rs

+            .to_option()
+            .and_then(|lp| lp.as_length());
+        let clamp = |l: Length| l.clamp_between_extremums(min_inline_size, max_inline_size);
+
        // Percentages for 'width' are treated as 'auto'
        let inline_size = inline_size.map(|lp| lp.as_length());
        // The (inner) min/max-content are only used for 'auto'
        let mut outer = match inline_size.non_auto().flatten() {


Can't you use and_then to define inline_size and then remove the flatten call?

The call two lines above is LengthPercentageOrAuto::map, not Option::map. Maybe we should have LengthPercentageOrAuto::and_then that return Auto when its closure returns Option::None?

nox · 2019-12-09T11:31:42Z

components/layout_2020/dom_traversal.rs

@@ -299,7 +298,7 @@ impl Drop for BoxSlot<'_> {
 pub(crate) trait NodeExt<'dom>: 'dom + Copy + LayoutNode + Send + Sync {
    fn is_element(self) -> bool;
    fn as_text(self) -> Option<String>;
-    fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<Length>)>;
+    fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<f64>)>;


Why the type change, and why not CSSFloat?

It’s not Length because it is measured in image pixels rather than CSS px.

I wanted to make it u32 but it’s already divided by script::dom::htmlimageelement::ImageRequest::current_pixel_density. I’m not sure what that density means exactly or if diving there is correct.

It could be CSSFloat. It probably wouldn’t affect much since it’s not kept in memory for a long time.

nox · 2019-12-09T11:31:42Z

components/layout_2020/replaced.rs

+        let intrinsic_size = physical::Vec2 {
+            x: self.intrinsic_width,
+            y: self.intrinsic_height,
+        };


Why not store the intrinsic size as a physical::Vec2<Option<_>>?

Could be either. An earlier version of the comment on the field declaration talked about "all three components"

nox · 2019-12-09T11:31:42Z

components/layout_2020/replaced.rs

+        let clamp = |inline_size: Length, block_size: Length| Vec2 {
+            inline: inline_size.clamp_between_extremums(min_box_size.inline, max_box_size.inline),
+            block: block_size.clamp_between_extremums(min_box_size.block, max_box_size.block),


Wondering if we should have a meethod for that, don't we do that for non-replaced things elsewhere in the code?

I grepped through the code and didn’t find another instance of computing both at the same time.

In block layout for example, we compute the used width before recurring and the used height after.

nox · 2019-12-09T11:31:42Z

components/layout_2020/replaced.rs

            (LengthOrAuto::LengthPercentage(inline), LengthOrAuto::LengthPercentage(block)) => {
                clamp(inline, block)
            },
            (LengthOrAuto::LengthPercentage(inline), LengthOrAuto::Auto) => {
-                clamp(inline, inline / intrinsic_ratio)
+                let block = if let Some(i_over_b) = intrinsic_ratio {


This name confuses me, please just name it ratio.

The problem these new names try to address is that when A divided by B is called a ratio, B divided by A is also called a ratio although they’re not interchangeable. Code that uses that ratio sometimes to multiple and sometimes to divide a length, having a variable name that indicates the “direction” of the ratio helps check if those are correct by doing pseudo-dimensional analysis.

We could declare that an aspect ratio is by convention always a width divided by a height. But this code work in flow-relative dimensions and accesses a ratio field in physical dimensions. I’ve added inline_size_over_block_size_intrinsic_ratio to make that conversion.

I’m open to other names, but I’d prefer if they indicate the “direction” of the ratio somehow. Or maybe we could provide that indication in doc-comments and code comments?

Fair enough, I don't mind the name.

nox · 2019-12-09T11:31:42Z

components/layout_2020/replaced.rs

+        &self,
+        style: &ComputedValues,
+    ) -> Option<CSSFloat> {
+        self.intrinsic_ratio.map(|width_over_height| {


Sorry, but again, I find the code more readable when that is just named ratio.

nox · 2019-12-09T11:31:42Z

components/layout_2020/positioned.rs

+            Ok(replaced) => {
+                // https://drafts.csswg.org/css2/visudet.html#abs-replaced-width
+                // https://drafts.csswg.org/css2/visudet.html#abs-replaced-height
+                let u = replaced.used_size_as_if_inline_element(&containing_block.into(), style);


Not sur I like u.

nox · 2019-12-09T11:31:42Z

components/layout_2020/positioned.rs

+                    let margin_end;
+                    let used_size;
+                    if let LengthOrAuto::LengthPercentage(s) = size {
+                        used_size = s;


Name size was just fine, why shorten it?

nox · 2019-12-09T11:31:42Z

components/layout_2020/positioned.rs

@@ -208,7 +208,7 @@ impl<'a> AbsolutelyPositionedFragment<'a> {
            padding_border_sum: Length,
            computed_margin_start: LengthOrAuto,
            computed_margin_end: LengthOrAuto,
-            solve_margins: impl FnOnce(Length) -> (Length, Length),
+            avoid_negative_margin_start: bool,


Please remove this change or introduce yet another new type for this boolean.

Even with exactly two call sites, that both have a comment to repeat the name of the parameter?

IIRC the collapsing margin stuff doesn't have more callers than that, and the comment can go if you introduce a type, but as I said on IRC, all my nits are optional.

SimonSapin · 2019-12-09T16:56:18Z

This conflicts with #25203, I’ll push a rebase + changes after that one lands.


        Move `clamp_*` functions to methods of `Length`


        Replace `percent_resolved_*` functions with methods


        Account for min/max-width in outer intrinsic sizing


        Add min/max-width/height support for `inline-block`

(… and other non-replaced atomic inline-level boxes.)


        Move replaced box used size computation to a method of `ReplacedConte…

…nts`


        Add sizing of inline replaced boxes


        Don’t assume replaced elements have an intrinsic size


        Fix min/max-content of replaced boxes


        Remove inline/block_size from AbsolutelyPositionedFragment


        Remove AbsoluteBoxOffsets’s type parameter


        Rename ReplacedContent::used_size to used_size_as_if_inline_element


        impl From<&'_ DefiniteContainingBlock> for ContainingBlock


        Move two AbsoluteBoxOffsets fields into a Vec2


        Implement replaced abspos


        Delayed initialization over mutation


        Conditionsals over closures


        Move `solve_axis` function to module level


        Struct with named fields over large tuple


        Fix a “Accessing content size that was not requested” panic

Percentage `width` are treated as `auto` for the purpose of min/max-content computation, so they also need to be considered when testing “wether width is auto”

SimonSapin · 2019-12-10T14:20:32Z

@bors-servo r=nox

bors-servo · 2019-12-10T14:20:33Z

📌 Commit 53ce714 has been approved by nox

bors-servo · 2019-12-10T15:49:19Z

⌛ Testing commit 53ce714 with merge 6cf618a...


        Auto merge of #25207 - servo:replaced, r=nox

min/max-width/height, replaced elements

bors-servo · 2019-12-10T16:55:33Z

☀️ Test successful - status-taskcluster
Approved by: nox
Pushing 6cf618a to master...

highfive assigned jdm Dec 8, 2019

SimonSapin requested a review from nox Dec 8, 2019

highfive added the S-awaiting-review label Dec 8, 2019

SimonSapin added this to In progress in Layout 2020 via automation Dec 8, 2019

SimonSapin added the A-layout/2020 label Dec 8, 2019

nox reviewed Dec 9, 2019

View changes

jdm assigned nox and unassigned jdm Dec 9, 2019

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 9, 2019

SimonSapin added 18 commits Dec 7, 2019

Move `clamp_*` functions to methods of `Length`

a2c2b29

Replace `percent_resolved_*` functions with methods

ce7e84b

Account for min/max-width in outer intrinsic sizing

999dd72

Add min/max-width/height support for `inline-block`

bf96988

(… and other non-replaced atomic inline-level boxes.)

Move replaced box used size computation to a method of `ReplacedConte…

c40583b

…nts`

Add sizing of inline replaced boxes

b73eb49

Don’t assume replaced elements have an intrinsic size

8996be3

Fix min/max-content of replaced boxes

80b2b5f

Remove inline/block_size from AbsolutelyPositionedFragment

f43dc3a

Remove AbsoluteBoxOffsets’s type parameter

e86222d

Rename ReplacedContent::used_size to used_size_as_if_inline_element

14ddf39

impl From<&'_ DefiniteContainingBlock> for ContainingBlock

f09c14a

Move two AbsoluteBoxOffsets fields into a Vec2

1fcdde9

Implement replaced abspos

1fa20e9

Delayed initialization over mutation

c07c980

Conditionsals over closures

2906722

Move `solve_axis` function to module level

be8df1d

Struct with named fields over large tuple

a17db21

Fix a “Accessing content size that was not requested” panic

Loading status checks…

53ce714

Percentage `width` are treated as `auto` for the purpose of min/max-content computation, so they also need to be considered when testing “wether width is auto”

SimonSapin force-pushed the replaced branch from 8667cf9 to 53ce714 Dec 10, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 10, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 10, 2019

bors-servo merged commit 53ce714 into master Dec 10, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

Layout 2020 automation moved this from In progress to Merged / resolved Dec 10, 2019

bors-servo deleted the replaced branch Dec 10, 2019

highfive removed the S-awaiting-merge label Dec 10, 2019

bors-servo mentioned this pull request Dec 10, 2019

Fix `--layout-threads 1` in layout 2020 #25230

Merged

servo / servo

min/max-width/height, replaced elements #25207

min/max-width/height, replaced elements #25207

SimonSapin commented Dec 8, 2019

highfive commented Dec 8, 2019

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Dec 9, 2019

SimonSapin commented Dec 10, 2019

bors-servo commented Dec 10, 2019

bors-servo commented Dec 10, 2019

bors-servo commented Dec 10, 2019

servo / servo

Join GitHub today

min/max-width/height, replaced elements #25207

min/max-width/height, replaced elements #25207

Conversation

SimonSapin commented Dec 8, 2019

highfive commented Dec 8, 2019

nox left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SimonSapin commented Dec 9, 2019

SimonSapin commented Dec 10, 2019

bors-servo commented Dec 10, 2019

bors-servo commented Dec 10, 2019

bors-servo commented Dec 10, 2019