Closed Bug 1669588 Opened 2 years ago Closed 2 years ago

Scrolling to input in desktop viewport is broken in some cases

Categories

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

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
84 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox82 --- wontfix
firefox83 --- wontfix
firefox84 --- fixed

People

(Reporter: agi, Assigned: botond)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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

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.

Assignee: nobody → 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.

Component: General → Panning and Zooming
Product: GeckoView → Core

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.

Severity: -- → S3
OS: All → Android
Priority: -- → P2
Regressed by: 1149300
Has Regression Range: --- → yes

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

Attachment #9182241 - Attachment description: Bug 1669588 - Factor out an APZPostRefreshObserver utility class. r=kats → Bug 1669588 - Factor out a OneShotPostRefreshObserver utility class. r=kats
Attachment #9182241 - Attachment description: Bug 1669588 - Factor out a OneShotPostRefreshObserver utility class. r=kats → Bug 1669588 - Factor out a OneShotPostRefreshObserver utility class. r=kats
Pushed by bballo@mozilla.com:
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
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5a87527d022
Backed out 2 changesets for Backout conflicts with Bug 1654103. CLOSED TREE
Pushed by bballo@mozilla.com:
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
Pushed by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38f373bf1d36
Remove unnecessary rebase conflict line. a=fix

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

Flags: needinfo?(botond)
Pushed by bballo@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
You need to log in before you can comment on or make changes to this bug.