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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug, {testcase})

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
Posted file 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.
(Assignee)

Comment 1

3 years ago
Posted patch fixSplinter Review
Attachment #8800495 - Flags: review?(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.
Attachment #8800495 - Flags: review?(dholbert) → review+
(Assignee)

Comment 4

3 years ago
(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

3 years ago
Pushed by mpalmgren@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/682ba994390b
https://hg.mozilla.org/mozilla-central/rev/dff23ddb74cf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.