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-animations-2] Allow animation-composition to be set using the animation shorthand (closes #6929) #6930

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

graouts
Copy link
Contributor

@graouts graouts commented Jan 4, 2022

I believe we must add <single-animation-composition> before the animation-name to avoid issues with named animations using add, accumulate or replace as a name.

@heycam
Copy link
Contributor

heycam commented Jan 4, 2022

This will change the meaning of declarations like animation: 1s replace. Are we OK with that, or should we add some (potentially confusing) special parsing logic to make replace/add/accumulate match against the <keyframes-name> first?

@SebastianZ
Copy link
Contributor

For reference, the potential conflict arises from this note in the spec.:

Note that order is also important within each animation definition for distinguishing <keyframes-name> values from other keywords. When parsing, keywords that are valid for properties other than animation-name whose values were not found earlier in the shorthand must be accepted for those properties rather than for animation-name.

I'd base that decision on statistics of pages currently using the <single-animation-composition> keywords as <keyframe-name>s.
Alternatively, the value for animation-composition could be added using a different syntax, e.g. separated via a slash. Though that would probably also be a somewhat arbitrary syntax change.

Sebastian

@birtles
Copy link
Contributor

birtles commented Jan 13, 2022

I'd rather not risk Web compat breakage with this, even if we have statistics that suggest no one currently uses "replace", "accumulate" or "add" as keyframes names. ("add" seems pretty likely at any rate.)

Partly because I'm not confident we could get comprehensive statistics (and any breakage is pretty annoying for authors here) but partly because I guess we're going to encounter the same situation if we ever try to add new keywords for animation-fill-mode, animation-play-state, animation-direction, or animation-timing-function in future. That seems very likely to me—especially for new timing function keywords—so we should probably work out how to fix this in general.

This will change the meaning of declarations like animation: 1s replace. Are we OK with that, or should we add some (potentially confusing) special parsing logic to make replace/add/accumulate match against the <keyframes-name> first?

I'm not sure I totally follow this suggestion but I think something like this is probably best.

The trouble, as I understand it, is if you have animation: 1s replace add, normally that would mean animation-composition: replace with animation-name: add but in this case it would be the reverse.

It would be nice if we can somehow continue to tell authors / tools to always put the animation-name last. Is it even possible to have a rule that lets animation: 1s replace give us animation-name: replace while animation: 1s replace add gives us animation-name: add?

@SebastianZ
Copy link
Contributor

It would be nice if we can somehow continue to tell authors / tools to always put the animation-name last. Is it even possible to have a rule that lets animation: 1s replace give us animation-name: replace while animation: 1s replace add gives us animation-name: add?

This is what the spec. says. I.e. any keywords are preferrably interpreted as values for other properties than animation-name.
Though, as you already mention, this can be a problem for any keyword we add to one of the longhands.

There doesn't seem to be any perfect solution for this. Also, that's a more general issue, so I've created #6946 to discuss possible solutions.

Sebastian

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

Successfully merging this pull request may close these issues.

None yet

4 participants