Closed Bug 1379515 Opened 7 years ago Closed 7 years ago

Run test_restyle.html in content

Categories

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

Product:
Core
Component:
DOM: Animation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(7 files)

Bug 1379515 - Unify test cases that checks animation are/were in scrolled out elements.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1379515 - Change a child element position to be able to move into view of the parent by mouse wheel.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1379515 - Make test cases that use synthesizeWheelAtPoint proper.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1379515 - Propagate testharness functions only if we use testharness.js.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1379515 - Run test_restyles.html in content.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1379515 - Add another variant of sendWheelAndPaint but without flushing layout for obtaining target element position before sending the wheel event.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1379515 - Set layout.reflow.synthMouseMove false to avoid synthetic mouse move.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
I have to fix this for the test case for bug 1190721.
Assignee: nobody → hikezoe
Blocks: 1190721
Status: NEW → ASSIGNED
Do'h!  I shouldn't have used autoland. And forgot adding an important test file.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec83f1711fb18a466de44cd90c8c45680a92605a
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
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
SynthMouseMove would be a synthetic (fake) mouse move.
Yep, I am trying to understand how it happens.
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.
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.
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
(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
Attachment #8923009 - Flags: review?(bbirtles)
Attachment #8923013 - Flags: review?(bbirtles)
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 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
Attachment #8923012 - Flags: review?(bugs) → review+
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?)
Attachment #8923007 - Flags: review?(bbirtles) → review+
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
Attachment #8923008 - Flags: review?(bbirtles) → review+
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?
Attachment #8923009 - Flags: review?(bbirtles) → review+
Comment on attachment 8923010 [details]
Bug 1379515 - Propagate testharness functions only if we use testharness.js.

https://reviewboard.mozilla.org/r/194214/#review199324
Attachment #8923010 - Flags: review?(bbirtles) → review+
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
Attachment #8923011 - Flags: review?(bbirtles) → review+
(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 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.)
Attachment #8923013 - Flags: review?(bbirtles) → review+
Thank you for being so thorough and taking the time to track down all the test failures and tidy up the tests properly!
Thank you as well for the review!
A final try just in case;  (It's only run on Linux64 though)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63d0a3da6bcc005426ba2077b0444728b089e774
Pushed by hikezoe@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: