Closed Bug 1478335 Opened 2 years ago Closed 2 years ago

Double tap to zoom out does not work

Categories

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

63 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 --- verified

People

(Reporter: sflorean, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

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
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.
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!
Component: Theme and Visual Design → General
Blocks: 1423011
Kashav, Tanushree, it seems that your patch in bug 1423011 caused this regression, could you have a look please? Thanks
Flags: needinfo?(tanushree.podder.103)
Flags: needinfo?(kmadan)
I'm not able to reproduce this (tested with a local build based on recent mozilla-central). Double-tapping seems to work fine.
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).
Summary: Double tap to zoom in/out doesn't work → Double tap to zoom out does not work
Good point, the behaviour may be device dependent. I was testing with a Sony Z3C.
(Screen size dependent, in particular, would be my guess.)
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.
Flags: needinfo?(tanushree.podder.103)
Component: General → Panning and Zooming
Priority: -- → P3
Product: Firefox for Android → Core
Whiteboard: [gfx-noted]
Version: Firefox 63 → 63 Branch
Assignee: nobody → kmadan
Flags: needinfo?(kmadan)
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
(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().
checking in on this bug, any update?
Flags: needinfo?(kmadan)
Per triage team - Hello Kashav - Any updates on this bug? Thanks.
David, could you help finding us an owner for this bug? It's been stalled for a month now. Thanks!
Flags: needinfo?(ddurst)
I'll take this bug over as Kashav has finished his internship.
Assignee: kshvmdn → botond
Flags: needinfo?(kshvmdn)
Flags: needinfo?(ddurst)
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.
(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.
Blocks: 1490102
Comment on attachment 9007878 [details]
Bug 1478335 - Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats

Kartikaya Gupta (email:kats@mozilla.com) (parental leave) has approved the revision.
Attachment #9007878 - Flags: review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edc165c5253a
Use the visual viewport offset to compute the visible area in CalculateRectToZoomTo(). r=kats
https://hg.mozilla.org/mozilla-central/rev/edc165c5253a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify+
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.
Attachment #9007878 - Flags: approval-mozilla-beta?
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.
Attachment #9007878 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.