1309407 - [css-grid] Stretching flexible tracks with an indefinite containing block length should respect the min/max size

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: Testcase

https://drafts.csswg.org/css-grid/#algo-flex-tracks
"If the free space is an indefinite length: [...]
If using this flex fraction would cause the grid to be smaller than the grid container’s min-width/height (or larger than the grid container’s max-width/height), then redo this step, treating the free space as definite [...]"

I don't see that we have any code that even tries to do that so I'm
guessing this is a spec change that happened after we implemented it.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Attachment: reftests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbb680fc8a26004918b0ddb06cae7e070e4c76c5&exclusion_profile=false

Comment 3

[Daniel Holbert [:dholbert]]

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

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

Commit message nit:
> Bug 1309407 - [css-grid] Apply min/max-sizes after stretching flex items with an indefinite CB size and re-run the algo with a definite size if the grid size changed. r=dholbert

"flex item" has a specific definition, so you shouldn't use that term (for something other than that definition) here.  Perhaps better: "flex tracks" or "flexible lengths", or something like that?

::: layout/generic/nsGridContainerFrame.cpp
@@ +4764,5 @@
> +    float fr = FindUsedFlexFraction(aState, aGridItems, flexTracks,
> +                                    aFunctions, aAvailableSize);
> +    if (fr != 0.0f) {
> +      bool applyMinMax = (minSize != 0 || maxSize != NS_UNCONSTRAINEDSIZE) &&
> +                         aAvailableSize == NS_AUTOHEIGHT;

Nit: s/NS_AUTOHEIGHT/NS_UNCONSTRAINEDSIZE/ (though they're both nscoord_MAX, so technically it doesn't matter)

This seems like it would be more correct, because:
(1)  IIRC we generally use NS_AUTOHEIGHT as computed "height:auto" values, vs NS_UNCONSTRAINEDSIZE for unconstrained available-space. And this seems to be more the latter. (though I suppose it comes from the content-box size, up a few levels, so *shrug*.)

(2) This function's caller uses NS_UNCONSTRAINEDSIZE when checking this very same quantity [called "freeSpace" at that point, though it gets passed in as aAvailableSize arg to this function]:
https://dxr.mozilla.org/mozilla-central/rev/dc89484d4b45abf442162e5ea2dd46f9de40197d/layout/generic/nsGridContainerFrame.cpp#3876
So, we should be consistent about which sentinel name we use for this quantity.  Alternately, if you strongly feel that NS_AUTOHEIGHT is the right sentinel to use here, perhaps we should be using that in the caller as well?

@@ +4791,5 @@
> +        newSize += sumOfGridGaps;
> +        if (newSize > maxSize) {
> +          aAvailableSize = maxSize;
> +        }
> +        if (newSize < minSize) {

If you like, this could be "else if" (since these min/max values come from our ReflowInput, which enforces that min < max. So it's not possible for newSize to violate both thresholds.

@@ +4794,5 @@
> +        }
> +        if (newSize < minSize) {
> +          aAvailableSize = minSize;
> +        }
> +        if (aAvailableSize != NS_AUTOHEIGHT) {

As noted above, this should probably be NS_UNCONSTRAINEDSIZE, for consistency.

Comment 4

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> "flex item" has a specific definition

He, right. :-)  I changed it to "<flex> grid items".
(and fixed the other nits as suggested)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/682ba994390b
[css-grid] Apply min/max-sizes after stretching <flex> grid items with an indefinite CB size and re-run the algo with a definite size if the grid size changed.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/dff23ddb74cf
[css-grid] Reftest for track flex max-sizing with min/max-sizes.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/682ba994390b
https://hg.mozilla.org/mozilla-central/rev/dff23ddb74cf