Closed Bug 1314664 Opened 3 years ago Closed 3 years ago

[css-grid] Very weird percentage resolution for rows with indefinite height grids

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: svillar, Assigned: mats)

References

Details

(Keywords: regression, testcase)

Attachments

(8 files, 2 obsolete files)

Attached file test.html
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/603.1 (KHTML, like Gecko)  Version/10.0 Safari/603.1 Debian/buildd-unstable (3.22.1-1) Epiphany/3.22.1

Steps to reproduce:

Check the attached example. It contains two grids that should behave identically. The first one is using a percentage for rows while the second one is directly using auto. Percentage values against indefinite sizes should be resolved to auto.


Actual results:

Firefox computes an absurdly tall grid (8947850px 8947850px according to the inspector). 


Expected results:

Both grids should be 20px tall made of two 10px rows.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b4965bb11c0649ce8e0757c708b6c13d5f1e854&tochange=aff267eaa3b4ab8f25bf7b75acdfdffd46d863d2
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
Mats, can you please take a look at this recent regression?
Flags: needinfo?(mats)
Local builds confirm this is a regression from bug 1302541.
Blocks: 1302541
Flags: in-testsuite?
Blocks: 1217086
Flags: needinfo?(mats)
(Restoring ni from comment 2; I think it was cleared accidentally.)

(Note that this is marked as blocking bug 1217086 (the "ship grid" bug), whose pref-enabling-patch has now landed on Nightly & will soon land on Aurora.  So presumably we'd like to fix & uplift this soon, too.)
Flags: needinfo?(mats)
Sorry for the delay.  So, this removes percentage track sizes from the percentage sum when back-computing the grid container intrinsic size.  As the reporter points out, it leads to unexpected results.  This makes grid compatible to what we do for blocks, where if you have e.g. "width:50%; padding:20%;" the percentage width is treated as 'auto' and only the percentage padding is included, this makes sense since we include the min-content size as the coord-sum in the formula and it would be weird to include *both* that and the 'width' percentage.  So I think that's what leads to weird results here to - we include track sizes (where children contributes its sizes) and the track percentages too.  We should just include tracks as the "min-content size" which contributes to the coord-sum instead.
Flags: needinfo?(mats)
Attachment #8816768 - Flags: review?(dholbert)
Assignee: nobody → mats
Attachment #8816769 - Flags: review?(dholbert)
Comment on attachment 8816768 [details] [diff] [review]
part 1 - [css-grid] Don't include percentage tracks in the back-computed intrinsic size equation.

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

This seems fine.

(Is there any spec text about how this is supposed to work? i.e. about how percent-valued tracks/gaps influence intrinsic sizing?)
Attachment #8816768 - Flags: review?(dholbert) → review+
Attachment #8816769 - Flags: review?(dholbert) → review+
Attachment #8816770 - Flags: review?(dholbert) → review+
Attachment #8816771 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 8816768 [details] [diff] [review]
> part 1 - [css-grid] Don't include percentage tracks in the back-computed
> intrinsic size equation.
> 
> Review of attachment 8816768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine.
> 
> (Is there any spec text about how this is supposed to work? i.e. about how
> percent-valued tracks/gaps influence intrinsic sizing?)

There are two relevant places in the specs:
1) "If the size of the grid container depends on the size of its tracks, then the <percentage> must be treated as auto." 
2) 
"The max-content size of a grid container is the sum of the grid container’s track sizes (including gutters) in the appropriate axis, when the grid is sized under a max-content constraint.

The min-content size of a grid container is the sum of the grid container’s track sizes (including gutters) in the appropriate axis, when the grid is sized under a min-content constraint."
fantasai have agreed that backcomputing percentages is allowed in Grid
layout, but there's nothing explicit in the spec yet.

There's also an issue open on how to compute percentages for intrinsic sizing
in general:  https://github.com/w3c/csswg-drafts/issues/347
I am very strongly in favor of what fantasai propose: to backcompute
percentages like Gecko does, because 1. the UA should honor what
the author specifies, 2. backcomputing it like we do avoids overflow.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/186f083d8354
part 1 - [css-grid] Don't include percentage tracks in the back-computed intrinsic size equation.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3493452eba
part 2 - [css-grid] Don't re-resolve percentage track sizes since there is no need.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/5425d9c7e4d4
part 3 - [css-grid] Remove unused eIndefinitePercentMinSizing bit.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4fa411c43f3
part 4 - [css-grid] Don't include percentage tracks in the repeat track calculation.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5e0f0cfbef
part 5 - [css-grid] Tweak reftests.
tracking this css-grid issue for 52
Comment on attachment 8816772 [details] [diff] [review]
part 5 - [css-grid] Tweak reftests.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: weird grid layout in some cases involving percentage sizes
[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?]:only affects Grid layout
[String changes made/needed]:none

This is part of the CSS Grid release, please uplift to Aurora 52, thanks.
Attachment #8816772 - Flags: approval-mozilla-aurora?
Comment on attachment 8816772 [details] [diff] [review]
part 5 - [css-grid] Tweak reftests.

css-grid updates for aurora52 (only flagging this patch, but approval goes for all 5 patches from this bug)
Attachment #8816772 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting qe-verify- based on Mats' assessment on manual testing needs (see Comment 16) and the fact that this fix has automated coverage.
Flags: qe-verify-
Attached file Another testcase (obsolete) —
Attached image Screenshot of testcase above (obsolete) —
Attached file Another testcase
Attachment #8919967 - Attachment is obsolete: true
Attachment #8919968 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.