The computed value of grid-template-{columns|rows} should not expand repeat()
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | affected |
People
(Reporter: boris, Assigned: emilio)
References
(Blocks 1 open bug, )
Details
Attachments
(2 files, 1 obsolete file)
Based on the spec [1][2], it seems the repeat() notation is resolved in resolved values, instead of computed track list. The current Gecko expands repeat() (with numeric repeat count) in ToComputedValue [3]. This is incorrect, and will have impact on interpolation between repeat()s.
[1] https://drafts.csswg.org/css-grid/#computed-track-list
[2] https://drafts.csswg.org/css-grid/#resolved-track-list
[3] https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/servo/components/style/values/specified/grid.rs#327
BTW, there is a discussion about interpolating repeat(): https://github.com/w3c/csswg-drafts/issues/3503
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
•
|
||
The spec issue, https://github.com/w3c/csswg-drafts/issues/3503, is resolved by a more restrictive rule for now:
Define repeat interpolation using the more restrictive rule in https://github.com//issues/3503#issuecomment-453674839
It could be loosen, but for now we should follow the restrictive rule: require that the first arg is a number, and identical between start/end; different values, or keyword values, both go discrete
That means we have to fix this bug to match the updated spec and fix the wpts [1]/[2], which uses a loosen way if the first arg is a number (because Gecko uses an incorrect computed value of repeat()).
[1] https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/testing/web-platform/tests/css/css-grid/animation/grid-template-columns-interpolation.html#133-144
[2] https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/testing/web-platform/tests/css/css-grid/animation/grid-template-rows-interpolation.html#133-144
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 2•6 years ago
|
||
It seems the biggest problem is: nsStyleCoord
doesn't support general function type (i.e. only calc()
is acceptable), and we use m{Max|Min}TrackSizingFunctions
to represent all the different types of <track-size>, but not <track-repeat> (i.e. repeat()
notation). In order to make sure we keep repeat()
notation in the computed value, we have to
- Do not expand
repeat()
inToComputedValue
- Redesign
nsStyleGridTemplate
so it could represent <track-size>|<track-repeat>_ - Update the conversion code in gekco.mako.rs, and then update all the layout code to use the new type. (And maybe need to resolve it by adding an utility function)
Looks like we have a lot of things to do.
Reporter | ||
Comment 3•6 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #1)
fix the wpts [1]/[2], which uses a loosen way if the first arg is a number (because Gecko uses an incorrect computed value of repeat()).
[1] https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/testing/web-platform/tests/css/css-grid/animation/grid-template-columns-interpolation.html#133-144
[2] https://searchfox.org/mozilla-central/rev/39265dd58298c40bed029617ad408bf806cce761/testing/web-platform/tests/css/css-grid/animation/grid-template-rows-interpolation.html#133-144
Updated spec: https://drafts.csswg.org/css-grid/#repeat-interpolation
Assignee | ||
Comment 4•5 years ago
|
||
I have a wip patch that removes nsStyleGridTemplate and does this while at it. Still needs a bunch of work, but if you're not actively working on this mind if I take it?
Reporter | ||
Comment 5•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)
I have a wip patch that removes nsStyleGridTemplate and does this while at it. Still needs a bunch of work, but if you're not actively working on this mind if I take it?
Sure. Please feel free to take this. (It’s better to fix this together with using cbindgen.)
Assignee | ||
Comment 7•5 years ago
|
||
While we're at it we may as well do this.
Depends on D36349
Comment 8•5 years ago
|
||
Comment on attachment 9074825 [details]
Bug 1519958 - Drive by, remove unused CSS keywords. r=#style,boris
Revision D36350 was moved to bug 1562269. Setting attachment 9074825 [details] to obsolete.
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/3669b147ccc6 Drive by, remove unused CSS keywords. r=boris
Reporter | ||
Updated•5 years ago
|
Comment 11•5 years ago
|
||
bugherder |
Assignee | ||
Comment 12•5 years ago
|
||
I'm really sorry for the size of the patch. I tried to do this in two steps
but it was a lot of work and pretty ugly.
This patch makes us use cbindgen for grid-template-{rows,columns}, in order to:
-
Make us preserve repeat() at computed-value time. This is per spec since
interpolation needs to know about repeat(). Except for subgrid, which did the
repeat expansion at parse-time and was a bit more annoying (plus it doesn't
really animate yet so we don't need it to comply with the spec). -
Tweaks the WPT tests for interpolation to adopt the resolution at:
https://github.com/w3c/csswg-drafts/issues/3503.
Trade-off here, as this patch stands, is that this change makes us use less
long-living memory, since we expand repeat() during layout, but at the cost of a
bit of CPU time during layout (conditional on the property applying though,
which wasn't the case before). It should be very easy to store a cached version
of the template, should this be too hot (I expect it isn't), or to change the
representation in other ways to optimize grid layout code if it's worth it.
Another trade-off: I've used SmallPointerArray to handle line-name merging,
pointing to the individual arrays in the style data, rather than actually
heap-allocating the merged lists. This would also be pretty easy to change
should we measure and see that it's not worth it.
This patch also opens the gate to potentially improving memory usage in some
other ways, by reference-counting line-name lists for example, though I don't
have data that suggests it is worth it.
In general, this patch makes much easier to tweak the internal representation of
the grid style data structures. Overall, I think it's a win, the amount of magic
going on in that mako code was a bit huge; it took a bit to wrap my head around
it.
This patch comments out the style struct size assertions. They will be
uncommented in a follow-up patch which contains some improvements for this type,
which are worth getting reviewed separately.
Also, this patch doesn't remove as much code as I would've hoped for because of
I tried not to change most of the dom/grid code for inspector, but I think a
fair bit of the nsGridContainerFrame.cpp code that collects information for it
can be simplified / de-copy-pasted to some extent. But that was a pre-existing
problem and this patch is already quite massive.
Assignee | ||
Comment 13•5 years ago
|
||
This re-enables the assertion which was disabled on the previous patch by doing
a bit of boxing around.
Depends on D36598
Assignee | ||
Updated•5 years ago
|
Comment 14•5 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/a1eb75e785de Refactor grid types to preserve repeat() at computed value time and use cbindgen. r=mats,boris https://hg.mozilla.org/integration/autoland/rev/786ba5f970ac Improve stack size of grid templates and re-enable style struct size assertions disabled in the previous patch. r=boris
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/18283 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 17•5 years ago
|
||
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/6e82b588cd77 followup: Fix bustage in 32-bit builds and rust unit tests. r=me
Comment 18•5 years ago
|
||
bugherder |
Upstream PR merged
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Description
•