1610960 - pinch to zoom jumps to top left if text is selected

Status: RESOLVED FIXED

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

Description

[__oldboek]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

1. Open Firefox Preview;
2. Go to a page with text(ex. wikipedia.org/wiki/Main_Page);
3. Select any plain text;
4. Start to pinch-zoom but don't leave the fingers from the screen.

https://github.com/mozilla-mobile/fenix/issues/6945

Actual results:

The page will zoom in but jump on the top-left part of the site.

Expected results:

The page will zoom in.

Comment 1

[__oldboek]

This might be another bug that is related?
https://github.com/mozilla-mobile/fenix/issues/7042

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Botond, is this APZ or text selection (or both)?

Comment 3

[Botond Ballo [:botond]]

I can reproduce this in Fenix but not in GVE. Makes me suspect perhaps some JS code in a component that's part of Fenix but not GVE.

Comment 4

[Botond Ballo [:botond]]

It does not seem to be triggered by JS, at least not directly.

Let's provisionally treat this as an APZ issue.

Comment 5

[Botond Ballo [:botond]]

Ahh, this is caused by the "force-disable APZ" mechanism.

Something related to text selection is causing AccessibleCaretManager to force-disable APZ for a portion of the pinch gesture. This has the effect of forcing the scroll offset to jump from the visual scroll offset to the layout scroll offset (since the visual scroll offset "only exists in APZ").

Basically, the "force-disable APZ" mechanism was designed with desktop in mind, and does bad things on mobile when we're zoomed in and the visaul and layout viewports diverge.

This is technically a regression from bug 1526268, and there may be an underlying text selection issue here (i.e. I'm not sure if we legitimately want to force-disable APZ in this scenario), but we definitely need an APZ fix here as well (to make the force-disable mechanism behave reasonably on mobile).

Comment 6

[Botond Ballo [:botond]]

I suspect that this bug is worse than the bugs which motivated the use of the "force-disable APZ" mechanism, so I think we should disable that mechanism on Android for the time being. I'll file a follow-up bug for a proper APZ fix.

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1610960 - Do not force-disable APZ on Android. r=TYLin

Force-disabling APZ currently has the effect of forcing the visual viewport
offset to be the same as the layout viewport offset, which breaks zooming
on Android.

Comment 8

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [spotty responses until Feb 19] from comment #6)

I'll file a follow-up bug for a proper APZ fix.

Tracking this in bug 1612750.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This is technically a regression from bug 1526268, and there may be an underlying text selection issue here (i.e. I'm not sure if we legitimately want to force-disable APZ in this scenario), but we definitely need an APZ fix here as well (to make the force-disable mechanism behave reasonably on mobile).

bug 1526268 is designed to only disable APZ when AccessibleCaret is in position:fixed subtree or when its position is changed. The reporter's STR in comment 0 shouldn't disable APZ because the selection is not on position:fixed. I filed bug 1612983 with an analysis of a bug in AccessibleCaret.

Comment 10

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] [spotty responses until Feb 19] from comment #6)

I suspect that this bug is worse than the bugs which motivated the use of the "force-disable APZ" mechanism, so I think we should disable that mechanism on Android for the time being. I'll file a follow-up bug for a proper APZ fix.

We now have proper fixes in bug 1612983 and bug 1612750, so we don't need to land this stop-gap measure.

Comment 11

[Botond Ballo [:botond]]

I'm going to mark this as fixed by bug 1612983 and bug 1612750.

Bug 1612983 landed in 74, and bug 1612750 landed in 75; since for the STR in this bug, bug 1612983 alone is enough to fix it, I'm going to mark this bug as status-firefox74:fixed.