1176775 - [css-grid] Implement "Implied Minimum Size of Grid Items" (special min-width/height:auto behavior)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

http://dev.w3.org/csswg/css-grid/#min-size-auto

Flexbox spec is identical afaict:
http://dev.w3.org/csswg/css-flexbox/#min-size-auto
but afaict its implementation is integrated into the reflow so I don't
see anything there that I can reuse.  Daniel, please let me know if
I missed something.

For Grid we can't do that since we need to resolve column/row sizes
up front before starting to reflow items for real.

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #0)
> http://dev.w3.org/csswg/css-grid/#min-size-auto
> 
> Flexbox [...] afaict its implementation is integrated into the reflow so I don't
> see anything there that I can reuse.

Correct; the flex-item min-[width|height]:auto resolution is implemented in nsFlexContainerFrame.cpp, during reflow, largely in PartiallyResolveAutoMinSize and MeasureFlexItemContentHeight. [which will soon be renamed with s/Height/BSize/]

> For Grid we can't do that since we need to resolve column/row sizes
> up front before starting to reflow items for real.

(I'm assuming "can't do that" means "can't resolve min-[width|height]:auto during nsGridContainerFrame::Reflow)

Is this because "min-width" on the grid items must be resolved for our GetPrefISize()/GetMinISize() implementation?

Comment 2

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Is this because "min-width" on the grid items must be resolved for our
> GetPrefISize()/GetMinISize() implementation?

Well, not just that.  Grid items contributes to the sizing of the tracks
they span, and each track can have have many items contributing to its size.
It's an indirect model, much like table layout.  It's only after processing
all items that the track sizes are known and only then can you calculate
the CB to reflow the item.  (Bug 1151212, bug 1176619 and bug 1176621
implements the various steps in sizing the tracks.)

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix

This extends MinSizeContributionForAxis so that it calculates all the
definite values needed for https://drafts.csswg.org/css-grid/#min-size-auto
I.e. when we have either:
* a definite min-width
* min-width:auto + overflow!=visible (treated as min-width:0)
* min-width:auto + overflow:visible + a definite width, this returns
  the "specified/transferred size"
(or s/width/height/ when that gives the isize)

In all other cases NS_UNCONSTRAINEDSIZE is returned.

Then MinSize is extended to take the std::min of that and the
"content size" part (i.e. the result of ContentContribution).

A new flag (MIN_INTRINSIC_ISIZE) is added to IntrinsicForAxis
so that it uses min-width|height instead of width|height.
This is for calculating the contribution of for example
min-width:-moz-min-content etc.

(The division of work between nsLayoutUtils and the Grid code is to keep
the ContentContribution call on the Grid side so that we can optimize
that (avoiding reflows) at some point.)

Comment 4

[Mats Palmgren (inactive)]

Attachment: tests

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

See comment 3.

Comment 6

[Mats Palmgren (inactive)]

Attachment: tests

Comment 7

[Mats Palmgren (inactive)]

Attachment: tests

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

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8668206 [details] [diff] [review]
fix

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

::: layout/base/nsLayoutUtils.cpp
@@ +4726,5 @@
> +  nscoord* fixedMinSize = nullptr;
> +  if (style->GetUnit() == eStyleUnit_Auto) {
> +    if (aFrame->StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE) {
> +      style = aAxis == eAxisHorizontal ? &stylePos->mWidth
> +        : &stylePos->mHeight;

Two things:
 (1) This assignment a bit hard to read - please indent the ":" so that the two options are at the same level of indentation (so this looks like the first "style = [...]" assignment that you've got above this).  That, or add a newline before the "?" (with the same goal -- having the options at the same level of indentation).

 (2) This whole chunk (for eStyleUnit_Auto considering 'overflow') is only applicable for grid items & flex items, I think.  Really, I think this whole function should only be called for grid or flex items, right?  Since it kind of revolves around this chunk.  (Right now we only call this for grid items, but it's not obviously grid-specific in its naming/documentation.)  Could you mention this restriction in its documentation, and add an assertion that aFrame->IsFlexOrGridItem() towards the beginning? (maybe replacing the aFrame->GetParent() assertion)

@@ +4728,5 @@
> +    if (aFrame->StyleDisplay()->mOverflowX == NS_STYLE_OVERFLOW_VISIBLE) {
> +      style = aAxis == eAxisHorizontal ? &stylePos->mWidth
> +        : &stylePos->mHeight;
> +      if (GetAbsoluteCoord(*style, minSize)) {
> +        // We have a definite width/height.  This is the "specified size" /

I don't think GetAbsoluteCoord() is the right way to check for "definite-ness".  In particular, GetAbsoluteCoord fails for percentages & calc(%), but percentages *do* count as definite as long as we've got a definite percent-basis, per
 https://drafts.csswg.org/css-sizing-3/#definite

Because of this, right now I think this code would treat a grid item with e.g. "min-width:auto; width: 30%" as having an undefined "specified size" [using terminology from https://drafts.csswg.org/css-grid/#min-size-auto ], and we'll end up returning NS_UNCONSTRAINEDSIZE and having the caller treat this as a content-based sizing.  But really it may have a definite size, depending on whether or not it's got a valid percent basis for its 30%.

I'm assuming this could make us misrender -- but if I'm wrong & you're sure this is actually OK [e.g. if we can't possibly have a valid percent basis if we get here, or we'll handle it correctly elsewhere], then we probably need to explain/clarify why this is OK in a code-comment here.

::: layout/base/nsLayoutUtils.h
@@ +1340,5 @@
>     * This considers the child's 'min-width' property (or 'min-height' if the
>     * given axis is vertical), and its padding, border, and margin in the
> +   * corresponding dimension.  If the 'min-' property is 'auto' (and 'overflow'
> +   * is 'visible') then it uses the corresponding 'width'/'height' instead if
> +   * it's definite to calculate the "specified / transferred size" for:

This sentence is hard to parse, particularly around "...instead if it's definite to calculate...".

Maybe put "if it's definite" in parentheses?  Or, list all of the if's (including definite-ness of width/height) at the beginning of the sentence, rather than having that one requirement being an afterthought.

::: layout/generic/nsGridContainerFrame.cpp
@@ +2036,5 @@
> +  // transferred size" for min-width:auto if overflow == visible (as min-width:0
> +  // otherwise), or NS_UNCONSTRAINEDSIZE for other min-width intrinsic values
> +  // (which results in always taking the "content size" part below).
> +  nscoord sz = nsLayoutUtils::MinSizeContributionForAxis(axis, aRC, aChild,
> +                                                      nsLayoutUtils::MIN_ISIZE);

The slighlt-deindentation-to-avoid-going-over-80-cols seems a bit bogus here.  Can you just add a newline after "=", so that the whole MinSizeContributionForAxis expression can be legitimately de-indented & fit more cleanly?

@@ +2045,5 @@
> +    // Now calculate the "content size" part and return whichever is smaller.
> +    MOZ_ASSERT(unit != eStyleUnit_Enumerated || sz == NS_UNCONSTRAINEDSIZE);
> +    sz = std::min(sz, ContentContribution(aChild, aRS, aRC, aCBWM, aAxis,
> +                                          nsLayoutUtils::MIN_ISIZE,
> +                                          nsLayoutUtils::MIN_INTRINSIC_ISIZE));

The similar-enum-naming here frightens me a bit.  If I "accidentally" replace MIN_INTRINSIC_ISIZE with MIN_ISIZE here (a typo that I fully expect someone to make at some point in the future when using this API), this still compiles just fine.  That's problematic.

To protect against this, could you upgrade IntrinsicISizeType into an enum class? (That's the type of MIN_ISIZE, which is a true enum, not a "flags" list).  That way it won't let itself be accidentally coerced to a uint32_t flag, and my example typo above would fail to compile. (Might be worth doing this upgrading in its own patch & maybe on its own bug).

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8668206 [details] [diff] [review]
fix

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

Marking this r- for now, since there's still a bit left here, particularly for the GetAbsoluteCoord concern in my previous comment.

I went over this again (since I hadn't made it quite through before, but now I think I have), and I've got two additional review notes:

::: layout/base/nsLayoutUtils.cpp
@@ +4490,5 @@
>    const nsStyleCoord& styleMinISize =
>      horizontalAxis ? stylePos->mMinWidth : stylePos->mMinHeight;
> +  const nsStyleCoord& styleISize =
> +    (aFlags & MIN_INTRINSIC_ISIZE) ? styleMinISize :
> +    (horizontalAxis ? stylePos->mWidth : stylePos->mHeight);

So, this piece of code (the only place where we react to this new flag) is a little confusing, because:
 * This flag *says* "get my min intrinsic size" (a content-based thing)
 * ...but in practice, it just makes us use "min-width" in place of "width". And that's not really an "intrinsic" / "content-based" thing to do.

Hypothetically if someone called this method & passed this flag for a frame that has "min-width: 10px", then we'd just go through this function as-normal except we'd be pretending we have "width:10px", and we wouldn't do anything special for the contents (not as a result of the flag at least).

Happily, right now, it turns out that styleMinISize *must* be something intrinsic-flavored here ("auto" or an enum value), but that's only because of the guards you have around the one place where you're passing this flag.  So my hypothetical about "min-width:10px" can't actually happen in practice.  But, that's not at all clear at this point in the code, and we could easily run into this sort of trouble in the future, if we add new callers that have different sets of guards & pass this flag.

SO: If we've been passed the MIN_INTRINSIC_ISIZE flag, I think we need to really do one or both of the following:

 (1) Point styleISize at a dedicated local nsStyleCoord (or a lazily-constructed Maybe<nsStyleCoord>) which is *explicitly* set to "auto".  Don't depend on styleMinISize being something intrinsic-flavored -- let's just use a dummy variable that clearly has that value.

...and/or:
 (2) Assert that styleMinISize is something intrinsic-flavored (if we depend on that, or simply want to sanity-check that).  (If you like, this could mean we don't need (1), though I still like (1)'s explicitness.)

@@ +4732,5 @@
> +        // We have a definite width/height.  This is the "specified size" /
> +        // "transferred size" cases in:
> +        // https://drafts.csswg.org/css-grid/#min-size-auto
> +        fixedMinSize = &minSize;
> +      }

The code-comment here says this might be the "transferred size", but I don't think it is. In fact, I don't think we calculate the transferred size anywhere right now (though this function's documentation & the comment at the callsite says we do). (This is MinSizeContributionForAxis btw.)

The "transferred size" is supposed to be derived from the perpendicular axis (i.e. from the "height", if we're resolving min-width:auto).  And I don't see us doing anything like that -- we only have code that inspects the same-axis size property right now.

(Please correct me if I'm just overlooking it.)

I think the "transferred size" computation code would need to go inside of a new "else" clause that would be chained off of our existing "Do we have a specified size in this axis?" check (which currently is a GetAbsoluteCoord call, though that needs to change per comment 8).  We'd put it in this "else" because, per the chart at https://drafts.csswg.org/css-grid/#min-size-auto , we only really care about the "transferred size" when the "specified size" is undefined.

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix

(In reply to Daniel Holbert [:dholbert] from comment #8)
>  (1) This assignment a bit hard to read - please indent the ":" 

Fixed.

> Could you mention this restriction in its documentation, 
> and add an assertion that
> aFrame->IsFlexOrGridItem() towards the beginning? (maybe replacing the
> aFrame->GetParent() assertion)

Sure.  The code here doesn't depend on the frame type though,
but I agree it would be good to catch any accidental use.

> I don't think GetAbsoluteCoord() is the right way to check for
> "definite-ness".  In particular, GetAbsoluteCoord fails for percentages &
> calc(%), but percentages *do* count as definite as long as we've got a
> definite percent-basis, per
>  https://drafts.csswg.org/css-sizing-3/#definite

Good point, but the percent-basis for a grid item is the size of its
grid area, which is unknown at the time 4.4 is applied (it's that size
that we're trying to determine!).  I think "unknown" counts as "indefinite"
in this context so a percentage 'width' counts as indefinite and we should
use the "content size" instead per 4.4.

Compare for example
layout/reftests/w3c-css/submitted/values3/calc-width-block-intrinsic-1.html
which is a very similar test for blocks.  Note that the blocks with
percent-calc() are 200px, which is the min-content size, so the explicitly
specified 'width' has no effect due to the percentage. (except for the one
with an explicit "0%" but I think zero percent counts as non-percentage and
is therefore definite)

I did find a couple of other issues while investigating this though.

First, 'min-width:<percentage>' wasn't handled correctly.  We got
NS_UNCONSTRAINEDSIZE back from MinSizeContributionForAxis() and then
MinSize() simply returned that, oops.  So *for 'min-width'*
the GetAbsoluteCoord() is not enough; MinSizeContributionForAxis
needs to handle percentage explicitly (treat it as zero).

Secondly, I didn't forwarded the wrong max-width/height style
in the call to AddIntrinsicSizeOffset.

> To protect against this, could you upgrade IntrinsicISizeType
> into an enum class?

Sure.  That hides these symbols though and given our archaic 80-column
rule I didn't even try replacing every use with
nsLayoutUtils::IntrinsicISizeType::*_ISIZE
so I introduced a couple of "static const" synonyms there so that
we can continue to use nsLayoutUtils::*_ISIZE.


(In reply to Daniel Holbert [:dholbert] from comment #9)
> > +  const nsStyleCoord& styleISize =
> > +    (aFlags & MIN_INTRINSIC_ISIZE) ? styleMinISize :
> > +    (horizontalAxis ? stylePos->mWidth : stylePos->mHeight);
> 
> So, this piece of code (the only place where we react to this new flag) is a
> little confusing, because:
>  * This flag *says* "get my min intrinsic size" (a content-based thing)
>  * ...but in practice, it just makes us use "min-width" in place of "width".
> And that's not really an "intrinsic" / "content-based" thing to do.

It's only supposed to be used for min-width/height 'auto' and the enums
'min-content', 'fit-content' etc.  The spec calls them "intrinsic sizes":
https://drafts.csswg.org/css-sizing-3/#width-height-keywords

Feel free to suggest a better name.

>  (2) Assert that styleMinISize is something intrinsic-flavored 

I added an assertion.

> The code-comment here says this might be the "transferred size", but I don't
> think it is. In fact, I don't think we calculate the transferred size
> anywhere right now

Oops, I was calling IntrinsicForAxis() here at some point (which does
handle it) but that didn't work out, then I simply forgot about it.

I would prefer to add this in a follow-up bug though if you don't mind.
It doesn't affect items that doesn't have an intrinsic ratio and only
when there's an explicit auto/definite combo for those that does, like
"width:auto; height:20px".  I don't think that (somewhat rare) case
needs to block the current patch from landing.  I'll fix this soon
though while we still have this fresh in our minds.  OK?

Comment 11

[Mats Palmgren (inactive)]

Attachment: tests

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

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #10)
> > I don't think GetAbsoluteCoord() is the right way to check for
> > "definite-ness".  In particular, GetAbsoluteCoord fails for percentages &
> > calc(%), but percentages *do* count as definite as long as we've got a
> > definite percent-basis, per
> >  https://drafts.csswg.org/css-sizing-3/#definite
> 
> Good point, but the percent-basis for a grid item is the size of its
> grid area, which is unknown at the time 4.4 is applied

I think you might be focusing on cases where we're intrinsically sizing a grid based on its items -- but I'm focusing on cases where we're simply resolving "min-width:auto" on grid items during layout.  I agree that the percent basis may be unknown in the former case, but I think it is (or can be) *known* in the latter case.  Can't we have explicitly-sized grid areas? If so, I'd think a grid item with "min-width:auto; width: 25%" should be able to resolve that percentage when resolving its min-width:auto.

> Compare for example
> layout/reftests/w3c-css/submitted/values3/calc-width-block-intrinsic-1.html
> which is a very similar test for blocks.

It's similar test to intrinsic-sizing-of-the-grid, I think.  But it's not similar for simply resolving percentages to compute "min-width:auto" during layout.
 * In calc-width-block-intrinsic-1.html, the % width is *being resolved against* something which is being intrinsically sized (a float).
...whereas:
 * During "min-width:auto" resolution for a grid item, the % width is *on* something which is being intrinsically sized (a grid item).  And its %-basis (grid area) could have an explicit size, I think.

> so I introduced a couple of "static const" synonyms there so that
> we can continue to use nsLayoutUtils::*_ISIZE.

Sounds good.

> It's only supposed to be used for min-width/height 'auto' and the enums
> 'min-content', 'fit-content' etc.  The spec calls them "intrinsic sizes":
> https://drafts.csswg.org/css-sizing-3/#width-height-keywords
> 
> Feel free to suggest a better name.

(My objection wasn't about the name, but rather that we had no guarantee that the name was accurate. With the assertion that you added (addressing my suggestion labeled "(2)"), that's better now, I think.  Thanks!)

> > The code-comment here says this might be the "transferred size", but I don't
> > think it is. In fact, I don't think we calculate the transferred size
> > anywhere right now
> 
> Oops, I was calling IntrinsicForAxis() here at some point (which does
> handle it) but that didn't work out, then I simply forgot about it.
> 
> I would prefer to add this in a follow-up bug though if you don't mind.

That's fine, but please adjust the comment in MinSizeContributionForAxis to reflect current reality. Right now it says:
  // [...] "This is the "specified size" /
  // "transferred size"
...but the quantity used there isn't ever the transferred size, right now at least. So maybe drop that from the comment, and at least add an XXX comment somewhere saying that we need to consider the transferred size. (ideally w/ a bug number, if you're filing a followup for that)

Comment 13

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Can't we have explicitly-sized grid areas?

Not really.  The size of an item's grid area is derived from the size
of the tracks the item spans and the spacing between those tracks
due to gutters/alignment, so the size of grid areas can't possibly
be known during track sizing.  It would be a circular dependency.

This code is only called during track sizing, which is invoked from
intrinsic sizing and reflow of the container.  It's not invoked
during reflow of the item.  "min-width:auto; width: 25%" have its
usual meaning when setting up the nsHTMLReflowState to reflow
the item, the used value for 'min-width' is zero, and the used value
for the 'width' is 25%  of the grid area size, which is known at that
point.

Comment 14

[Daniel Holbert [:dholbert]]

Thanks, that makes sense.

(In reply to Mats Palmgren (:mats) from comment #13)
> This code is only called during track sizing, which is invoked from
> intrinsic sizing and reflow of the container.

Hmm, this part worries me though.  What if we have e.g. a single track in each dimension, with a small explicit sizing function (like say "10px"), and a single grid item in that grid area with lots of content (like say "aaaaaaaaaaaaaaaaaa")?  I'd expect that, by default, that single grid item should honor its min-width:auto and be sized to be the width of its content ("aaaaaaaaa...") rather than the size that's imposed on it by its tracks.

But if we don't consider min-width:auto after track sizing, then I'm not sure that'd happen.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: testcase to exercise "min-width:auto" on a grid item with wide content

In other words: it sounds like maybe this patch makes us consider "min-width:auto" for grid intrinsic-sizing and track-sizing purposes, but not for actual grid-item sizing purposes -- correct?  If so, I think that's a pretty key missing piece.

Here's a testcase to demonstrate this, with a grid whose tracks are all 50px wide & 50px tall. The second grid item has wide content, so (by virtue of "min-width:auto") that item should be as wide as its content. In Chrome 48 (w/ Experimental Web Platform Features enabled), it is that wide -- but in mozilla-central (with this patch attached & the grid pref tweaked), it's not.

Comment 16

[Mats Palmgren (inactive)]

Hmm, I think you're right.  We need to apply 4.4 also when setting up the nsHTMLReflowState
for reflowing the item.  Good catch!

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: testcase where min-width:auto's influence is clamped by specified sizes

...and here's a testcase where we should notice the "specified size" from the item's definite width (in one case, a percent-width resolved against an explicitly-sized grid area), and that should reduce our resolved "min-width:auto".

We render this testcase correctly right now (aside from a percent-resolution bug, for which I filed bug 1217206).  I'm posting it here because we should be sure not to break this testcase when fixing my previous testcase.

Comment 18

[Daniel Holbert [:dholbert]]

Cool, glad we're on the same page. Thanks!

Comment 19

[Daniel Holbert [:dholbert]]

And just to be clear, my concerns about percentages being definite [& resolved against definite grid area sizes] do apply to this new missing piece.  If we don't take that into account, I think we may break the second testcase that I just attached.

So I think we do really need to take definite-percent-values into account in the min-width:auto resolution code -- we'll need to pass in a percent basis, or a reflow state where the percent "width" has already been resolved, or something like that.  (And we can stifle that during track-sizing by e.g. passing in NS_UNCONSTRAINEDSIZE for the percent-basis, or a null reflow state pointer, or whatever makes sense.)

Comment 20

[Mats Palmgren (inactive)]

Attachment: Combined test, with BSize tests added

I merged your two tests and added BSize variations.

Comment 21

[Mats Palmgren (inactive)]

Attachment: screenshot

So does this look correct to you?
(This is with the percent-patches also applied.)

Comment 22

[Daniel Holbert [:dholbert]]

Yup, that looks right. Thanks!

Comment 23

[Mats Palmgren (inactive)]

Attachment: grid-item-minimal-width-in-reflow

This adds the missing piece for reflow.
(I'll fold this into the first part before landing.)

Comment 24

[Mats Palmgren (inactive)]

Attachment: tests

Added tests that covers the missing piece.  Also in vertical writing-mode.

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

Comment 25

[Mats Palmgren (inactive)]

FTR: Note that the tests here depend on percentage working correctly (bug 1163435).

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8678130 [details] [diff] [review]
grid-item-minimal-width-in-reflow

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

I think this takes care of it for the block axis, but section 4.4. "Implied Minimum Size of Grid Items" applies in both axes, IIUC.  Do we need a followup to implement this for the block axis?  (e.g. to resolve "min-height:auto" on grid items in a constrained-height grid area, in a document with english text & default "writing-mode")

::: layout/generic/nsFrame.cpp
@@ +4362,5 @@
>      result.ISize(aWM) = std::min(maxISize, result.ISize(aWM));
>    }
>  
> +  bool isGridItem = (parentFrameType == nsGkAtoms::gridContainerFrame &&
> +                     !(GetStateBits() & NS_FRAME_OUT_OF_FLOW));

Consider putting this up next to the "isFlexItem" declaration, since the two expressions are near-identical. (and it's easier to see/verify that they're the same if they're side-by-side.  And it'll be more likely that changes to one expression are also made to the other expression, if they're side-by-side).

@@ +4372,5 @@
>        nsLayoutUtils::ComputeISizeValue(aRenderingContext, this,
>          aCBSize.ISize(aWM), boxSizingAdjust.ISize(aWM), boxSizingToMarginEdgeISize,
>          minISizeCoord);
>    } else {
> +    // Treat "min-width: auto" as 0, except for flex/grid items.

The "except for flex/grid items" doesn't seem right here, since you only check "if (MOZ_UNLIKELY(isGridItem))".

@@ +4381,5 @@
>      minISize = 0;
> +    if (MOZ_UNLIKELY(isGridItem)) {
> +      // This implements "Implied Minimum Size of Grid Items".
> +      // https://drafts.csswg.org/css-grid/#min-size-auto
> +      minISize = std::min(maxISize, GetMinISize(aRenderingContext));

Seems like the logic around this section could be structured a little better, to make the flow clearer and to avoid doing an unnecessary "minISize = 0" assignment which we later stomp on.

I think this clause should be refactored into the following (starting at the existing "} else {" right above this section):

    } else if (MOZ_UNLIKELY(isGridItem)) {
      // Resolve "min-width: auto" as "Implied Minimum Size of Grid Items".
      [grid-specific code]
    } else {
      // Treat "min-width: auto" as 0.
      minISize = 0;
    }

@@ +4384,5 @@
> +      // https://drafts.csswg.org/css-grid/#min-size-auto
> +      minISize = std::min(maxISize, GetMinISize(aRenderingContext));
> +      if (stylePos->ISize(aWM).IsCoordPercentCalcUnit()) {
> +        minISize = std::min(minISize, result.ISize(aWM));
> +      }

So, despite the code-comment, this doesn't fully implement https://drafts.csswg.org/css-grid/#min-size-auto -- it's missing the "transferred size" piece, right?

Similar to end of comment 12, could you file a bug on implementing the "transferred size" piece of min-width:auto, and drop an XXX comment here indicating that that's missing?

Comment 27

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> > +    // Treat "min-width: auto" as 0, except for flex/grid items.
> 
> The "except for flex/grid items" doesn't seem right here, since you only
> check "if (MOZ_UNLIKELY(isGridItem))".

(Expanding on this slightly: we actually treat ALL min-width values as 0 here for flex items, including "min-width:auto".  That's because the flexbox layout algorithm does "min-width" clamping at a specific spot later in the flex layout algorithm, and if we apply the clamping at this point, it'll mess up the results and may make the element flex more than it should.

A simple example is e.g. a flex item like:
   <div style="flex: 1 auto; min-width: 200px">a</div>.
That flex item needs to start out at its content size (the width of "a"), and flex from there.  If it happens to flex to something >=200px, all is well. If it flexes to something smaller than that, then the flexbox layout algorithm clamps it to 200px and re-resolves other flexible lengths with this item frozen.  If we respected its "min-width" in nsFrame::ComputeSize, though, then the flex item would *start out* at 200px and grow from there, which means it'd get a head-start & would get extra space that it doesn't deserve when space is plentiful, and we'd produce the wrong layout. To prevent this, we disable "min-width" *entirely* for flex items, and only apply it at the right point during flex-container reflow.)


tl;dr: please drop 'flex' from that comment.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8675426 [details] [diff] [review]
fix

Main patch is r=me, with:
 - code-comments updated / XXX'd to reflect current reality RE "transferred size", per end of comment 12.
 - followup folded in (w/ feedback in comment 26 considered/addressed)

Comment 29

[Daniel Holbert [:dholbert]]

Comment on attachment 8678130 [details] [diff] [review]
grid-item-minimal-width-in-reflow

(marking followup r+, w/ feedback in comment 26.)

Comment 30

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> I think this takes care of it for the block axis, but section 4.4. "Implied
> Minimum Size of Grid Items" applies in both axes, IIUC.

I'm sure you mean inline axis, and yes 4.4 applies to both axis.

> Do we need a followup to implement this for the block axis?

css-sizing says it's equal to max-content size:
https://drafts.csswg.org/css-sizing/#min-content-block-size
And we're always reflowing items with unconstrained available height
currently (to avoid fragmentation), so I think that means we get
the correct results.  Did you spot a case where we don't?

> > +      // https://drafts.csswg.org/css-grid/#min-size-auto
> > +      minISize = std::min(maxISize, GetMinISize(aRenderingContext));
> > +      if (stylePos->ISize(aWM).IsCoordPercentCalcUnit()) {
> > +        minISize = std::min(minISize, result.ISize(aWM));
> > +      }
> Similar to end of comment 12, could you file a bug on implementing the
> "transferred size" piece of min-width:auto, and drop an XXX comment here
> indicating that that's missing?

Right, I'll file a bug and add a XXX comment here.
(We currently have the same layout as Chrome, but it's wrong.)

Comment 31

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #30)
> (In reply to Daniel Holbert [:dholbert] from comment #26)
> > > +      // https://drafts.csswg.org/css-grid/#min-size-auto
> > > +      minISize = std::min(maxISize, GetMinISize(aRenderingContext));
> > > +      if (stylePos->ISize(aWM).IsCoordPercentCalcUnit()) {
> > > +        minISize = std::min(minISize, result.ISize(aWM));
> > > +      }
> > Similar to end of comment 12, could you file a bug on implementing the
> > "transferred size" piece of min-width:auto, and drop an XXX comment here
> > indicating that that's missing?
> 
> Right, I'll file a bug and add a XXX comment here.
> (We currently have the same layout as Chrome, but it's wrong.)

Actually, no, I don't think nsFrame::ComputeSize should ever
be called for frames that have an intrinsic ratio because it
simply can't handle that (for anything, not just grid items).

Instead, frame subclasses that has an intrinsic ratio should override
ComputeSize and call nsLayoutUtils::ComputeSizeWithIntrinsicDimensions
instead which does handle it.  E.g. nsImageFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsImageFrame.cpp?rev=36e9b7333038#807

I added an assertion about that and it passed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeaa37f22eda

So I'll add that assertion instead here, and deal with adding
grid item specific code to ComputeSizeWithIntrinsicDimensions
in bug 1218178.

Comment 32

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #30)
> > Do we need a followup to implement this for the block axis?
> [...] we're always reflowing items with unconstrained available height
> currently (to avoid fragmentation), so I think that means we get
> the correct results.  Did you spot a case where we don't?

Ah, thanks. (No, I didn't spot a case.)

Can you make sure there's a testcase for this?  (Something like the testcase I attached here earlier, but with the grid item's content being tall instead of wide, e.g. a paragraph of text.)  It'd pass trivially right now, but it may stop doing so when we add fragmentation support (at which point we'll actually need special-case code to handle "min-height:auto" in a horizontal writing-mode).

(Maybe one of your testcases already covers this.)

(In reply to Mats Palmgren (:mats) from comment #31)
> Actually, no, I don't think nsFrame::ComputeSize should ever
> be called for frames that have an intrinsic ratio because it
> simply can't handle that (for anything, not just grid items).
[...]
> 
> So I'll add that assertion instead here, and deal with adding
> grid item specific code to ComputeSizeWithIntrinsicDimensions
> in bug 1218178.

Thanks!

Comment 33

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> Consider putting this up next to the "isFlexItem" declaration,

Sure.

> The "except for flex/grid items" doesn't seem right here, since you only
> check "if (MOZ_UNLIKELY(isGridItem))".

Fair enough, I removed the "flex".

> Seems like the logic around this section could be structured a little
> better, to make the flow clearer and to avoid doing an unnecessary "minISize
> = 0" assignment which we later stomp on.

Fixed, thanks.

Comment 34

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91269e0c3c2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7ad766c217

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/91269e0c3c2d
https://hg.mozilla.org/mozilla-central/rev/fa7ad766c217

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/91269e0c3c2d
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fa7ad766c217

Comment 37

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #36)
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/91269e0c3c2d
> https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fa7ad766c217

backed this out on request from mats from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965