Skip to content
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] Resolved values of grid-template-rows/columns don't round-trip #4475

Open
Loirooriol opened this issue Oct 31, 2019 · 26 comments
Open

Comments

@Loirooriol
Copy link
Contributor

According to @fantasai in #4444 (comment),

Roundtripping getComputedStyle() values is probably the most fundamental rule about defining them.

However, grid-template-rows and grid-template-columns don't always round-trip. That's because they set explicit tracks, which start just after the 1st grid line. However, when serialized, they also include implicit tracks, which may exist before the 1st grid line.

gridContainer.style.gridAutoRows = "1px";
gridContainer.style.gridTemplateRows = "2px";
var cs = getComputedStyle(gridContainer);
cs.gridTemplateRows; // "2px"
gridItem.style.gridRow = "auto / 1";
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 1px 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px 1px 1px 1px 2px"

On the one hand, knowing the final size of these implicit tracks can be useful. On the other hand, failing to round-trip sucks.

@Loirooriol
Copy link
Contributor Author

I have been thinking that we may be able to keep leading implicit tracks in the resolved value and also ensure round-tripping. We only need to allow grid-template-rows and grid-template-columns to define explicit tracks before the grid line number 1.

The current basic syntax is

<explicit-track-list> = [ <line-names>? <track-size> ]+ <line-names>?
<line-names>          = '[' <custom-ident>* ']'

Maybe we could change <line-names> to '[' <custom-ident>* || 1? ']', with the constraint that this 1 can only appear once in the <explicit-track-list>.

This would define that line name to be the number 1. Tracks before that line would belong to the explicit grid, but items wouldn't be able to use integers in grid-row/column to be placed in such tracks (since 0 is invalid and negative integers refer to the end). Items could still reference line names in order to be placed there.

Then resolved values could preserve this 1 line name if it was specified, and otherwise it could be inserted when there are leading implicit tracks. The previous example would become

cs.gridTemplateRows; // "2px"
gridItem.style.gridRow = "auto / 1";
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px [1] 2px"
gridContainer.style.gridTemplateRows = cs.gridTemplateRows; // "1px [1] 2px"

@tabatkins
Copy link
Member

Whoa, that's definitely a bug; I see how the spec can easily be read to say that, but it is absolutely not the intention that the resolved value of those properties includes implicit tracks. That's just something completely different, not in any way a gridTemplateRows/Columns value.

(I believe it was meant to be referring to tracks created by, say, repeat(auto-fit), which aren't explicitly created by the author.)

@Loirooriol
Copy link
Contributor Author

Oh, the spec seemed pretty clear to me:

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

Every track listed individually, whether implicitly or explicitly created, without using the repeat() notation.

https://drafts.csswg.org/css-grid/#implicit-grids

The grid-template-rows, grid-template-columns, and grid-template-areas properties define a fixed number of tracks that form the explicit grid. When grid items are positioned outside of these bounds, the grid container generates implicit grid tracks

All Chromium, WebKit, Firefox and Edge implemented it this way. There are also various WPT tests that expect this behavior. Not sure if it may be too late to remove implicit tracks from the resolved value, there might be code relying on that.

@tabatkins
Copy link
Member

It may be clear to you, but it's definitely not what we meant to write. ^_^ There's absolutely no world in which implicit tracks make any sense whatsoever to show up in the value of properties that solely define the explicit tracks. (Not unless we alter the properties themselves to allow defining implicit tracks, which muddies the water quite a bit...)

I doubt anyone's depending on this, and if they are, uh, too bad I guess? If anything breaks we'll have to go back and see if we can change grid-template-rows/columns as you suggest to allow sizing implicit tracks as well; the current situation is just utterly broken.

@mrego
Copy link
Member

mrego commented Oct 31, 2019

Maybe things like DevTools use that information to know the size of the implicit tracks and the number of them.

@tabatkins
Copy link
Member

Agenda+ to discuss whatever needs to happen to make the resolved value of grid-template-rows/columns actually be a valid value for grid-template-rows/columns, either:

  1. clarify wording to not imply that the implicit grid is serialized as part of the value, going against all implementations
  2. add sufficient new features to grid-template-rows/columns to allow them to size implicit tracks too, and change the serialization of the resolved values to use those features, again going against all implementations.

We cannot stick with what we have today, where all implementations agree on serializating g-t-r/c as a value that is not valid for those properties (the serialization is accidentally valid, but as a different g-t-r/c value, not corresponding to the used value that's supposed to be serializing).

Because both possibilities require all implementations to change, and (2) is actually rather complicated and of fairly minimal value I think, I strongly support (1).

@fantasai
Copy link
Collaborator

fantasai commented Nov 1, 2019

This edit was added in 4646eb1 in response to https://www.w3.org/Bugs/Public/show_bug.cgi?id=16906

It's very clear that including implicit tracks was intended. CC @atanassov

@tabatkins
Copy link
Member

tabatkins commented Nov 1, 2019

Oh jeez, so this dates back to the original MS draft, and we just rewrote it for phrasing correctness without thinking about the round-tripping implications.

Okay so this is still very wrong, and we have to choose either (1) or (2) from my previous comment; the status quo is utterly broken and unusable.

@Loirooriol
Copy link
Contributor Author

I think being able to define which grid line is the number 1 might have its usecases other than this, e.g. if you have 3 columns with auto min-content max-content but is there is enough space you want to add some spacing before the first column and after the last one. If you use grid-template-columns: 1fr auto min-content max-content 2fr then you have to alter the integer indices in placement properties. You can use grid-auto-columns: 2fr 1fr and place ::before and ::after in implicit tracks, but it looks a bit dirty. grid-template-columns: 1fr [1] auto min-content max-content [-1] 2fr seems easier and more elegant.

However, something like this would probably need some thought and should target css-grid-3 instead of css-grid-1, but for getComputedStyle we need to fix css-grid-1.

So choosing (1) sounds reasonable. But if in the future we end up adding (2), then it will be sad if now we choose (1), because we will be preventing authors from knowing the used size of the implicit tracks.

@tabatkins
Copy link
Member

There are a number of things people want to get out of grids that can't be reflected via properties right now, like where a particular item was positioned, etc. Someday we may offer APIs for those.

But sizing implicit tracks is a complicated issue that I don't want to try and define here. Your use-case can be done by just adding padding to the grid container, btw. ^_^

@Loirooriol
Copy link
Contributor Author

Sure, padding works if you want a fixed amount, but you can't use padding-left: 1fr.

Another usecase could be having a grid with a main table, but also with some side comments per row in an extra column at the end. You may want to size this extra column explicitly, but not include it when using negative integers to place items in the main table. I guess subgrid can be an alternative solution for this, but it may not work that well depending on the HTML structure.

Anyways, I have written a quick draft patch for Chromium that would exclude implicit tracks from the resolved value. It produces failures in 13 WPT tests, see https://crrev.com/c/1897931

I haven't looked at them in detail but I expect some of them to be checking that the sizes of the implicit tracks are correct. This will be difficult to do if the sizes are not included... I guess they will need .getBoundingClientRect() on the grid items or something.

@heycam
Copy link
Contributor

heycam commented Nov 13, 2019

cc @MatsPalmgren

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed Resolved values of grid-template-rows/columns don't round-trip, and agreed to the following:

  • RESOLVED: Give the 2nd option a chance and see if there are compat reasons to instead keep current behavior
The full IRC log of that discussion <dael> Topic: Resolved values of grid-template-rows/columns don't round-trip
<dael> github: https://github.com//issues/4475
<fantasai> See https://github.com//issues/4475#issuecomment-548908448
<dael> TabAtkins: Looks like reminant, as spec grid-template-row/column when you asked for resolved value you get width of implicit tracks as well as explicit. Given you can't spec implicit tracks this doesn't make any sense at all.
<dael> TabAtkins: Getting the width of implicit tracks is worthwhile. Functionality is reasonable, but a number of useful grid things it would be great to get from layout that aren't in properties right now.
<dael> TabAtkins: IN past proposed things that would require new grid API to get
<dael> TabAtkins: Resolved value of grid-templates does not round trip.
<dael> TabAtkins: Options; 1) leave as is. Resolved value is not a valid value and confusing because unless you know number of implicit rows you don't know where explicit starts
<dael> TabAtkins: 2) Change to only reflect explicit rows on resolved values. implicit we leave for a more explicit API
<dael> TabAtkins: 3) continue to allow grid-template-rows to express implicit but change grammar so it's valid. There is some value b/c only explicit are used for auto positioning by default. Being able to give bounds to auto while sizing outside could be worthwhile
<dael> TabAtkins: Would need to be able to spec when the explicit grid starts and stops which would also need to return in the resolved value
<dael> TabAtkins: We leave as is, change return of gCS for this so that it allows roundtripping either way
<dael> TabAtkins: Need to decide, this was an accident. If we leave as is need to be more explicit
<dael> TabAtkins: I prefer changing to be just explicit tracks. I could accept any of the 3.
<dael> fantasai: web compat is a substantial concern. Might be stuck with #1
<dael> emilio: Also [missed]
<emilio> s/[missed]/I also prefer 2 if we can get away with the compat issue/
<dael> TabAtkins: Web compat is alwyas a concern and we might be struck with 1. Between 2 and 3 is group okay if we try for 2 and revert if web compat proves otherwise?
<dael> oriol: In issue I propose feature which allows define where grid line could be. It could place it in another place. I think something like this could have its own uses outside this issue. However I agree this prob needs more thought and be in something like Grid 3.
<dael> oriol: If we want to fix roundtripping we need mroe urgent for L1 so it's reasonable to try and remove implicit tracks
<dael> TabAtkins: Youre suggestion was option 3, but as you say requires additional work. How it interacts is unclear right now and a breaking change anywya. If breaking, might as well do one that's easier to work with. If we ever want explicit/implicit we can do that later
<dael> TabAtkins: Any strong preference for keeping current behavior? Or is everyone okay with trying to change gCS and falling back to no change if there's web compat problems?
<dbaron> please make it round-trip correctly :-)
<florian> I like the proposal
<dael> Rossen_: Try it out. It makes sense.
<dael> fantasai: We don't have syntax for that right?
<dael> TabAtkins: Not for 3. No one is suggesting widen the grammar first. This is keep as is and have gCS report explicit only or have gCS report more accurately
<fantasai> sorry, I was confused
<dael> Rossen_: Resolution would be for give the 2nd option a chance and see if there are compat reasons to instead keep current
<dael> Rossen_: Other opinions or objections?
<dael> emilio: No obj but I want to makes ure both Geck oa nd Chromium...info from gCS is not useful. Both Chromium and FF have special dev tools b/c gCS is not enough.
<dael> Rossen_: We can look at extending OM for grid
<emilio> s/make sure/ note
<dael> Rossen_: I think you're pointing out more general issue. I don't disagree
<emilio> https://searchfox.org/mozilla-central/source/dom/webidl/Grid.webidl, fwiw
<dael> TabAtkins: Point is valid in that what's currently returned is not enough for current use cases. So w're not losing anything and we should look into more advanced on
<dael> Rossen_: agreed
<dael> Rossen_: Objectins?
<dael> RESOLVED: Give the 2nd option a chance and see if there are compat reasons to instead keep current behavior

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 19, 2019
w3c/csswg-drafts#4475 resolved to try to stop
including implicit tracks in the resolved value of grid-template-columns
and grid-template-rows.

This implies that the resolved values will have less information now,
which affects lots of tests. I have edited them depending on the case:
 - When the size of the track was irrelevant (e.g. just checking whether
   some value was syntactically valid), I have just updated the expected
   value.
 - When the size of the track was relevant, but it wasn't important for
   the tracks to be implicit, I have made them explicit in order to get
   the same value.
 - When the test was about the sizes of implicit tracks, I have added
   new checks for the size and position of the grid items.

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

Bug: 1024927

Change-Id: I4677c184be263eaab08d4aee95bb868306d58228
@mrego
Copy link
Member

mrego commented Dec 11, 2019

This still needs edits in the spec.

Loirooriol added a commit to Loirooriol/csswg-drafts that referenced this issue Dec 11, 2019
Loirooriol added a commit to Loirooriol/csswg-drafts that referenced this issue Dec 11, 2019
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 7, 2020
w3c/csswg-drafts#4475 resolved to try to stop
including implicit tracks in the resolved value of grid-template-columns
and grid-template-rows.

This implies that the resolved values will have less information now,
which affects lots of tests. I have edited them depending on the case:
 - When the size of the track was irrelevant (e.g. just checking whether
   some value was syntactically valid), I have just updated the expected
   value.
 - When the size of the track was relevant, but it wasn't important for
   the tracks to be implicit, I have made them explicit in order to get
   the same value.
 - When the test was about the sizes of implicit tracks, I have added
   new checks for the size and position of the grid items.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/EKvyx2lZJXI

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

Bug: 1024927

Change-Id: I4677c184be263eaab08d4aee95bb868306d58228
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 7, 2020
w3c/csswg-drafts#4475 resolved to try to stop
including implicit tracks in the resolved value of grid-template-columns
and grid-template-rows.

This implies that the resolved values will have less information now,
which affects lots of tests. I have edited them depending on the case:
 - When the size of the track was irrelevant (e.g. just checking whether
   some value was syntactically valid), I have just updated the expected
   value.
 - When the size of the track was relevant, but it wasn't important for
   the tracks to be implicit, I have made them explicit in order to get
   the same value.
 - When the test was about the sizes of implicit tracks, I have added
   new checks for the size and position of the grid items.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/EKvyx2lZJXI

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

Bug: 1024927

Change-Id: I4677c184be263eaab08d4aee95bb868306d58228
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897931
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#728894}
@geddski
Copy link

geddski commented Apr 3, 2020

I believe this change would be a mistake. How else would user-land be able to create Grid inspectors/overlays?

By no longer returning the implicit tracks, this change breaks every Grid overlay/inspector/generator tool on the web, including:

my Grid Critters game:
gc-example

Webflow's Grid Inspector
image

If this change is necessary, then we need some other way of getting all the grid track information. Firefox devtools' Grid Inspector uses a privileged element.getGridFragments() method that includes implicit grid tracks, but it's not available in user-land. getComputedStyle() is all we have ATM. Please don't break it.

@tabatkins
Copy link
Member

Yes, we need a way to get more information out of the grid. There's a few requests for different types of information, which are all pretty reasonable.

However, serializations of values must round-trip; their entire purpose is to give you a property value that could reproduce the current world. So this change was necessary.

@geddski
Copy link

geddski commented Apr 6, 2020

@tabatkins got it. Even though the current behavior is a bug, it's being relied on by many products. Any chance we could accelerate one of those proposals? Until then our web apps will be broken for a long time with no possible way to fix.

@tabatkins
Copy link
Member

No work's been done on those yet, but I can put it into my "do soon" todo list.

@geddski
Copy link

geddski commented Apr 6, 2020

@tabatkins thank you!

@anhtaiH
Copy link

anhtaiH commented Apr 10, 2020

Hi! Just wanted to piggyback on @geddski's concern regarding breaking products that built a grid overlay — this is affecting our users today. The main issue is that we need to know how many implicit tracks are created, and without that information the user experience is completely broken. Please fix this or provide an API for us to access grid information soon 🙏. Better yet, is it possible to postpone (revert) the changes until we have an API to work with?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 13, 2020
This reverts commit be7909b47a8b489e33b6d7f415d123d4b63663c5.

Reason for revert: This appears to be breaking site authoring tools
which were relying on the previous behaviour.

Bug: 1069866

Original change's description:
> [css-grid] Exclude implicit grid tracks from the resolved value
>
> w3c/csswg-drafts#4475 resolved to try to stop
> including implicit tracks in the resolved value of grid-template-columns
> and grid-template-rows.
>
> This implies that the resolved values will have less information now,
> which affects lots of tests. I have edited them depending on the case:
>  - When the size of the track was irrelevant (e.g. just checking whether
>    some value was syntactically valid), I have just updated the expected
>    value.
>  - When the size of the track was relevant, but it wasn't important for
>    the tracks to be implicit, I have made them explicit in order to get
>    the same value.
>  - When the test was about the sizes of implicit tracks, I have added
>    new checks for the size and position of the grid items.
>
> Intent to Implement and Ship:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/EKvyx2lZJXI
>
> Spec: https://drafts.csswg.org/css-grid/#resolved-track-list
>
> Bug: 1024927
>
> Change-Id: I4677c184be263eaab08d4aee95bb868306d58228
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897931
> Commit-Queue: Oriol Brufau <obrufau@igalia.com>
> Reviewed-by: Manuel Rego <rego@igalia.com>
> Cr-Commit-Position: refs/heads/master@{#728894}

TBR=jfernandez@igalia.com,rego@igalia.com,obrufau@igalia.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1024927
Change-Id: I8ef626b6ed84a9ee581d542072bda63685a46c42
@bfgeek
Copy link

bfgeek commented Apr 13, 2020

We are going to revert Chrome's behaviour change due to breakage on live sites. We won't change our behaviour in the future until an alternate way of accessing this information is provided.

@tabatkins
Copy link
Member

Fair. I'll put together a proposal soon and get it front of the WG.

@geddski
Copy link

geddski commented Apr 13, 2020

Thanks so much @bfgeek & @tabatkins, y'all are awesome.

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Apr 13, 2020
This reverts commit be7909b.

Reason for revert: This appears to be breaking site authoring tools
which were relying on the previous behaviour.

WPT tests are not reverted for now.

Bug: 1069866

Original change's description:
> [css-grid] Exclude implicit grid tracks from the resolved value
>
> w3c/csswg-drafts#4475 resolved to try to stop
> including implicit tracks in the resolved value of grid-template-columns
> and grid-template-rows.
>
> This implies that the resolved values will have less information now,
> which affects lots of tests. I have edited them depending on the case:
>  - When the size of the track was irrelevant (e.g. just checking whether
>    some value was syntactically valid), I have just updated the expected
>    value.
>  - When the size of the track was relevant, but it wasn't important for
>    the tracks to be implicit, I have made them explicit in order to get
>    the same value.
>  - When the test was about the sizes of implicit tracks, I have added
>    new checks for the size and position of the grid items.
>
> Intent to Implement and Ship:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/EKvyx2lZJXI
>
> Spec: https://drafts.csswg.org/css-grid/#resolved-track-list
>
> Bug: 1024927
>
> Change-Id: I4677c184be263eaab08d4aee95bb868306d58228
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897931
> Commit-Queue: Oriol Brufau <obrufau@igalia.com>
> Reviewed-by: Manuel Rego <rego@igalia.com>
> Cr-Commit-Position: refs/heads/master@{#728894}

TBR=jfernandez@igalia.com,rego@igalia.com,obrufau@igalia.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1024927
Change-Id: I8ef626b6ed84a9ee581d542072bda63685a46c42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2147814
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Oriol Brufau <obrufau@igalia.com>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758566}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue May 29, 2020
…true. r=emilio

Chrome backed out their change to omit implicit tracks; see
w3c/csswg-drafts#4475 (comment).

Differential Revision: https://phabricator.services.mozilla.com/D77390
xeonchen pushed a commit to xeonchen/gecko that referenced this issue May 29, 2020
…true. r=emilio

Chrome backed out their change to omit implicit tracks; see
w3c/csswg-drafts#4475 (comment).

Differential Revision: https://phabricator.services.mozilla.com/D77390
tabatkins added a commit that referenced this issue Jul 7, 2020
…ved values, and noting that we intend to remove it. #4475
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=204588

Reviewed by Manuel Rego Casasnovas.

LayoutTests/imported/w3c:

Import WPT tests.

* web-platform-tests/css/css-grid/grid-definition/grid-inline-support-flexible-lengths-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-support-grid-template-columns-rows-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-support-named-grid-lines-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-support-repeat-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-support-repeat-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-template-columns-rows-resolved-values-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-inline-template-columns-rows-resolved-values-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-support-flexible-lengths-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-support-grid-template-columns-rows-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-support-named-grid-lines-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-support-repeat-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-support-repeat-001.html:
* web-platform-tests/css/css-grid/grid-definition/grid-template-columns-rows-resolved-values-001-expected.txt:
* web-platform-tests/css/css-grid/grid-definition/grid-template-columns-rows-resolved-values-001.html:
* web-platform-tests/css/css-grid/grid-items/grid-minimum-size-grid-items-021.html:
* web-platform-tests/css/css-grid/grid-layout-properties-expected.txt:
* web-platform-tests/css/css-grid/grid-layout-properties.html:
* web-platform-tests/css/css-grid/parsing/grid-template-columns-computed-withcontent-expected.txt:
* web-platform-tests/css/css-grid/parsing/grid-template-columns-computed-withcontent.html:
* web-platform-tests/css/css-grid/parsing/grid-template-rows-computed-withcontent-expected.txt:
* web-platform-tests/css/css-grid/parsing/grid-template-rows-computed-withcontent.html:

Source/WebCore:

w3c/csswg-drafts#4475 resolved to try to stop
including implicit tracks in the resolved value of grid-template-columns
and grid-template-rows.

This implies that the resolved values will have less information now,
which affects lots of tests. I have edited them depending on the case:
 - When the size of the track was irrelevant (e.g. just checking whether
   some value was syntactically valid), I have just updated the expected
   value.
 - When the size of the track was relevant, but it wasn't important for
   the tracks to be implicit, I have made them explicit in order to get
   the same value.
 - When the test was about the sizes of implicit tracks, I have added
   new checks for the size and position of the grid items.

Tests: fast/css-grid-layout/grid-auto-columns-rows-get-set.html
       fast/css-grid-layout/grid-columns-rows-get-set.html
       fast/css-grid-layout/grid-template-shorthand-get-set.html
       fast/css-grid-layout/mark-as-infinitely-growable.html
       fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks.html
       fast/css-grid-layout/negative-growth-share-as-infinity-crash.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-support-flexible-lengths-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-support-grid-template-columns-rows-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-support-named-grid-lines-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-support-repeat-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-inline-template-columns-rows-resolved-values-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-flexible-lengths-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-grid-template-columns-rows-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-named-grid-lines-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-support-repeat-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-definition/grid-template-columns-rows-resolved-values-001.html
       imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-minimum-size-grid-items-021.html
       imported/w3c/web-platform-tests/css/css-grid/grid-layout-properties.html
       imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-columns-computed-withcontent.html
       imported/w3c/web-platform-tests/css/css-grid/parsing/grid-template-rows-computed-withcontent.html

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::valueForGridTrackList):
* rendering/RenderGrid.cpp:
(WebCore::RenderGrid::trackSizesForComputedStyle const):

LayoutTests:

Update tests.

* fast/css-grid-layout/grid-auto-columns-rows-get-set-expected.txt:
* fast/css-grid-layout/grid-auto-columns-rows-get-set.html:
* fast/css-grid-layout/grid-columns-rows-get-set.html:
* fast/css-grid-layout/grid-template-shorthand-get-set-expected.txt:
* fast/css-grid-layout/grid-template-shorthand-get-set.html:
* fast/css-grid-layout/mark-as-infinitely-growable.html:
* fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks-expected.txt:
* fast/css-grid-layout/named-grid-lines-computed-style-implicit-tracks.html:
* fast/css-grid-layout/negative-growth-share-as-infinity-crash.html:


Canonical link: https://commits.webkit.org/219372@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@254561 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@fantasai fantasai added the Agenda+ Whiteboard This is a large discussion that will benefit from whiteboard discussion. label Jul 27, 2023
@fantasai fantasai added Needs Data and removed Agenda+ Whiteboard This is a large discussion that will benefit from whiteboard discussion. labels Feb 12, 2024
@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [css-grid] Resolved values of grid-template-rows/columns don't round-trip.

The full IRC log of that discussion <fantasai> oriol: Problem is that in grid-template properties, the sizes you assign are assigned only to explicit grid tracks
<florian> s/.../Especially if you have the ability to define variables that are local to the mixing, we run into the kind of problems you have with hygienic macros vs non hygienic macros in lisp
<fantasai> ... but the value you get back includes implicit tracks
<fantasai> oriol: if you try to round-trip, then these implicit track sizes increase the size of the explicit grid
<fantasai> oriol: and change the beahvior
<fantasai> oriol: we considered not including the sizes of the implicit tracks
<fantasai> oriol: but this broke some sites
<fantasai> oriol: Then we thought about offering an alternative API for such websites, but this was along time ago, and probably we have too much dependency
<fantasai> oriol: So we might have to keep the broken behavior
<fantasai> oriol: one idea was to define which line is line #1 in the syntax
<fantasai> oriol: so that you could distinguish the implicit vs explicit tracks
<fantasai> oriol: by marking line 1 or line -1
<fantasai> oriol: it might be more compatible
<fantasai> oriol: but if a website doesn't use line names at all, they might not be expecting line name definitions
<fantasai> oriol: so... possible to be Web-compatible and better than current broken thing?
<fantasai> oriol: so maybe browsers could investigate whether compatible?
<fantasai> oriol: that's the idea.
<fantasai> oriol: The specific syntax would be to use brackets, instead of idents just use '1' or '-1', and could only do this once in the track listing
<fantasai> astearns: I'm not sure I entirely understand the proposal, but sounds like it would be a new syntax that opt into that could round-trip
<fantasai> astearns: but we would still have round-trip problem otherwise?
<fantasai> oriol: we would try to use this syntax when serializing, as a way to distinguish implicit vs explicit tracks
<fantasai> oriol: this way if you assign back the value, you wouldn't be growing the grid, because items would still be placed according to existing line
<fantasai> astearns: would that be true with content changes?
<fantasai> astearns: would there be a scenario where initial content A had a particular layout, and roundtripped value matched, but that if you went and serialized with content A and did roundtrip, would content B give a different result?
<fantasai> TabAtkins: if we end up generating different numbers of implicit tracks because different number of children
<fantasai> TabAtkins: then ... but that wouldn't be different from today
<fantasai> astearns: but the new declared value gives you one result, but don't content A, and then applying round-trip to content B gives you a third layout
<fantasai> TabAtkins: Today if any implicit rows or tracks, you will gradually mutate your grid
<fantasai> TabAtkins: this will prevent you from accidentally mutating your grid
<fantasai> TabAtkins: right now you will add explicit tracks that weren't there originally
<astearns> ack fantasai
<emilio> fantasai: If you're changing the content after you shove the computed value back you're gonna run into two problems
<emilio> ... you're going to have explicit tracks which were explicit
<dholbert> s/were explicit/were implicit/
<emilio> ... but then the non-fixed sizes will not fit their contents
<emilio> ... so either way if you change the content you're going to run into problems
<dholbert> q+
<emilio> ... you run into more problems if we don't fix this
<dholbert> q-
<emilio> ... so that is not super new, the issue is the current behavior will change the number of columns without content changes
<fantasai> iank_: would this break people who are naively parsing?
<emilio> iank_: Is it true that this might break people that are naively parsing the output
<fantasai> oriol: Yeah, if they're not using line-names, they might not be parsing the bracketed line name sets correctly
<fantasai> TabAtkins: We do show line-names in the output if we're using e.g. grid-template-areas
<fantasai> TabAtkins: but it might be in a place you're not expecting line names
<fantasai> TabAtkins: but less likely that it will be completely bad
<fantasai> iank_: Given breakage last time, I think it's still pretty high risk
<fantasai> astearns: are you willing to try it?
<fantasai> iank_: we tried going first last time and broke the world, someone else can try this time :)
<fantasai> astearns: do we have a volunteer?
<fantasai> emilio: I could patch Gecko
<astearns> ack fantasai
<emilio> fantasai: question would be whether we parse these numbers we actually respect the implicit lengths
<emilio> ... or keep our current behavior
<emilio> oriol: haven't thought about that
<fantasai> fantasai: two ways to ignore: one keep in the computed value (acutal computed value, not gCS) but ignore during layout
<fantasai> fantasai: or throw it out during style computation
<emilio> fantasai: third option is not ignoring, would override grid-auto-{rows,columns}
<emilio> ... for the columns for which you have a matching value
<emilio> astearns: we don't know the ramifications of that but don't love to ignore it
<emilio> fantasai: we could ignore it just in gCS
<emilio> ... first question is whether this is compatible at all
<fantasai> s/ignore it/allow it/
<emilio> ... so maybe emilio implements whatever's easier and see that first before figuring this out
<emilio> astearns: iank_, do you know off-hand which sites were broken?
<emilio> oriol: some of them commented in the issue
<emilio> astearns: so leave as is with an experiment to see if this round-tripping idea can work at all
<emilio> ... would that be ok?
<fantasai> emilio: seems fine
<fantasai> emilio: I'll check with oriol about the specifics, but I can do it
<fantasai> iank_: I think webflow was the primary site that broke
<fantasai> astearns: so we leave this issue again, and try out this experiment see whether web-compatible at all, and if it is, work out further details
<dholbert> (other sites that maybe-broke: wix sites, grid critters)

@emilio
Copy link
Collaborator

emilio commented Feb 12, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Monday afternoon
Development

No branches or pull requests

10 participants