1228137 - RestyleManager::UpdateOnlyAnimationStyles should call AddStyleUpdatesTo even if it is called from PresShell::HandleEvent immediately after animations are manipulated by web-animation APIs

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A mochitest that mouse event is fired on the element for finished animation

Attaching mochitest fails on current trunk.
One of the failure reason is that UpdateOnlyAnimationStyles is not called when the function is called from PresShell::HandleEvent because doCSS[1] flag is set to false at that time.

[1] https://dxr.mozilla.org/mozilla-central/rev/099f695d31326c39595264c34988a0f4b7cbc698/layout/base/RestyleManager.cpp#1840

CommonAnimationManager::AddStyleUpdatesTo should be also passed the event time.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: pass event time to UpdateOnlyAnimationStyles and AddStyleUpdatesTo

The test case in attachment 8692183 [details] [diff] [review] still fails with this patch.

AnimationCollection::mStyleRuleRefreshTime is replaced with the last refresh time of RefreshDriver by someone before we process FlushAnimations. I guess CommonAnimationManager::GetAnimationRule is called from somewhere before FlushAnimations.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm almost certain we don't want to use event times here--for one, they're not guaranteed to be monotonically increasing.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #2)
> I'm almost certain we don't want to use event times here--for one, they're
> not guaranteed to be monotonically increasing.

Do you have another idea how we can process AddStyleUpdatesTo in UpdateOnlyAnimationStyles at the time when mouse movement happens soon after animations are manipulated by web-animation APIs?
In that cases, I guess animations which are triggered by :hover will not start correctly.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> (In reply to Brian Birtles (:birtles) from comment #2)
> > I'm almost certain we don't want to use event times here--for one, they're
> > not guaranteed to be monotonically increasing.
> 
> Do you have another idea how we can process AddStyleUpdatesTo in
> UpdateOnlyAnimationStyles at the time when mouse movement happens soon after
> animations are manipulated by web-animation APIs?
> In that cases, I guess animations which are triggered by :hover will not
> start correctly.

I'm currently building the patches from bug 1219236 to work out what the problem is, but the most obvious thing that comes to mind is simply to clear RestyleManager.mLastUpdateForThrottledAnimations when we get a call to RequestRestyle(Layer).

Comment 5

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #4)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > (In reply to Brian Birtles (:birtles) from comment #2)
> > > I'm almost certain we don't want to use event times here--for one, they're
> > > not guaranteed to be monotonically increasing.
> > 
> > Do you have another idea how we can process AddStyleUpdatesTo in
> > UpdateOnlyAnimationStyles at the time when mouse movement happens soon after
> > animations are manipulated by web-animation APIs?
> > In that cases, I guess animations which are triggered by :hover will not
> > start correctly.
> 
> I'm currently building the patches from bug 1219236 to work out what the
> problem is, but the most obvious thing that comes to mind is simply to clear
> RestyleManager.mLastUpdateForThrottledAnimations when we get a call to
> RequestRestyle(Layer).

Ah, that will work! I am wondering if we can unify those update times...

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Clear the last update time in RestyleManager

This patch does what Brian suggested in comment #4.

AddStyleUpdatesTo is called with this patch (Yay!), but the test case in attachment 869283 still fails. I need to investigate in further details.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Test that Test that mouse events on the finished animation is surely fired on the animation element when the mouse events happen immediately after animation.finish() is called

Another failure reason was that attachment 8692183 [details] [diff] [review] does not call waitForPaints after adding a new element.

The style for the new element is not calculated in event handling. Though I am not sure it's intentional or not, This new test avoids the behavior, adds an element without animation properties and waits for its paint, and then adds animation properties.

This test passes with attachment 8692285 [details] [diff] [review], does not pass without attachment 8692285 [details] [diff] [review].

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Clear RestyleManager::mLastUpdateForThrottledAnimations whenever RequestRestyle(Layer) is called

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I cleared review requests since the test case in attachment 8692334 [details] [diff] [review] failed on try server. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc8742458356

Near perma on e10s linux. I can't reproduce the failure locally at all yet.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

On E10S the test relies on part 1 patch (attachment 8688402 [details] [diff] [review]) for bug 1219236. Without the part 1 patch, mouse events are not passed to content window. As a result the test was timed out.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cbbbc8a5334
Try result with the part 1 patch seems good.

I will ask to land the part 1 patch in bug 1219236, and then I will ask to review patches in this bug.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8692334 [details] [diff] [review]
Part 2: Test that Test that mouse events on the finished animation is surely fired on the animation element when the mouse events happen immediately after animation.finish() is called

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

(Sorry, accidentally r+ed this patch when I meant to set it on part 1. r=me anyway, but with these comments addressed.)

::: layout/style/test/file_animations_styles_on_event.html
@@ +48,5 @@
> +
> +    div.addEventListener("mousemove", function(event) {
> +      is(event.target.id, "bug1228137",
> +         "The target of the animation should receive the mouse move event" +
> +         "on the position of the animation's effect end.");

Nit: There is no space between "event" and "on" here.

@@ +58,5 @@
> +    animation.finish();
> +
> +    // Mouse over where the animation is positioned at finished state.
> +    // We can't use synthesizeMouse here since synthesizeMouse causes
> +    // layout flush. We need to check the position without exlicit flushes.

"explicit flushes"

(or "without explicitly flushing layout")

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test that mouse events on the finished animation is surely fired on the animation element when the mouse events happen immediately after animation.finish() is called

Removed part number in the commit log, addressed review comments.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcfcc13b4a97

Now this bug has been fixed by patches in recent animation refactoring.  I guess it's bug 1232577?

We can land the test now.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/20d90d9a12ce

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/20d90d9a12ce