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] Specify the interaction between API methods and animation-* properties #4822

Closed
birtles opened this issue Mar 3, 2020 · 14 comments

Comments

@birtles
Copy link
Contributor

birtles commented Mar 3, 2020

This is something we've discussed a number of times and the general consensus is that once you tweak a property from the API, any changes to the corresponding markup (animation-* property or @keyframes rule) is ignored.

Furthermore, the proposal is that this should apply to animation play state too (rather than the very convoluted behavior that is currently specced where pause is sticky but play is not).

Once interesting edge case with regards to play state is setting the startTime of an animation since that is equivalent to pausing or playing in some cases.

e.g. (1) - no change to play state

div.style.animation = 'anim 10s';
const animation = div.getAnimations()[0];
await animation.ready;
// Seek using the start time. This should probably NOT override animation-play-state.
animation.startTime += 1000;
// This should probably make the animation paused
div.style.animationPlayState = 'paused';

e.g. (2) - play state is changed

div.style.animation = 'anim 10s';
const animation = div.getAnimations()[0];
await animation.ready;
// Pause using the start time
animation.startTime = null;
// This will have no effect since the animation is already paused
div.style.animationPlayState = 'paused';
getComputedStyle(div).animationPlayState;
// This should probably have no effect since the author has taken over
// play control of the animation?
div.style.animationPlayState = 'running';

That's my intuition anyway. What do others think?

I'm looking to spec this in the next few days so any feedback would be much appreciated.

/cc @graouts @stephenmcgruer @flackr @kevers-google

@birtles birtles changed the title [css-animations-2] Specify the iteraction between API methods and animation-* properties [css-animations-2] Specify the interaction between API methods and animation-* properties Mar 3, 2020
@kevers-google
Copy link
Contributor

Basing stickiness on whether a web-animation API call changes the play state makes sense. This change would also address reverse from a paused state, or playbackRate in an onfinish handler.

@birtles
Copy link
Contributor Author

birtles commented Mar 4, 2020

Basing stickiness on whether a web-animation API call changes the play state makes sense. This change would also address reverse from a paused state, or playbackRate in an onfinish handler.

Currently what I have in mind (and what I am implementing in Firefox) is that for startTime and reverse(), we simply detect a change between paused and any state that is not paused. For play() and pause() they always unconditionally trigger the "play state is being overridden by API" behavior.

That is, setting the startTime such that an animation goes from finished to running would not trigger the "play state is being overridden by API" behavior.

What do you think?

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 4, 2020
…pler "once overridden always overridden" approach

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: 02295b7142ac27bced7328970a4e1f676bf40cf4
gecko-integration-branch: autoland
gecko-reviewers: boris
@birtles
Copy link
Contributor Author

birtles commented Mar 4, 2020

Thinking about this a little further, I think I would be ok with startTime and reverse() not triggering the override behavior. It does feel a little magical to have these sometimes trigger the override behavior and sometimes not. I'd be curious to hear what others prefer.

xeonchen pushed a commit to xeonchen/gecko that referenced this issue Mar 4, 2020
…ate with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 4, 2020
…ate with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

--HG--
extra : moz-landing-system : lando
@flackr
Copy link
Contributor

flackr commented Mar 4, 2020

Just to be clear, does this example (from your original post) still pause, but just not set the sticky override?

div.style.animation = 'anim 10s';
const animation = div.getAnimations()[0];
await animation.ready;
// Pause using the start time
animation.startTime = null;

@flackr
Copy link
Contributor

flackr commented Mar 4, 2020

To expediate the discussion:

If the above pauses, the div.style.animationPlayState would still resolve to running, so we would have to have some level of stickiness in order to ensure the pause, right?

Then does flipping it cause the animation to start running?

div.style.animationPlayState = 'paused';
animation.playState; // still paused, style flushed
div.style.animationPlayState = 'running';
// is it now running?

If the above doesn't pause, then setting the start time has no effect?

stephenmcgruer added a commit to web-platform-tests/wpt that referenced this issue Mar 4, 2020
…22077)

* Extract common keyframe comparison test methods

We will use this in the next patch to test the result after calling setKeyframes.

Differential Revision: https://phabricator.services.mozilla.com/D65096

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: c206b97b74471740983bec1da249c70a0e840dd5
gecko-integration-branch: autoland
gecko-reviewers: boris

* Ignore subsequent changes to @Keyframes after calling setKeyframes

Differential Revision: https://phabricator.services.mozilla.com/D65097

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: 7c8d563e57ea6ea8b39efe857fcb5599a0ed5f67
gecko-integration-branch: autoland
gecko-reviewers: boris

* Allow CSS animation timing properties to be overridden using the Web Animations API

Differential Revision: https://phabricator.services.mozilla.com/D65098

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: d6f2c615730f6ccbe93e82e398bb3f17cd69e7f6
gecko-integration-branch: autoland
gecko-reviewers: boris

* Allow CSS animation properties to be overridden by replacing the effect

Differential Revision: https://phabricator.services.mozilla.com/D65099

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: 4c24b62d2686a6b359f275f53407592f39dd2242
gecko-integration-branch: autoland
gecko-reviewers: boris

* Replace CSSAnimation interaction with animation-play-state with a simpler "once overridden always overridden" approach

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1459536
gecko-commit: 02295b7142ac27bced7328970a4e1f676bf40cf4
gecko-integration-branch: autoland
gecko-reviewers: boris

Co-authored-by: Brian Birtles <birtles@gmail.com>
@birtles
Copy link
Contributor Author

birtles commented Mar 4, 2020

Just to be clear, does this example (from your original post) still pause, but just not set the sticky override?

Yes, that's right. This is purely about the override behavior.

If the above pauses, the div.style.animationPlayState would still resolve to running, so we would have to have some level of stickiness in order to ensure the pause, right?

Right, the proposal is that we use the same "stickiness" for timing properties that map to CSS animation-* properties. That is, once you override them from the API, we ignore the value set via style, even if it changes.

Until I write up the spec text for this (hopefully today), it might be easiest to follow by looking at the patches / tests I just landed for this in Mozilla bug 1459536.

Then does flipping it cause the animation to start running?

Flipping it does not cause the animation to start running. Once overridden, always overridden.

@flackr
Copy link
Contributor

flackr commented Mar 5, 2020

Thinking about this a little further, I think I would be ok with startTime and reverse() not triggering the override behavior. It does feel a little magical to have these sometimes trigger the override behavior and sometimes not. I'd be curious to hear what others prefer.

I think there may have been a confusion - i meant to refer to this comment ^.

I think that it probably should be sticky as it seems less confusing than having the properties not work as expected.

@birtles
Copy link
Contributor Author

birtles commented Mar 5, 2020

Ok, let me check I understand.

The original proposal is:

  1. Calling setKeyframes() means that any subsequent changes to @keyframes will be ignored.
  2. Calling updateTiming() means that any subsequent changes to the set properties will be ignored. (i.e. calling updateTiming({ duration: 1000 }) means subsequent changes to animation-duration will be ignored, but changes to animation-delay will still be reflected).
  3. Calling play() or pause() means that any subsequent changes to animation-play-state will be ignored.
  4. Calling reverse() or setting the startTime means that any subsequent changes to animation-play-state will be ignored, but only if the call to reverse() or startTime resulted in the animation transitioning to or from the 'paused' play state.

The point I was wondering about was whether we drop point 4 altogether or simply the condition from point 4.

@birtles
Copy link
Contributor Author

birtles commented Mar 5, 2020

The point I was wondering about was whether we drop point 4 altogether or simply the condition from point 4.

As I understand, your comment that "it should probably be sticky", would mean we should not drop point 4 altogether. Either we should keep it, or we should modify it so that it is unconditional.

I'm a little averse to making it unconditional since I can imagine people wanting to reverse() an animation while still keeping it responsive to animation-play-state. That would suggest sticking with the original proposal.

(And I forgot to include in the original proposal that replacing animation.effect altogether means that all changes to animation-* properties other than animation-name and animation-play-state, and all changes to @keyframes rules other than dropping the last matching @keyframes rule, would be ignored.)

@flackr
Copy link
Contributor

flackr commented Mar 5, 2020

The point I was wondering about was whether we drop point 4 altogether or simply the condition from point 4.

As I understand, your comment that "it should probably be sticky", would mean we should not drop point 4 altogether. Either we should keep it, or we should modify it so that it is unconditional.

Correct, that is what I was suggesting.

I'm a little averse to making it unconditional since I can imagine people wanting to reverse() an animation while still keeping it responsive to animation-play-state. That would suggest sticking with the original proposal.

Agreed. It's a little magical, but I think trying to avoid it has stranger ramifications.

(And I forgot to include in the original proposal that replacing animation.effect altogether means that all changes to animation-* properties other than animation-name and animation-play-state, and all changes to @keyframes rules other than dropping the last matching @keyframes rule, would be ignored.)

I think this is reasonable.

birtles added a commit to birtles/csswg-drafts that referenced this issue Mar 5, 2020
@birtles
Copy link
Contributor Author

birtles commented Mar 5, 2020

I started working on this today over at: birtles@83dd532

I didn't quite finish it but hopefully the outline is mostly there. Please take a look when you get a chance. I hope to finish it off tomorrow.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Mar 5, 2020
…ate with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

UltraBlame original commit: 02295b7142ac27bced7328970a4e1f676bf40cf4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Mar 5, 2020
…ate with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

UltraBlame original commit: 02295b7142ac27bced7328970a4e1f676bf40cf4
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Mar 5, 2020
…ate with a simpler "once overridden always overridden" approach; r=boris

This corresponds to the approach outlined in w3c/csswg-drafts#4822

Specifically:

* Calling play() / pause() always triggers the "animation play state is being
  overridden by the API" behavior (unless the method throws an exception).

* Setting the startTime or calling reverse() only triggers this override
  behavior if it results in a change between a playState that is paused and a
  playState that is not paused.

Differential Revision: https://phabricator.services.mozilla.com/D65100

UltraBlame original commit: 02295b7142ac27bced7328970a4e1f676bf40cf4
birtles added a commit to birtles/csswg-drafts that referenced this issue Mar 6, 2020
@birtles birtles closed this as completed in 52cc866 Mar 6, 2020
@birtles
Copy link
Contributor Author

birtles commented Mar 6, 2020

I went ahead and merged my work on this. It's not perfect but it's better than nothing and I've spent quite a while on it by now. (I was really hoping someone else would step in to do this.)

Hopefully we can tweak it from here.

@kevers-google
Copy link
Contributor

What is the expected override behavior in the following case:

div.style.animation = 'anim 100s';
const animation = div.getAnimations()[0];
div.style.animationPlayState = 'paused';
reverse();
div.style.animationPlayState = 'paused';

Should reverse override animationPlayState since going from a pending pause state to pending play? In other words, should reverse and startTime be flushing any pending changes to play state? Admittedly a very contrived case.

@birtles
Copy link
Contributor Author

birtles commented Mar 9, 2020

Should reverse override animationPlayState since going from a pending pause state to pending play? In other words, should reverse and startTime be flushing any pending changes to play state?

I think they probably should. The spec wording...

...all operations included in programming interface defined in this specification, as well as those operations defined in Web Animations [WEB-ANIMATIONS] that may return objects or animation state defined by this specification, must produce a result consistent with having fully processed any such pending changes to computed values.

is not 100% clear, but the intention is that all these methods operate on a fully-updated state. It's somewhat unfortunate to have to make all these methods flush, but I think that's the most understandable behavior.

Also, it's quite possible Firefox doesn't flush for reverse() or startTime yet. Last week I just fixed it to flush for getTiming() and getComputedTiming(), but I didn't check reverse().

blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Mar 11, 2020
…mations.

w3c/csswg-drafts#4822

Bug: 1059772
Change-Id: Id21c3c152f8cf6771703a16fcab8e80686608170
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090197
Commit-Queue: Kevin Ellis <kevers@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749013}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants