1379515 - Run test_restyle.html in content

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

See bug 1326306 comment 41.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I have to fix this for the test case for bug 1190721.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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

I hope no failure happens here.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Do'h!  I shouldn't have used autoland. And forgot adding an important test file.

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Unfortunately a transition test starts failing intermittently.  Given that a try on Mac [1] never failed, the failure happens only on Linux?  Why are there much differences depending on platforms?

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I could reproduce the failure, but...  it's unbelievable, a redundant restyle was caused by a mouse move event, I never touched mouse while running the test case.  The stack at that time is:

#16 0x00007f18f1ad7c03 in mozilla::PresShell::HandleEvent (this=0x7f18cbbcf000, aFrame=0x7f18cbbd2120, aEvent=0x7ffda81a5700, aDontRetargetEvents=false, aEventStatus=0x7ffda81a55c0, 
    aTargetContent=0x0) at /home/ikezoe/central/layout/base/PresShell.cpp:7360
#17 0x00007f18f165d979 in nsViewManager::DispatchEvent (this=0x7f18cd9f1b80, aEvent=0x7ffda81a5700, aView=0x7f18cd9eac00, aStatus=0x7ffda81a55c0)
    at /home/ikezoe/central/view/nsViewManager.cpp:812
#18 0x00007f18f1acaa3a in mozilla::PresShell::DispatchSynthMouseMove (this=0x7f18c47d1000, aEvent=0x7ffda81a5700, aFlushOnHoverChange=true)
    at /home/ikezoe/central/layout/base/PresShell.cpp:3756
#19 0x00007f18f1ad1a28 in mozilla::PresShell::ProcessSynthMouseMoveEvent (this=0x7f18cbbcf000, aFromScroll=false) at /home/ikezoe/central/layout/base/PresShell.cpp:5713
#20 0x00007f18f1afaa8b in mozilla::PresShell::nsSynthMouseMoveEvent::WillRefresh (this=0x7f18ad0ffca0, aTime=...)
    at /home/ikezoe/central/obj-firefox/dist/include/mozilla/PresShell.h:655
#21 0x00007f18f1a8aa9c in nsRefreshDriver::Tick (this=0x7f18cc96f400, aNowEpoch=1509091582629509, aNowTime=...) at /home/ikezoe/central/layout/base/nsRefreshDriver.cpp:1843

Comment 6

[Brian Birtles (:birtles)]

SynthMouseMove would be a synthetic (fake) mouse move.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Yep, I am trying to understand how it happens.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I am still tracking down how it happens, but now I know why the synthetic mouse move does not happens when the test runs as chrome mochitest.  The reason is that when the test runs as chrome mochitest, the test_restyle.html is not for the root pres context, the root pres context is browser.xul, so it never happens on child's pres context.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I understand what happens.  To start working the synthetic mouse move, mMouseLocation needs to be updated.  To update mMouseLocation, it requires an event.  The event actually comes from  enter_notify_event_cb in widget/gtk/nsWindow.cpp.  The 'enter-notify' happens when the window ifself created.  That's the reason why the failure happens only on Linux.  I am going to set layout.reflow.synthMouseMove false for the test.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

In content window, there seems to be a case that scrolling frame is not processed within a frame.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1837ae50056ad449017030df0c13aebe268152a8&selectedJob=140186741

I guess that's because it processes asynchronously?

A new try with waiting two frames there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b8e605f1e4bb6361d019b265c262df4a7c7e44

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> In content window, there seems to be a case that scrolling frame is not
> processed within a frame.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1837ae50056ad449017030df0c13aebe268152a8&selectedJob=1
> 40186741
> 
> I guess that's because it processes asynchronously?

Given the fact that there is no failures on Linux opt build, it's plausible.  So once we made the test run on Android again (I mean on more slower platforms), it might be possible to fail there, but I am not sure since it did not fail with waiting a single frame on this try.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements.

It much makes more sence to check the animation in out-of-view is throttled
and unthrottled once it got visible in a single test.

Review commit: https://reviewboard.mozilla.org/r/194208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194208/

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Change a child element position to be able to move into view of the parent by mouse wheel.

Before this patch the child element actually did not move into the view and
observed an unrelated restyle maker.  In the next patch we fix the observation
of the unrelated restyle maker.

Review commit: https://reviewboard.mozilla.org/r/194210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194210/

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Make test cases that use synthesizeWheelAtPoint proper.

We shouldn't call synthesizeWheelAtPoint() in an rAF callback and observe
animation restyle makers there since we might end up observing an animation
restyle maker which is brought by flushing throttled animation styles for
hit-testing caused by the wheel event.  To avoid the confusion, we need to
wait for a frame and starts observing after the frame.

Review commit: https://reviewboard.mozilla.org/r/194212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194212/

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Propagate testharness functions only if we use testharness.js.

We are going to use SimpleTest.js in the sub window.

Review commit: https://reviewboard.mozilla.org/r/194214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194214/

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Run test_restyles.html in content.

Review commit: https://reviewboard.mozilla.org/r/194216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194216/

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event.

The synthetic mouse move flushes animation styles during running test cases on
Linux, it breaks these tests!

Review commit: https://reviewboard.mozilla.org/r/194218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194218/

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1379515 - Set layout.reflow.synthMouseMove false to avoid synthetic mouse move.

In content window, mouse wheel event is not sometimes processed in a single
frame.

Review commit: https://reviewboard.mozilla.org/r/194220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/194220/

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Cleared review some requests since they didn't fix the failure properly. What we really need here is another variant of EventUtils.sendWheelAndPaint without flushing layout. We need to wait for wheel event just like the function does.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923007 [details]
Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194208/diff/1-2/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923008 [details]
Bug 1379515 - Change a child element position to be able to move into view of the parent by mouse wheel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194210/diff/1-2/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923012 [details]
Bug 1379515 - Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194218/diff/1-2/

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923009 [details]
Bug 1379515 - Make test cases that use synthesizeWheelAtPoint proper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194212/diff/1-2/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923010 [details]
Bug 1379515 - Propagate testharness functions only if we use testharness.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194214/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923011 [details]
Bug 1379515 - Run test_restyles.html in content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194216/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923013 [details]
Bug 1379515 - Set layout.reflow.synthMouseMove false to avoid synthetic mouse move.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194220/diff/1-2/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

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

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8923012 [details]
Bug 1379515 - Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event.

https://reviewboard.mozilla.org/r/194218/#review199214

Comment 30

[Brian Birtles (:birtles)]

Comment on attachment 8923007 [details]
Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements.

https://reviewboard.mozilla.org/r/194208/#review199318

::: commit-message-ecef0:3
(Diff revision 2)
> +Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements. r?birtles
> +
> +It much makes more sence to check the animation in out-of-view is throttled

Nit: sense

::: dom/animation/test/chrome/test_restyles.html:304
(Diff revision 2)
>  
>      is(markers.length, 0,
>         'Animations running on the main-thread for elements ' +
>         'which are scrolled out should never cause restyles');
>  
> +    var parentRect = parentElement.getBoundingClientRect();

This added section could do with a one line comment to say what we're doing.

::: dom/animation/test/chrome/test_restyles.html:313
(Diff revision 2)
> +    var markers = await observeStyling(1, function() {
> +      // We can't use synthesizeWheel here since synthesizeWheel causes
> +      // layout flush.
> +      synthesizeWheelAtPoint(centerX, centerY,
> +                             { deltaMode: WheelEvent.DOM_DELTA_PIXEL,
> +                               deltaY: 100 });

(Would deltaY: parentRect.height make more sense?)

Comment 31

[Brian Birtles (:birtles)]

Comment on attachment 8923008 [details]
Bug 1379515 - Change a child element position to be able to move into view of the parent by mouse wheel.

https://reviewboard.mozilla.org/r/194210/#review199320

Comment 32

[Brian Birtles (:birtles)]

Comment on attachment 8923009 [details]
Bug 1379515 - Make test cases that use synthesizeWheelAtPoint proper.

https://reviewboard.mozilla.org/r/194212/#review199322

::: commit-message-1b11b:6
(Diff revision 2)
> +hit-testing caused by the wheel event.  To avoid the confusion, we need to
> +wait for a frame and starts observing after the frame.

I don't see any waiting for a frame here. Is this comment right?

Comment 33

[Brian Birtles (:birtles)]

Comment on attachment 8923010 [details]
Bug 1379515 - Propagate testharness functions only if we use testharness.js.

https://reviewboard.mozilla.org/r/194214/#review199324

Comment 34

[Brian Birtles (:birtles)]

Comment on attachment 8923011 [details]
Bug 1379515 - Run test_restyles.html in content.

https://reviewboard.mozilla.org/r/194216/#review199326

::: commit-message-e3aab:1
(Diff revision 2)
> +Bug 1379515 - Run test_restyls.html in content. r?birtles

test_restyles.html

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #30)

> ::: dom/animation/test/chrome/test_restyles.html:304
> (Diff revision 2)
> >  
> >      is(markers.length, 0,
> >         'Animations running on the main-thread for elements ' +
> >         'which are scrolled out should never cause restyles');
> >  
> > +    var parentRect = parentElement.getBoundingClientRect();
> 
> This added section could do with a one line comment to say what we're doing.

Will do.

> ::: dom/animation/test/chrome/test_restyles.html:313
> (Diff revision 2)
> > +    var markers = await observeStyling(1, function() {
> > +      // We can't use synthesizeWheel here since synthesizeWheel causes
> > +      // layout flush.
> > +      synthesizeWheelAtPoint(centerX, centerY,
> > +                             { deltaMode: WheelEvent.DOM_DELTA_PIXEL,
> > +                               deltaY: 100 });
> 
> (Would deltaY: parentRect.height make more sense?)

Good catch!  Indeed, we should now use the parent rect value here.

Comment 36

[Brian Birtles (:birtles)]

Comment on attachment 8923013 [details]
Bug 1379515 - Set layout.reflow.synthMouseMove false to avoid synthetic mouse move.

https://reviewboard.mozilla.org/r/194220/#review199328

(For my own reference, it looks like this synth mouse move is so that when we update layout we apply hover styles etc.. Bug 20022.)

Comment 37

[Brian Birtles (:birtles)]

Thank you for being so thorough and taking the time to track down all the test failures and tidy up the tests properly!

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Thank you as well for the review!

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923007 [details]
Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194208/diff/2-3/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923008 [details]
Bug 1379515 - Change a child element position to be able to move into view of the parent by mouse wheel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194210/diff/2-3/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923012 [details]
Bug 1379515 - Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194218/diff/2-3/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923009 [details]
Bug 1379515 - Make test cases that use synthesizeWheelAtPoint proper.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194212/diff/2-3/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923010 [details]
Bug 1379515 - Propagate testharness functions only if we use testharness.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194214/diff/2-3/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923011 [details]
Bug 1379515 - Run test_restyles.html in content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194216/diff/2-3/

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8923013 [details]
Bug 1379515 - Set layout.reflow.synthMouseMove false to avoid synthetic mouse move.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/194220/diff/2-3/

Comment 46

[Hiroyuki Ikezoe (:hiro)]

A final try just in case;  (It's only run on Linux64 though)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d0a3da6bcc005426ba2077b0444728b089e774

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e4c1121dea3
Unify test cases that checks animation are/were in scrolled out elements. r=birtles
https://hg.mozilla.org/integration/autoland/rev/1f139cd6bdae
Change a child element position to be able to move into view of the parent by mouse wheel. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d20341a660b6
Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event. r=smaug
https://hg.mozilla.org/integration/autoland/rev/dbf516eb07c5
Make test cases that use synthesizeWheelAtPoint proper. r=birtles
https://hg.mozilla.org/integration/autoland/rev/657f8cd386bb
Propagate testharness functions only if we use testharness.js. r=birtles
https://hg.mozilla.org/integration/autoland/rev/92408ba892fa
Run test_restyles.html in content. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8ee193c2b872
Set layout.reflow.synthMouseMove false to avoid synthetic mouse move. r=birtles

Comment 48

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4e4c1121dea3
https://hg.mozilla.org/mozilla-central/rev/1f139cd6bdae
https://hg.mozilla.org/mozilla-central/rev/d20341a660b6
https://hg.mozilla.org/mozilla-central/rev/dbf516eb07c5
https://hg.mozilla.org/mozilla-central/rev/657f8cd386bb
https://hg.mozilla.org/mozilla-central/rev/92408ba892fa
https://hg.mozilla.org/mozilla-central/rev/8ee193c2b872