Closed Bug 1470504 Opened Last year Closed Last year

Enable zoom tests on desktop platforms

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: kashav, Assigned: botond)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(11 files, 1 obsolete file)

46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
kats
: review+
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
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
Priority: -- → P3
Whiteboard: [gfx-noted]
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?
Flags: needinfo?(kshvmdn)
Blocks: 1490102
(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
Flags: needinfo?(kshvmdn)
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.
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.
(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.
Blocks: 1492194
(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.
(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.)
Assignee: kshvmdn → 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).
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
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
Blocks: 1495580
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
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.
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
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
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
Retained displaylists can produce empty paints, which wasn't the case before.

Depends on D7342
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
With a couple of exceptions:
  - WebRender, which doesn't support zooming yet
  - Windows, which is tracked in bug 1495580

Depends on D7347
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.
Attachment #9013437 - Flags: review+
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 on attachment 9013435 [details]
Bug 1470504 - Remove the assertion in ScrollMetadata::SetUsesContainerScrolling(). r?kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.
Attachment #9013435 - Flags: review+
Comment on attachment 9013438 [details]
Bug 1470504 - Always log the high-resolution displayport to APZ test data. r?kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.
Attachment #9013438 - Flags: review+
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:kats@mozilla.com) has approved the revision.
Attachment #9013439 - Flags: review+
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:kats@mozilla.com) has approved the revision.
Attachment #9013441 - Flags: review+
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:kats@mozilla.com) has approved the revision.
Attachment #9013443 - Flags: review+
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:kats@mozilla.com) has approved the revision.
Attachment #9013444 - Flags: review+
Comment on attachment 9013445 [details]
Bug 1470504 - Increase the layout viewport size in helper_basic_doubletap_zoom.html. r?kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.
Attachment #9013445 - Flags: review+
Comment on attachment 9013447 [details]
Bug 1470504 - Enable test_group_zoom on desktop platforms. r?kats

Kartikaya Gupta (email:kats@mozilla.com) has approved the revision.
Attachment #9013447 - Flags: review+
Attachment #9013442 - Attachment is obsolete: true
Attachment #9013435 - Attachment description: Bug 1470504 - Remove the assertion in ScrollMetadata::SerUsesContainerScrolling(). r?kats → Bug 1470504 - Remove the assertion in ScrollMetadata::SetUsesContainerScrolling(). r?kats
Pushed by bballo@mozilla.com:
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
^ Landed on m-i because histedit makes Lando sad.
You need to log in before you can comment on or make changes to this bug.