Closed Bug 1309407 Opened 8 years ago Closed 8 years ago

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


(Core :: Layout, defect)

Not set



Tracking Status
firefox52 --- fixed


(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)


(Blocks 1 open bug)


(Keywords: testcase)


(3 files)

Attached file Testcase
"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.
Attached patch fixSplinter Review
Attachment #8800495 - Flags: review?(dholbert)
Comment on attachment 8800495 [details] [diff] [review]

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]:
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+
(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)
Pushed by
[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
[css-grid] Reftest for track flex max-sizing with min/max-sizes.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.