1478335 - Double tap to zoom out does not work

Status: VERIFIED FIXED

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

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: Nexus 5(Android 6.0.1), Samsung Galaxy S8(Android 8.0);
Build: Nightly 63.0a1 (2017-07-24);

Steps to reproduce:
1. Go to planet.mozilla.org;
2. Double tap on the page content;
3. Double tap on the page content.

Expected result:
After step 2 the page will zoom in. After step 3 the page will zoom out.

Actual result: the page is not zoomed in/out.

Notes: 
- couldn't reproduce on Samsung Galaxy Tab3(Android 8.0)
Video: https://drive.google.com/open?id=10Y3dt9AWUQficeS-zCKui1LA8nrn2wP9

Comment 1

[Pascal Chevrel:pascalc]

I can kind of reproduce it with 2017-07-31 in that sometimes zooming out does not work after zooming in, looks like a regression, tracking.

Comment 2

[Mira Lobontiu (Android QA)]

Hello,

Ran a regression using Nexus 5 (Android 6.0.1).

Regression-window:
Last good revision: 2018-07-11
First bad revision:  2018-07-12

Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b62d582090e11edcdd01c2c707e0b07b33ad0b12&tochange=ecafdbe899a7b2e574922250e356f8610788ad75

Thanks!

Comment 3

[Pascal Chevrel:pascalc]

Kashav, Tanushree, it seems that your patch in bug 1423011 caused this regression, could you have a look please? Thanks

Comment 4

[Botond Ballo [:botond]]

I'm not able to reproduce this (tested with a local build based on recent mozilla-central). Double-tapping seems to work fine.

Comment 5

[Kevin Brosnan [Ex-Mozilla]]

I am able to zoom in on today's Nightly however zoom out seems to be broken on several devices (Pixel 2 XL, Galaxy S8, Moto G5).

Comment 6

[Botond Ballo [:botond]]

Good point, the behaviour may be device dependent. I was testing with a Sony Z3C.

Comment 7

[Botond Ballo [:botond]]

(Screen size dependent, in particular, would be my guess.)

Comment 8

[Botond Ballo [:botond]]

I reproduced the problem on Moto G5, but kind of intermittently: even when it reproduces, scrolling a bit and then trying again tends to work. Other times, double-tapping results in just a small change to the zoom level, rather than a significant amount of zooming in or out.

Kashav, if you have a phone that can reproduce this, and have some time to look into it, please feel free to, but I would prioritize getting bug 1465616 landed first.

Comment 9

[Botond Ballo [:botond]]

Some pointers about where in the code double-tap is handled:

* When APZ detects a double-tap gesture, AsyncPanZoomController::OnDoubleTap() is called [1]
* That code dispatches a message to the main thread, which is handled in ChromeProcessController::HandleDoubleTap() [2] on Android, or TabChild::HandleDoubleTap() [3] on desktop.
* In either case, a function CalculateRectToZoomTo() is called which takes the location of the double-tap, performs a main-thread hit test to see what element was hit, and based on that chooses a rect to zoom in to, or an empty rect which signifies "zoom out".
* The chosen rect is then sent back to APZ and processed in AsyncPanZoomController::ZoomToRect() [4], which starts a zoom animation to the chosen rect (or to zoom out).

[1] https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/gfx/layers/apz/src/AsyncPanZoomController.cpp#2646
[2] https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/gfx/layers/apz/util/ChromeProcessController.cpp#138
[3] https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/dom/ipc/TabChild.cpp#1360
[4] https://searchfox.org/mozilla-central/rev/e9d2dce0820fa2616174396459498bcb96ecf812/gfx/layers/apz/src/AsyncPanZoomController.cpp#4327

Comment 10

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

(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #8)
> I reproduced the problem on Moto G5, but kind of intermittently: even when
> it reproduces, scrolling a bit and then trying again tends to work. Other
> times, double-tapping results in just a small change to the zoom level,
> rather than a significant amount of zooming in or out.

FWIW this description makes it sound like the zoom computation is happening but the animation gets canceled or interrupted by something else. Racy behaviour with respect to animation cancellation might explain the intermittent-ness too. So I would look for things like extra layers updates or such happening around the zoom animation that might call CancelAnimation().

Comment 11

[Jim Mathies [:jimm]]

checking in on this bug, any update?

Comment 12

[Marcia Knous [:marcia]]

Per triage team - Hello Kashav - Any updates on this bug? Thanks.

Comment 13

[Pascal Chevrel:pascalc]

David, could you help finding us an owner for this bug? It's been stalled for a month now. Thanks!

Comment 14

[Botond Ballo [:botond]]

I'll take this bug over as Kashav has finished his internship.

Comment 15

[Botond Ballo [:botond]]

The problem here is that CalculateRectToZoomTo() uses the mScrollOffset of the FrameMetrics computed by CalculateBasicFrameMetrics() to reason about which portion of the page is visible on-screen. That value is set based on the nsIScrollableFrame scroll offset.

Bug 1423011 changed the scroll offset we store in the RCD-RSF's nsIScrollableFrame from the visual viewport offset to the layout viewport offset, which is no longer what we want for the zoom-to-rect computation.

Comment 16

[Botond Ballo [:botond]]

WIP patch + test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26316ac8df46b025f8b9c80dd26a25ee7bc429f4

Comment 17

[Botond Ballo [:botond]]

Attachment: Bug 1478335 - Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #16)
> WIP patch + test:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=26316ac8df46b025f8b9c80dd26a25ee7bc429f4

I put up the patch for review. I'm going to split off the test into a separate bug so it doesn't block landing the fix.

Comment 19

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

Comment on attachment 9007878 [details]
Bug 1478335 - Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Kartikaya Gupta (email:[e-mail]) (parental leave) has approved the revision.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edc165c5253a
Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/edc165c5253a

Comment 22

[Botond Ballo [:botond]]

Comment on attachment 9007878 [details]
Bug 1478335 - Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Approval Request Comment
[Feature/Bug causing the regression]:
  Bug 1423011 (part of APZ viewport compat work).

[User impact if declined]:
  Double-tap-to-zoom behaviour is broken on some websites.

[Is this code covered by automated tests?]:
  There is some existing coverage in APZ mochitests, which
  is being expanded in bug 1490102.

[Has the fix been verified in Nightly?]:
  Yes, by me.

[Needs manual test from QE? If yes, steps to reproduce]: 
  No.

[List of other uplifts needed for the feature/fix]:
  None.

[Is the change risky?]:
  Very low.

[Why is the change risky/not risky?]:
  It is a one-line change, specific to double-tap-to-zoom
  code, that's obvious in retrospect.

[String changes made/needed]:
  None.

Comment 23

[Pascal Chevrel:pascalc]

Comment on attachment 9007878 [details]
Bug 1478335 - Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Low-risk patch that fixes an annoying UX regression for users, uplift approved for 63 beta 6, thanks.

Comment 24

[Bogdan Surd, QA [:bsurd, NI]]

Devices:
 - Samsung Galaxy S8 (Android 8.0);
 - OnePlus 5T (Android 8.1.0);

Verified as fixed in the latest version of Nightly. Leaving the qe-verify flag to recheck once this lands in beta as well.

Comment 25

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c22ce5828890

Comment 26

[Stefan Deiac]

Verified as fixed in Firefox 63.0b7 on LG G4 (Android 5.1), Nexus 5 (6.0.1) and Samsung Galaxy S8 (Android 8.0.0).