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
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: testcase)
Attachments
(3 files)
835 bytes,
text/html
|
Details | |
3.97 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
14.21 KB,
patch
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Attachment #8800495 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbb680fc8a26004918b0ddb06cae7e070e4c76c5&exclusion_profile=false
Comment 3•8 years ago
|
||
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•8 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)
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/682ba994390b https://hg.mozilla.org/mozilla-central/rev/dff23ddb74cf
Status: NEW → RESOLVED
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.
Description
•