1218178 - [css-grid] Implement the "transferred size" piece for "Implied Minimum Size of Grid Items"

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Mats Palmgren (inactive)]

Follow-up from bug 1176775

Comment 1

[Mats Palmgren (inactive)]

Attachment: Testcase #1: track auto min-sizing with min-width|height:auto

Comment 2

[Mats Palmgren (inactive)]

Attachment: Screenshot of Testcase #1

Just to double check I understood the spec(s) correctly,
does this look right to you?

Comment 3

[Daniel Holbert [:dholbert]]

I think the grid-item sizes there all look correct (though the rules of influence here can be deceptively complex, so I might be missing something).

(BTW, I focused on the grid *item* sizes, and didn't really think about the grid containers' sizes in the testcase-screenshot.  I recall less about how grid container intrinsic-sizing works, and I'm not sure that's what you're focusing on here. Let me know if you think that needs some sanity-checking as well, though.)

Comment 4

[Mats Palmgren (inactive)]

Sorry, I should have been more clear.  The top part tests the resulting column size when
there's a single item (image) with 'min-width:auto' and the other styles you see in the test.
The width of the grid container (black border) wraps that column's size, so the width
of that box is what's interesting.  (You can ignore the vertical dimension here.)

The bottom part tests the resulting row height when there's an image with 'min-height:auto'
and the other styles you see in the test.  It's the height of the container that's
interesting here.  (You can ignore the horizontal dimension here.)

Comment 5

[Daniel Holbert [:dholbert]]

RE the width of the container being "interesting" for the top part -- by that, you mean that the container's width indicates what we think the grid item's "min-width" is, correct?  (I initially glossed over the "grid-template-columns:minmax(auto,0)", having forgotten the exact meaning of the syntax, but on re-examination I think that this is what you're trying to get at with that CSS?)

(and similar for the bottom part)

Comment 6

[Mats Palmgren (inactive)]

Exactly.  It's the item's "Implied Minimum Size" per 4.4 that decides the column width (top)
and row height (bottom).

Comment 7

[Daniel Holbert [:dholbert]]

Great, thanks.

There are two more values that I think you should be testing here, in "coltest" (the first part) (and probably similar in the second part, too)
 (1) "min-height: 4px"
 (2) "min-height: 4px; max-width: 30px"

As described in the "content size" definition on section 4.4, the "min-height" value should be converted through the aspect-ratio (producing 48px) & should provide a lower bound for the "content size". So, I'd expect case (1) to produce a grid that is 48px wide.

The content size is *further* clamped by the "max" value in the same dimension (if definite), and case (2) should test that we're doing that -- I'd expect that one to produce a 30px-wide grid.

Comment 8

[Daniel Holbert [:dholbert]]

(I guess comment 7 is technically not about the "transferred size" -- it's about a similar piece of the "content size".  If you'd like to handle that piece in a followup, feel free; but it might make sense to lump it in here.)

Comment 9

[Daniel Holbert [:dholbert]]

Renderings still look good to me, looking w/ fresh eyes in light of comment 5 / comment 6.

(One possible test-bug that I noticed: the very last example in this testcase has "width:1px; min-height:30px" on its grid-item -- notice that it's explicitly setting "min-height".  It's in a section that's ostensibly testing "min-height:auto" resolution, but it does not have "min-height:auto".  Seems like that might be a mistake, but maybe you included it for completeness or as a bonus or something.)

Comment 10

[Mats Palmgren (inactive)]

It looks like we need to implement two separate behaviors here:
stretch-with-preserved-ratio for 'normal' (the default), and
and stretch-without-preserved-ratio for 'stretch'.
https://github.com/w3c/csswg-drafts/issues/523

The CSSWG resolution says:
Stretching image grid items in both dimensions
----------------------------------------------

  - RESOLVED: The normal value of align-self and justify-self
              preserves aspect of image but stretch causes it to
              stretch to fit containing block. Default value
              continues to be normal.

https://lists.w3.org/Archives/Public/www-style/2016Oct/0068.html

Comment 11

[Mats Palmgren (inactive)]

Attachment: part 1 - [css-grid] Implement the transferred size part of Automatic Minimum Size for grid items.

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 2 - Minor code cleanup (idempotent).

I originally had some changes in this method which I later reversed.
I did some code cleanup though which I think is worth keeping.
Regarding the removed "If we're a flex item..." comment: I think
it's not very informative and it should be fairly obvious from
the isGridItem/isFlexItem bools, and their use, that we're doing
things "a bit differently" for those.

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid][css-align] Add more reftests with image grid items.

Comment 15

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f3ffbd9fa09&exclusion_profile=false
(the linux64 orange is due to CI changes and looks unrelated to my changes)

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8803657 [details] [diff] [review]
part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio.

FYI, I've fixed an indentation issue in this patch locally:

       stretchI = inlineAxisAlignment == NS_STYLE_ALIGN_NORMAL ||
-                          inlineAxisAlignment == NS_STYLE_ALIGN_STRETCH;
+                 inlineAxisAlignment == NS_STYLE_ALIGN_STRETCH;

       stretchB = blockAxisAlignment == NS_STYLE_ALIGN_NORMAL ||
-                          blockAxisAlignment == NS_STYLE_ALIGN_STRETCH;
+                 blockAxisAlignment == NS_STYLE_ALIGN_STRETCH;

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio.

(Fixing an edge case bug I discovered while writing some reftests for <canvas> grid items.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8803655 [details] [diff] [review]
part 1 - [css-grid] Implement the transferred size part of Automatic Minimum Size for grid items.

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

::: layout/base/nsLayoutUtils.cpp
@@ +5053,5 @@
>          nscoord bSizeTakenByBoxSizing =
>            GetBSizeTakenByBoxSizing(boxSizing, aFrame, horizontalAxis,
>                                     aFlags & IGNORE_PADDING);
>  
> +        nscoord minContentSize = result;

Observation: this assignment/naming is a bit mysterious -- i.e. it's not clear why "result" actually contains the min-content size here. (It looks like it probably *is* the min-content size, but only because |result| hasn't been further-refined based on other constraints yet.)

Perhaps it'd be better to rewrite the code above this point to use "minContentSize" instead of "result", and then declare "nscoord result = minContentSize;" somewhere around here?  (That could happen in a followup patch/bug.)

@@ +5088,5 @@
> +        if (MOZ_UNLIKELY(aFlags & nsLayoutUtils::MIN_INTRINSIC_ISIZE)) {
> +          // This is the "transferred size" piece of:
> +          // https://www.w3.org/TR/css-flexbox-1/#min-width-automatic-minimum-size
> +          // https://drafts.csswg.org/css-grid/#min-size-auto
> +          result = std::min(result, minContentSize);

Please add something like this at the beginning of this comment:
  // This flag means we're resolving min-[width|height]: auto.
(Is that strictly true? Hopefully it is -- otherwise, this new code isn't quite right.)

That makes the following references to "transferred size" a bit clearer.  (I didn't initially recall that MIN_INTRINSIC_ISIZE was directly part of "min-{...}: auto" -- so this min-size-auto-specific transferred-size code initially seemed out of place to me.)

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8803656 [details] [diff] [review]
part 2 - Minor code cleanup (idempotent).

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

r=me

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8806213 [details] [diff] [review]
part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio.

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

r=me, though this might not be quite right yet. I'll let you decide whether to address the "normal" thing (my second point below) here or in a followup.

::: layout/base/nsLayoutUtils.cpp
@@ +5528,5 @@
> +        !aFrame->StyleMargin()->HasInlineAxisAuto(aWM)) {
> +      auto inlineAxisAlignment =
> +        aWM.IsOrthogonalTo(aFrame->GetParent()->GetWritingMode()) ?
> +          stylePos->UsedAlignSelf(aFrame->StyleContext()->GetParent()) :
> +          stylePos->UsedJustifySelf(aFrame->StyleContext()->GetParent());

I'm pretty sure you actually want aFrame->GetParent()->StyleContext() here.

(You want the parent frame's style context -- not the style context's parent.)

Otherwise you'll run into issues like the one I fixed in bug 1306213. If aFrame is a nsTableOuterFrame (for a table inside of a grid), then aFrame->GetParent()->StyleContext() would give you the nsTableFrame's style context! And that's not what you want here (for resolving "align-self:auto").  You want to be using the grid's style context (the parent frame's style context).

@@ +5690,5 @@
>        }
>  
>        if (aIntrinsicRatio != nsSize(0, 0)) {
> +        if (stretchI || stretchB) {
> +          // Stretch within the CB size with preserved intrinsic ratio.

Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values?

Right now, it looks like we'll enter this code even if we have "align-self:stretch; justify-self: stretch".  And I don't think we're supposed to...  This is supposed to be a special thing that only happens for the "normal" value, IIUC (and stretch is allowed to deform away from the aspect ratio).

Comment 21

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> > +        nscoord minContentSize = result;
> 
> Observation: this assignment/naming is a bit mysterious -- i.e. it's not
> clear why "result" actually contains the min-content size here. (It looks
> like it probably *is* the min-content size, but only because |result| hasn't
> been further-refined based on other constraints yet.)

Well, it is the min-content size at that point for the MIN_INTRINSIC_ISIZE
case, which is the only use.

> Perhaps it'd be better to rewrite the code above this point to use
> "minContentSize" instead of "result", and then declare "nscoord result =
> minContentSize;" somewhere around here?  (That could happen in a followup
> patch/bug.)

So this is probably not a good idea.

I don't know what else to call it though, let me know if you have a suggestion.

> > +        if (MOZ_UNLIKELY(aFlags & nsLayoutUtils::MIN_INTRINSIC_ISIZE)) {
> > +          // This is the "transferred size" piece of:
> > +          // https://www.w3.org/TR/css-flexbox-1/#min-width-automatic-minimum-size
> > +          // https://drafts.csswg.org/css-grid/#min-size-auto
> 
> Please add something like this at the beginning of this comment:
>   // This flag means we're resolving min-[width|height]: auto.
> (Is that strictly true? Hopefully it is -- otherwise, this new code isn't
> quite right.)

We pass MIN_INTRINSIC_ISIZE also for min-width/height:min-content etc:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/generic/nsGridContainerFrame.cpp#3833
I'll investigate if this is a problem and file a follow-up bug if needed...

Comment 22

[Mats Palmgren (inactive)]

> I'm pretty sure you actually want aFrame->GetParent()->StyleContext() here.

Good catch, I've fixed this locally, and also the same bug in the existing block-axis code!

> Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values?

Yeah, I think I misunderstood the CSSWG minutes the first time around but after reading it
again I think you're right that's their intention (no actual spec text for this yet though).

I'd prefer to fix this in a follow-up bug though, if you don't mind, because bug 1300369
stomps on this code a bit (for clamping) so I'd rather do the fix on top of that.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #21)
> Well, it is the min-content size at that point for the MIN_INTRINSIC_ISIZE
> case, which is the only use.

Yeah, but that only-use is much further down. So the declaration here (which lacks a MIN_INTRINSIC_ISIZE check) is still hand-wavy & mysterious.

Two suggestions:
 (1) Perhaps add a comment above its decl saying something like:
  // NOTE: This is only the minContentSize if we've been passed MIN_INTRINSIC_ISIZE
  // (which is fine, because this should only be used inside a check for that flag).

...and/or:
 (2) You could make this validity more explicit by only giving this variable a value *inside* of a a dedicated 1-liner MIN_INTRINSIC_ISIZE check.  Then, if we accidentally add bogus uses of this variable in the future (outside of a MIN_INTRINSIC_ISIZE check), the compiler and/or valgrind+ASAN will help us with uninitialized-usage warnings.  But maybe that's overkill.

(In reply to Mats Palmgren (:mats) from comment #22)
> > Shouldn't this be conditioned on whether we had "normal" as one (or both) of the alignment values?
> 
> Yeah [...] I'd prefer to fix this in a follow-up bug though, if you don't mind

Sure, sounds good.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28dad79c4333
part 1 - [css-grid] Implement the transferred size part of Automatic Minimum Size for grid items.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d6fd3a6aa8
part 2 - Minor code cleanup (idempotent).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9d3d21f20d
part 3 - [css-grid][css-align] Implement ratio-preserving 'stretch' alignment for grid items with an intrinsic ratio.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c652ff1d1e
part 4 - [css-grid][css-align] Add more reftests with image grid items.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/28dad79c4333
https://hg.mozilla.org/mozilla-central/rev/60d6fd3a6aa8
https://hg.mozilla.org/mozilla-central/rev/4e9d3d21f20d
https://hg.mozilla.org/mozilla-central/rev/30c652ff1d1e