Closed Bug 1219236 Opened 9 years ago Closed 9 years ago

Constant style recalculations (shown in devtools) while moving mouse, after CSS transition/animation has finished

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
44 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: gordon, Assigned: hiro)

References

Details

Attachments

(5 files, 4 obsolete files)

example.jpg
(deleted), image/jpeg
Details
testcase.html
(deleted), text/html
Details
testcase 2 (slightly reduced)
(deleted), text/html
Details
Part 1: Flush throttle styles for all descendant sub documents and *root* document itself
(deleted), patch
roc
: review+
cbook
: checkin+
Details | Diff | Splinter Review
Part 2: Update animation styles only if necessary
(deleted), patch
Details | Diff | Splinter Review
Attached image example.jpg (deleted) — 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

Move the mouse over any page within our application.  Keep the movements within a confined area so as not to trigger any style changes due to :hover styles.


Actual results:

Firefox Dev tools profiler shows constant style recalculations with the hint:  CSSTransitions.


Expected results:

There should have been no style recalculations.

I've been trying to understand what's going on here for days.  The attached screenshot shows me scrolling but the issue occurs whenever the mouse is moved.

We have removed all "transitions" from our CSS and have even removed all mouse events added via javascript.  But still this occurs.

The animation inspector verifies that there are no animations running and no transitions occurring during this time.  

The recalculations are tiny and do not impact performance.  But I would like to understand where there are so many every frame when from my understanding, there should not be.
I should add... Unfortunately for various reasons I am unable to provide a public test case right now.  Obviously it is quite possible that it's something we are doing that is causing this as I don't seem to be able to replicate the issue on a static site.


The issue goes away if we add the following css:

body * {
    transition : none;
}

But as explained, we have tried removing every single occurrence of "transition" from our css and the problem remains. (And we have verified this by looping over every element in javascript and checking their computed styles)
Attached file testcase.html (deleted) — 
Well we have finally narrowed it down.  And cannot explain why it is happening.

I have create a small test case to illustrate the problem.  Open dev tools and run the profiler, you will see that whenever the mouse moves over the page a constant stream of style recalculations occur.

For this bug to occur the following conditions have to be met:

1. Opacity set at 0 via css stylesheet.
2. Applying a csstransition to the element via JS and then setting the opacity to 1.

Even removing the transition property after the element has finished animating still causes the bug.  

Perhaps it is some kind of animation optimization gone wrong?  Could it be that Firefox notices that the stylesheet is set as opacity 0 and but that the element is set at 1 via JS, and therefore keep calculating some sort of optimization as it is expecting the element to fade back to 0 at any given time?

That could be way off the mark, but other than that hunch I have absolutely no idea what is going on here.
Component: Untriaged → DOM: CSS Object Model
Product: Firefox → Core
A few more observations:

1. The issue occurs once an element has been transitioned once.
2. The constant style recalculations will continue until the element is removed from the DOM or until you remove the "transition" property via javascript.
3.  They continue to occur even if you set the element to "display:none", leaving zero visible elements in the DOM.

It is almost as though whatever animation loop is being used to interpolate CSS transitions is continuing to run after the transition is complete, and continuing to causing style recalculations forever more.
Component: DOM: CSS Object Model → Untriaged
Product: Core → Firefox
Component: DOM: CSS Object Model → CSS Parsing and Computation
Flags: needinfo?(dbaron)
One more observastion... The issue only occurs with async animations on.

layers.offmainthreadcomposition.async-animations = true;

Which of course is now default.
I can reproduce with latest Nightly as well as a debug build, FWIW. Hence, marking as NEW.

Though I'm not sure these style flushes are actually that expensive concerning -- they seem like no-op style flushes (unsurprisingly).  That is, when we hit RestyleManager::ProcessRestyles (via PresShell::FlushPendingNotifications calling ProcessPendingRestyles), we bail immediately because aRestyleTracker.Count() is 0.

Definitely worth considering whether these style flushes should be happening (or if they're inexpensive, if we should be reporting them & worrying web developers), though.
Status: UNCONFIRMED → NEW
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Product: Firefox → Core
It looks like the transition is staying in our TransitionManager, long after the transition has finished playing. I think the restyle is (indirectly) triggered by nsPresShell's call:
> 4088           mPresContext->TransitionManager()->FlushAnimations();
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=37d64688bf9d#4088

...which triggers a "RequestRestyle" call on behalf of the transition:
> 320 void
> 321 CommonAnimationManager::FlushAnimations()
> 322 {
> 324   for (AnimationCollection* collection = mElementCollections.getFirst();
> 325        collection; collection = collection->getNext()) {
[...]
> 335     collection->RequestRestyle(AnimationCollection::RestyleType::Standard);
> 336   }
https://mxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.cpp?rev=89ddaac28785#335

If I inspect collection->mElement, I can see that it's a 'div'; and collection->mAnimations->mHdr->mLength is 1 at this point (for the transition).

This RequestRestyle call is the source of the problem; it's what ends up making us register to be notified by the refreshdriver (for this sample).

birtles, is it surprising that we're keeping the transition object indefinitely in the TransitionManager in this case? (I suspect it is?)
Flags: needinfo?(bbirtles)
To clarify, the code-path I'm describing in comment 7 (FlushAnimations, collection->RequestRestyle, etc.) is triggered on every mouse-move (and is responsible for the style flush for that mouse-movement).
Attachment #8680130 - Attachment description: testcase 2 (slighlty reduced) → testcase 2 (slightly reduced)
(In reply to Daniel Holbert [:dholbert] from comment #6)

> Though I'm not sure these style flushes are actually that expensive
> concerning -- they seem like no-op style flushes (unsurprisingly).  That is,
> when we hit RestyleManager::ProcessRestyles (via
> PresShell::FlushPendingNotifications calling ProcessPendingRestyles), we
> bail immediately because aRestyleTracker.Count() is 0.
> 
> Definitely worth considering whether these style flushes should be happening
> (or if they're inexpensive, if we should be reporting them & worrying web
> developers), though.

Indeed, they are all sub 1ms with most being sub 0.1ms.  However, that is of course on a page with a single element.  

It appears that we get a single style recalc for every single transition that has ever been and gone where the element remains in the DOM.  Therefore performance deteriorates over time as you move around an app and initiate more transitions.  On our admittedly pretty large web app we start off with about 10 of these every frame after a page refresh with an entrance animation, but once you've used the app for some time (almost every interaction is animated) we end up with 150+ of these recalcs for every single frame.  Which adds up and eats in to our frame budget.

While for the majority of applications the impact would be negligible but it does point to something quite wrong (and likely quite easy to fix once identified)in the async animations code, that potentially may have a bigger impact elsewhere.

And yes, regardless of a fix, reporting them in developer tools is both worrying for developers and breaking on any large app.  We have 100-150 of these calls every frame on average, the profiler ends up grinding to a halt and it becomes impossible to see whats going on in a single frame.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> This RequestRestyle call is the source of the problem; it's what ends up
> making us register to be notified by the refreshdriver (for this sample).
> 
> birtles, is it surprising that we're keeping the transition object
> indefinitely in the TransitionManager in this case? (I suspect it is?)

No, that's expected. In bug 960465 the handling of transitions was changed to retain the latest transition for each property. That's needed to implement the updated spec behavior of transitions.

As for avoiding calling RequestRestyle for such transitions, I outlined a strategy in bug 1166500 comment 12 (specifically part d, "Look for no-change animations") that I think would fix this. Hiro is currently in process of implementing part of that.
Flags: needinfo?(bbirtles)
Thanks! I think this all makes sense then; sounds like we need to wait on the fix for bug 1166500. Marking as depends-on that bug, and clearing ni=dbaron.
Depends on: 1166500
Flags: needinfo?(dbaron)
Summary: Constant style recalculations while moving mouse → Constant style recalculations (shown in devtools) while moving mouse, after a CSS transition has played
Oh, wait, the strategy in bug 1166500 only helps avoid restyles on ticks, not on calls to FlushAnimations().

I think this might actually be a dupe of bug 1179535.
(Note that Bug 1179535 is about "mouse movement *during* OMT animation", per its summary, whereas this bug is about mouse movement *after* OMT animation. Though maybe it's all the same, because of how we retain the latest transition per comment 10 here.)
...and also, Bug 1179535 is focused on unnecessary repainting, whereas there's no repainting going on here, as far as I can tell w/ paint-flashing.  So I suspect this is not a dupe, though maybe I'm wrong & it's all the same root cause.
Actually, I think we'll fix this in bug 1190235 since that should make us ignore animations that are not "in effect" (which includes completed transitions) altogether for the sake of style updates.

(Basically, there's a lot of refactoring going on of animation at the moment but hopefully when the dust settles this and a number of similar issues should be either fixed or much easier to fix.)
I just want to add that in further testing we are seeing huge FPS drops during certain actions and animations due to this bug.

By disabling transitions globally with body { transition : none; } and enabling a single CSS transition our FPS remains at a steady 60FPS for any given animation.  The second we allow all transitions to be active, despite only the same single animation actually occurring, we drop below 20 FPS.

The dev tools timeline shows more than 200 of these no-op style recalculations for every single frame.  With each taking 0.3-0.8ms, the impact is undeniably huge.

I am of this opinion that this might be worth tackling more directly than simply waiting on the refactoring as it certainly appears to have a dramatic impact on any animation heavy application.
I don't think bug 1166500 is related to this. I'd say this issue is related to bug 1179535. The CSS transition is actually finished in the test case but I am guessing AnimationCollection (or something else) is considering the transition is still alive on mouse movements.
My guess was unusually right. Adding HasCurrentAnimations check just before calling EnsureStyleRuleFor in CommonAnimationManager::AddStyleUpdatesTo [1] stops unnecessary restyling.

[1] https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/style/AnimationCommon.cpp#222

We should remove finished transitions from AnimationCollection though.
Not only for CSS transtion but also CSS animation.
Summary: Constant style recalculations (shown in devtools) while moving mouse, after a CSS transition has played → Constant style recalculations (shown in devtools) while moving mouse, after CSS transition/animation has finished
According to bug 960465 comment 21, we should not remove the finished animations from AnimationCollection. 
So, I think adding HasCurrentAnimations check is the right thing to do here.
There is another trigger to update restyling.
Adding the HasCurrentAnimations check stops restying from https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/base/nsPresShell.cpp#7147. But another restyling comes from https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/base/nsPresShell.cpp#7878.

The latter might be a root cause of bug 1179535.
PreHandleEvent in PresShell::HandleEventInternal[1] ends up to call FlushAnimations.

[1] https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/base/nsPresShell.cpp#7878

The call stack is:

EventStateManager::PreHandleEvent
 PresShell::FlushPendingNotifications
  CommonAnimationManager::FlushAnimations

FlushAnimations is also called when animation is finished because we need to paint the last state of the animation. So we can not add HasCurrentAnimations in FlushAnimations.
This patch stops the unnecessary styling on *non* E10S. 
On E10S, the unnecessary styling still comes from EventStateManager::PreHandleEvent.
I found why atachment 8688287 does not fix this issue on E10S. That's because FlushThrottledStyles is called for neither the root document itself at [1] nor descendant sub documents at [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/base/nsPresShell.cpp#7147
[2] https://dxr.mozilla.org/mozilla-central/rev/4294bf91174b71ed7440dc491dac5d15394ec227/layout/base/nsPresShell.cpp#6573

So, yes, attachment 8688287 [details] [diff] [review] does not solve the issue in grandchild document on *non* E10S.
:roc, could you please review this since you commented for the original code in this patch in bug 914847 comment 49?
Attachment #8688287 - Attachment is obsolete: true
Attachment #8688402 - Flags: review?(roc)
Daniel, could you please review this?
Unfortunately I can not provide any automation tests for this issue because there is no way to check whether restyling is done or not in mochittest, I think.
Attachment #8688404 - Flags: review?(dholbert)
Comment on attachment 8688404 [details] [diff] [review]
Part 2: Don't update style for finished animations while mouse is moving.

I'll defer to birtles here; he's got a better idea of how this code fits together, I think.

(If he can't review for some reason, I'll be happy to take a closer look.)
Attachment #8688404 - Flags: review?(dholbert) → review?(bbirtles)
Comment on attachment 8688404 [details] [diff] [review]
Part 2: Don't update style for finished animations while mouse is moving.

I'm a bit worried this will break once we start updating animations through means other than style.

For example, if we have this:

  elem.style.animation = 'anim 2s 2'; // 2 iterations
  anim.finish();
  requestAnimationFrame(function() {
    anim.effect.timing.iterations = 1.5;
  });

We'll need to update style even though we remain finished.

Is it possible to rework this to use the style state rather than the timing state? For example, could we store the initial value of mStyleRule and compare it after running EnsureStyleRuleFor and if it hasn't changed then we'll know we don't need to add the pending restyle? (Even just querying mStyleChanging might work though I'm not sure.)
Flags: needinfo?(hiikezoe)
(In reply to Brian Birtles (:birtles) from comment #28)
> Comment on attachment 8688404 [details] [diff] [review]
> Part 2: Don't update style for finished animations while mouse is moving.
> 
> I'm a bit worried this will break once we start updating animations through
> means other than style.
> 
> For example, if we have this:
> 
>   elem.style.animation = 'anim 2s 2'; // 2 iterations
>   anim.finish();
>   requestAnimationFrame(function() {
>     anim.effect.timing.iterations = 1.5;
>   });
> 
> We'll need to update style even though we remain finished.
> 
> Is it possible to rework this to use the style state rather than the timing
> state? For example, could we store the initial value of mStyleRule and
> compare it after running EnsureStyleRuleFor and if it hasn't changed then
> we'll know we don't need to add the pending restyle? (Even just querying
> mStyleChanging might work though I'm not sure.)

Nice! I think that will work well. Then I would like to change EnsureStyleRuleFor to return boolean value and rename it to something, I have no good idea for the name though. TryToComposeStyle?
Flags: needinfo?(hiikezoe)
Also, when did this regress?  Is there still a chance we can avoid shipping it?
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Nice! I think that will work well. Then I would like to change
> EnsureStyleRuleFor to return boolean value and rename it to something, I
> have no good idea for the name though. TryToComposeStyle?

I think the name EnsureStyleRuleFor makes sense. It basically says, "Make sure mStyleRule is up to date." 

However, on further thought, I'm not sure if what I suggested will work. We call EnsureStyleRuleFor in a number of places so it's possible we could have updated the style rule already, then when we reach AddStyleUpdatesTo, when we call EnsureStyleRuleFor we'll think nothing has changed and so we won't add the pending restyle.

Also, I'm not confident the original patch would work either since I think HasCurrentAnimations() will return false on the first tick after we finish but we still want to add pending restyles in that case.
If we use mStyleChanging I think we'll face the same problem.

(I hope bug 1190235 will make this a lot easier but there's probably something we can do in the meantime.)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #30)
> Also, when did this regress?  Is there still a chance we can avoid shipping
> it?

In CSS animations case:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d05f091bf4d8d28c789e27a122bc9bfc096e8d63&tochange=2eddf957653d6d1a790ac681a8d86282295e4526

In CSS transitions case, the unnecessary restyling has been happened before this year. I did not track further. It might be more frequent after bug 960465.
Flags: needinfo?(hiikezoe)
(In reply to Brian Birtles (:birtles) from comment #28)
> Comment on attachment 8688404 [details] [diff] [review]
> Part 2: Don't update style for finished animations while mouse is moving.
> 
> I'm a bit worried this will break once we start updating animations through
> means other than style.
> 
> For example, if we have this:
> 
>   elem.style.animation = 'anim 2s 2'; // 2 iterations
>   anim.finish();
>   requestAnimationFrame(function() {
>     anim.effect.timing.iterations = 1.5;
>   });
> 
> We'll need to update style even though we remain finished.
> 
> Is it possible to rework this to use the style state rather than the timing
> state? For example, could we store the initial value of mStyleRule and
> compare it after running EnsureStyleRuleFor and if it hasn't changed then
> we'll know we don't need to add the pending restyle? (Even just querying
> mStyleChanging might work though I'm not sure.)

It seems like this is making us do a substantial amount of extra work in a common case to support an edge case.  Is there a way that the edge case could instead invalidate the style?
(In reply to Brian Birtles (:birtles) from comment #31)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > Nice! I think that will work well. Then I would like to change
> > EnsureStyleRuleFor to return boolean value and rename it to something, I
> > have no good idea for the name though. TryToComposeStyle?
> 
> I think the name EnsureStyleRuleFor makes sense. It basically says, "Make
> sure mStyleRule is up to date." 

OK. What do you think of returning boolean?

> However, on further thought, I'm not sure if what I suggested will work. We
> call EnsureStyleRuleFor in a number of places so it's possible we could have
> updated the style rule already, then when we reach AddStyleUpdatesTo, when
> we call EnsureStyleRuleFor we'll think nothing has changed and so we won't
> add the pending restyle.
> 
> Also, I'm not confident the original patch would work either since I think
> HasCurrentAnimations() will return false on the first tick after we finish
> but we still want to add pending restyles in that case.
> If we use mStyleChanging I think we'll face the same problem.

In that case FlushAnimations is surely processed in a subsequent tick. 

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
>--- a/layout/style/AnimationCommon.cpp
>+++ b/layout/style/AnimationCommon.cpp
>@@ -214,16 +214,26 @@ CommonAnimationManager::SizeOfIncludingT
> 
> void
> CommonAnimationManager::AddStyleUpdatesTo(RestyleTracker& aTracker)
> {
>   TimeStamp now = mPresContext->RefreshDriver()->MostRecentRefresh();
> 
>   for (AnimationCollection* collection = mElementCollections.getFirst();
>        collection; collection = collection->getNext()) {
>+    // Don't update style if the collection has no current animations,
>+    // otherwise unncessary restyling will be done while mouse is moving
>+    // for finished animations.
>+    if (!collection->HasCurrentAnimations()) {
>+      // In case of there is no current animations we need to update
>+      // mStyleRuleRefreshTime explicitly so that FlushAnimations skips
>+      // updating styles for those finished animations.
>+      collection->mStyleRuleRefreshTime = now;

FlushAnimations what I am trying to skip here is come from PresShell::HandleEvent (as I described in comment #22), which means the FlushAnimations is caused by *mouse moving*. So in the subsequent tick,
RefreshDriver()->MostRecentRefresh() will be advanced and then FlushAnimations will be surely processed.

In addition, ProcessPendingRestyles (which is the one of the two caller sides of AddStyleUpdatesTo, the other is FlushThrottledStyles in part 1 patch) is called from PresShell::FlushPendingNotifications, PresShell::Initialize and PresShell::ResizeReflowIgnoreOverride. 

In case of FlushPendingNotifications, FlushAnimations is called prior to ProcessPendingRestyles if necessary.
In the other cases, it ends up calling FlushAnimations when we finish animations in a subsequent tick.
Hiro, I don't really understand what you're proposing. What state are you going to check to decide if we should skip adding pending style updates or not?


(As we fill in more of the animation interfaces we'll have more situations where script can mutate animation state after the animation has finished. In each case, we'll call RequestRestyle(RestyleType::Layer):

  https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/layout/style/AnimationCommon.cpp#626

So we need to make sure whatever optimization we add in here, doesn't cause calls to RequestRestyle(RestyleType::Layer) to be effectively ignored.)
(In reply to Brian Birtles (:birtles) from comment #35)
> Hiro, I don't really understand what you're proposing. What state are you
> going to check to decide if we should skip adding pending style updates or
> not?
> 
> 
> (As we fill in more of the animation interfaces we'll have more situations
> where script can mutate animation state after the animation has finished. In
> each case, we'll call RequestRestyle(RestyleType::Layer):

Ah, I am sorry I was misunderstanding that you are talking about RestyleType::Standard case when animations are finished.

> https://dxr.mozilla.org/mozilla-central/rev/
> a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/layout/style/AnimationCommon.cpp#626
> 
> So we need to make sure whatever optimization we add in here, doesn't cause
> calls to RequestRestyle(RestyleType::Layer) to be effectively ignored.)

Though I might be still misunderstanding, RequestRestyle adds pending style updates in either cases (Standard or Layer) apart from AddStyleUpdatesTo.  Does that answer to what you are worrying about?

[1] https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/layout/style/AnimationCommon.cpp#646
Comment on attachment 8688404 [details] [diff] [review]
Part 2: Don't update style for finished animations while mouse is moving.

I was a bit concerned about using timing state here for this, but I think you've convinced me it's ok.

I've filed bug 1226047 and added a comment there to make sure we check this optimization doesn't cause problems once we allow manipulating animations directly after they've finished.
Attachment #8688404 - Flags: review?(bbirtles) → review+
Brian's concern was right.
The change caused intermittent failure on try. (Thanks for the test!)
https://treeherder.mozilla.org/logviewer.html#?job_id=13910414&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=13914146&repo=try

I have never seen the failure locally yet (over 100times on linux64 debug build) for now.
Although I've never reproduce the failure locally yet, I guess the failure reason is that mStyleRuleRefreshTime is updated in AddStyleUpdatesTo.
In failure cases, AddStyleUpdatesTo was called somehow before calling RequestRestyle when transition is finished.

This patch updates last style update time in FlushThrottledStyles instead of updating mStyleRuleRefreshTime in AddStyleUpdatesTo.  This is much safer because FlushThrottledStyles is called only from PresShell::HandleEvent (i.e. not from refresh driver tick), on the other hand
AddStyleUpdatesTo is called from either places.

Here are dozen of try runs with this new patch on environments that previously failed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=982a6d57cfde (mochitest 5)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a2c4dc80c3 (mochitest 25)
Attachment #8688404 - Attachment is obsolete: true
Attachment #8690008 - Flags: review?(bbirtles)
I'm having a really hard time trying to make sure this is right. Here's what I understand so far.

If we have an animation that finishes, we'll call AnimationCollection::RequestRestyle(AnimationCollection::RestyleType::Standard) from Animation::Tick() because we have special code there to detect when the animation finishes (i.e. it *won't* request a throttled restyle in this case).

AnimationCollection::RequestRestyle(AnimationCollection::RestyleType::Standard) will cause us to add a pending animation style to the pres context's restyle manager's mPendingRestyles.

As I understand it, that pending restyle will be processed before we come to handle any events (as part of the remainder of nsRefreshDriver::Tick).

If that's right, then it would seem to be safe to skip finished animations in UpdateOnlyAnimationStyles, at least for animations that finish during normal ticking. Any newly-finished animation will have already added a pending restyle that will have been processed by the time we get to HandleEvent.

If we manually cause an animation to finish (by seeking it or calling finish()), then we'll end up calling AnimationCollection::RequestRestyle(AnimationCollection::RestyleType::Layer) which will add a pending restyle and also reset mStyleRuleRefreshTime, mStyleChanging (setting it to true), and nsPresContext::mLastStyleUpdateForAllAnimations.

I suppose the pending restyle will be processed whenever we next call PresShell::FlushPendingNotifications which happens all over the place but it's not clear to me if anywhere guarantees that will happen by the time we process the next mouse event.

So, I'm wondering if the following would work:

1. Drop the change to FlushThrottledStyles in part 2. (Actually, I don't understand why this was added.)
2. Make the change to AddStyleUpdatesTo check for:
   if (!collection->HasCurrentAnimations() && !collection->mStyleChanging)

That's not very nice, but first I'd like to know if my understanding is correct and if that works.
(In reply to Brian Birtles (:birtles) from comment #40)
> I suppose the pending restyle will be processed whenever we next call
> PresShell::FlushPendingNotifications which happens all over the place but
> it's not clear to me if anywhere guarantees that will happen by the time we
> process the next mouse event.

The pending restyle in AddStyleUpdatesTo is processed soon in UpdateOnlyAnimationStyles [1]

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp#1877

So, the procedure on a mouse event is:

 PresShell::HandleEvent
  FlushThrottledStyles
   UpdateOnlyAnimationStyles
    AddStyleUpdatesTo
    ProcessRestyles
  ...
  PresShell::HandleEventInternal
   EventStateManager::PreHandleEvent
    PresShell::FlushPendingNotifications
    CommonAnimationManager::FlushAnimations

> 1. Drop the change to FlushThrottledStyles in part 2. (Actually, I don't
> understand why this was added.)

As far as I can tell, the change needs to skip the FlushAnimations in above procedure list.
But the change in attachment 8690008 [details] [diff] [review] might be overkill. We should skip the FlushAnimations only if there are no running animations.

I think, in case of finished animations, we can skip restylings both in AddStyleUpdatesTo and FlushAnimations caused by mouse events. But in case of running animations I am now thinking we can not skip the FlushAnimations called from PreHandleEvent. The FlushAnimations should not involve any paints in case of compositor animations though (bug 1179535).
dbaron?
Flags: needinfo?(dbaron)
Comment on attachment 8690008 [details] [diff] [review]
Part 2: Don't update style for finished animations while mouse is moving v2

Clearing review request.
I am waiting for dbaron's opinions.
Attachment #8690008 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> (In reply to Brian Birtles (:birtles) from comment #40)
> > I suppose the pending restyle will be processed whenever we next call
> > PresShell::FlushPendingNotifications which happens all over the place but
> > it's not clear to me if anywhere guarantees that will happen by the time we
> > process the next mouse event.
> 
> The pending restyle in AddStyleUpdatesTo is processed soon in
> UpdateOnlyAnimationStyles [1]
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.
> cpp#1877

That's processing the restyles in the local RestyleTracker, |tracker|, not restyles in RestyleManager::mPendingRestyles, though right?

> So, the procedure on a mouse event is:
> 
>  PresShell::HandleEvent
>   FlushThrottledStyles
>    UpdateOnlyAnimationStyles
>     AddStyleUpdatesTo
>     ProcessRestyles
>   ...
>   PresShell::HandleEventInternal
>    EventStateManager::PreHandleEvent
>     PresShell::FlushPendingNotifications
>     CommonAnimationManager::FlushAnimations
> 
> > 1. Drop the change to FlushThrottledStyles in part 2. (Actually, I don't
> > understand why this was added.)
> 
> As far as I can tell, the change needs to skip the FlushAnimations in above
> procedure list.
> But the change in attachment 8690008 [details] [diff] [review] might be
> overkill. We should skip the FlushAnimations only if there are no running
> animations.

Ok.
(In reply to Brian Birtles (:birtles) from comment #43)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > (In reply to Brian Birtles (:birtles) from comment #40)
> > > I suppose the pending restyle will be processed whenever we next call
> > > PresShell::FlushPendingNotifications which happens all over the place but
> > > it's not clear to me if anywhere guarantees that will happen by the time we
> > > process the next mouse event.
> > 
> > The pending restyle in AddStyleUpdatesTo is processed soon in
> > UpdateOnlyAnimationStyles [1]
> > 
> > [1]
> > https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.
> > cpp#1877
> 
> That's processing the restyles in the local RestyleTracker, |tracker|, not
> restyles in RestyleManager::mPendingRestyles, though right?

Right. That means pending restyles in normal tick are processed regardless of AddStyleUpdatesTo.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> (In reply to Brian Birtles (:birtles) from comment #43)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > > (In reply to Brian Birtles (:birtles) from comment #40)
> > > > I suppose the pending restyle will be processed whenever we next call
> > > > PresShell::FlushPendingNotifications which happens all over the place but
> > > > it's not clear to me if anywhere guarantees that will happen by the time we
> > > > process the next mouse event.
> > > 
> > > The pending restyle in AddStyleUpdatesTo is processed soon in
> > > UpdateOnlyAnimationStyles [1]
> > > 
> > > [1]
> > > https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.
> > > cpp#1877
> > 
> > That's processing the restyles in the local RestyleTracker, |tracker|, not
> > restyles in RestyleManager::mPendingRestyles, though right?
> 
> Right. That means pending restyles in normal tick are processed regardless
> of AddStyleUpdatesTo.

But what about the pending restyles from AnimationCollection::RequestRestyle(AnimationCollection::RestyleType::Layer) which was called by Animation::Finish()?

It's not the normal tick case I'm concerned about or AddStyleUpdatesTo. I'm concerned about when script makes an animation become finished outside of the normal tick.
(In reply to Brian Birtles (:birtles) from comment #45)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> > (In reply to Brian Birtles (:birtles) from comment #43)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > > > (In reply to Brian Birtles (:birtles) from comment #40)
> > > > > I suppose the pending restyle will be processed whenever we next call
> > > > > PresShell::FlushPendingNotifications which happens all over the place but
> > > > > it's not clear to me if anywhere guarantees that will happen by the time we
> > > > > process the next mouse event.
> > > > 
> > > > The pending restyle in AddStyleUpdatesTo is processed soon in
> > > > UpdateOnlyAnimationStyles [1]
> > > > 
> > > > [1]
> > > > https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.
> > > > cpp#1877
> > > 
> > > That's processing the restyles in the local RestyleTracker, |tracker|, not
> > > restyles in RestyleManager::mPendingRestyles, though right?
> > 
> > Right. That means pending restyles in normal tick are processed regardless
> > of AddStyleUpdatesTo.
> 
> But what about the pending restyles from
> AnimationCollection::RequestRestyle(AnimationCollection::RestyleType::Layer)
> which was called by Animation::Finish()?
> 
> It's not the normal tick case I'm concerned about or AddStyleUpdatesTo. I'm
> concerned about when script makes an animation become finished outside of
> the normal tick.

In that case, AddPendingRestyle is called against mPendingRestyles in RestyleManager::PostRestyleEvent.
https://dxr.mozilla.org/mozilla-central/rev/d3d286102ba7f8801e9dfe12d534f49554ba50c0/layout/base/RestyleManager.cpp#1898

Call trace at that time:

AnimationCollection::RequestRestyle(Layer)
 AnimationCollection::PostRestyleForAnimation
  nsIPresShell::RestyleForAnimation
   RestyleManager::PostRestyleEvent

So, if we call RequestRestyle(Layer) (Animation::PostUpdate?) whenever we need to update animation styles which are caused by script, then the style is surely updated, I think.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> > It's not the normal tick case I'm concerned about or AddStyleUpdatesTo. I'm
> > concerned about when script makes an animation become finished outside of
> > the normal tick.
> 
> In that case, AddPendingRestyle is called against mPendingRestyles in
> RestyleManager::PostRestyleEvent.
> https://dxr.mozilla.org/mozilla-central/rev/
> d3d286102ba7f8801e9dfe12d534f49554ba50c0/layout/base/RestyleManager.cpp#1898
> 
> Call trace at that time:
> 
> AnimationCollection::RequestRestyle(Layer)
>  AnimationCollection::PostRestyleForAnimation
>   nsIPresShell::RestyleForAnimation
>    RestyleManager::PostRestyleEvent
> 
> So, if we call RequestRestyle(Layer) (Animation::PostUpdate?) whenever we
> need to update animation styles which are caused by script, then the style
> is surely updated, I think.

The question is when is the pending restyle processed?

"I suppose the pending restyle will be processed whenever we next call PresShell::FlushPendingNotifications which happens all over the place but it's not clear to me if anywhere guarantees that will happen by the time we process the next mouse event."
(In reply to Brian Birtles (:birtles) from comment #47)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #46)
> > > It's not the normal tick case I'm concerned about or AddStyleUpdatesTo. I'm
> > > concerned about when script makes an animation become finished outside of
> > > the normal tick.
> > 
> > In that case, AddPendingRestyle is called against mPendingRestyles in
> > RestyleManager::PostRestyleEvent.
> > https://dxr.mozilla.org/mozilla-central/rev/
> > d3d286102ba7f8801e9dfe12d534f49554ba50c0/layout/base/RestyleManager.cpp#1898
> > 
> > Call trace at that time:
> > 
> > AnimationCollection::RequestRestyle(Layer)
> >  AnimationCollection::PostRestyleForAnimation
> >   nsIPresShell::RestyleForAnimation
> >    RestyleManager::PostRestyleEvent
> > 
> > So, if we call RequestRestyle(Layer) (Animation::PostUpdate?) whenever we
> > need to update animation styles which are caused by script, then the style
> > is surely updated, I think.
> 
> The question is when is the pending restyle processed?
> 
> "I suppose the pending restyle will be processed whenever we next call
> PresShell::FlushPendingNotifications which happens all over the place but
> it's not clear to me if anywhere guarantees that will happen by the time we
> process the next mouse event."

Thanks! Now I understand what you are concerned.
In that case, PresShell::FlushPendingNotifications called from EventStateManager::FlushPendingEvents processes the pending restyle. That means we can not skip FlushAnimations called from PreHandleEvent.
Clearing ni? since we should call FlushAnimations in case of finished animations.
Flags: needinfo?(dbaron)
This test case checks that mouse movement on the place where the animation is positioned on finished state is surely fired against correct target element soon after animation.finish() is called.

If I understand Brian's concern, this test can be used for checking overkill of finished animations restyling when this bug is fixed.
*BUT* unfortunately this test case fails on current trunk. So the test case might be wrong or I am still misunderstanding something.

Brian, any ideas?
Assignee: nobody → hiikezoe
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> Created attachment 8691748 [details] [diff] [review]
> A mochitest that mouse event is fired on the element for finished animation
> 
> This test case checks that mouse movement on the place where the animation
> is positioned on finished state is surely fired against correct target
> element soon after animation.finish() is called.
> 
> If I understand Brian's concern, this test can be used for checking overkill
> of finished animations restyling when this bug is fixed.
> *BUT* unfortunately this test case fails on current trunk.

I found one of the failure reasons of this test. (Filed as bug 1228137)
I think there are still other reason of the failure.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #50)
> Created attachment 8691748 [details] [diff] [review]
> A mochitest that mouse event is fired on the element for finished animation
> 
> This test case checks that mouse movement on the place where the animation
> is positioned on finished state is surely fired against correct target
> element soon after animation.finish() is called.
> 
> If I understand Brian's concern, this test can be used for checking overkill
> of finished animations restyling when this bug is fixed.
> *BUT* unfortunately this test case fails on current trunk. So the test case
> might be wrong or I am still misunderstanding something.
> 
> Brian, any ideas?

I don't really understand how bug 1228137 is a problem. We shouldn't need the event time. Normally animations will only update on a refresh driver tick and that should happen before we process events. So the refresh driver time should be the correct one to use. If script is modifying animations after then I could imagine that we'd need to clear RestyleManager's mLastUpdateForThrottledAnimations in order to get those changes reflected but I don't think that's happening in this test.
(In reply to Brian Birtles (:birtles) from comment #52)
> If script is modifying animations
> after then I could imagine that we'd need to clear RestyleManager's
> mLastUpdateForThrottledAnimations in order to get those changes reflected
> but I don't think that's happening in this test.

Actually, this test calls finish() so that is what is happening. I think we discussed in bug 1228137 a simple way to make sure AddStyleUpdatesTo gets called in this case. I don't quite understand how that will fix the problem however, since part 2 from this bug adds an early return to AddStyleUpdatesTo if the following is true:

  if (!collection->HasCurrentAnimations()) {

And in this case, we won't have any current animations so we'll return early.

So I don't quite understand how it helps. I thought we want to process the pending restyles in the restyle manager in order to update here, not the pending restyles added to the tracker in AddStyleUpdatesTo.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #53)
> (In reply to Brian Birtles (:birtles) from comment #52)
> > If script is modifying animations
> > after then I could imagine that we'd need to clear RestyleManager's
> > mLastUpdateForThrottledAnimations in order to get those changes reflected
> > but I don't think that's happening in this test.
> 
> Actually, this test calls finish() so that is what is happening. I think we
> discussed in bug 1228137 a simple way to make sure AddStyleUpdatesTo gets
> called in this case. I don't quite understand how that will fix the problem
> however, since part 2 from this bug adds an early return to
> AddStyleUpdatesTo if the following is true:
> 
>   if (!collection->HasCurrentAnimations()) {
> 
> And in this case, we won't have any current animations so we'll return early.

Thanks! Your concern was very helpful. Now I am also thinking the current animations check is not suitable there in the particular case.
Bug 1228137 is less relevant to this finished animations issue because bug 1228137 is caused by setting currentTime instead of calling finish().
Blocks: 1228137
This patch does something similar to what Brian suggested in comment #28 and I suggested in comment #29.

EnsureStyleRuleFor returns a boolean value indicating whether the style rule was changed or not. And AddStyleUpdatesTo checks the return value to add a pending style.
Attachment #8690008 - Attachment is obsolete: true
Attachment #8692674 - Flags: review?(bbirtles)
Comment on attachment 8691748 [details] [diff] [review]
A mochitest that mouse event is fired on the element for finished animation

This test case was moved into bug 1228137.
Attachment #8691748 - Attachment is obsolete: true
(In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> Created attachment 8692674 [details] [diff] [review]
> Part 2: Update animation styles only if necessary
> 
> This patch does something similar to what Brian suggested in comment #28 and
> I suggested in comment #29.

I don't think that works since EnsureStyleRuleFor gets called from many different places. Something else might call EnsureStyleRuleFor before we get to CommonAnimationManager::AddStyleUpdatesTo. Then in AddStyleUpdatesTo we will think nothing has changed and skip adding the pending restyle right? See comment 31.

I wonder if we should just wait for bug 1190235 to land which I'm working on now. Part of the plan there is to maintain a hash of effects that have been throttled. Then when we come to flush throttled animation styles we only need to update those effects.
(In reply to Brian Birtles (:birtles) from comment #57)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #55)
> > Created attachment 8692674 [details] [diff] [review]
> > Part 2: Update animation styles only if necessary
> > 
> > This patch does something similar to what Brian suggested in comment #28 and
> > I suggested in comment #29.
> 
> I don't think that works since EnsureStyleRuleFor gets called from many
> different places. Something else might call EnsureStyleRuleFor before we get
> to CommonAnimationManager::AddStyleUpdatesTo. Then in AddStyleUpdatesTo we
> will think nothing has changed and skip adding the pending restyle right?
> See comment 31.

Right. That is actually what I intend to do in this patch. 

(In reply to Brian Birtles (:birtles) from comment #57)
> I wonder if we should just wait for bug 1190235 to land which I'm working on
> now. Part of the plan there is to maintain a hash of effects that have been
> throttled. Then when we come to flush throttled animation styles we only
> need to update those effects.

I don't quite understand what bug 1190235 achieves, so, I am waiting what happens in bug 1190235 specifically.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #58)
> (In reply to Brian Birtles (:birtles) from comment #57)
> > I don't think that works since EnsureStyleRuleFor gets called from many
> > different places. Something else might call EnsureStyleRuleFor before we get
> > to CommonAnimationManager::AddStyleUpdatesTo. Then in AddStyleUpdatesTo we
> > will think nothing has changed and skip adding the pending restyle right?
> > See comment 31.
> 
> Right. That is actually what I intend to do in this patch. 

But EnsureStyleRuleFor can be called without also posting pending restyles. In that case we should post the pending restyle in AddStyleUpdatesTo, right?
Comment on attachment 8692674 [details] [diff] [review]
Part 2: Update animation styles only if necessary

Clearing review request since this patch still includes what I don't quite understand how styling process is done between tick and event handling.
Attachment #8692674 - Flags: review?(bbirtles)
Comment on attachment 8688402 [details] [diff] [review]
Part 1: Flush throttle styles for all descendant sub documents and *root* document itself

Hello sheriffs,

Can you please check part 1 patch in?  The part 1 patch is also necessary for bug 1228137.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dd6824ddee0
Attachment #8688402 - Flags: checkin?
Keywords: leave-open
(In reply to Hiroyuki Ikezoe (:hiro) from comment #61)
> Comment on attachment 8688402 [details] [diff] [review]
> Part 1: Flush throttle styles for all descendant sub documents and *root*
> document itself
> 
> Hello sheriffs,
> 
> Can you please check part 1 patch in?  The part 1 patch is also necessary
> for bug 1228137.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dd6824ddee0

Hi, this caused a bustage due to a typo error so i had to push a follow up to fix the error like:

    1.12 -        FlushThrottledStyles(GetRootPresShell()->GetDOcument(), nullptr); (that was the typo)
    1.13 +        FlushThrottledStyles(GetRootPresShell()->GetDocument(), nullptr);
Flags: needinfo?(hiikezoe)
Thank you, and I am sorry for the mistake. I forgot to update attachment 8688402 [details] [diff] [review] before setting checkin flag.
Flags: needinfo?(hiikezoe)
Attachment #8688402 - Flags: checkin? → checkin+
Depends on: 1237454
I confirmed this issue has been fixed by a patch for bug 1232577 using test cases in bug 123528.
The patch is actually part 17 in that bug.

I think the talos regression has also been fixed now.
Status: NEW → RESOLVED
Closed: 9 years ago
Depends on: 1232577
No longer depends on: 1166500, 1237454
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: