1669588 - Scrolling to input in desktop viewport is broken in some cases

Status: RESOLVED FIXED

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

Description

[[ex-Mozilla] Agi Sferro | :agi]

While looking at Bug 1667497, we noticed there's a different problem with zoomToFocusedInput. The testcase can be found here https://bugzilla.mozilla.org/show_bug.cgi?id=1667497#c1

Comment 1

[Botond Ballo [:botond]]

I've looked into this a bit. A zoom animation with the correct destination is started, but then gets cancelled mid-animation (near the beginning actually) due to a scroll update from the main thread.

I haven't tracked down what's causing the scroll update. One theory is that it's from the ScrollContentIntoView call in ZoomToFocusedInput(), and the IPC message for the transaction carrying the scroll update is racing with the IPC message carrying the ZoomToRect call, but I haven't confirmed this yet.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)

I haven't tracked down what's causing the scroll update. One theory is that it's from the ScrollContentIntoView call in ZoomToFocusedInput(), and the IPC message for the transaction carrying the scroll update is racing with the IPC message carrying the ZoomToRect call, but I haven't confirmed this yet.

I've now confirmed that this is what's happening. Even though ScrollContentIntoView is performing instant scrolling, it queues up a scroll update to APZ that rides the next transaction. The ZoomToRect call uses its own IPC message which arrives first, starts the animation, and then the scroll update cancels it.

My best idea for how to fix this is to delay the ZoomToRect call to a post-refresh observer the way we do with SetTargetAPZC here.

Comment 3

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

A post-refresh observer seems reasonable, but maybe we should try to restrict it to the case where ScrollContentIntoView actually does useful work (which should be infrequent - in most cases the user will focus an input element by tapping on something already visible). Otherwise we will be adding some avoidable latency to the zoom animation start which can confuse users into wondering whether or not their tap actually worked.

Comment 4

[Botond Ballo [:botond]]

Attachment: Bug 1669588 - Factor out a OneShotPostRefreshObserver utility class. r=kats

Comment 5

[Botond Ballo [:botond]]

Attachment: Bug 1669588 - Delay the ZoomToRect call in ZoomToFocusedInput until after a refresh if appropriate. r=kats

ZoomToFocusedInput calls ScrollContentIntoView() which may queue up
one or more main-thread scroll position updates that get sent to
APZ as part of the next transaction.

If such updates were produced, we want the ZoomToRect call to arrive
at APZ after the updates, otherwise the updates can cancel the
zoom animation that APZ starts in response to the ZoomToRect.

Depends on D93897

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2b9b7986865
Factor out a OneShotPostRefreshObserver utility class. r=kats
https://hg.mozilla.org/integration/autoland/rev/d6360093fb5c
Delay the ZoomToRect call in ZoomToFocusedInput until after a refresh if appropriate. r=kats

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5a87527d022
Backed out 2 changesets for Backout conflicts with Bug 1654103. CLOSED TREE

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c429989882c2
Factor out a OneShotPostRefreshObserver utility class. r=kats
https://hg.mozilla.org/integration/autoland/rev/f21fc6d5768f
Delay the ZoomToRect call in ZoomToFocusedInput until after a refresh if appropriate. r=kats

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38f373bf1d36
Remove unnecessary rebase conflict line. a=fix

Comment 10

[Alexandru Michis [:malexandru]]

Backed out 3 changesets (Bug 1669588) for causing mochitest failures in test_group_zoomToFocusedInput.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/b0c83856ebcf4548dfaf208d32c58380262b2628

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Ctsan%2Copt%2Cmochitests%2Ctest-linux1804-64-tsan%2Fopt-mochitest-plain-e10s%2C6&fromchange=2309e130ea8d75bd6fb4a5fc056e503fae7f886c&tochange=b0c83856ebcf4548dfaf208d32c58380262b2628&selectedTaskRun=InBM-nA1QUWwZIiO2LmJmA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319327093&repo=autoland&lineNumber=12144

Comment 11

[Botond Ballo [:botond]]

Attachment: Bug 1669588 - Avoid data races accessing APZCTreeManager::mTestSampleTime. r=kats

This variable is set from the compositor thread, and read from both
the sampler thread (e.g. when sampling animations) and the
controller thread (e.g. during input block creation), so access to
it needs to be synchronized.

Depends on D93898

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/937bd02f15aa
Factor out a OneShotPostRefreshObserver utility class. r=kats
https://hg.mozilla.org/integration/autoland/rev/b0f29632ca85
Delay the ZoomToRect call in ZoomToFocusedInput until after a refresh if appropriate. r=kats
https://hg.mozilla.org/integration/autoland/rev/a7003d573886
Avoid data races accessing APZCTreeManager::mTestSampleTime. r=kats

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/937bd02f15aa
https://hg.mozilla.org/mozilla-central/rev/b0f29632ca85
https://hg.mozilla.org/mozilla-central/rev/a7003d573886