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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Sergio Villar]

Attachment: 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

[YF (Yang)]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3b4965bb11c0649ce8e0757c708b6c13d5f1e854&tochange=aff267eaa3b4ab8f25bf7b75acdfdffd46d863d2

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Mats, can you please take a look at this recent regression?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Local builds confirm this is a regression from bug 1302541.

Comment 4

[Daniel Holbert [:dholbert]]

(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.)

Comment 5

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2 - [css-grid] Don't re-resolve percentage track sizes since there is no need.

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 3 - [css-grid] Remove unused eIndefinitePercentMinSizing bit.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 4 - [css-grid] Don't include percentage tracks in the repeat track calculation.

Comment 9

[Mats Palmgren (inactive)]

Attachment: part 5 - [css-grid] Tweak reftests.

Comment 10

[Daniel Holbert [: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?)

Comment 11

[Sergio Villar]

(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."

Comment 12

[Mats Palmgren (inactive)]

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

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/186f083d8354
https://hg.mozilla.org/mozilla-central/rev/5b3493452eba
https://hg.mozilla.org/mozilla-central/rev/5425d9c7e4d4
https://hg.mozilla.org/mozilla-central/rev/c4fa411c43f3
https://hg.mozilla.org/mozilla-central/rev/fe5e0f0cfbef

Comment 15

[Julien Cristau [:jcristau] (back April 22)]

tracking this css-grid issue for 52

Comment 16

[Mats Palmgren (inactive)]

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.

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

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)

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/346b243ed5db
https://hg.mozilla.org/releases/mozilla-aurora/rev/2cae3655368c
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d3901293a70
https://hg.mozilla.org/releases/mozilla-aurora/rev/3043bcf55783
https://hg.mozilla.org/releases/mozilla-aurora/rev/a80f31823e23

Comment 19

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Mats' assessment on manual testing needs (see Comment 16) and the fact that this fix has automated coverage.

Comment 20

[Mats Palmgren (inactive)]

Attachment: Another testcase

Comment 21

[Mats Palmgren (inactive)]

Attachment: Screenshot of testcase above

Comment 22

[Mats Palmgren (inactive)]

Attachment: Another testcase

Comment 23

[Mats Palmgren (inactive)]

Attachment: Screenshot of the above testcase