Closed Bug 1346699 Opened 3 years ago Closed 3 years ago

[css-grid] FR Unit in grid-template-row not filling viewport

Categories

(Core :: Layout, defect)

52 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- disabled
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 + fixed
firefox54 + fixed
firefox55 --- fixed

People

(Reporter: chrisw, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: compat, testcase, Whiteboard: [parity-chrome])

Attachments

(6 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.98 Safari/537.36

Steps to reproduce:

1. Create a div [a] with display grid
2. Create a child also with display grid [b]
3. Add another div [c] with some content in it, a heading, and a paragraph with dummy copy
3. On the parent [a] give the grid grid-template-rows: 1fr;
4. On the child [b] add align-items: center


Actual results:

The the div [b] never stretched to the bottom of the viewport, so alignment appeared to be broken (when the issue was actually the FR unit not applying)


Expected results:

The element [b] to stretch the viewport like it does in Chrome 57 and Safari TP.
See demo in different browsers here:
http://codepen.io/chriswrightdesign/pen/ea81b46110728f5b010234e1bdb75691?editors=1100
Blocks: css-grid
Component: Untriaged → Layout
Product: Firefox → Core
Summary: CSS Grid - FR Unit in grid-template-row not filling viewport → [css-grid] FR Unit in grid-template-row not filling viewport
Flags: needinfo?(mats)
Attached file Reduced testcase
Hmm, I'll have to check what the spec says about this case...
Assignee: nobody → mats
Flags: needinfo?(mats)
The spec sections that apply are:
https://drafts.csswg.org/css-grid/#fr-unit
"When the available space is infinite (which happens when the grid container’s width or height is indefinite), ..."
which dictates we should use "If the free space is an indefinite length:" section here:
https://drafts.csswg.org/css-grid/#algo-flex-tracks
Which ends with:
"
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 and the available grid space as equal to the grid container’s content box size when it’s sized to its min-width/height (max-width/height).
"

We implement this clause here (from bug 1309407):
http://searchfox.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#4780
When I single-step through here it seems we never make origSizes.isSome()
true though, so we never execute the step that applies min/max.
In the for-loop that precedes it, we have one flex-track and we have:
applyMinMax = true
fr = 120
flexFactor = 1
flexLength = 120
base = @0x7ffd6e68a770: 120
so we don't execute the "if (flexLength > base)" code that would emplace it.

Hmm, I don't really recall why I did that... maybe I thought that if no mBase
was growing then applying min/max wouldn't matter?

Anyway, it doesn't seem like a valid optimization.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
There are actually two separate logical errors in this method,
so I'll fix them in separate patches for easier reviewing.

The first part is that "origSizes.isSome()" is simply a bogus
requirement for applying min/max-sizes here.  I'm still keeping
the optimization of not needlessly copying the mSizes array
(as originally intended) since it's a quite common case.
Attachment #8847278 - Flags: review?(dholbert)
The second bug is that min/max-sizes were only applied under
the "if (fr != 0.0f)" block.  This is bogus since the calculated
'fr' value depends on 'aAvailableSize' which might change by
applying min/max-sizes and thus 'fr' could become non-zero in
the second round.  So this patch just moves "applyMinMax" block
out one level.
Attachment #8847283 - Flags: review?(dholbert)
Attached patch part 2 (wdiff)Splinter Review
This adds reftests that cover both issues mentioned above.
Given that this is compat issue with Chrome we should probably uplift this
straight to beta (v53).  It's a low-risk change that clearly only affects
CSS Grid layout.
Tracking this compat issue for 53/54.
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

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

r=me
Attachment #8847278 - Flags: review?(dholbert) → review+
Comment on attachment 8847283 [details] [diff] [review]
part 2 - [css-grid] Don't require 'fr' to be non-zero to apply min/max-size

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

r=me

(Thanks for the wdiff, too.)
Attachment #8847283 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49afac936ad
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/73be2cb53669
part 2 - [css-grid] Don't require 'fr' to be non-zero to apply min/max-size.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/534086689e77
part 3 - [css-grid] Additional reftests for min/max-sizes affecting flexible track sizing.
Flags: in-testsuite+
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: possible rendering errors in CSS Grid layout; it would be nice to take this for compat with Chrome
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]: minor changes in a single function in nsGridContainerFrame.cpp, so it can't cause regressions in anything else than grid layout
[String changes made/needed]:none
Attachment #8847278 - Flags: approval-mozilla-beta?
Attachment #8847278 - Flags: approval-mozilla-aurora?
(the uplift request is for all parts in case that wasn't clear)
Comment on attachment 8847278 [details] [diff] [review]
part 1 - [css-grid] Don't require that some (flexible) track size changed to apply min/max-size

Fix a CSS Grid layout issue. Aurora54+ and Beta53+.
Attachment #8847278 - Flags: approval-mozilla-beta?
Attachment #8847278 - Flags: approval-mozilla-beta+
Attachment #8847278 - Flags: approval-mozilla-aurora?
Attachment #8847278 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1350855
You need to log in before you can comment on or make changes to this bug.