New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid][cssom] Disallow repeat() syntax in grid-template-rows/columns resolved values #2427

Closed
MatsPalmgren opened this Issue Mar 9, 2018 · 11 comments

Comments

Projects
None yet
8 participants
@MatsPalmgren

MatsPalmgren commented Mar 9, 2018

https://drafts.csswg.org/css-grid/#resolved-track-list

A contiguous run of two or more tracks that have the same size and associated line names may be serialized with the repeat() notation.

I tend to think that the above sentence should be removed from the spec and that the resolved values should always use the expanded form, i.e one value per track. UAs are currently returning incompatible values: Edge use repeat() syntax and other UAs don't. It's a burden for script to have to deal with two formats. I think one-value-per-track is far easier to work with so I propose we settle on that as the mandatory format.

@mrego mrego added the css-grid-1 label Mar 21, 2018

@mrego

This comment has been minimized.

Member

mrego commented Mar 21, 2018

Yes, I agree it'd be nice to just have one option here, and avoiding repeat() seems the simplest one.
This will also help to simplify some of the WPT tests, as they need to check both options right now.

I created a very simple example to show the issue: https://wptest.center/#/nsrrq1
I guess we need @atanassov feedback about this, if Microsoft people are fine I guess the rest won't have any concern.

@fantasai fantasai added the Agenda+ label Mar 21, 2018

@FremyCompany

This comment has been minimized.

Contributor

FremyCompany commented Mar 23, 2018

If Edge is actually the only browser using repeat then I guess it's only a matter of time before we get a compat case involving that. My initial reaction would be converge on the behavior of the majority in this case. I didn't think this through though, just a quick opinion.

@tabatkins

This comment has been minimized.

Member

tabatkins commented Mar 26, 2018

Agenda+ because the editors don't care which way this goes, just tell us which way to edit it. ^_^

@atanassov

This comment has been minimized.

Collaborator

atanassov commented Mar 28, 2018

I'm not sure I quite agree with the argument here. Dealing with repeating syntax isn't new - see repeating gradients as an example. Expanding to a variable list that happened to fit a given content (in the case of auto placement) will give authors more trouble for parsing that if they deal with the short form.

My preference would be for keeping the current text and open bugs for other impls to catch up.

@css-meeting-bot

This comment has been minimized.

Member

css-meeting-bot commented Mar 28, 2018

The Working Group just discussed Disallow repeat() syntax in grid-template-rows/columns resolved values, and agreed to the following resolutions:

  • RESOLVED: serialization of the repeat() MUST use the repeat syntax
The full IRC log of that discussion <dael> Topic: Disallow repeat() syntax in grid-template-rows/columns resolved values
<dael> github: https://github.com//issues/2427
<dael> Rossen_: Summary: there was a question about hey we are seeing a couple of ways repeat() is being serialized, why can't we have one?
<dael> Rossen_: Edge supports serialization of repeat() by using repeat syntax and computed values inside.
<dael> Rossen_: FF, Webkit, and Chrome serialize as a list of computed values.
<dael> Rossen_: Question from TabAtkins and fantasai was just tell us which you want, we don't care.
<leaverou_> What if the number to repeat by is a var()?
<Rossen_> https://wptest.center/#/nsrrq1
<dael> Rossen_: Seems Igalia is pushing for simpliest which is not use repeat() syntax and they asked MS to voice our thoughts. My position is having repeat() serialize as a list is more difficult for editors and script to handle. Test case inside the issue serialization is okay, but if the repeat >2 in chome if you use 5000 you get 100 columns and it drops. FF is between
<fantasai> leaverou_, it's the used value that gets returned by gCS
<leaverou_> Oh right
<dael> Rossen_: Having to parse this many is harder. Finally, this isn't new. grandant-repeat() has the same challenge but we're not serializing a list.
<dael> Rossen_: Our opinion is preserve the repeat syntax and have others catch up.
<dael> Rossen_: Looking for other opinions.
<dael> AmeliaBR: What if author has spec columns that could be collapse?
<dael> AmeliaBR: If the author has literally specified multiple coluns without using repeat syntax but they could be collapsed using repeat would you serialize that using repleat?
<dael> Rossen_: Not really. That's like saying if we have width 10px why not serialize as calc(10px). I don't believe shorter is the goal here.
<dael> AmeliaBR: Just so long as there's a clear definition. You agrue keep as spec by author
<dael> Rossen_: Right. Roundtrip of repeat serializes as repeat, not a bunch of values.
<Rossen_> grid-template-columns: repeat(2000, 100px);
<dael> Rossen_: In the test case from Igalia you have the repeat syntax that looks okay, but if you try this ^ the serialization is crazy.
<dael> Rossen_: Any other opinions?
<dael> Rossen_: If not, can we resolve?
<dael> Rossen_: Objections to keeping repeat syntax as is currently in the spec and open bugs for other impl to do that?
<dael> frremy: Spec is a may. So there is no implementation bug to file unless it's a must.
<dael> Rossen_: A ha. good point frremy. I didn't realize this was a may.
<dael> Rossen_: Objections to specifying the repeat() syntax serialization as a must?
<AmeliaBR> +1 to interop!
<dael> TabAtkins: Yes, we should spec one way or the other as a must.
<dael> Rossen_: I'm not hearing objections.
<dael> RESOLVED: serialization of the repeat() MUST use the repeat syntax

@fantasai fantasai added the Agenda+ label Mar 29, 2018

@fantasai

This comment has been minimized.

Contributor

fantasai commented Mar 29, 2018

OK, I didn't care which way this went, but the resolution above isn't valid. You can't serialize a specified value of repeat(5, auto) using repeat syntax, because we're serializing the used value of the track size and unless all your items are the same size, repeat(5, auto) isn't going to yield five tracks with identical pixel sizes that can be collaped as repeat(5, 25px) or whatever.

We can have getComputedStyle use repeat() syntax according to some set of collapsing rules, but we can't make it mirror the structure of the specified style. Keeping in mind that we are serializing the used value here, the options here are:

  • Don't use repeat() in serialization. Easy, not nice for scaling up to giant grids.
  • Use repeat() in serialization to collapse any sequence of identically-sized columns. Simplest option for using repeat().
  • Use repeat() in serialization to collapse any sequence of identically-sized columns only if they happen to be sourced from the same specified repeat(). NOTE: A specified repeat() can be split into a sequence of multiple serialized repeat()s and/or externally-listed sizes if the repeated track list contains sizes that are not <length-percentage>. E.g. 20px repeat(6, auto) would serialize out as 20px repeat(2, 20px) 35px repeat(3, 19px) if that is what the track sizes end up as.
  • Some other option which is less aggressive than the previous, e.g. only collapse columns if they're in a repeat() that only contains <length-percentage> track listings.
@javifernandez

This comment has been minimized.

Contributor

javifernandez commented Mar 30, 2018

@mrego

This comment has been minimized.

Member

mrego commented Apr 2, 2018

  • Don't use repeat() in serialization. Easy, not nice for scaling up to giant grids.
  • Use repeat() in serialization to collapse any sequence of identically-sized columns. Simplest option for using repeat().

I'd go for either one of these 2 options, I don't see the benefit of doing something more complex than that.

@FremyCompany

This comment has been minimized.

Contributor

FremyCompany commented Apr 4, 2018

I would just note that in addition to the size difference, you also have to take care of the line names being the same for the duration of the repetition. Still siding on the don't use repeat side, because 3 browsers are already doing that.

@css-meeting-bot

This comment has been minimized.

Member

css-meeting-bot commented Apr 4, 2018

The Working Group just discussed Disallow repeat() syntax in grid-template-rows/columns resolved values, and agreed to the following resolutions:

  • RESOLVED: Specify the current behavior in all the browsers except Edge. Just don't use repeat() in grid serialization
The full IRC log of that discussion <dael_> Topic: Disallow repeat() syntax in grid-template-rows/columns resolved values
<dael_> github: https://github.com//issues/2427
<fantasai> ScribeNick: fantasai
<fantasai> TabAtkins: So we resolved to serialize out the repeat()s that were specified,
<fantasai> TabAtkins: but actually you can't do that as fantasai points out in https://github.com//issues/2427#issuecomment-377357237
<fantasai> TabAtkins: So we need to choose some different option
<fantasai> TabAtkins: There are a few possible options
<fantasai> TabAtkins: First is to reverse previous resolution: don't use repeat() in serialization of gCS
<fantasai> TabAtkins: This is very simple and straightforward
<fantasai> TabAtkins: downside is it can potentially produce very long values for grid-template*
<fantasai> TabAtkins: if you do something like grid-template-rows: repeat(10000, 1px)
<fantasai> TabAtkins: Second option is to compress any adjacent tracks that have the same track size (and line names)
<fantasai> astearns: Clarification on 2nd option, is that only when specified value was repeat() or anytime?
<fantasai> TabAtkins: Anytime
<fantasai> TabAtkins: more complicated variants are to track which tracks came from a repeat(), and then collapse them if possible
<fantasai> (These options are summarized in https://github.com//issues/2427#issuecomment-377357237 )
<fantasai> TabAtkins: The Igalia folks don't like tracking which things were in repeat() originally
<fantasai> TabAtkins: seems like too much complexity for little gain
<fantasai> TabAtkins: I think the best thing to do is to not serialize repeat(). The only issue is a blow-out of string sizes if you have long ones
<fantasai> TabAtkins: but there is a cap on the number of tracks so it won't be too crazy
<fantasai> florian: I think I'd go with non-collapsing things
<fantasai> florian: Seems to me that there are a lot of corner cases
<dael> florian: I think I'd go with non-collapsing things as well. As i'm looking through there seems to be lots of corner cases. I'm not sure they'll all be fine.
<dael> florian: Given there's limited value le's skip the pain.
<fantasai> ScribeNick: dael_
<fantasai> ScribeNick: dae
<fantasai> ScribeNick: dael
<dael> florian: Given that you get things like calc that are somewhat the same. I'd rather just not go there.
<dael> astearns: Only edge rep is melanierichards . Since Edge is the browser that retains repeat() and thought we should. I'd like to get an Edge opinion.
<dael> fantasai: frremy says still siding on the don't use repeat() sidde
<dael> TabAtkins: Rossen wants repeat() and frremt would rather not.
<dael> astearns: Since frremy commented on the issue my Edge concern is satisfied.
<dael> astearns: Other comments on if we should deal with repeats or throw them out?
<dael> astearns: Prop: Specify the behavior in all the browsers except Edge. just don't use repeat() in grid serialization
<dael> RESOLVED: Specify the current behavior in all the browsers except Edge. Just don't use repeat() in grid serialization
@mrego

This comment has been minimized.

Member

mrego commented Apr 23, 2018

@atanassov please confirm you're fine with this as I'd like to update the tests accordingly.

fergald added a commit to fergald/csswg-drafts that referenced this issue May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment