1300369 - [css-grid] Clamp the item min-size to its grid area if track max-sizing are definite

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

It appears that the "Implied Minimum Size of Grid Items" has changed again...
https://lists.w3.org/Archives/Public/www-style/2016Jul/0051.html
https://github.com/w3c/csswg-drafts/issues/283
https://github.com/w3c/csswg-drafts/issues/394

https://drafts.csswg.org/css-grid/#min-size-auto

Comment 1

[Brad Werth [:bradwerth]]

Attachment: grid-auto-min-sizing-definite-002.html

Comment 2

[Brad Werth [:bradwerth]]

Attachment: grid-auto-min-sizing-definite-002-ref.html

Comment 3

[Brad Werth [:bradwerth]]

I've just attached a reasonable reftest for the new behavior, and I'll follow up with a code patch.

Comment 4

[Mats Palmgren (inactive)]

Hmm, I think the reference you've attached for the test is not the desired rendering.
The "Implied Minimum Size" for the grid items in the test looks correct to me in Nightly.

Comment 5

[Brad Werth [:bradwerth]]

(In reply to Mats Palmgren (:mats) from comment #4)
> Hmm, I think the reference you've attached for the test is not the desired
> rendering.

Are you sure? Help me understand the language then in the spec:

> However, if the grid item spans only grid tracks that have a fixed max track sizing function, its automatic minimum size in that dimension is further clamped to less than or equal to the size necessary to fit its margin box within the resulting grid area

These grid items are all auto min-width and min-height, and they all have fixed max track sizing functions in both directions (except for the "auto" cases). That's the criteria specified, I think. The proposed reftest shows their minimum sizes as clamped to maximum size of the tracks. Since the items don't have a specified size, they'll render at their minimum sizes. What's wrong with this reading of the spec?

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1300369 Part 1: Implement revised automatic minimum size for grid items which have fixed max track sizing functions.

Review commit: https://reviewboard.mozilla.org/r/87746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87746/

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1300369 Part 2: Add a new reftest to verify revised automatic minimum size of grid items in tracks with fixed max track sizing functions.

Review commit: https://reviewboard.mozilla.org/r/87748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87748/

Comment 8

[Mats Palmgren (inactive)]

OK, the github discussion have new info that I haven't seen before so
I agree the expected rendering is correct for these tests.  However,
it seems you're only addressing the 'auto' track min-sizing case.
IIUC, the conclusion in https://github.com/w3c/csswg-drafts/issues/283
is that it applies for any min-sizing function, even 20px.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8803504 [details]
Bug 1300369 Part 1: Implement revised automatic minimum size for grid items which have fixed max track sizing functions.

https://reviewboard.mozilla.org/r/87746/#review86782

::: layout/generic/nsFrame.cpp
(Diff revision 1)
> -    if (inlineStyleCoord->IsCoordPercentCalcUnit()) {
> -      minISize = std::min(minISize, result.ISize(aWM));
> +    minISize = std::min(minISize, result.ISize(aWM));

I doubt this is correct.  By removing the check here you're basically setting minISize to zero during reflow.  Didn't any reftests fail due to this?  If not, we should add some.

::: layout/generic/nsGridContainerFrame.cpp:3949
(Diff revision 1)
> +    // If the item spans only tracks with a fixed max track sizing function,
> +    // further clamp this value to be no more than the sum of those track sizes.

This is the wrong place to do this clamping.  And you should use mBase, not mLimit to get the track size.  And it needs to check that min-width/height is actually 'auto'. etc.

Comment 10

[Mats Palmgren (inactive)]

BTW, I already have an old WIP patch that pretty much does the track sizing
part already.  The missing part is figuring out how to do that clamping also
during reflow.  Before doing that though, I want to sort out the spec issue
since it still seems a bit vague what behavior they actually want.

Could you work on fixing the Grid API coordinates in bug 1297072 instead?

Comment 11

[Mats Palmgren (inactive)]

I think I have working patches for this now.  Man, this was a lot more complicated
then I thought initially. :-)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=665c7081be69469e9f3a1d5f2d5aaf6eda557972&exclusion_profile=false

Comment 12

[Mats Palmgren (inactive)]

Attachment: part 1 - Add two new ComputeSizeFlags flags e{I,B}ClampMarginBoxMinSize and associated reflow state flags to indicate we want margin-box min-size clamping in the indicated axis.

Comment 13

[Mats Palmgren (inactive)]

Attachment: part 2 - Make nsLayoutUtils::IntrinsicForAxis handle margin-box min-size clamping.

Comment 14

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Implement margin-box min-size clamping during track sizing.

Comment 15

[Mats Palmgren (inactive)]

Attachment: part 4 - Refactor all ComputeAutoSize methods to take the full ComputeSizeFlags instead of just a "bool aShrinkWrap" for the eShrinkWrap flag (idempotent patch).

Comment 16

[Mats Palmgren (inactive)]

Attachment: part 5 - Make nsFrame::ComputeSize and nsFrame::ShrinkWidthToFit handle margin-box min-size clamping.

Comment 17

[Mats Palmgren (inactive)]

Attachment: part 6 - Make nsBlockFrame::ComputeFinalSize handle margin-box min-size clamping.

Comment 18

[Mats Palmgren (inactive)]

Attachment: part 7 - [css-grid] Implement margin-box min-size clamping for grid item reflow.

Comment 19

[Mats Palmgren (inactive)]

Attachment: part 8 - Move nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to a nsFrame method (idempotent patch).

Comment 20

[Mats Palmgren (inactive)]

Attachment: part 9 - Add a ComputeSizeFlags param to nsFrame::ComputeSizeWithIntrinsicDimensions (idempotent patch).

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 10 - Make nsFrame::ComputeSizeWithIntrinsicDimensions handle margin-box min-size clamping.

Comment 22

[Mats Palmgren (inactive)]

Attachment: part 11 - [css-grid] Reftests for margin-box min-size clamping.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea69fcaf3152080ffc4dc8e927857aeb846713b4

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8804315 [details] [diff] [review]
part 8 - Move nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to a nsFrame method (idempotent patch).

I forgot to motivate why I'm moving this function.
In the next patch (part 9) I'm adding a ComputeSizeFlags param.
ComputeSizeFlags lives in nsIFrame.h which we can't #include
in nsLayoutUtils.h because it leads to a circular dependency.

That might be possible to fix, but ComputeSizeWithIntrinsicDimensions
is currently taking an aFrame param which all callers pass 'this' for,
which is a strong indication this really should've been a frame
method in the first place.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8804308 [details] [diff] [review]
part 1 - Add two new ComputeSizeFlags flags e{I,B}ClampMarginBoxMinSize and associated reflow state flags to indicate we want margin-box min-size clamping in the indicated axis.

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

r=me, one optional nit:

::: layout/generic/nsIFrame.h
@@ +1996,5 @@
>       * corresponding computed value. (e.g. to get an intrinsic height for flex
>       * items with "min-height: auto" to use during flexbox layout.) */
> +    eUseAutoBSize =      1 << 1,
> +    /**
> +     * Indicates that we should clamp the margin-box min-size to the given CB

(Extreme nit: for the preexisting enum values above this one, we start the documentation with just "/*" instead of "/**".  Maybe drop the second "*", for consistency with those?)

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8804309 [details] [diff] [review]
part 2 - Make nsLayoutUtils::IntrinsicForAxis handle margin-box min-size clamping.

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

r=me

::: layout/base/nsLayoutUtils.cpp
@@ +5104,5 @@
>  
>    nsIFrame::IntrinsicISizeOffsetData offsets =
>      MOZ_LIKELY(aAxis == ourInlineAxis) ? aFrame->IntrinsicISizeOffsets()
>                                         : aFrame->IntrinsicBSizeOffsets();
> +  nscoord contentSize = result;

"contentSize" sounds like "the size of our content".  (Especially since this is a function that's about intrinsic sizing).

I think this really is meant to represent the content *box* size. So maybe call it contentBoxSize?  (and "newContentBoxSize" below)

@@ +5113,5 @@
>                                    haveFixedMaxISize ? &maxISize : nullptr,
>                                    styleMaxISize,
>                                    aFlags, aAxis);
> +  nscoord overflow = result - aMarginBoxMinSizeClamp;
> +  if (MOZ_UNLIKELY(overflow > 0)) {

And for extra clarity (since "result" means something completely different at each stage of this function), it'd be nice to have a comment somewhere around here saying:
 // "result" now represents the margin-box-size. Clamp it if needed.

@@ +5115,5 @@
>                                    aFlags, aAxis);
> +  nscoord overflow = result - aMarginBoxMinSizeClamp;
> +  if (MOZ_UNLIKELY(overflow > 0)) {
> +    nscoord newContentSize = std::max(nscoord(0), contentSize - overflow);
> +    // XXXmats we should deal with percentages better

(I don't immediately understand the significance of percentages to the code around this line. Consider clarifying this XXXmats comment slightly, so its meaning is clearer?)

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8804310 [details] [diff] [review]
part 3 - [css-grid] Implement margin-box min-size clamping during track sizing.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +847,5 @@
>  
> +  bool ShouldClampMinSize(WritingMode aContainerWM,
> +                          LogicalAxis aContainerAxis) const
> +  {
> +    const auto& pos = mFrame->StylePosition();

(The "&" in "const auto&" here seems a bit confusing.  At first it had me thinking that "pos" was a nsStylePosition&.  But since StylePosition returns a pointer, it's really a nsStylePosition*& -- a reference to a pointer.

That seems a bit needlessly-complex; "const auto pos" seems clearer (or just const nsStylePosition* pos). Though maybe the version with the reference is more idiomatic or more maintainable somehow?)

@@ +850,5 @@
> +  {
> +    const auto& pos = mFrame->StylePosition();
> +    const auto& size = aContainerAxis == eLogicalAxisInline ?
> +      pos->ISize(aContainerWM) : pos->BSize(aContainerWM);
> +    if (size.GetUnit() == eStyleUnit_Auto) {

I'm not clear on why we're checking |size.GetUnit()| (the width/height) here.  The spec text in https://drafts.csswg.org/css-grid/#min-size-auto doesn't seem to depend on the value of "width"/"height" at all.

This might still be correct, but it'd be nice to explain what's going on (and how it relates to the spec text) with a comment.

@@ +857,5 @@
> +      return minSize.GetUnit() == eStyleUnit_Auto &&
> +             mFrame->StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE;
> +    }
> +    return false;
> +  };

Drop the unwanted semicolon here (after the function closing curly-brace).

(I'd expect this would trigger a compile error, but maybe not).

@@ +3858,5 @@
> +    nscoord overflow = size - aMinSizeClamp;
> +    if (MOZ_UNLIKELY(overflow > 0)) {
> +      nscoord contentSize = child->ContentBSize(childWM);
> +      nscoord newContentSize = std::max(nscoord(0), contentSize - overflow);
> +      // XXXmats deal with percentages better

(If you clarify the other XXXmats percentages comment, be sure to make the same clarification here.)

Comment 27

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> I think this really is meant to represent the content *box* size. So maybe
> call it contentBoxSize?  (and "newContentBoxSize" below)

Correct.

> > +    // XXXmats we should deal with percentages better
> 
> (I don't immediately understand the significance of percentages to the code
> around this line. Consider clarifying this XXXmats comment slightly, so its
> meaning is clearer?)

If we have percentage margin/padding that contributed to our margin-box size
and then reduce our content size (due to clamping) then we've changed
the percentage base for those percentages.  (AddIntrinsicSizeOffset calls
AddPercent with the sum of coords, where the content-box size is included.)
I don't see a simple solution to that problem though...  and perhaps we don't
really care at this point anyway when I think about it, since we only want
the intrinsic size contribution here.  I'll remove the comment here since
I think it only matters where we actually need to calculate the content-box
size that would result in the margin-box clamp size.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8804314 [details] [diff] [review]
part 7 - [css-grid] Implement margin-box min-size clamping for grid item reflow.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +3850,5 @@
>      nscoord cbBSize = NS_UNCONSTRAINEDSIZE;
>      auto childWM = child->GetWritingMode();
> +    const bool isOrthogonal = childWM.IsOrthogonalTo(aCBWM);
> +    // The next two variables are MinSizeClamp values in the child's axis.
> +    nscoord iMinSizeClamp = NS_MAXSIZE;

s/axis/axes/, I think? (plural)

(Or if I'm reading that wrong, then I'm not clear what "in the child's axis" means in this comment.  The child doesn't just have one axis.  But I'm guessing you meant "axes" (plural) instead of "axis" -- i.e. this is meaning to indicate that "i" = "child's inline axis", and "b" = "child's block axis", in the variables below this.)

@@ +4071,5 @@
>    TrackSize& sz = mSizes[aRange.mStart];
>    WritingMode wm = aState.mWM;
> +  // Calculate data for "Automatic Minimum Size" clamping, if needed.
> +  if (((sz.mState & TrackSize::eIntrinsicMinSizing) ||
> +       aConstraint == SizingConstraint::eNoConstraint) &&

Looks like there's room for simplification here.

This "||" condition at the beginning of this if-check seems strictly unnecessary (unless it's an optimization of some sort, if one of the later conditions is expensive?)  Assuming for the moment it's not an optimization -- you should be able to remove this "A || B" condition entirely from the outer "if" check here without impacting the logic.  (That's because the contents of this if-check's control block is just "if (A) { stuff}  if (B) { stuff}".  So we're already checking A and B individually before we do anything. And if neither are true, then we won't do anything.)

@@ +5304,5 @@
>    if (isConstrainedBSize) {
>      reflowSize.BSize(wm) = toFragmentainerEnd;
>    }
>    LogicalSize childCBSize = reflowSize.ConvertTo(childWM, wm);
> +  nscoord childBSize = childCBSize.BSize(childWM);

"childBSize" is kinda of a misleading name here. This is not the child B-size. It's the child's containing block's B-Size.

Could we just get rid of the variable, and just replace its one usage with "childCBSize.BSize(childWM)"?

This is very-slightly tricky, because we have some special case code (directly below the line I'm quoting above) that stomps on  childCBSize.BSize, before the patch's current childBSize usage. Not to worry, though -- we can move that special case code to happen afterwards, I think.

SO: I think you should do the following:
 (1) Move the "BSize(childWM) = NS_UNCONSTRAINEDSIZE" special-case (which is currently right below this) down a bit, to happen just before "LogicalSize percentBasis" decl.
 (2) Replace the one "childBSize" usage in your new code here with a hardcoded "childCBSize.BSize(childWM);"

@@ +5309,2 @@
>    if (!isConstrainedBSize) {
>      childCBSize.BSize(childWM) = NS_UNCONSTRAINEDSIZE;

(This is the special-case that I'm suggesting needs to move.)

@@ +5316,5 @@
> +    auto childIAxis = isOrthogonal ? eLogicalAxisBlock : eLogicalAxisInline;
> +    if (aGridItemInfo->mState[childIAxis] & ItemState::eClampMarginBoxMinSize) {
> +      flags |= ReflowInput::I_CLAMP_MARGIN_BOX_MIN_SIZE;
> +    }
> +    auto childBAxis = isOrthogonal ? eLogicalAxisInline : eLogicalAxisBlock;

If you like, you could (slightly) simplify this assignment to:
  auto childBAxis = GetOrthogonalAxis(childIAxis);
...for a slight reduction in copypasted-ish logic, and for slightly-stricter/clearer enforcement that childBAxis & childIAxis are indeed orthogonal.

@@ +5319,5 @@
> +    }
> +    auto childBAxis = isOrthogonal ? eLogicalAxisInline : eLogicalAxisBlock;
> +    if (aGridItemInfo->mState[childBAxis] & ItemState::eClampMarginBoxMinSize) {
> +      flags |= ReflowInput::B_CLAMP_MARGIN_BOX_MIN_SIZE;
> +      aChild->Properties().Set(BClampMarginBoxMinSizeProperty(), childBSize);

(This is the sole usage of childBSize, which [per above] we should just replace with [newline] childCBSize.BSize(childWM) probably.)

Comment 29

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #28)
> s/axis/axes/, I think? (plural)

Yes, I meant plural.

> > +  // Calculate data for "Automatic Minimum Size" clamping, if needed.
> > +  if (((sz.mState & TrackSize::eIntrinsicMinSizing) ||
> > +       aConstraint == SizingConstraint::eNoConstraint) &&
> 
> Looks like there's room for simplification here.

Yes, it's meant as an optimization, since ShouldClampMinSize is a bit more
expensive.

> Could we just get rid of the variable, and just replace its one usage with
> "childCBSize.BSize(childWM)"?

Yeah, that's better, thanks.

> > +    auto childBAxis = isOrthogonal ? eLogicalAxisInline : eLogicalAxisBlock;
> 
> If you like, you could (slightly) simplify this assignment to:
>   auto childBAxis = GetOrthogonalAxis(childIAxis);

Sure.

Comment 30

[Daniel Holbert [:dholbert]]

Thanks! So I think the only thing I'm iffy on here the |size.GetUnit()| check that I noted in comment 26 (which is the reason I didn't r+ "part 3").

Comment 31

[Mats Palmgren (inactive)]

Yeah, that's a good point.  I think that came from "if the box has an aspect ratio and no specified size, its automatic minimum size is the smaller of its content size and its transferred size." in
https://www.w3.org/TR/css-flexbox-1/#min-width-automatic-minimum-size
But I guess if the box doesn't have an aspect ratio we should also clamp a specified size.
I wonder if this really matters in practice though... let me know if you can think of a case
where it does.  Meanwhile, I'll rebuild without that check and see if anything breaks...

Comment 32

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #31)
> Yeah, that's a good point.  I think that came from "if the box has an aspect
> ratio and no specified size, its automatic minimum size is the smaller of
> its content size and its transferred size."
> But I guess if the box doesn't have an aspect ratio we should also clamp a specified size.

I don't think it's supposed to matter *where* the automatic minimum size comes from (transferred vs. content vs. specified) -- the grid spec-text says just says to apply this final track-based clamping to the "automatic minimum size", not to its components. ("...its automatic minimum size in that dimension is further clamped...") https://drafts.csswg.org/css-grid/#min-size-auto

> I wonder if this really matters in practice though... let me know if you can
> think of a case where it does.

I'd expect it'd matter in a case like this:
 <grid style="grid-template-columns: 15px;">
   <item style="width: 1px"></item>
 </grid>

IIUC, the spec is saying that <item> is supposed to end up having an automatic minimum size of 15px (taken from the tracks that it spans), which means it should end up 15px wide, despite its specified "width: 1px".

Another example would be items with "width:auto" vs. "width:-moz-max-content".  I'd expect this spec text should apply similarly to each of those items (they should be clamped in the same way) -- but right now Part 3 explicitly checks for "eStyleUnit_Auto" and not other keywords.  So if we really do need that eStyleUnit_Auto special case, I'd expect that we'd want it to be a broader "definite vs. non-definite" check, rather than an "auto" check.

Comment 33

[Mats Palmgren (inactive)]

> I don't think it's supposed to matter *where* the automatic minimum size comes from

Fair enough.  But note that we're talking about the 'min-content size' here,
in contrast to 'min-content contribution'.  The grid track sizing algorithm
is asking for the latter.  So that's why I don't think it matters when the width(height)
is specified, because the 'min-content contribution' *is* that specified size.

> IIUC, the spec is saying that <item> is supposed to end up having an automatic minimum size of 15px (taken
> from the tracks that it spans), which means it should end up 15px wide, despite its specified "width: 1px".

Nope, I'm quite sure the spec only wants us to clamp (make smaller), not floor.
(Filling the area is what 'stretch' is for.)

The only case I can think of, with a specified size, where this actually matters is:
<div style="display: grid; grid-template-columns: minmax(min-content,20px);">
  <x style="width:-moz-min-content">XXXXX</x>
</div>

Because here we're explicitly basing our 'min-content contribution' on our
'min-content size' which should be clamped to 20px.  So the item should be
20px in this case and its text contents should overflow.

> Another example would be ... "width:-moz-max-content".

I don't think it matters in that case, because while the 'min-content size'
should be clamped here too, I think we can rely on the invariant
"min-content size <= max-content size".

> I'd expect that we'd want it to be a broader "definite vs. non-definite" check

That's a good point.  But, a percentage width(height) during *intrinsic sizing*
always counts as zero, right?  So clamping should matter in this case either
(since the content-box size we're supposed to shrink can't be smaller).
During reflow... well, a grid item always has a definite percentage basis in
both axes, so I'm not sure clamping the min-content size is observable.
It seems it should be equivalent to width:Npx to me.

I'll make a bug fix for the width(height):-moz-min-content case for now...

Comment 34

[Mats Palmgren (inactive)]

Attachment: part 3b, bug fix for width/height:-moz-min-content

I figured it'd be easier to review this bug fix standalone.
I'll fold it into part 3 before landing.

It does pass existing reftests locally.  I'll add a few that fails
without this patch before landing.

Comment 35

[Mats Palmgren (inactive)]

Comment on attachment 8806943 [details] [diff] [review]
part 3b, bug fix for width/height:-moz-min-content

Hang on, this isn't done yet... It does make the column 20px for the example
I gave above, but the item is still the size of the text...

Comment 36

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #33)
> Nope, I'm quite sure the spec only wants us to clamp (make smaller), not
> floor.
> (Filling the area is what 'stretch' is for.)

(Ahh, you're right. I read "less than or equal" and imagined a "min()" operation, but really it's a "max" operation. This makes this feature make more sense to me -- it did feel like it was colliding with "stretch" functionality. Thanks for clarifying.)

Comment 37

[Mats Palmgren (inactive)]

Attachment: part 3b, first part of bug fix for width/height:-moz-min-content

This doesn't fix the width:-moz-min-content case by itself, but a separate
patch is coming up that does.  (I also addressed a couple of nits here to
avoid merge conflicts later.)

Comment 38

[Mats Palmgren (inactive)]

Attachment: part 11 - Move nsLayoutUtils::ComputeISizeValue to a nsIFrame method (idempotent patch).

(this is to prepare for the next patch which will add a ComputeISizeValue arg)

Comment 39

[Mats Palmgren (inactive)]

Attachment: part 12 - Add a ComputeSizeFlags arg to nsIFrame::ComputeISizeValue; make it clamp 'min-content' when eIClampMarginBoxMinSize is in aFlags...

The remaining part of the width:-moz-min-content fix.

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8806974 [details] [diff] [review]
part 3b, first part of bug fix for width/height:-moz-min-content

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +856,5 @@
>      const auto& size = aContainerAxis == eLogicalAxisInline ?
>        pos->ISize(aContainerWM) : pos->BSize(aContainerWM);
> +    if (size.GetUnit() == eStyleUnit_Auto ||
> +        (size.GetUnit() == eStyleUnit_Enumerated &&
> +         size.GetIntValue() == NS_STYLE_WIDTH_MIN_CONTENT)) {

Before landing, could you add a code-comment here to hint at why we're singling out these specific width values ("auto" and "min-content") and not other indefinite-ish values? (e.g. "fit-content", "max-content")

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8806981 [details] [diff] [review]
part 12 - Add a ComputeSizeFlags arg to nsIFrame::ComputeISizeValue; make it clamp 'min-content' when eIClampMarginBoxMinSize is in aFlags...

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

In the interests of separating large non-functional changes from targeted functional changes: it'd ideally be nice to split this into 2 parts:
 - the first part should add the new flags arg to ComputeISizeValue (and update all the callsites).
 - the second part would make the very targeted change inside of ComputeISizeValue to handle eIClampMarginBoxMinSize.

(But if you'd rather not, it's probably OK like this too.)

::: layout/generic/nsFrame.cpp
@@ +5359,5 @@
>          NS_ASSERTION(result >= 0, "inline-size less than zero");
> +        if (MOZ_UNLIKELY(aFlags & ComputeSizeFlags::eIClampMarginBoxMinSize)) {
> +          auto available = aContainingBlockISize -
> +                           (aBoxSizingToMarginEdge + aContentEdgeToBoxSizing);
> +          result = std::min(available, result);

This new code needs a comment to explain what we're doing here & why we don't need to do it for any of the other cases in this function.

Stepping back, I'm also not clear why this is the right place to make this change.  The spec text says we should be doing this clamping on the "automatic minimum size" (the resolved min-width:auto/min-height:auto").  Why do we have to do it at this specific targeted place, rather than at the particular callsite (up a few levels perhaps) where we actually determine the automatic minimum size?

Comment 42

[Mats Palmgren (inactive)]

I've investigated the fit-content/fill and percentage cases,
and I believe you're right that we need to clamp those too
in various situations...

fit-content during reflow:
http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#5284
we shouldn't use an unclamped 'min' here since that could
make the result larger than it should be.

fit-content/fill and percentage during intrinsic sizing under
a min-content constraint:
http://searchfox.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#5016

Comment 43

[Mats Palmgren (inactive)]

Permalinks for the two references above, for posterity:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/base/nsLayoutUtils.cpp#5284
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/layout/base/nsLayoutUtils.cpp#5016

Comment 44

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Implement margin-box min-size clamping during track sizing.

This is the new ShouldClampMinSize() that includes the additional cases.
I included any enum other than 'max-content' for now, since I think it's
likely that any new values will depend on the min-content size in some
situations.  (You only need to review ShouldClampMinSize again here.)

Comment 45

[Mats Palmgren (inactive)]

Attachment: part 2 - Make nsLayoutUtils::IntrinsicForAxis handle margin-box min-size clamping.

With the expanded ShouldClampMinSize() we now need to exclude a few cases from
clamping under a max-content constraint.  The added chunk you need to review is:

+    if (aType != MIN_ISIZE) {
+      // At this point, |styleISize| is auto/-moz-fit-content/-moz-available or
+      // have a percentage.  The intrinisic size for those under a max-content
+      // constraint is the max-content contribution which we shouldn't clamp.
+      aMarginBoxMinSizeClamp = NS_MAXSIZE;
+    }

Note that this is in the else-branch of:
   if (styleISize.GetUnit() == eStyleUnit_Enumerated &&
       (styleISize.GetIntValue() == NS_STYLE_WIDTH_MAX_CONTENT ||
        styleISize.GetIntValue() == NS_STYLE_WIDTH_MIN_CONTENT)) {

Comment 46

[Mats Palmgren (inactive)]

Attachment: part 12a - Add a ComputeSizeFlags arg to nsIFrame::ComputeISizeValue. Propagate aFlags in nsFrame::ComputeSize in its ComputeISizeValue calls (idempotent patch).

(this is the idempotent part of part 12 as requested)

Comment 47

[Mats Palmgren (inactive)]

Attachment: part 12b - Make nsIFrame::ComputeISizeValue clamp the min-content size when eIClampMarginBoxMinSize is in aFlags.

This adds clamping for the -moz-fit-content case.

Comment 48

[Mats Palmgren (inactive)]

Attachment: part 7b, some code cleanup (idempotent)

Cleaned up around the ShouldClampMinSize() calls to simplify the code.
(will fold into part 7 before landing)

Comment 49

[Mats Palmgren (inactive)]

Attachment: testcase

Here's some tests I wrote for the various enum values under different track sizing constraints.  I believe we now handle these correctly.

Comment 50

[Mats Palmgren (inactive)]

Attachment: screenshot of the testcase above

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8807391 [details] [diff] [review]
part 3 - [css-grid] Implement margin-box min-size clamping during track sizing.

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

r=me, though I've got two requests for ShouldClampMinSize:

::: layout/generic/nsGridContainerFrame.cpp
@@ +848,5 @@
>      *aBaselineOffset = mBaselineOffset[aAxis];
>      return aAlign;
>    }
>  
> +  bool ShouldClampMinSize(WritingMode aContainerWM,

Maybe add a link to https://drafts.csswg.org/css-grid/#min-size-auto above this function (or a mention of "grid spec section 6.6"), to give a hint about what flavor of the "Should Clamp Min Size" question we're aiming to answer here. (There's lots of types of clamping that happens when determining/applying a min size.)

@@ +857,5 @@
> +    const auto& size = aContainerAxis == eLogicalAxisInline ?
> +      pos->ISize(aContainerWM) : pos->BSize(aContainerWM);
> +    if (size.GetUnit() == eStyleUnit_Auto ||
> +        ::IsPercentOfIndefiniteSize(size, aPercentageBasis) ||
> +        (size.GetUnit() == eStyleUnit_Enumerated &&

Please add a comment somewhere to explain *why* we're checking the particular things we're checking here in this function.  I suspect you're aiming to return true if:
 - ...we have an automatic minimum size in the given axis (based on its unit & the "overflow" value).
 - ...and that automatic minimum size might actually influence our size in the given axis (i.e. it's not already clamped due to us having a definite size in that axis).

If both of those things are true, then we do need to clamp the min-size. Otherwise, we don't need to bother.  I think that's the idea here? The second bullet-point in particular is subtle, and without a comment, it's non-obvious what the thinking is behind the definite-size logic here. That's a good chunk of what tripped me up in the first place, in comment 26.

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 8807396 [details] [diff] [review]
part 7b, some code cleanup (idempotent)

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

r=me

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8807395 [details] [diff] [review]
part 12b - Make nsIFrame::ComputeISizeValue clamp the min-content size when eIClampMarginBoxMinSize is in aFlags.

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

r=me

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8807392 [details] [diff] [review]
part 2 - Make nsLayoutUtils::IntrinsicForAxis handle margin-box min-size clamping.

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

Sorry, missed this update until now.

r=me; just one comment-typo-nit on the new section:

::: layout/base/nsLayoutUtils.cpp
@@ +5005,5 @@
>      ++gNoiseIndent;
>  #endif
> +    if (aType != MIN_ISIZE) {
> +      // At this point, |styleISize| is auto/-moz-fit-content/-moz-available or
> +      // have a percentage.  The intrinisic size for those under a max-content

s/have/has/

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/292c5a180ea3
part 1 - Add two new ComputeSizeFlags flags e{I,B}ClampMarginBoxMinSize and associated reflow state flags to indicate we want margin-box min-size clamping in the indicated axis.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/a89255f21073
part 2 - Make nsLayoutUtils::IntrinsicForAxis handle margin-box min-size clamping.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/84cfc7a757b9
part 3 - [css-grid] Implement margin-box min-size clamping during track sizing.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ee55e916c6
part 4 - Refactor all ComputeAutoSize methods to take the full ComputeSizeFlags instead of just a "bool aShrinkWrap" for the eShrinkWrap flag (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8692ff0a6b3b
part 5 - Make nsFrame::ComputeSize and nsFrame::ShrinkWidthToFit handle margin-box min-size clamping.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9dd8fc25788
part 6 - Make nsBlockFrame::ComputeFinalSize handle margin-box min-size clamping.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/488d1410c7be
part 7 - [css-grid] Implement margin-box min-size clamping for grid item reflow.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc29ce155bd
part 8 - Move nsLayoutUtils::ComputeSizeWithIntrinsicDimensions to a nsFrame method (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/96b1c6a38446
part 9 - Add a ComputeSizeFlags param to nsFrame::ComputeSizeWithIntrinsicDimensions (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc204b727b53
part 10 - Make nsFrame::ComputeSizeWithIntrinsicDimensions handle margin-box min-size clamping.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/b553c7d661e9
part 11 - Move nsLayoutUtils::ComputeISizeValue to a nsIFrame method (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c76a7dbadc1
part 12a - Add a ComputeSizeFlags arg to nsIFrame::ComputeISizeValue.  Propagate aFlags in nsFrame::ComputeSize in its ComputeISizeValue calls (idempotent patch).  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bc6e35c804
part 12b - Make nsIFrame::ComputeISizeValue clamp the min-content size when eIClampMarginBoxMinSize is in aFlags.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/728204430c0b
part 13 - [css-grid] Reftests for margin-box min-size clamping.

Comment 56

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/292c5a180ea3
https://hg.mozilla.org/mozilla-central/rev/a89255f21073
https://hg.mozilla.org/mozilla-central/rev/84cfc7a757b9
https://hg.mozilla.org/mozilla-central/rev/c9ee55e916c6
https://hg.mozilla.org/mozilla-central/rev/8692ff0a6b3b
https://hg.mozilla.org/mozilla-central/rev/e9dd8fc25788
https://hg.mozilla.org/mozilla-central/rev/488d1410c7be
https://hg.mozilla.org/mozilla-central/rev/4bc29ce155bd
https://hg.mozilla.org/mozilla-central/rev/96b1c6a38446
https://hg.mozilla.org/mozilla-central/rev/dc204b727b53
https://hg.mozilla.org/mozilla-central/rev/b553c7d661e9
https://hg.mozilla.org/mozilla-central/rev/8c76a7dbadc1
https://hg.mozilla.org/mozilla-central/rev/e3bc6e35c804
https://hg.mozilla.org/mozilla-central/rev/728204430c0b