1315383 - [css-grid] Restrict ratio-preserving stretching to 'normal' only

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

follow-up from bug 1218178 comment 20 (last paragraph)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid][css-align] Make 'normal' preserve the intrinsic ratio, but not 'stretch'.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] 'stretch' in either axis loses the intrinsic ratio when applying the min/max-size.

Here's another bug that I found after writing more exhaustive tests.
ComputeAutoSizeWithIntrinsicDimensions tries to preserve the ratio of
the passed in tentISize/tentBSize, but if we stretched that with 'stretch'
the ratio is likely incorrect now.  (It was somewhat documented in
the else-clause here, but I missed that, so I'm moving that up before
the call to make it more prominent.)

We could perhaps preserve the ratio for 'stretch' / 'normal' and
'normal' / 'stretch' in some cases (depending on the exact min/max-width/height
values), but it seems simpler to just clamp all 'stretch' combinations without
preserving the ratio.  Otherwise, we'd probably have to extend the "Constraint
Violations" table at the end of §10.4:
https://www.w3.org/TR/CSS22/visudet.html#propdef-max-width
with *a lot* more combinations, which doesn't seem worth it.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] More reftests for align|justify-self:normal|stretch on intrinsic ratio items, with and without Automatic Minimum Size clamping.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8808444 [details] [diff] [review]
part 1 - [css-grid][css-align] Make 'normal' preserve the intrinsic ratio, but not 'stretch'.

Review of attachment 8808444 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with one nit, and some food-for-thought for a possible followup.

::: layout/generic/nsFrame.cpp
@@ +4969,5 @@
>      aMargin.ISize(aWM) + aBorder.ISize(aWM) + aPadding.ISize(aWM) -
>        boxSizingAdjust.ISize(aWM);
>  
>    nscoord iSize, minISize, maxISize, bSize, minBSize, maxBSize;
> +  // true if we have a Grid item with 'stretch' in the inline dimension

Perhaps s/have/are/ for all the comments here.

("if we have a grid item" sounds a lot like "if this frame contains a grid item [as a child]" -- but that's not the meaning you're going for here.)

@@ +5000,2 @@
>            (aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize)) {
>          iSize = std::max(nscoord(0), cbSize -

Observation: as in bug 1316029, I think this would be much more understandable if we adjusted this code inside of the "if (normalI || stretchI || ...)" check to instead set a variable named "iSizeToFillCB" (instead of just iSize), which we can later use to set (or influence) the main "iSize" variable.

That refactoring/renaming (if it happens) should probably happen separately; I'm just noting it here since this patch adds a bunch more usages of this variable "iSize", some of which explicitly want iSizeToFillCB (e.g. for the "stretch / normal" case) and others of which might want a semantically-different non-stretched iSize (i.e. whatever we've resolved iSize to be in a non-stretched scenario).

The lineage of "iSize" is hard to follow through this function, so it'd be nice if code further down that definitely-wants-the-CB-stretched-size to be able to just use a variable called iSizeToFillCB.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8808445 [details] [diff] [review]
part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

Review of attachment 8808445 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure I understand the logic changes here. Could you clarify the code-comments a bit, to explain what we're aiming to do & why?

::: layout/generic/nsFrame.cpp
@@ +5155,5 @@
>        } else {
>          tentISize = nsPresContext::CSSPixelsToAppUnits(300);
>        }
>  
> +      // Clamp the intrinsic inline size, but it's not needed for 'stretch'.

This comment could stand to be a bit clearer. (In part because this check doesn't guard any actual clamping -- just some boolean assignments for "stretchI" and "normalI".)

Maybe:
  // If we're calculating the min-ISize and clamping it to fit the CB,
  // we can share the "stretch" codepath, or the "normal" codepath if
  // [...explanation for why we make a distinction... here...]

@@ +5174,5 @@
>        } else {
>          tentBSize = nsPresContext::CSSPixelsToAppUnits(150);
>        }
>  
> +      // Clamp the intrinsic block size, but it's not needed for 'stretch'.

This needs a similar explanation to above, or a short reference to the above explanation.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

(update the code comments)

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8808447 [details] [diff] [review]
part 3 - [css-grid] 'stretch' in either axis loses the intrinsic ratio when applying the min/max-size.

Review of attachment 8808447 [details] [diff] [review]:
-----------------------------------------------------------------

Commit message nit:
> Bug 1315383 part 3 - [css-grid] 'stretch' in either axis loses the intrinsic ratio when applying the min/max-size.  r=dholbert

This needs a minor rephrasing -- right now it sounds too much like it might be a statement of a problem that this patch is fixing.

Please reword to make it clearer that you're describing the changed/expected behavior. Perhaps:
 "Don't try to preserve intrinsic ratio when applying min/max-size, if either axis has 'stretch' alignment".

::: layout/generic/nsFrame.cpp
@@ +5235,5 @@
>          // conversion here, but just assign the fields directly to our result.
>          iSize = autoSize.width;
>          bSize = autoSize.height;
>        } else {
> +        // No intrinsic ratio, so just clamp the dimensions independently.

"No intrinsic ratio" is misleading, because the frame might very well *have* an intrinsic ratio.  The point is rather that we're not *honoring* the intrinsic ratio here.

So, maybe replace this comment with:
        // Not honoring an intrinsic ratio: clamp the dimensions independently.

(I think that should still fit)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8808888 [details] [diff] [review]
part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

Review of attachment 8808888 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +5156,5 @@
>          tentISize = nsPresContext::CSSPixelsToAppUnits(300);
>        }
>  
> +      // If we need to clamp the inline size to fit the CB, we use the 'stretch'
> +      // or 'normal' codepath.  We use the ratio-preserving 'normal' codepath

Sorry, I'm still confused about this. It seems odd that we'd entirely share the "stretchI" codepath for this scenario.

In particular, it seems to me like:
 - eIClampMarginBoxMinSize is intending to *reduce* the value that we're resolving (down to the CB size, potentially)
 - stretchI is intending to *increase* the value that we're resolving (up to the CB size)

Both have to do with the CB size, but it seems like they make us take action in opposite scenarios.  (One to clamp, one to grow.)

Can you help me understand this a bit more?

(Possible explanation: if eIClampMarginBoxMinSize is set, then perhaps (??) it means we definitely have a min-isize value that's greater than the CB size -- and if that's the case, then we definitely need to stretch our isize up to at least the CB size, to honor our [clamped] min-isize property.  This makes some sense to me as a possible-explanation, though I'm not sure about the "perhaps (??)" part -- hence, I'm just throwing it out there as one possible way I can see this making sense -- maybe it makes sense for a completely different reason. :))

Comment 10

[Mats Palmgren (inactive)]

> eIClampMarginBoxMinSize is intending to *reduce* the value

Yes.  The "tentISize > iSize" check ensures that we only reduce the size
due to clamping.  (to the extent possible; there might be conflicting goals,
like 'stretch' in the other axis)

> stretchI is intending to *increase* the value

Not exactly, stretchI==true just means we have *-self:'stretch', which
resizes (reduce *or* increase the size) the item to fill the CB. (and 'normal'
also reduce/increase the size, the only difference is it's ratio-preserving).
https://drafts.csswg.org/css-align-3/#valdef-justify-self-stretch
"Note: The 'stretch' keyword can cause elements to shrink, to fit their container"

Comment 11

[Mats Palmgren (inactive)]

Sorry, I meant "unless we have 'stretch' in the other axis".  That is, if we
have 'stretch' in the other axis and our size needs to be clamp in this axis
then we prefer 'stretch' / 'stretch' beahvior (i.e. we ignore ratio).
Otherwise (not 'stretch' in other axis), then we prefer to clamp using 'normal'
in this axis to (try to) clamp with preserved ratio.  Does that makes sense?

Comment 12

[Daniel Holbert [:dholbert]]

I think so, yeah. One followup question, which I'll post as another patch-review-comment:

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8808888 [details] [diff] [review]
part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

Review of attachment 8808888 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +5162,4 @@
>        if ((aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize) &&
> +          !stretchI && tentISize > iSize) {
> +        if (stretchB) {
> +          stretchI = true;

If this frame happens to have "justify-self: normal", it seems like this codepath will end up giving us *both* normalI = true (from the earlier explicit justify-self check in Part 1) and stretchI = true (from this line here).

Does this combination of booleans (both stretchI and normalI being true) make any sense?  It doesn't seem like a sensible state to be in, based on the documentation for those booleans. Does the code handle that correctly?  Or, is this state impossible, due to something I'm not seeing? [if it's impossible, it might be worth asserting somewhere that stretchI and normalI are mutually exclusive...]

(Perhaps we should really collapse Part 1's bools into a 3-possible-values enum (with 1 instance of the enum per axis), with values "eNoStretch", "eStretch", and "eStretchPreservingRatio"?)

Comment 14

[Mats Palmgren (inactive)]

> Does this combination of booleans (both stretchI and normalI being true) make any sense?

Not really.  But I think we do the right thing in the code that follows
because we handle stretch before normal.

Yeah, an enum is probably clearer.  I'll try that...

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid][css-align] Make 'normal' preserve the intrinsic ratio, but not 'stretch'.

Slightly less readable in the sense that it's more wordy, IMO.
But it's semantically clearer, so probably a win overall.

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Don't try to preserve intrinsic ratio when applying min/max-size, if either axis has 'stretch' alignment.

Comment 18

[Mats Palmgren (inactive)]

(BTW, don't worry about the line length.  I'll file a cleanup bug about replacing
"logicalRatio.ISize(aWM)" with a "ratioI" variable etc.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8809208 [details] [diff] [review]
part 1 - [css-grid][css-align] Make 'normal' preserve the intrinsic ratio, but not 'stretch'.

Review of attachment 8809208 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +4969,5 @@
>      aMargin.ISize(aWM) + aBorder.ISize(aWM) + aPadding.ISize(aWM) -
>        boxSizingAdjust.ISize(aWM);
>  
>    nscoord iSize, minISize, maxISize, bSize, minBSize, maxBSize;
> +  enum Stretch {

"enum class", probably?

Otherwise I suspect you can treat this as a boolean, without the compiler complaining at all -- and that's a *very* easy mistake to make.  e.g.  "if (stretchI)" superficially looks OK to a human, but is not what it seems -- and we should make this mistake impossible, since we can.

(This will require the code to be a bit more verbose, due to enum-class namespacing.  Hopefully not too bad, though -- might just require adding a linebreak between pieces of some multi-component if-checks.)

@@ +4972,5 @@
>    nscoord iSize, minISize, maxISize, bSize, minBSize, maxBSize;
> +  enum Stretch {
> +    // we have justify|align-self:normal in the relevant axis
> +    eStretchPreservingRatio,
> +    // we have justify|align-self:stretch in the relevant axis

This document isn't strictly true -- you should probably add "(or equivalent)" to both lines of documentation here, i.e.
    // we have justify|align-self:normal (or equivalent) in the relevant axis
    // we have justify|align-self:stretch (or equivalent) in the relevant axis
...since IIUC you're planning on applying this enum value in some cases where we don't actually have that style applied. (like the code quoted in comment 13)

(Or make some similar generifying tweak, e.g. "// We should stretch to fill the CB in the relevant axis")

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c299f308a1d
part 1 - [css-grid][css-align] Make 'normal' preserve the intrinsic ratio, but not 'stretch'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/54aa92ad290b
part 2 - [css-grid] Make Automatic Minimum Size clamping preserve the intrinsic ratio, unless either axis has 'stretch'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffc5238b6a0
part 3 - [css-grid] Don't try to preserve intrinsic ratio when applying min/max-size, if either axis has 'stretch' alignment.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8718ab22261d
part 4 - [css-grid] More reftests for align|justify-self:normal|stretch on intrinsic ratio items, with and without Automatic Minimum Size clamping.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c299f308a1d
https://hg.mozilla.org/mozilla-central/rev/54aa92ad290b
https://hg.mozilla.org/mozilla-central/rev/cffc5238b6a0
https://hg.mozilla.org/mozilla-central/rev/8718ab22261d