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

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Other Branch
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8692183 [details] [diff] [review]
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8692214 [details] [diff] [review]
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.
I'm almost certain we don't want to use event times here--for one, they're not guaranteed to be monotonically increasing.
(Assignee)

Comment 3

3 years ago
(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.
(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).
(Assignee)

Comment 5

3 years ago
(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...
(Assignee)

Updated

3 years ago
Summary: RestyleManager::UpdateOnlyAnimationStyles should be passed event time when it is called from PresShell::HandleEvent → RestyleManager::UpdateOnlyAnimationStyles should call AddStyleUpdatesTo even if it is called from PresShell::HandleEvent
(Assignee)

Updated

3 years ago
Summary: RestyleManager::UpdateOnlyAnimationStyles should call AddStyleUpdatesTo even if it is called from PresShell::HandleEvent → RestyleManager::UpdateOnlyAnimationStyles should call AddStyleUpdatesTo even if it is called from PresShell::HandleEvent immediately after animations are manipulated by web-animation APIs
(Assignee)

Comment 6

3 years ago
Created attachment 8692285 [details] [diff] [review]
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.
Attachment #8692214 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created 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

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].
Attachment #8692183 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8692335 [details] [diff] [review]
Part 1: Clear RestyleManager::mLastUpdateForThrottledAnimations whenever RequestRestyle(Layer) is called
Assignee: nobody → hiikezoe
Attachment #8692285 - Attachment is obsolete: true
Attachment #8692335 - Flags: review?(bbirtles)
(Assignee)

Updated

3 years ago
Attachment #8692334 - Flags: review?(bbirtles)
(Assignee)

Updated

3 years ago
Attachment #8692334 - Flags: review?(bbirtles)
(Assignee)

Updated

3 years ago
Attachment #8692335 - Flags: review?(bbirtles)
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8692335 - Attachment description: Part 2: Clear RestyleManager::mLastUpdateForThrottledAnimations whenever RequestRestyle(Layer) is called → Part 1: Clear RestyleManager::mLastUpdateForThrottledAnimations whenever RequestRestyle(Layer) is called
(Assignee)

Comment 10

3 years ago
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.
Depends on: 1219236
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8692334 - Flags: review?(bbirtles)
(Assignee)

Updated

3 years ago
Attachment #8692335 - Flags: review?(bbirtles)
Attachment #8692334 - Flags: review?(bbirtles) → review+
Attachment #8692335 - Flags: review?(bbirtles) → review+
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")
(Assignee)

Updated

3 years ago
Attachment #8692335 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8718547 [details] [diff] [review]
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.
Attachment #8692334 - Attachment is obsolete: true
Attachment #8718547 - Flags: review+
(Assignee)

Comment 14

3 years ago
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.
Keywords: checkin-needed

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20d90d9a12ce
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.