Closed Bug 1218178 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mats, Assigned: mats)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Follow-up from bug 1176775
Attached image Screenshot of Testcase #1 (obsolete) —
Just to double check I understood the spec(s) correctly,
does this look right to you?
Flags: needinfo?(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.)
Flags: needinfo?(dholbert)
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.)
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)
Exactly.  It's the item's "Implied Minimum Size" per 4.4 that decides the column width (top)
and row height (bottom).
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.
(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.)
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.)
Blocks: 1217086
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
Depends on: 1308929
Attachment #8679407 - Attachment is obsolete: true
Attachment #8679409 - Attachment is obsolete: true
Attachment #8803655 - Flags: review?(dholbert)
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.
Attachment #8803656 - Flags: review?(dholbert)
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 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;
(Fixing an edge case bug I discovered while writing some reftests for <canvas> grid items.)
Attachment #8803657 - Attachment is obsolete: true
Attachment #8803657 - Flags: review?(dholbert)
Attachment #8806213 - Flags: review?(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.)
Attachment #8803655 - Flags: review?(dholbert) → review+
Comment on attachment 8803656 [details] [diff] [review]
part 2 - Minor code cleanup (idempotent).

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

r=me
Attachment #8803656 - Flags: review?(dholbert) → review+
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).
Attachment #8806213 - Flags: review?(dholbert) → review+
(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...
> 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.
(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.
Depends on: 1315383
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
Depends on: 1315857
You need to log in before you can comment on or make changes to this bug.