1459536 - Record which fields of CSS animations / transitions have been overridden by script and ignore changes from style

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Brian Birtles (:birtles)]

After bug 1456394 lands, there will no longer be any *ReadOnly interfaces. So, what should happen if script modifies the properties attached to a CSSAnimation / CSSTransition?

This will ultimately need to be specced in css-animations-2 / css-transitions-2 but having discussed this with Apple and Google we agree that if you override a property from script, the script-set value should prevail. In future, we may add API to reset/detect this case but for now it doesn't appear to be necessary.

In this bug we should implement that behavior. Until we ship getAnimations() this behavior won't be shipped, however, so this doesn't need to be blocked on the spec work (although hopefully spec shouldn't be too far behind, and ideally ahead).

Comment 1

[Brian Birtles (:birtles)]

Given that CSS transitions properties are snapshotted when the transition is created (i.e. changing transition-duration doesn't affect running transitions), I think that once bug 1292001 is done, the only thing left to do for transitions will be to add tests that changing the timing properties via the API actually does take effect.

As a result, this bug is mostly concerned with CSS Animations. Boris, if you are interested, feel free to take this bug. I think it mostly involves writing a bunch of WPT for each of the different timing properties / keyframes. Actually recording which properties changed is hopefully not too difficult? I think it might just involve sticking a bitfield on CSSAnimation that tracks which of the following have been overridden:

• The effect as a whole (in which case changes to any of the following should be ignored)
  • The effect's keyframes
  • Each of the effect's timing properties
• The animation's play state

The last point might be a separate bug. The current thinking is we should drop the special playback behavior defined in the spec: https://drafts.csswg.org/css-animations-2/#animation-play-state and simply say that if the author ever calls play(), pause(), reverse() etc. or sets the startTime etc. then the animation ignores changes to animation-play-state from that point on.

I don't think we'd need to track if the target/pseudo is changed, for example, since that's not possible to change via CSS. Likewise, technically we don't need to track if endDelay is changed, necessarily.

I'm happy to help with any spec changes for that.

Comment 2

[Brian Birtles (:birtles)]

Attachment: WIP patch covering just setKeyframes

Hi Boris, I think we need to get this landed within the next 2~3 days before the soft code freeze.

I hope you don't mind but I wrote a patch just to outline what I think we need to do. It only covers setKeyframes() but hopefully it explains the approach I have in mind.

If you like I can take this bug and fill out the rest of the patches? If you are already working on this though, that's fine. Maybe this patch will be of some use.

Comment 3

[Boris Chiou [:boris]]

Sorry I haven't started to work on this yet. Please feel free to take this and finish your patch. :)

Comment 4

[Boris Chiou [:boris]]

Comment on attachment 9130065 [details] [diff] [review]
WIP patch covering just setKeyframes

Review of attachment 9130065 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsAnimationManager.h
@@ +278,5 @@
>  } /* namespace dom */
>  
> +// A subclass of KeyframeEffect that reports when specific properties have been
> +// overridden via the Web Animations API.
> +class CSSAnimationKeyframeEffect : public dom::KeyframeEffect {

So, unfortunately, we still need something like CSSTransitionKeyframeEffect for transition, though we just drop ElementPropertyTransition.

I'm thinking is it possible to add a type flag in Keyframe, so we can just check this flag, instead of doing this by polymophism.

Comment 5

[Brian Birtles (:birtles)]

(In reply to Boris Chiou [:boris] from comment #4)

+// A subclass of KeyframeEffect that reports when specific properties have been
+// overridden via the Web Animations API.
+class CSSAnimationKeyframeEffect : public dom::KeyframeEffect {

So, unfortunately, we still need something like CSSTransitionKeyframeEffect
for transition, though we just drop ElementPropertyTransition.

I don't think we need it for CSS transitions, only CSS animations. For CSS transitions, you can't update the running transition by changing the CSS since the values are snapshotted at the point when the transition is generated.

I'm thinking is it possible to add a type flag in Keyframe, so we can just
check this flag, instead of doing this by polymophism.

Since we only need it for CSS animations, I'm inclined to use the subclass approach in order to keep KeyframeEffect simple. I agree that if we needed this for both CSS transitions and CSS animations it would be worth adding to KeyframeEffect however.

Comment 6

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles) from comment #5)

I don't think we need it for CSS transitions, only CSS animations. For CSS transitions, you can't update the running transition by changing the CSS since the values are snapshotted at the point when the transition is generated.

Oh right. This is for setKeyframes(), so don't need to do this for CSS transition. :)

Comment 7

[Brian Birtles (:birtles)]

Hopefully this works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5180892f7a5997da26d6a4c1bfda2effae6900f5

Comment 8

[Brian Birtles (:birtles)]

Attachment: Bug 1459536 - Extract common keyframe comparison test methods; r?boris

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

Comment 9

[Brian Birtles (:birtles)]

Attachment: Bug 1459536 - Ignore subsequent changes to @keyframes after calling setKeyframes; r?boris

Depends on D65096

Comment 10

[Brian Birtles (:birtles)]

Attachment: Bug 1459536 - Allow CSS animation timing properties to be overridden using the Web Animations API; r?boris

Depends on D65097

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1459536 - Allow CSS animation properties to be overridden by replacing the effect; r?boris

Depends on D65098

Comment 12

[Brian Birtles (:birtles)]

Attachment: Bug 1459536 - Replace CSSAnimation interaction with animation-play-state with a simpler "once overridden always overridden" approach; r?boris

This corresponds to the approach outlined in https://github.com/w3c/csswg-drafts/issues/4822

Depends on D65099

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c206b97b7447
Extract common keyframe comparison test methods; r=boris
https://hg.mozilla.org/integration/autoland/rev/7c8d563e57ea
Ignore subsequent changes to @keyframes after calling setKeyframes; r=boris
https://hg.mozilla.org/integration/autoland/rev/d6f2c615730f
Allow CSS animation timing properties to be overridden using the Web Animations API; r=boris
https://hg.mozilla.org/integration/autoland/rev/4c24b62d2686
Allow CSS animation properties to be overridden by replacing the effect; r=boris
https://hg.mozilla.org/integration/autoland/rev/02295b7142ac
Replace CSSAnimation interaction with animation-play-state with a simpler "once overridden always overridden" approach; r=boris

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/22077 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/22077
* Community-TC (pull_request) (https://community-tc.services.mozilla.com/tasks/groups/bHgz1WwFThW2kcjvrhXaWA)

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/c206b97b7447
https://hg.mozilla.org/mozilla-central/rev/7c8d563e57ea
https://hg.mozilla.org/mozilla-central/rev/d6f2c615730f
https://hg.mozilla.org/mozilla-central/rev/4c24b62d2686
https://hg.mozilla.org/mozilla-central/rev/02295b7142ac

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by stephenmcgruer