1172648 - Add a full-stack APZ mochitest for bug 1151667

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 1151667 is an example of an APZ bug with simple STR (scroll the mouse wheel after page load of a particular page: the wrong element is scrolled) for which we can now (as a result of the infrastructure added in bug 1161206) write full-stack mochitests. Let's write one.

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> (as a result of the infrastructure added in bug 1161206)

(And bug 1146349 and bug 1161634.)

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Comment 3

[Botond Ballo [:botond]]

I verified that this test passes both with and without APZ, and fails with APZ on and the fix for bug 1151667 not present.

Try push that includes this patch:

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

That try push didn't look so good, although it looks like bustage from earlier in the patch queue. Also I'm assuming you meant to flag me for review here.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://reviewboard.mozilla.org/r/10485/#review9287

::: gfx/layers/apz/test/mochitest.ini:15
(Diff revision 1)
> +skip-if = (os == 'android') || (os == 'b2g') || (buildapp == 'mulet') # wheel events not supported on mobile; see bug 1164274 for mulet

You should be ok to run this test on mulet.

::: gfx/layers/apz/test/test_bug1151667.html:24
(Diff revision 1)
> +    height: 1000px;

I'd increase the height of #page-content to something like 5000 just to make sure. Some people may have tall monitors where the window is >1000px tall in which case this might not have the desired effect. #subframe-content should be fine at 1000px because the size of #subframe is fixed at 500px.

::: gfx/layers/apz/test/test_bug1151667.html:34
(Diff revision 1)
> +  <div id="subframe-content"></div>

It would be nice to also put some text or non-empty content in here, so that when the thing scrolls you can visually see it move.

::: gfx/layers/apz/test/test_bug1151667.html:45
(Diff revision 1)
> +  synthesizeNativeWheelAndWaitForScrollEvent(subframe, 100, 150, 0, -10, continueTest);

This is going to put the mouse at 100,150 from the top-left of the window. However, what you want is for the mouse to be inside the subframe. I'd change this to use the getBoundingClientRect (and devicePixelRatio) the way :handyman did in attachment 8613616 [details] [diff] [review]. That way even if #subframe gets pushed down on the page it will work.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> https://reviewboard.mozilla.org/r/10485/#review9287
> 
> ::: gfx/layers/apz/test/mochitest.ini:15
> (Diff revision 1)
> > +skip-if = (os == 'android') || (os == 'b2g') || (buildapp == 'mulet') # wheel events not supported on mobile; see bug 1164274 for mulet
> 
> You should be ok to run this test on mulet.

Indeed, fixed.

> ::: gfx/layers/apz/test/test_bug1151667.html:24
> (Diff revision 1)
> > +    height: 1000px;
> 
> I'd increase the height of #page-content to something like 5000 just to make
> sure. Some people may have tall monitors where the window is >1000px tall in
> which case this might not have the desired effect. #subframe-content should
> be fine at 1000px because the size of #subframe is fixed at 500px.

Good point; fixed.

> ::: gfx/layers/apz/test/test_bug1151667.html:34
> (Diff revision 1)
> > +  <div id="subframe-content"></div>
> 
> It would be nice to also put some text or non-empty content in here, so that
> when the thing scrolls you can visually see it move.

I added a gradient (borrowed from test_wheel_scroll.html).

> ::: gfx/layers/apz/test/test_bug1151667.html:45
> (Diff revision 1)
> > +  synthesizeNativeWheelAndWaitForScrollEvent(subframe, 100, 150, 0, -10, continueTest);
> 
> This is going to put the mouse at 100,150 from the top-left of the window.
> However, what you want is for the mouse to be inside the subframe. I'd
> change this to use the getBoundingClientRect (and devicePixelRatio) the way
> :handyman did in attachment 8613616 [details] [diff] [review]. That way even
> if #subframe gets pushed down on the page it will work.

Done.

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8616876 [details]
MozReview Request: Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8616876 [details]
MozReview Request: Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8616876 [details]
MozReview Request: Bug 1172648 - Full-stack APZ mochitest for bug 1151667. r=kats

https://reviewboard.mozilla.org/r/10485/#review9473

Comment 10

[Botond Ballo [:botond]]

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

Comment 11

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=21ddd7ab8373

All green.

Setting checkin-needed as the tree's been closed all day.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/303ced6f21b9

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/303ced6f21b9