1173580 - Write full-stack mochitest for APZ layerization

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 1161206 added our first full-stack APZ mochitest, and bug 1172648 our second.

I will write some more, exercising various pieces of basic APZ functionality (that we can't test for with gtests).

Comment 1

[Botond Ballo [:botond]]

Let's have one bug per test.

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1173580 - Record content descriptions in APZ test data. r=kats

Bug 1173580 - Record content descriptions in APZ test data. r=kats

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1173580 - Make synthesizeNativeWheelAnd*() functions iframe-friendly. r=kats

Bug 1173580 - Make synthesizeNativeWheelAnd*() functions iframe-friendly. r=kats

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1173580 - Add utilities for synthesizing mouse move events to apz_test_native_event_utils.js. r=kats

Bug 1173580 - Add utilities for synthesizing mouse move events to apz_test_native_event_utils.js. r=kats

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1173580 - Full-stack APZ layerization mochitest. r=kats

Bug 1173580 - Full-stack APZ layerization mochitest. r=kats

Comment 6

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

Comment on attachment 8621370 [details]
MozReview Request: Bug 1173580 - Record content descriptions in APZ test data. r=kats

https://reviewboard.mozilla.org/r/10965/#review9605

Comment 7

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

Comment on attachment 8621371 [details]
MozReview Request: Bug 1173580 - Make synthesizeNativeWheelAnd*() functions iframe-friendly. r=kats

https://reviewboard.mozilla.org/r/10967/#review9603

::: gfx/layers/apz/test/apz_test_native_event_utils.js:70
(Diff revision 1)
> -    window.removeEventListener("wheel", wheelWaiter);
> +function synthesizeNativeWheelAndWaitForWheelEvent(aElement, aX, aY, aDeltaX, aDeltaY, aCallback, aEventTarget=window) {

Instead of having to pass this in explicitly, can we just get the window from the element?
var eventTarget = aElement.ownerDocument.defaultView;

Comment 8

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

Comment on attachment 8621372 [details]
MozReview Request: Bug 1173580 - Add utilities for synthesizing mouse move events to apz_test_native_event_utils.js. r=kats

https://reviewboard.mozilla.org/r/10969/#review9607

::: gfx/layers/apz/test/apz_test_native_event_utils.js:115
(Diff revision 1)
> +function synthesizeNativeMouseMoveAndWaitForEvent(aElement, aX, aY, aCallback, aEventTarget=window) {

Rename this to ...WaitForMoveEvent for consistency.

Comment 9

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

Comment on attachment 8621373 [details]
MozReview Request: Bug 1173580 - Full-stack APZ layerization mochitest. r=kats

https://reviewboard.mozilla.org/r/10971/#review9609

::: gfx/layers/apz/test/helper_iframe1.html:3
(Diff revision 1)
> +     identiable to show up in the root scroll frame's content description,

s/identiable/identifiable/. Ditto for the other iframe

::: gfx/layers/apz/test/test_layerization.html:108
(Diff revision 1)
> +  ok(!isLayerized('outer4'), "initially 'outer3' should not be layerized");

copypasta error on outer4/inner4 lines

::: gfx/layers/apz/test/test_layerization.html:125
(Diff revision 1)
> +  var outer3 = document.getElementById('outer3');

If you change the eventTarget thing like I suggested in part 2, then you'd want to set outer3 to |document.getElementById('outer3').contentDocument.documentElement| here (similar to what you're doing in the outer4 case).

It might also be more convenient if we change synthesizeNativeWheel to take the mozInnerScreenX/Y from the aElement's window, and then we only need to worry about passing in the getBoundingClientRect from the element relative to it's immediate containing window.

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Comment on attachment 8621371 [details]
> MozReview Request: Bug 1173580 - Make synthesizeNativeWheelAnd*() functions
> iframe-friendly. r=kats
> 
> https://reviewboard.mozilla.org/r/10967/#review9603
> 
> ::: gfx/layers/apz/test/apz_test_native_event_utils.js:70
> (Diff revision 1)
> > -    window.removeEventListener("wheel", wheelWaiter);
> > +function synthesizeNativeWheelAndWaitForWheelEvent(aElement, aX, aY, aDeltaX, aDeltaY, aCallback, aEventTarget=window) {
> 
> Instead of having to pass this in explicitly, can we just get the window
> from the element?
> var eventTarget = aElement.ownerDocument.defaultView;

We can if we require that whenever the event is targeted at a subdocument, aElement is inside the subdocument. That seems reasonable, I'll change it.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> ::: gfx/layers/apz/test/test_layerization.html:125
> (Diff revision 1)
> > +  var outer3 = document.getElementById('outer3');
> 
> If you change the eventTarget thing like I suggested in part 2, then you'd
> want to set outer3 to
> |document.getElementById('outer3').contentDocument.documentElement| here
> (similar to what you're doing in the outer4 case).
> 
> It might also be more convenient if we change synthesizeNativeWheel to take
> the mozInnerScreenX/Y from the aElement's window, and then we only need to
> worry about passing in the getBoundingClientRect from the element relative
> to it's immediate containing window.

Thanks, this makes the test much cleaner!

Note that I folded parts of this change into the Part 2 and Part 3 patches as appropriate.

I fixed all the other comments as well, will post updated patches.

Comment 12

[Botond Ballo [:botond]]

Attachment: Part 2 - Make synthesizeNativeWheelAnd*() functions iframe-friendly

Posting patches manually because MozReview is currently unusable due to bug 1174353.

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 3 - Add utilities for synthesizing mouse move events to apz_test_native_event_utils.js

Carrying r+.

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 4 - Full-stack APZ layerization mochitest

Carrying r+.

Comment 15

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

Comment on attachment 8622704 [details] [diff] [review]
Part 2 - Make synthesizeNativeWheelAnd*() functions iframe-friendly

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

::: gfx/layers/apz/test/apz_test_native_event_utils.js
@@ +42,2 @@
>    aX += window.mozInnerScreenX;
>    aY += window.mozInnerScreenY;

Technically this is fine, but it's a little scary to me to have a local variable called |window| which shadows the global variable, as it could result in confusion. I'd rather call this "targetWindow" or even just "win"

@@ +71,4 @@
>  function synthesizeNativeWheelAndWaitForWheelEvent(aElement, aX, aY, aDeltaX, aDeltaY, aCallback) {
> +  var eventTarget = aElement.ownerDocument.defaultView;
> +  eventTarget.addEventListener("wheel", function wheelWaiter(e) {
> +    eventTarget.removeEventListener("wheel", wheelWaiter);

"targetWindow" might be a good name here too.

Comment 16

[Botond Ballo [:botond]]

Renamed things to "targetWindow" locally. New Try push:

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7df200578ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fddc8dcca78
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f163b3a20a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5011b666d31

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7df200578ce
https://hg.mozilla.org/mozilla-central/rev/2fddc8dcca78
https://hg.mozilla.org/mozilla-central/rev/1f163b3a20a8
https://hg.mozilla.org/mozilla-central/rev/d5011b666d31