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

[web-animations-1] Animatable.animate() should not flush #3613

Closed
birtles opened this issue Feb 5, 2019 · 2 comments · Fixed by #3650
Closed

[web-animations-1] Animatable.animate() should not flush #3613

birtles opened this issue Feb 5, 2019 · 2 comments · Fixed by #3650

Comments

@birtles
Copy link
Contributor

birtles commented Feb 5, 2019

From Mozilla bug 1524480:

It turns out that whether or not Element.animate flushes is observable (and it doesn't flush in Blink but does in Gecko and WebKit).

Suppose we have:

div {
  opacity: 0.1;
  transition: 1s opacity linear;
}

And then we do:

getComputedStyle(div).opacity;

At this point the computed opacity of div is 0.1.

Now, suppose we update the specified opacity to 0.2 but DON'T flush style:

div.style.opacity = '0.2';

Then trigger an animation on the same element:

div.animate({ opacity: [0.6, 1] }, 1000);

At this point if Element.animate() flushes style it will cause us to trigger a transition from 0.1 to 0.2.

However, if Element.animate() does NOT flush style we will:

  • Generate a new Animation animating from 0.6 to 1.
  • Then on the next restyle we will have a before-change style opacity of 0.6 (since we bring declarative animations up-to-date as part of calculating the before-change style).
  • And we we also have an after-change style of 0.6 (for the same reason).

Since the before-change style and after-change style have not changed, no transition will be generated.

Test case: https://codepen.io/birtles/pen/YBxxBd

I propose we spec that Animatable.animate() (and similarly KeyframeEffect.setKeyframes(), etc.) do not flush style (or, in CSS transitions spec terms, trigger a style change event).

The reason is that if we require UAs to flush, it would mean that Element.animate() both flushes AND dirties style. As a result, if the author triggers a number of animations in a single animation frame, the browser would be required to flush multiple times per frame.

@birtles
Copy link
Contributor Author

birtles commented Feb 7, 2019

Discussed this with Google, Apple, and Microsoft folk and agreed animate() should not flush. I suspect all mutating methods should not flush (except CSSAnimation.pause() etc.) in general but I need to verify that this makes sense and check what engines currently do before writing the spec text and WPT for this.

@birtles
Copy link
Contributor Author

birtles commented Feb 15, 2019

WPT for this have been written and will be landed via Mozilla bug 1525809.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 15, 2019
… that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  w3c/csswg-drafts#3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

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

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Feb 16, 2019
… that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  w3c/csswg-drafts#3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

Differential Revision: https://phabricator.services.mozilla.com/D18916
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 20, 2019
…s API

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1525809
gecko-commit: 0738638178a900d7a44546181d5b401f9dd5a198
gecko-integration-branch: autoland
gecko-reviewers: hiro
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 21, 2019
… Web Animations API; r=hiro

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this issue Feb 21, 2019
jgraham pushed a commit to web-platform-tests/wpt that referenced this issue Feb 25, 2019
…s API

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1525809
gecko-commit: 0738638178a900d7a44546181d5b401f9dd5a198
gecko-integration-branch: autoland
gecko-reviewers: hiro
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Jul 23, 2019
…s API

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1525809
gecko-commit: 0738638178a900d7a44546181d5b401f9dd5a198
gecko-integration-branch: autoland
gecko-reviewers: hiro
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
… that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  w3c/csswg-drafts#3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

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

UltraBlame original commit: 5dac0d4a3c4428a9e553ba7e1c47da7a81636c2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
… Web Animations API; r=hiro

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

UltraBlame original commit: 0738638178a900d7a44546181d5b401f9dd5a198
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
… that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  w3c/csswg-drafts#3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

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

UltraBlame original commit: 5dac0d4a3c4428a9e553ba7e1c47da7a81636c2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 4, 2019
… Web Animations API; r=hiro

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

UltraBlame original commit: 0738638178a900d7a44546181d5b401f9dd5a198
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
… that does not flush style and use it; r=hiro

A forthcoming spec change will require that Animatable.animate() and other
methods that mutate animations do not flush style:

  w3c/csswg-drafts#3613

Bug 1525809 will add web-platform-tests for this change once it is made (and
tweak the behavior introduced in this patch if necessary).

Currently Firefox and WebKit will flush styles when calling
Animatable.animate(). This is undesirable since this method will _also_
invalidate style. As a result, if content triggers multiple animations in
a single animation frame, it will restyle every time it creates an animation.

This patch removes the style flush from a number of these methods.

In general the style flush is not necessary. For example, we don't need to flush
style before getting the computed style to pass to UpdateProperties. That's
because if there are pending style changes, then UpdateProperties will be called
with the updated style once they are applied anyway. Flushing style first means
that we may end up resolving style twice, when once would be sufficient.

For GetKeyframes() however, when used on a CSS animation, it should return
the most up-to-date style so for that call site we *do* want to flush style.

The test case added in this patch will fail without the code changes in the
patch. Specifically, we will observe 10 non-animation restyles (from the
5 animations) if we flush styles from SetKeyframes.

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

UltraBlame original commit: 5dac0d4a3c4428a9e553ba7e1c47da7a81636c2a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 4, 2019
… Web Animations API; r=hiro

Issue: w3c/csswg-drafts#3613
Corresponding spec change: w3c/csswg-drafts@78dc281

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

UltraBlame original commit: 0738638178a900d7a44546181d5b401f9dd5a198
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.

1 participant