1470504 - Enable zoom tests on desktop platforms

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[u608768]

Zoom tests currently only run on Android [1]. We should be able to enable them on desktop platforms (not including WebRender) by setting |apz.allow_zooming| and |dom.meta-viewport.enabled| (see [2]).

[1] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/gfx/layers/apz/test/mochitest/mochitest.ini#28-29
[2] https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/gfx/layers/apz/test/mochitest/test_group_zoom.html#11

Comment 1

[Botond Ballo [:botond]]

Kashav, could you jog my memory: did we abandon work on this because we realized we'd also need to set |layout.scroll.root-frame-containers|, and that was a "Once" pref?

Comment 2

[u608768]

(In reply to Botond Ballo [:botond] from comment #1)
> Kashav, could you jog my memory: did we abandon work on this because we
> realized we'd also need to set |layout.scroll.root-frame-containers|, and
> that was a "Once" pref?

That's correct. IIRC, helper_bug1280013.html and helper_fixed_position_scroll_hittest.html require the pref to be enabled (note that helper_bug1280013.html assumes that device width is less than 980px [1], so we have to change that before the test actually runs (and fails) on Desktop).

Also, even with |apz.allow_zooming| enabled, double-tap to zoom doesn't work on Desktop (so helper_basic_doubletap_zoom.html won't run properly on Desktop until that works).

[1] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/gfx/layers/apz/test/mochitest/helper_bug1280013.html#5-7

Comment 3

[Botond Ballo [:botond]]

Thanks Kashav! I guess there are several issues that need to be fixed here.

Another one I discovered is that helper_bug1280013.html expects to find a "criticalDisplayport" property recorded in the APZ test data, but that property is only recorded (and only meaningful) on Android.

Comment 4

[Botond Ballo [:botond]]

Another issue is that retained displaylists break the assumption made in several APZ mochitests, that in the APZ test data that's logged per-paint, the bucket for the most recent paint will contain information for every scroll frame that was painted. With retained DL, we can get paints that skip display list building for some scroll frames because we are re-using their display list from the previous paint.

This has not been an issue so far because Android does not use retained DL yet.

Comment 5

[Botond Ballo [:botond]]

(In reply to Kashav Madan [:kashav] from comment #2)
> (note that helper_bug1280013.html assumes that device width is less than
> 980px [1], so we have to change that before the test actually runs (and
> fails) on Desktop)

I initially increased the viewport width it to 1500px, only to discover that that's still not enough - it needs to be wide enough that the corresponding viewport height is tall enough to allow enough scrolling to get the iframe under the cursor.

The subtest is still not passing though - the next issue seems to be that flushApzRepaints() is not waiting long enough for dispatch-to-content based scrolling to actually occur.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)
> The subtest is still not passing though - the next issue seems to be that
> flushApzRepaints() is not waiting long enough for dispatch-to-content based
> scrolling to actually occur.

It looks like flushApzRepaints() is not designed to do that, nor is waitForApzFlushedRepaints().

Looking through our tests, we don't actually have any that wait for the dispatch-to-content mechanism. Tests that need to scroll (not just try to scroll / hit test, but actually scroll) a subframe that starts unlayerized, first layerize it by calling nsIDOMWindowUtils.setDisplayPortForElement(), and then synthesize a scroll gesture.

An additional gotcha is that you can't actually layerize an iframe through the nsIDOMWindowUtils obtained from the toplevel window; you have to do it using an nsIDOMWindowUtils obtained from the iframe's own window.

With those fixes in place, helper_bug1280013.html is passing for me, modulo a debug assertion related to the enablement of layout.scroll.root-frame-containers. helper_zoomed_pan.html and helper_fixed_position_scroll_hittest.html are still failing.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
> Looking through our tests, we don't actually have any that wait for the
> dispatch-to-content mechanism. Tests that need to scroll (not just try to
> scroll / hit test, but actually scroll) a subframe that starts unlayerized,
> first layerize it by calling nsIDOMWindowUtils.setDisplayPortForElement(),
> and then synthesize a scroll gesture.

(That's not entirely true - we have tests that wait for the dispatch-to-content mechanism by listening for the eventual 'scroll' event. We could probably do that here too, but I don't think it's material whether we do that, or explicitly layerize first.)

Comment 8

[Botond Ballo [:botond]]

Patches so far:

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

Comment 9

[Botond Ballo [:botond]]

I think the issue in helper_zoomed_pan is that the test is not accounting for scrollbars taking up layout size (which they don't on Android since Android uses overlay scrollbars).

Comment 10

[Botond Ballo [:botond]]

After fixing one more issue related to IGNORE_ROOT_SCROLL_FRAME (which becomes less of an Android-only thing when we starting doing zooming-type things on desktop), I have the test passing locally.

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

Comment 11

[Botond Ballo [:botond]]

helper_basic_doubletap_zoom.html is still failing in automation. (Kashav, this is what you may have been referring to in comment 2 when you said "double-tap zoom doesn't work on desktop".)

I think the issue is related to screen size, specifically this code [1] disabling double-tap zoom when the screen is wider than the layout viewport.

[1] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/layout/base/ZoomConstraintsClient.cpp#235

Comment 12

[Botond Ballo [:botond]]

The test is now passing in automation on non-WebRender Linux and OSX [1].

It's not expected to pass on WebRender because WebRender doesn't support zooming yet.

I don't know why it's not passing on Windows, but seeing as the main objective here is to allow debugging future test failures on desktop, and I use Linux to do that, I'm happy to punt on getting it to work on Windows for now (filed bug 1495580 to track).

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=f0caa2c0ad6c4e9812d57daf766b047a628ad941

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Remove the assertion in ScrollMetadata::SetUsesContainerScrolling(). r?kats

The assertion is not serving much purpose. Now that container scrolling is
a Live pref, checking it on the compositor side is racy if the pref is
flipped, and on the content side it's clear from the code that it will
only be set to true if the pref is turned on.

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Make layout.scroll.root-frame-containers a Live pref. r?mattwoodrow

All of its callers are in painting code, so changes will take effect on the
next paint, so there is no need for it to be a Once pref.

Making it Live allows us to selectively enable it in specific mochitests.

Depends on D7339

Comment 15

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Always log the high-resolution displayport to APZ test data. r?kats

Previously we would log the displayport and the critical displayport separately,
which made it more difficult to write cross-platform APZ tests.

Depends on D7340

Comment 16

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Make helper_Bug 1470504.html wide enough that it's zoomed in even on desktop. r?kats

And also wide enough that the corresponding, proportionally computed, viewport
height is tall enough to allow enough scrolling to bring the iframe under
the cursor.

Depends on D7341

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Skip empty paints when looking for the most recent paints in the APZ test data. r?kats

Retained displaylists can produce empty paints, which wasn't the case before.

Depends on D7342

Comment 18

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Layerize the iframe in helper_Bug 1470504.html before trying to scroll it. r?kats

Depends on D7343

Comment 19

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Consider the layout width taken up by scrollbars in helper_zoomed_pan.html. r?kats

Depends on D7344

Comment 20

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - If zooming is enabled, mark mouse events passing through APZ as 'ignore root scroll frame' on all platforms. r?kats

Depends on D7345

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Increase the layout viewport size in helper_basic_doubletap_zoom.html. r?kats

Double-tap zoom requires that the layout viewport be wider than the screen,
so if we want the test to run on desktop, we need a wide enough layout viewport.

Depends on D7346

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Enable test_group_zoom on desktop platforms. r?kats

With a couple of exceptions:
  - WebRender, which doesn't support zooming yet
  - Windows, which is tracked in bug 1495580

Depends on D7347

Comment 23

[Botond Ballo [:botond]]

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

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9013437 [details]
Bug 1470504 - Make layout.scroll.root-frame-containers a Live pref. r?mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 25

[Botond Ballo [:botond]]

Erm. moz-phab was a bit over-eager in rewriting a couple of those commit messages (filed bug 1495602).

(In reply to Botond Ballo [:botond] from comment #16)
> Bug 1470504 - Make helper_Bug 1470504.html wide enough that it's zoomed in
> even on desktop. r?kats

That should be "Make helper_bug1280013.html wide enough that it's zoomed in even on desktop"

(In reply to Botond Ballo [:botond] from comment #18)
> Bug 1470504 - Layerize the iframe in helper_Bug 1470504.html before trying
> to scroll it. r?kats

Likewise, that should be "Layerize the iframe in helper_bug1280013.html before trying to scroll it"

Comment 26

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

Comment on attachment 9013435 [details]
Bug 1470504 - Remove the assertion in ScrollMetadata::SetUsesContainerScrolling(). r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 27

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

Comment on attachment 9013438 [details]
Bug 1470504 - Always log the high-resolution displayport to APZ test data. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 28

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

Comment on attachment 9013439 [details]
Bug 1470504 - Make helper_Bug 1470504.html wide enough that it's zoomed in even on desktop. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 29

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

Comment on attachment 9013441 [details]
Bug 1470504 - Skip empty paints when looking for the most recent paints in the APZ test data. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 30

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

Comment on attachment 9013443 [details]
Bug 1470504 - Consider the layout width taken up by scrollbars in helper_zoomed_pan.html. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 31

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

Comment on attachment 9013444 [details]
Bug 1470504 - If zooming is enabled, mark mouse events passing through APZ as 'ignore root scroll frame' on all platforms. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 32

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

Comment on attachment 9013445 [details]
Bug 1470504 - Increase the layout viewport size in helper_basic_doubletap_zoom.html. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 33

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

Comment on attachment 9013447 [details]
Bug 1470504 - Enable test_group_zoom on desktop platforms. r?kats

Kartikaya Gupta (email:[e-mail]) has approved the revision.

Comment 34

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Add a clarifying comment to helper_bug1280013.html. r?kats

Depends on D7343

Comment 35

[Botond Ballo [:botond]]

Attachment: Bug 1470504 - Use IGNORE_ROOT_SCROLL_FRAME in PrepareForSetTargetAPZCNotification() whenever zooming is enabled. r?kats

Depends on D7347

Comment 36

[Botond Ballo [:botond]]

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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf3a63af2da7
Remove the assertion in ScrollMetadata::SetUsesContainerScrolling(). r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a02b46f1567e
Make layout.scroll.root-frame-containers a Live pref. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eac3912d003
Always log the high-resolution displayport to APZ test data. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a29d2de3f5b
Make helper_bug1280013.html wide enough that it's zoomed in even on desktop. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e0f01059c8
Skip empty paints when looking for the most recent paints in the APZ test data. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/cac83917e269
Add a clarifying comment to helper_bug1280013.html. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/8242a2fda82f
Consider the layout width taken up by scrollbars in helper_zoomed_pan.html. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/a445a3af8ac4
If zooming is enabled, mark mouse events passing through APZ as 'ignore root scroll frame' on all platforms. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd40fbd946f
Increase the layout viewport size in helper_basic_doubletap_zoom.html. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/85dc90d04f31
Use IGNORE_ROOT_SCROLL_FRAME in PrepareForSetTargetAPZCNotification() whenever zooming is enabled. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d3165782b4
Enable test_group_zoom on desktop platforms. r=kats

Comment 38

[Botond Ballo [:botond]]

^ Landed on m-i because histedit makes Lando sad.

Comment 39

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/cf3a63af2da7
https://hg.mozilla.org/mozilla-central/rev/a02b46f1567e
https://hg.mozilla.org/mozilla-central/rev/8eac3912d003
https://hg.mozilla.org/mozilla-central/rev/8a29d2de3f5b
https://hg.mozilla.org/mozilla-central/rev/e5e0f01059c8
https://hg.mozilla.org/mozilla-central/rev/cac83917e269
https://hg.mozilla.org/mozilla-central/rev/8242a2fda82f
https://hg.mozilla.org/mozilla-central/rev/a445a3af8ac4
https://hg.mozilla.org/mozilla-central/rev/7bd40fbd946f
https://hg.mozilla.org/mozilla-central/rev/85dc90d04f31
https://hg.mozilla.org/mozilla-central/rev/53d3165782b4