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.

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?

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?

(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 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.)

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

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