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-view-transitions-1] Disallow the name auto as view-transition-name #9639

Closed
nt1m opened this issue Nov 26, 2023 · 16 comments · Fixed by #9764
Closed

[css-view-transitions-1] Disallow the name auto as view-transition-name #9639

nt1m opened this issue Nov 26, 2023 · 16 comments · Fixed by #9764

Comments

@nt1m
Copy link
Member

nt1m commented Nov 26, 2023

Currently the name auto is allowed as view-transition-name. Given the talk about view-transition "classes" in #8319. I think it would be good to resolve on reserving the "auto" keyword so it can be leveraged if needed.

I'm opening this as a separate issue since this is probably something we want to change in VT level 1 rather than level 2.

@khushalsagar @fantasai @tabatkins @noamr

@nt1m nt1m added the css-view-transitions-1 View Transitions; Bugs only label Nov 26, 2023
@tabatkins
Copy link
Member

Yup, sounds good to me.

@noamr
Copy link
Collaborator

noamr commented Nov 29, 2023

I'm not sure if we'll have to use `auto for #8319, but I'm fine with disallowing it as a vt name.

@bramus
Copy link
Contributor

bramus commented Dec 7, 2023

Agenda+’ing this. The sooner we do this, the better.

@bramus bramus added the Agenda+ label Dec 7, 2023
@fantasai fantasai added the Async Resolution: Proposed Candidate for auto-resolve with stated time limit label Dec 12, 2023
@css-meeting-bot
Copy link
Member

css-meeting-bot commented Dec 20, 2023

The CSS Working Group just discussed [css-view-transitions-1] Disallow the name `auto` as `view-transition-name` , and agreed to the following:

  • RESOLVED: disallow auto for view-transitions-name/view-transitions
The full IRC log of that discussion <frances__> bramus: right now the property transition view name accepts custom in #8319, might be feasible to add alternate shorthand name, use value of auto in that case
<fantasai> bramus: this would create a conflict with view-transition-name, so propose to disallow auto
<bramus> s/custom/custom-ident
<frances__> RESOLUTION: disallow auto for view transitions
<fantasai> s/RESOLUTION/RESOLVED/
<fantasai> s/view transitions/view-transition-name/
<frances__> RESOLUTION: disallow auto for view-transitions-name/view-transitions
<frances__> rossen: keep going to next issue

@bramus bramus added Closed Accepted by CSSWG Resolution Needs Edits and removed Async Resolution: Proposed Candidate for auto-resolve with stated time limit labels Dec 20, 2023
aarongable pushed a commit to chromium/chromium that referenced this issue Dec 20, 2023
We've resolved here
w3c/csswg-drafts#9639 (comment)
to disallow view-transition-name: auto as a valid identifier/name.

Since this is a brekaing changing, this patch adds a use counter to count the
number of uses, so that we have data to support removing it.

R=bokan@chromium.org

Bug: 1513460
Change-Id: I0f46f3533565cefdb42d93082d1cd3cd4a3a88b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5141666
Auto-Submit: Vladimir Levin <vmpstr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1239874}
@noamr
Copy link
Collaborator

noamr commented Jan 4, 2024

@bramus @nt1m what do you expect when someone uses auto today? Same as none/having no view-transition-name?

@nt1m
Copy link
Member Author

nt1m commented Jan 4, 2024

@bramus @nt1m what do you expect when someone uses auto today? Same as none/having no view-transition-name?

It should be rejected until it does something useful. I would expect it to provide a name automatically for view transitions. It's useful in the case you have a class for instance and but don't want to provide a name to every single element that is affected by that class.

@noamr
Copy link
Collaborator

noamr commented Jan 4, 2024

@bramus @nt1m what do you expect when someone uses auto today? Same as none/having no view-transition-name?

It should be rejected until it does something useful. I would expect it to provide a name automatically for view transitions. It's useful in the case you have a class for instance and but don't want to provide a name to every single element that is affected by that class.

"Rejected" as in reject the whole transition?

@noamr
Copy link
Collaborator

noamr commented Jan 4, 2024

btw I don't think the name auto would be a solution for generating names because it's not something that can effectively work across documents. I think it would have to be something that generates a name from capturing attributes along the element's ancestor chain. envisioning something like:

section.list li.item[:id] img { view-transition-name: item-[:id]  }

or some such

@nt1m
Copy link
Member Author

nt1m commented Jan 4, 2024

"Rejected" as in reject the whole transition?

"Rejected" as in rejected by the CSS parser, so if you set it on the root, then it won't override view-transition-name: root set by the UA stylesheet. If you set it on another element, it'll just behave as view-transition-name: none.

It should be pretty obvious for the developer they're using an unsupported value as it'll be striked out in the DevTools.

@nt1m
Copy link
Member Author

nt1m commented Jan 4, 2024

btw I don't think the name auto would be a solution for generating names because it's not something that can effectively work across documents. I think it would have to be something that generates a name from capturing attributes along the element's ancestor chain. envisioning something like:
section.list li.item[:id] img { view-transition-name: item-[:id] }
or some such

Sounds effectively like attr().

@noamr
Copy link
Collaborator

noamr commented Jan 4, 2024

btw I don't think the name auto would be a solution for generating names because it's not something that can effectively work across documents. I think it would have to be something that generates a name from capturing attributes along the element's ancestor chain. envisioning something like:
section.list li.item[:id] img { view-transition-name: item-[:id] }
or some such

Sounds effectively like attr().

Yea, something like that was proposed in #8320 (comment). In either case it probably won't be a single keyword that generates a name.

@nt1m
Copy link
Member Author

nt1m commented Jan 4, 2024

In any case, I don't think leaving auto as a valid ident value does any service to developers fwiw.

@noamr
Copy link
Collaborator

noamr commented Jan 4, 2024

In any case, I don't think leaving auto as a valid ident value does any service to developers fwiw.

OK. The only difference between having it as an invalid value and having it behave the same as none is that it would serialize as none.

@nt1m
Copy link
Member Author

nt1m commented Jan 4, 2024

The other differences are:

  • It won't participate in the cascade (so it won't override another name like root)
  • It'll be clearly striked out in DevTools as an invalid value
  • The auto keyword can be reused for something else (if ever needed)

@bramus
Copy link
Contributor

bramus commented Jan 5, 2024

(#) Sounds effectively like attr().

Key difference here is that you’re able to capture an attribute from a parent element and use it in a child element. With attr() you’d need to store the [id] of the li into a variable to pass it onto the child img element.

/* Proposal by noamr */
section.list li.item[:id] img {
  view-transition-name: item-[:id];
}

/* Reworked using attr() and ident() from https://github.com/w3c/csswg-drafts/issues/9141 */
section.list li.item[id] {
  --id: attr(id);

  img {
    view-transition-name: ident("item-" var(--id));
  }
}

noamr added a commit that referenced this issue Jan 5, 2024
…ransition-name` (#9764)

* [css-view-transitions-1] Treat `auto` as an invalid value

Closes #9639

* Update css-view-transitions-1/Overview.bs

Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>

* Add issue link

---------

Co-authored-by: Khushal Sagar <63884798+khushalsagar@users.noreply.github.com>
@nt1m
Copy link
Member Author

nt1m commented Jan 17, 2024

WPT PR: web-platform-tests/wpt#44035

webkit-commit-queue pushed a commit to nt1m/WebKit that referenced this issue Jan 17, 2024
https://bugs.webkit.org/show_bug.cgi?id=267619
rdar://121094366

Reviewed by Anne van Kesteren.

Adopt the resolution from: w3c/csswg-drafts#9639

Add a custom parsing function since the auto-generated function does not support custom-ident restrictions.

* LayoutTests/imported/w3c/web-platform-tests/css/css-view-transitions/parsing/view-transition-name-invalid-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-view-transitions/parsing/view-transition-name-invalid.html:
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeViewTransitionName):
* Source/WebCore/css/parser/CSSPropertyParserHelpers.h:

Canonical link: https://commits.webkit.org/273118@main
@bramus bramus removed the Needs Edits label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@noamr @bramus @tabatkins @fantasai @nt1m @css-meeting-bot and others