If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

stylo: CSS animations does not stop when animation name in a CSS rule is changed to 'none' directly

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
28 days ago
25 days ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

28 days ago
Created attachment 8900050 [details]
An example

E.g;

 document.styleSheets[0].cssRules[0].style.animationName = 'none';

The background color of the element in attaching file should be green (not animating color), but red (animating color) on stylo.

We seems to fail needs_animations_update check or somewhere.
(Assignee)

Updated

28 days ago
Blocks: 1288269
(Assignee)

Updated

28 days ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 1

27 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fef612354d7a336fbf0cc6ff81d87c5bba0a16b8
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

26 days ago
mozreview-review
Comment on attachment 8900501 [details]
Bug 1392851 - A reftest to check that we don't share styles for elements that have animations which is about to be removed.

https://reviewboard.mozilla.org/r/171882/#review177158

::: commit-message-ff404:1
(Diff revision 1)
> +Bug 1392851 - A reftest to check that we don't share styles for elements that have animations which is about to be removed. r?birtles

Nit: which are

::: commit-message-ff404:3
(Diff revision 1)
> +Bug 1392851 - A reftest to check that we don't share styles for elements that have animations which is about to be removed. r?birtles
> +
> +This was a test case for bug 1379203 (Goole Inbox issue), but to pass this test

Google

::: commit-message-ff404:8
(Diff revision 1)
> +This was a test case for bug 1379203 (Goole Inbox issue), but to pass this test
> +also needs the fix in this series to cancel animations when changing
> +animation-name to 'none' in the specified CSS rule.
> +
> +Actually the fix in this series also fixes the Google Inbox issue so that this
> +test can pass without the fix for the Goole Inbox issue. But even so without

Google

::: layout/reftests/css-animations/no-style-sharing-with-animations.html:20
(Diff revision 1)
> +//requestAnimationFrame(() => {
> +//  // Start background color animations on div elements that have the same
> +//  // class name.
> +//  var targets = document.querySelectorAll('div');
> +//  targets[0].classList.add('animation');
> +//  targets[1].classList.add('animation');
> +//  requestAnimationFrame(() => {
> +//    // Stop the background color animations without changing the class name.
> +//    targets[0].classList.add('no-animation');
> +//    targets[1].classList.add('no-animation');
> +//    //document.styleSheets[0].cssRules[0].style.animationName = 'none';
> +//    //document.documentElement.classList.remove('reftest-wait');
> +//  });
> +//});

I guess this is not supposed to be here?
Attachment #8900501 - Flags: review?(bbirtles) → review+
I'm ok with part 1 if we have to do that (although I think the changeset description needs to make clear that this is only needed for the case when rules are manipulated using the CSSOM), but it sounds like if we find a fix for bug 1393323 then that might fix this bug too and we won't need part 1?
(Assignee)

Comment 6

26 days ago
(In reply to Brian Birtles (:birtles) from comment #5)
> I'm ok with part 1 if we have to do that (although I think the changeset
> description needs to make clear that this is only needed for the case when
> rules are manipulated using the CSSOM), but it sounds like if we find a fix
> for bug 1393323 then that might fix this bug too and we won't need part 1?

Yes, that's right.  To fix bug 1393323, we need to avoid replacing values in css rules directly, or skip the first animation-only restyle (i.e. do normal traversal prior to animation-only restyle) if any css rules changed.  The latter way makes our code more complicated, the former way is our of my knowledge.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > I'm ok with part 1 if we have to do that (although I think the changeset
> > description needs to make clear that this is only needed for the case when
> > rules are manipulated using the CSSOM), but it sounds like if we find a fix
> > for bug 1393323 then that might fix this bug too and we won't need part 1?
> 
> Yes, that's right.  To fix bug 1393323, we need to avoid replacing values in
> css rules directly, or skip the first animation-only restyle (i.e. do normal
> traversal prior to animation-only restyle) if any css rules changed.  The
> latter way makes our code more complicated, the former way is our of my
> knowledge.

Ok, if bug 1393323 is not going to be fixed soon then I'm ok with adding part 1 as long as we have clear comments to explain that it is a temporary fix.
Comment hidden (mozreview-request)
(Assignee)

Updated

26 days ago
Attachment #8900501 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 10

26 days ago
mozreview-review
Comment on attachment 8900500 [details]
Bug 1392851 - Try to update running CSS animations when CSS rules are changed.

https://reviewboard.mozilla.org/r/171880/#review177246

::: commit-message-11ba2:3
(Diff revision 2)
> +In the case where values in css rules changed directly by CSSOM, the old
> +value in the css rule block is immediately replaced by the new one. So if
> +the element, which is applied to the css rule, has running animations, the
> +new value is used during cascading process in animation-only restyle. Thus
> +in a subsequent normal restyle, we can't tell whether the value in the css
> +rule has changed or not. It results transitions is not triggered (bug 1393323)
> +or CSS animations is not cancelled if the new animation-name value 'none'
> +(this bug).

(Very very minor nit: s/css/CSS/)

::: commit-message-11ba2:8
(Diff revision 2)
> +rule has changed or not. It results transitions is not triggered (bug 1393323)
> +or CSS animations is not cancelled if the new animation-name value 'none'
> +(this bug).

s/It results.../As a result, transitions may not be triggered (bug 1393323) and CSS animations may not be cancelled if the updated animation-name is 'none' (this bug)/

::: commit-message-11ba2:12
(Diff revision 2)
> +For this CSS animations issue, we take a workaround that we try to update
> +running CSS animations if the traversal is kicked by css rule changes.

For the latter case of CSS animations where animation-name has been updated to 'none', this patch introduces a workaround whereby we trigger an update of running animations whenever the traversal is triggered by changes to CSS rules and we have existing CSS animations.

::: layout/reftests/css-animations/change-animation-name-to-inexistent-in-rule.html:21
(Diff revision 2)
> +</style>
> +<div id="target"></div>
> +<script>
> +document.addEventListener('MozReftestInvalidate', () => {
> +  requestAnimationFrame(() => {
> +    document.styleSheets[0].cssRules[0].style.animationName = 'inexistent';

s/inexistent/non-existent/

both here and in the filename (and mochitest.ini)

(You can use either non-existent or nonexistent. non-existent is more common but Merriam-Webster dictionary lists nonexistent.)
Attachment #8900500 - Flags: review?(bbirtles) → review+

Comment 11

26 days ago
mozreview-review
Comment on attachment 8900589 [details]
Bug 1392851 - A reftest to check that we don't share styles for elements that have animations which are about to be removed.

https://reviewboard.mozilla.org/r/171990/#review177252
Attachment #8900589 - Flags: review?(bbirtles) → review+
(Assignee)

Comment 12

26 days ago
Created attachment 8900633 [details] [review]
Servo PR
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

26 days ago
https://hg.mozilla.org/integration/autoland/rev/95c4a9f2a723

Comment 16

26 days ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ae12100c3e8
Try to update running CSS animations when CSS rules are changed. r=birtles
https://hg.mozilla.org/integration/autoland/rev/b7e692446499
A reftest to check that we don't share styles for elements that have animations which are about to be removed. r=birtles
https://hg.mozilla.org/mozilla-central/rev/9ae12100c3e8
https://hg.mozilla.org/mozilla-central/rev/b7e692446499
Status: ASSIGNED → RESOLVED
Last Resolved: 25 days ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.