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

RESOLVED FIXED in Firefox 52

Status

()

Core
Layout
RESOLVED FIXED
a year ago
3 months ago

People

(Reporter: Sergio Villar, Assigned: mats)

Tracking

({regression, testcase})

Trunk
mozilla53
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53 fixed)

Details

Attachments

(8 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8806786 [details]
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.

Comment 1

a year ago
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
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
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?
(Assignee)

Updated

a year ago
Blocks: 1217086
Flags: needinfo?(mats)
status-firefox53: --- → affected
(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)
(Assignee)

Comment 5

a year ago
Created attachment 8816768 [details] [diff] [review]
part 1 - [css-grid] Don't include percentage tracks in the back-computed intrinsic size equation.

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)

Comment 6

a year ago
Created attachment 8816769 [details] [diff] [review]
part 2 - [css-grid] Don't re-resolve percentage track sizes since there is no need.
Assignee: nobody → mats
Attachment #8816769 - Flags: review?(dholbert)
(Assignee)

Comment 7

a year ago
Created attachment 8816770 [details] [diff] [review]
part 3 - [css-grid] Remove unused eIndefinitePercentMinSizing bit.
Attachment #8816770 - Flags: review?(dholbert)
(Assignee)

Comment 8

a year ago
Created attachment 8816771 [details] [diff] [review]
part 4 - [css-grid] Don't include percentage tracks in the repeat track calculation.
Attachment #8816771 - Flags: review?(dholbert)
(Assignee)

Comment 9

a year ago
Created attachment 8816772 [details] [diff] [review]
part 5 - [css-grid] Tweak reftests.
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+
(Reporter)

Comment 11

a year ago
(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."
(Assignee)

Comment 12

a year ago
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.

Comment 13

a year ago
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
tracking-firefox52: --- → +
(Assignee)

Comment 16

a year ago
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-
(Assignee)

Comment 20

3 months ago
Created attachment 8919967 [details]
Another testcase
(Assignee)

Comment 21

3 months ago
Created attachment 8919968 [details]
Screenshot of testcase above
(Assignee)

Comment 22

3 months ago
Created attachment 8919970 [details]
Another testcase
Attachment #8919967 - Attachment is obsolete: true
Attachment #8919968 - Attachment is obsolete: true
(Assignee)

Comment 23

3 months ago
Created attachment 8919971 [details]
Screenshot of the above testcase
You need to log in before you can comment on or make changes to this bug.