1496864 - apz_test_native_event_utils.js functions don't take resolution into account

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

The functions for synthesizing native events in apz_test_native_event_utils.js do not appear to take the pres-shell resolution (which reflects the zoom caused by pinch-zooming) into account.

These functions use a helper function called coordinatesRelativeToScreen() [1], which starts with Element.getBoundingClientRect(), which is in CSS pixels, adds some deltas provided by the caller, and adjusts for the device pixel ratio and some offsets (but not resolution). The resulting coordinates are then passed to nsIDOMWindowUtils functions which interpret their arguments as being in screen coordinates. Missing in this link is the pres shell resolution that factors into the conversion between CSS and screen coordinates.

It appears that the existing APZ mochitests that use these native event functions while zoomed in happen to work either by chance (i.e. resulting coordinates are incorrect but still happen to hit the right element), or because the deltas passed in by the caller have been explicitly converted to screen coordinates (which then get added to the bounding client rect which is in CSS coordinates, which is nonsensical, but when the bounding client rect origin is close to (0,0), works out to be close enough numerically).

[1] https://searchfox.org/mozilla-central/rev/807a37c670c093b6e5201841a7c5315ba67ba8d5/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js#83

Comment 1

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> It appears that the existing APZ mochitests that use these native event
> functions while zoomed in happen to work either by chance (i.e. resulting
> coordinates are incorrect but still happen to hit the right element), or
> because the deltas passed in by the caller have been explicitly converted to
> screen coordinates (which then get added to the bounding client rect which
> is in CSS coordinates, which is nonsensical, but when the bounding client
> rect origin is close to (0,0), works out to be close enough numerically).

It's actually worse than that. Even if we ignore resolution, getBoundingClientRect() returns coordinates relative to the layout viewport, while the nsIDOMWindowUtils functions for synthesizing native events intepret their inputs as being relative to the visual viewport.

In helper_fixed_position_scroll_hittest.html, the zoom level is such that this discrepancy happens to cancel out the fact that resolution is not taken into account. Change the zoom level to something else, and the test fails.

Comment 2

[Botond Ballo [:botond]]

Rough draft of patches that fix this mess:

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

They're passing test_group_zoom locally (on a desktop build). Let's see how they're doing on other platforms.

Comment 3

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #2)
> Rough draft of patches that fix this mess:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=53f7876c15cbf4b45255e75d160d333b97500c56
> 
> They're passing test_group_zoom locally (on a desktop build). Let's see how
> they're doing on other platforms.

test_group_mouseevents is failing on desktop, and test_group_zoom on Android.

Comment 4

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from comment #3)
> test_group_mouseevents is failing on desktop

I've been investigating this failure; the specific subtest failing is helper_drag_scroll.html.

My approach for solving the issue described in comment 1 was to use nsIDOMWindowUtils.getVisualViewportOffsetRelativeToLayoutViewport() to convert the bounding rect computed in coordinatesRelativeToScreen() to be relative to the visual viewport.

However, there are two types of divergences between the visual and layout viewport offsets: (1) persistent divergences, which only occur during zooming; and (2) transient divergences which can occur during ordinary scrolling due to the asynchrony between the main thread and APZ.

In helper_drag_scroll.html, the test's JS code performs main thread scrolling, and then synthesizes native events without waiting for a round trip to APZ first. The synthesization goes through coordinatesRelativeToScreen(), and picks up a large transient divergence between the two viewport offsets, which results in screen-relative coordinates that are discarded by APZ because they are offscreen from APZ's point of view (which doesn't know about the main thread scroll yet).

In a sense, this is the converse of the problem we ran into a few years ago, where APZ needs to flush its async changes to the scroll offset before sending events to the main thread for hit-testing, otherwise the main thread hit testing might discard them on account of thinking they're offscreen.

I've been thinking about how to solve this, and evaluated three solution approaches:

 1. Require APZ tests to wait for a round trip to APZ (waitForApzFlushedRepaints)
    between performing main thread scrolling and synthesizing native events.
    
    This would basically avoid the problem by ensuring that when we query the
    divergence between the offsets during event sythesization, any transient
    component to the divergence has gone away.
    
    This would be a fairly straightforward test-only change, but it would require
    all new tests to be aware of and follow this rule. Moreover, I'm concerned
    that this might invalidate some of our tests, which are specifically trying
    to test behaviour under conditions where there is a divergence between the
    two offsets (I think helper_drag_scroll in paricular may fall into this
    category).
    
 2. Make native event synthesization "layers-dependent", i.e. make it wait until
    the next time we tell APZ about main-thread scroll updates via a layers
    transaction or WR display list update.

    We could then have the main thread _not_ apply the offset between the two
    viewports to synthesized events, and have APZ apply it instead (after processing 
    any main-thread scroll updates).
    
    This would be a more involved change requiring a fair amount of plumbing.
    It would also require a mechanism for APZ to distinguish synthesized events
    (which need the offset adjustment) from true user inputs. It's unclear to me
    whether this would still invalidate any of our tests.
    
 3. Have the main thread immediately update its view of the visual viewport offset
    when it performs main-thread scrolling.
    
    This is straightforward to implement, and doesn't involve delaying any messages, 
    thus not invalidating any of our tests.
    
    Unlike #1 or #2, this does affect codepaths other than native event
    synthesization, e.g. it affects codepaths where production chrome JS code 
    queries the visual viewport offset via nsIDOMWindowUtils. However, I believe
    those codepaths actually suffer from the same issue as the test does, and so
    we're actually fixing them as well.
    
I implemented approach #3, and it solved the helper_drag_scroll.html failure for me locally.

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [standards meeting Nov 5-10] from comment #3)
> and test_group_zoom on Android.

This just looks like rounding error. We are hit-testing right at the edge of the button, and rounding the synthesized event coordinates to integer screen pixels introduces enough inaccuracy in the presence of a resolution to cause the hit-test to just fail.

With that fixed, I have a clean Try push: 

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

The patches will need a bit of cleanup before review.

Comment 6

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Cause exceptions thrown while running an APZ test continuation to fail the test. r=kats

Otherwise, the exception is just silently swallowed by typical test invocation
code like:

  waitUntilApzStable()
  .then(runContinuation(test))
  .then(subtestDone);

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Have some apz_test_native_event_utils functions accept a window as an event target, as an alternative to an element. r=kats

Sometimes, it's easier for the caller to express the input coordinates
relative to the window, rather than relative to any element's origin,
particularly in cases where you're zoomed in and the origin of e.g.
the document element can be far away from the window origin.

Depends on D11225

Comment 8

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Convert coordinates to be relative to the visual viewport in coordinatesRelativeToScreen(). r=kats

This is necessary for correctness because the callers pass the resulting
coordinates to nsIDOMWindowUtils native event sythesization functions,
which interpret the event coordinates as being relative to the screen.

Depends on D11226

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - When performing a main thread scroll, update the main thread's view of the visual viewport offset right away. r=kats

Depends on D11227

Comment 10

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Move getResolution() into apz_test_native_utils.js. r=kats

Depends on D11228

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Take the resolution into account in coordinatesRelativeToScreen(). r=kats

This is again necessary for correctness as the nsIDOMWindowUtils.sendNative*
functions take coordinates in screen units which include the resolution.

Depends on D11229

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1496864 - Target the inside of the button rather than the edge in helper_fixed_position_scroll_hittest.html. r=kats

Depends on D11230

Comment 13

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

Thanks for cleaning this up! Must have been pretty painful to debug... :/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/634753a5e725
Cause exceptions thrown while running an APZ test continuation to fail the test. r=kats
https://hg.mozilla.org/integration/autoland/rev/0ebabb13f557
Have some apz_test_native_event_utils functions accept a window as an event target, as an alternative to an element. r=kats
https://hg.mozilla.org/integration/autoland/rev/c8909b617c85
Convert coordinates to be relative to the visual viewport in coordinatesRelativeToScreen(). r=kats
https://hg.mozilla.org/integration/autoland/rev/83db18e6c2bf
When performing a main thread scroll, update the main thread's view of the visual viewport offset right away. r=kats
https://hg.mozilla.org/integration/autoland/rev/4d5179093560
Move getResolution() into apz_test_native_utils.js. r=kats
https://hg.mozilla.org/integration/autoland/rev/7dd5ed97088f
Take the resolution into account in coordinatesRelativeToScreen(). r=kats
https://hg.mozilla.org/integration/autoland/rev/50861e73409e
Target the inside of the button rather than the edge in helper_fixed_position_scroll_hittest.html. r=kats

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/634753a5e725
https://hg.mozilla.org/mozilla-central/rev/0ebabb13f557
https://hg.mozilla.org/mozilla-central/rev/c8909b617c85
https://hg.mozilla.org/mozilla-central/rev/83db18e6c2bf
https://hg.mozilla.org/mozilla-central/rev/4d5179093560
https://hg.mozilla.org/mozilla-central/rev/7dd5ed97088f
https://hg.mozilla.org/mozilla-central/rev/50861e73409e

Comment 16

[Botond Ballo [:botond]]

Not planning to uplift this, either. Most of the changes here are test-only; the fourth patch ("When performing a main thread scroll, update the main thread's view of the visual viewport offset right away") is technically a platform change, but the underlying issue would be hard to observe and we have not received user reports about it, so I'd prefer to just let the patches bake.