stylo: Decide how to store and serialize specified value of repeat function in grid

RESOLVED FIXED in Firefox 57

Status

()

P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: canaltinova, Assigned: ferjm)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In gecko, repeat function is not preserved if it's `repeat(<number>, ...)` in specified value. It's becoming <number> times repeated values. We are losing specified value information here and this requires a lot allocation on parsing. Also blink and webkit does what gecko does but there is an open issue about this in blink[1] that says this is wrong. Currently, stylo implementation is the same with gecko.
We need to make sure which one is true.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=716114
In particular, this can make us allocate a huge amount of memory when using repeat(<somebignumber>), which is quite wasteful IMO.

Nazim, did you end up filing the chromium bug for they not roundtripping the serialization?
Blocks: 1293767
Flags: needinfo?(canaltinova)
I didn't because the bug I mentioned in comment #0 will make it obsolete.
But well, there is nothing wrong with one more additional bug :) Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=747074
Flags: needinfo?(canaltinova)
Priority: -- → P2
(Assignee)

Updated

a year ago
Assignee: nobody → ferjmoreno
Created attachment 8903148 [details] [diff] [review]
Test changes
Attachment #8903148 - Flags: review?(emilio)
Comment on attachment 8903148 [details] [diff] [review]
Test changes

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

Looks ok to me, but given we're changing this, I'd rather ask mats what's what he thinks it should happen here.

::: layout/style/test/test_grid_shorthand_serialization.html
@@ +69,5 @@
>      },
>      {
>          gridTemplateAreas: "\"a\"",
>          gridTemplateRows: "[foo] repeat(1, 20px) [bar]",
> +        shorthand: isStylo ? "[foo] \"a\" repeat(1, 20px) [bar]" : "[foo] \"a\" 20px [bar]",

Maybe we should special-case repeat(1)? Though usually in specified values we try not to, so this looks ok
Attachment #8903148 - Flags: review?(mats)
Attachment #8903148 - Flags: review?(emilio)
Attachment #8903148 - Flags: feedback+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> In particular, this can make us allocate a huge amount of memory when using
> repeat(<somebignumber>), which is quite wasteful IMO.

Meh.  Relative the size of a typical web page it's a small amount.
Also, repeat(<somebignumber>) is firmly in very-rare-edge-case-land.


(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> Looks ok to me, but given we're changing this, I'd rather ask mats what's
> what he thinks it should happen here.

Yes, we should definitely fix this since it's an error to expand repeat's
for /specified/ values (not for computed/resolved values though).

> > +        shorthand: isStylo ? "[foo] \"a\" repeat(1, 20px) [bar]" : "[foo] \"a\" 20px [bar]",
> 
> Maybe we should special-case repeat(1)? Though usually in specified values
> we try not to, so this looks ok

Serialization of specified values should use repeat(1) when that's specified.
We also shouldn't introduce repeat's when they weren't specified, i.e.
"20px 20px" should serialize to "20px 20px", not "repeat(2, 20px)".

For computed/resolved values we should never use 'repeat', even though
the spec say we "may" do so.  These values are what script use and it's
just annoying to have to deal with repeat's.  A flat list of px values,
one for each track, is much easier to digest.
Comment on attachment 8903148 [details] [diff] [review]
Test changes

It looks like the grammar doesn't allow 'repeat' in this position:
https://drafts.csswg.org/css-grid/#propdef-grid-template
it says <track-size>? which excludes "repeat(1, 20px)", so we must
use a plain "20px" here, or give up and return "".

We should probably make an exception in this particular place
from the general rule of preserving repeat(1) for specified values,
since the values are in essence representable as a shorthand.

BTW, does stylo accept the following declaration as valid?
grid-template: [foo] "a" repeat(1,20px) [bar];
(it shouldn't)
Attachment #8903148 - Flags: review?(mats) → review-
Created attachment 8904958 [details] [diff] [review]
Test changes

Thank you for your feedback Mats.

I have changed the implementation to serialize to "" for the grid-template case as suggested.
Attachment #8903148 - Attachment is obsolete: true
Attachment #8904958 - Flags: review?(mats)
(In reply to Mats Palmgren (:mats) from comment #7) 
> BTW, does stylo accept the following declaration as valid?
> grid-template: [foo] "a" repeat(1,20px) [bar];
> (it shouldn't)

No, stylo does not accept this declaration as valid.
Serializing to "" is OK, but I'd slightly prefer if repeat(1,20px)
in the longhand became 20px in the shorthand since the value *is*
valid in that form.  I don't feel that strongly about it though.

What do other UAs do in this situation?
Flags: needinfo?(ferjmoreno)
Chrome serializes to "". Safari to 20px. I can't tell about IE or Edge.
Flags: needinfo?(ferjmoreno)
Comment on attachment 8904958 [details] [diff] [review]
Test changes

OK, thanks.
Attachment #8904958 - Flags: review?(mats) → review+

Comment 13

a year ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8be3c451ff86
stylo: store specified value of grid layout repeat() function. Tests. r=mats

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8be3c451ff86
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.