Closed Bug 1111333 Opened 9 years ago Closed 7 years ago

Double-tap-drag to zoom (a.k.a. one-touch pinch)

Categories

(Firefox for Android Graveyard :: Toolbar, enhancement)

All
Android
enhancement
Not set
normal

Tracking

(relnote-firefox -, firefox55 disabled)

VERIFIED FIXED
Firefox 55
Tracking Status
relnote-firefox --- -
firefox55 --- disabled

People

(Reporter: prescott66, Assigned: contact)

References

Details

(Keywords: uiwanted)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141201171754

Steps to reproduce:

Zoom on page with double tapping


Actual results:

page is not good zoomed


Expected results:

It would be much better, if zooming can be done as in google chrome for example - double tap and while holding finger ondisplay and sliding up/down, page is zoomed. it is much comfortable way to zoom, mainly bigger pages.
You can use the two-finger pinch zoom to accomplish the same thing. Is there a specific reason you want the one-finger zoom feature?
Zooming with two fingers is one method and zooming with one finger is second method, that can be made more comfortable. I trust that many people dont like actual way of one-finger zooming. Sometimes makes me mad.
This will require the UX team to review and decide if they want this behaviour or not.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: uiwanted
OS: Linux → Android
Hardware: x86_64 → All
Summary: Problematic zooming → Enhancement: single finger to zoom
Once I learned of this gesture I often miss it when I use my phone in one hand. 

The two finger pinch-to-zoom is most known gesture but please note that this "Double touch drag" is also now officialy acknowledged gesture for scaling content and not something artificial or unique. http://developer.android.com/design/patterns/gestures.html

This gesture is detected in apps such as Chrome, Google Maps as well as various 3rd party apps because of native support for this QuickScale gesture in ScaleGestureDetector (it's also available in AppCompat library so it's not limited to higher versions of Android). https://developer.android.com/reference/android/support/v4/view/ScaleGestureDetectorCompat.html

Please consider that in the future there may be more and more apps supporting this gesture and users may expect it from Firefox as well.
Thanks for the info! I didn't realize it was officially acknowledged by Android. ni? to one of the UX designers for comment.
Flags: needinfo?(alam)
Offering this alternative for zooming is not a bad idea, particularly since it doesn't seem to affect the normal way people pinch and zoom. From what I'm understanding, this enhancement makes sense but I worry about it affecting the text-selection action.

Let's get a build to test this out. CC'ing a couple other people here.
Flags: needinfo?(alam)
To sum up: Fennec currently supports two ways to zoom.

The first is pinching.

The second is double-tap. Double-tapping on an element on a non-mobile site will zoom so that the tapped element approximately fits the screen. This works great for making a column of text the right size to fit on the screen.


This bug is proposing a third: double-tap-drag.

The gesture guide draws a subtle distinction between DTD and pinch: one is for _scaling_ and one is for _zooming_. The former is for maps, the latter is for images.

Some questions, then:

* Does a map-oriented metaphor apply here as well as an image-oriented metaphor? Apparently Chrome thinks it does.

* Should pinching and DTD do the same thing, or something different? E.g., text zoom, different behavior on desktop/mobile sites?

* Will this screw with scrolling, tap events, double-tap events, etc.? What happens if you double-tap in a textarea? Etc. etc.


Re comment 6: if we want to try this out, someone needs to prioritize the work and get it done; it's not free. (That person ain't me.)
Summary: Enhancement: single finger to zoom → Double-tap-drag to zoom
Version: Firefox 34 → Trunk
If someone wants to look at this, my opinion on it would be that we switch to the default scale detector on KitKat+. i.e. we have our own scale detector here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/SimpleScaleGestureDetector.java

that we wrote long ago for some reasons listed at the top of it. Some of those are fixed, and some are better now (also I always hated it that we wrote our own). I think we should just make our ScaleGestureDetector inherit from Android ScaleGestureDetector, and then on KitKat+ we can use the normal Android one. On KitKat-, we can use our old one:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#129

I tried writing that the other day, and more issues with the TouchEventHandler still stealing events during the pan. i.e. the long tail details here will take a bit of work.
Actually I think we'll want to implement this ourselves in the native APZ rather than use Android's gesture detection. Kats?
Flags: needinfo?(bugmail.mozilla)
I concur, we should implement it in the native APZ.
Flags: needinfo?(bugmail.mozilla)
As I couldn't live without one hand zoom I hacked it in Xposed (if anyone's interested, see http://forum.xda-developers.com/xposed/modules/mod-one-hand-zoom-enabler-adobe-reader-t3251070 ). 

With only few changes/fixes (basically to process double tap only after second tap release), replacing the custom scale detector with stock detector worked just fine. 
I also noticed that the JavaPanZoomController.MAX_ZOOM (4.0f) isn't currently limiting anything.
I'm not familiar with Xposed but it it might stop working in tomorrow's nightly since we just landed the patch to switch away from JavaPanZoomController in bug 1206872. That change will stay on nightly for the time being though, so your solution should continue to work in Aurora and up. I'm happy to mentor you through implementing this properly in our codebase if you would like. If not we will probably get to it eventually but probably not until next year at least.
I would like to take a stab at implementing this.

@Marcel it looks like you've done a lot already (https://github.com/moneytoo/AllMy.../blob/master/app/src/main/java/com/smartmadsoft/xposed/aio/tweaks/onehandzoomenabler/FF.java) but I wonder if there are any more recent architectural developments since the APZ and the SimpleScaleGestureDetector that :snorp and :wesj mention. :kats would you or someone else be able to mentor me on implementation?

Thanks all.
gizmoguy1, thanks for your interest! Since this bug was filed we have moved away from the JavaPanZoomController and SimpleScaleGestureDetector code mentioned above. Currently zooming is handled in C++ APZ ("async pan/zoom") code that is shared across platforms. Specifically there is a state machine in the GestureEventListener class [1] which detects different gestures (such as pinch-zooming, or double-tap) and then triggers the appropriate action. This state machine would need to be modified to detect the double-tap-drag gesture and use it to drive a zoom.

If you're interested in working on this bug I'd be happy to mentor you. The first step would be to get an android build going locally - there are instructions at [2] on how to get that setup. Once you have that going I can assign the bug to you and get into more specific details. Note that the implementation will be entirely in C++ and you'll need to be comfortable programming in that language in order to get this done.

[1] http://searchfox.org/mozilla-central/source/gfx/layers/apz/src/GestureEventListener.cpp
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build
Is this bug still up for grabs? I wouldn't mind giving it a go if no one else has solved it already. 

Kartikaya: Is the class documentation old, or has the situation changed since you posted your comment three months ago? I found this in GestureEventListener.h:
>Android doesn't use this class because it has its own built-in gesture event listeners that should generally be preferred.
Flags: needinfo?(bugmail)
(In reply to dennis.a.ek from comment #17)
> Is this bug still up for grabs? I wouldn't mind giving it a go if no one
> else has solved it already. 

Yes, it is still up for grabs. Please feel free to have a go at it!

> Kartikaya: Is the class documentation old, or has the situation changed
> since you posted your comment three months ago? I found this in
> GestureEventListener.h:
> >Android doesn't use this class because it has its own built-in gesture event listeners that should generally be preferred.

The comment in GestureEventListener.h is wrong, I'll fix it. Comment 16 is still accurate.
Flags: needinfo?(bugmail)
Attached patch A patch that solves the bug. (obsolete) — Splinter Review
Attachment #8860444 - Flags: review?(liuche)
Comment on attachment 8860444 [details] [diff] [review]
A patch that solves the bug.

Review of attachment 8860444 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review
Attachment #8860444 - Flags: review?(liuche) → review?(bugmail)
I kicked off a try push with this patch to make sure it passes our existing tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f2c08c16db683e277f66fd0dd14e209683959aa
Assignee: nobody → dennis.a.ek
Comment on attachment 8860444 [details] [diff] [review]
A patch that solves the bug.

Review of attachment 8860444 [details] [diff] [review]:
-----------------------------------------------------------------

So this partly works - in that you can zoom in quite well. However, it doesn't support zooming *out* particularly well. As it stands your code will generate a negative span if you move your finger far enough upwards on the second tap, and that results in the zoom behaviour just aborting with s snap-like behaviour which is not great. I think what would be better is that instead of returning the literal span computed from the touch events, we always start the pinch sequence with a span of 1.0 and then if the user moves their finger down we scale the span up multiplicatively, and if they move their finger up (so that current.y - start.y is negative) we scale the span down to fractional values. Does that make sense?

Other than that your code looks really good. Thanks for adding the comments and documentation as well! I found a few minor stylistic nits, pointed out below. Also when you upload the next version of your patch, please use -U8 in the hg export options to get 8 lines of context in the patch instead of the default of 3. You can also get this by adding the following to your ~/.hgrc file:

[diff]
unified = 8

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +284,5 @@
>  
> +  // The user has performed a double tap, but not lifted her finger.
> +  case GESTURE_SECOND_SINGLE_TOUCH_DOWN: {
> +    // If touch has moved noticeably, change state
> +    if(MoveDistanceIsLarge()) {

nit: please add a space between "if" and "("

@@ +294,5 @@
> +      ParentLayerCoord currentSpan = GetYSpanFromStartPoint();
> +      ParentLayerPoint currentFocus = GetCurrentOneTouchFocus();
> +
> +      // Change scaling direction, should it be necessary
> +      if(currentSpan < 0) {

Ditto here

@@ +379,5 @@
> +    ParentLayerCoord currentSpan = GetYSpanFromStartPoint();
> +    ParentLayerPoint currentFocus = GetCurrentOneTouchFocus();
> +
> +    // Change scaling direction, should it be necessary
> +    if(currentSpan < 0) {

Ditto
Attachment #8860444 - Flags: review?(bugmail) → feedback+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> that results in the zoom behaviour just aborting with s
> snap-like behaviour which is not great.

This behaviour actually seems unrelated to your patch. I can reproduce it with normal pinch-zoom as well, and I filed bug 1358771 for it. My other comment about not being able to zoom out still stands though.
You're right! I should have tested the patch more thouroughly before I uploaded the patch. I wll get a second version ready within the next couple of days (most likely today).
(In reply to Dennis Ek from comment #25)
> You're right! I should have tested the patch more thouroughly before I
> uploaded the patch. I wll get a second version ready within the next couple
> of days (most likely today).

Thanks! Also I posted a patch to bug 1358771 that you can apply locally, if the weird "snapping" behaviour is interfering with your testing. I'm not yet sure if that patch causes other regressions so I don't want to land it right away but it should at least unblock you from testing if you need it.
Attached patch Second version (obsolete) — Splinter Review
A second version of the patch. The bugs are fixed, and everything works on my Samsung Galaxy S5 Mini.
Attachment #8860444 - Attachment is obsolete: true
Attachment #8861324 - Flags: review?(bugmail)
Accidentally uploaded the wrong file. This is the correct one.
Attachment #8861324 - Attachment is obsolete: true
Attachment #8861324 - Flags: review?(bugmail)
Attachment #8861325 - Flags: review?(bugmail)
Comment on attachment 8861325 [details] [diff] [review]
Second version (correct file this time)

Review of attachment 8861325 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/GestureEventListener.cpp
@@ +374,5 @@
> +    // Calculate the amount of zoom.
> +    if (currentSpan.value < 0) {
> +      currentSpan = mPreviousSpan * (1.0f - ONE_TOUCH_PINCH_SPEED);
> +    } else {
> +      currentSpan = mPreviousSpan * (1.0f + ONE_TOUCH_PINCH_SPEED);

When I tested this patch, I observed the following behaviour:
- double-tap and move finger down. content zooms in
- as part of the same gesture, move finger up. content continues zooming in
- continue moving finger up. content eventually zooms out

I think what's happening here is that the span calculation you're using grows exponentially larger as long as your finger is below the start point, regardless of the direction it's traveling.

For example, say I start with my finger at y=100 and then move it to y=101, y=102, y=103, y=104, y=105. currentSpan starts off at 1.0 (with y=100) but then on each subsequent touchmove it gets multiplied by 1.04, so that by the time it reaches y=105, the span is 1.217. Now let's say I move it back down to y=104. At this point GetYSpanFromStartPoint().value is going to be 4 which is > 0, so you're still going to multiply by 1.04, and the computed span will be 1.265. Until your finger actually goes above mTouchStartPosition you're going to keep zooming in.

Also this algorithm doesn't take into account the distance of the finger from the start point. That means if you're moving your finger very slowly and sending touchmove events every 1 pixel, or if you're moving your finger fast and sending touchmove every 10 pixels, the behaviour will be the same per touchmove. This doesn't allow the user to move their finger slower to more gradually perform the zoom. i.e. they have no control over the amount of the zoom that happens per move.

I think what you want to be doing here is something like this:
  ParentLayerCoord currentSpan = GetYSpanFromStartPoint();
  float effectiveSpan = 1.0f + (fabsf(currentSpan.value) * ONE_TOUCH_PINCH_SPEED);
  if (currentSpan.y < 0) {
    effectiveSpan = 1.0f / effectiveSpan;
  }
  
and then use `effectiveSpan` instead of `currentSpan` in the places below. This will compute a span scaling linearly upwards from 1.0 based on the distance the finger is from the start point. If you are at a negative distance, it then inverts the span to zoom out instead of zoom in. You may need to adjust ONE_TOUCH_PINCH_SPEED to a lower value to get it to feel right.
Attachment #8861325 - Flags: review?(bugmail)
I actually thought that was my phone not processing the events fast enough, but I noticed it too now that I know about it. If no one posts a revised patch before I do, I will submit one later this week.
Dennis, are you still planning to finish this up? There should be very little work left from your last patch.
Flags: needinfo?(contact)
Attached patch Fixed bug.Splinter Review
Attachment #8861325 - Attachment is obsolete: true
Flags: needinfo?(contact)
Attachment #8870504 - Flags: review?(bugmail)
Comment on attachment 8870504 [details] [diff] [review]
Fixed bug.

Review of attachment 8870504 [details] [diff] [review]:
-----------------------------------------------------------------

This feels much better, thanks! I noticed one small typo in a comment (see below), I can fix that up when I land the patch shortly.

::: gfx/layers/apz/src/GestureEventListener.h
@@ +124,5 @@
>      // pinched enough for us to start zooming the screen.
>      // Allowed next states: GESTURE_NONE
> +    GESTURE_PINCH,
> +
> +    // The user has double tapped, but not moved her finger, and moved her

I think instead of "not moved her finger" you mean "not lifted her finger"?
Attachment #8870504 - Flags: review?(bugmail) → review+
That is indeed a typo. Your assumption is also correct.
https://hg.mozilla.org/mozilla-central/rev/f5a0a69abe4b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Nice work with the patch, Dennis! Please feel free to look around for other bugs to pick up, if you're interested in continuing to contribute to the project. I can also help you find something suitable if you would like.
This might be something for our release notes.

Dennis, if you press "Edit bug" and look in the "Firefox Tracking Flags" section are you able to set the "relnote-firefox" flag to "?" (and fill out the short form in the comment box)?
I can do that:

Release Note Request (optional, but appreciated)
[Why is this notable]: new user-facing feature
[Affects Firefox for Android]: yes (doesn't affect desktop)
[Suggested wording]: Added support for the double-tap-drag gesture for one-handed zooming, consistent with other Android applications
[Links (documentation, blog post, etc)]: none
relnote-firefox: --- → ?
adding to 55beta release notes
Feature is working in 55.0b7 but how do I disable this feature? I'm accidentally triggering it while scrolling. I don't see anything in the settings and I'm happy to use an about:config setting if I only knew what it was.
That's a good point. I've filed bug 1379394 to add a pref to allow disabling it.
Verified the new gesture works fine on Beta 55.0b8 (2017-11-07).
Devices:
-HTC Nexus 9 (Android 7.1.1)
-Prestigio Grace X5 (Android 4.4.2)
-HTC 10 (Android 6.0.1)
-LG G4 (Android 5.1)
Status: RESOLVED → VERIFIED
@Richard: you should now be able to disable the feature by setting apz.one_touch_pinch.enabled to false in about:config. It is in 55.0b8.
See Also: → 1387055
See Also: → 1387060
Summary: Double-tap-drag to zoom → Double-tap-drag to zoom (a.k.a. one-touch pinch)
Disabled in 55 rc2 in bug 1387078, will remove from release notes.
rbarker - is this still on your radar as something we may fix? 
I'm seeing it in release note triage, but not anywhere else, and am not sure it is an active project.
Flags: needinfo?(rbarker)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #46)
> rbarker - is this still on your radar as something we may fix? 
> I'm seeing it in release note triage, but not anywhere else, and am not sure
> it is an active project.

This is actually an APZ issue and not a Toolbar issue so probably should move it. My guess is there are a number of bugs labeled toolbar that are actually APZ. I have no idea on the current status.
Flags: needinfo?(rbarker)
As far as I can tell, out of the three genuine bugs blocking this, two have been resolved (bug 1391770, bug 1387060) with only bug 1387055 remaining and in any case I don't think this was disabled again in 56 (and certainly not in 57).
Do you think it is worth adding to the FxA 57 (or maybe 56, retroactively) relnotes?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #49)
> Do you think it is worth adding to the FxA 57 (or maybe 56, retroactively)
> relnotes?

We haven't had anyone do a round of QA on it, so probably not.
Blocks: 1463139
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.