Closed Bug 590246 Opened 9 years ago Closed 9 years ago

Multitouch swipe gestures for top/bottom/back/forward

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0b1+)

VERIFIED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: mbrubeck, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached patch mobile-browser patch (obsolete) — Splinter Review
Add a two-finger swipe gesture:

* Up: Scroll to top of page.
* Down: Scroll to bottom of page.
* Left: Go back.
* Right: Go forward.

We considered using a three-finger gesture, but some common Android hardware (including most HTC phones like the Nexus One) does not support three-finger multitouch events.
Attachment #468743 - Flags: review?(mark.finkle)
Attached patch Android patch (WIP) (obsolete) — Splinter Review
For testing only, not ready for checkin
tracking-fennec: --- → ?
Comment on attachment 468743 [details] [diff] [review]
mobile-browser patch

Nice. I assume the hard work happens in the platform :)
Attachment #468743 - Flags: review?(mark.finkle) → review+
tracking-fennec: ? → 2.0b1+
cmd_scroll{Top,Bottom} do not actually work in Fennec (probably the same cause as bug 547307).  Use browser methods instead, at least until bug 576192 lands.
Attachment #468743 - Attachment is obsolete: true
Attachment #468817 - Flags: review?(mark.finkle)
Attachment #468817 - Attachment is patch: true
Attachment #468817 - Attachment mime type: application/octet-stream → text/plain
Attached patch Android patch v2 (obsolete) — Splinter Review
Uses two thresholds to distinguish between pinch and swipe:  If the user zooms in or out more than a certain amount, then no swipe event will fire, even if they later move back toward their starting zoom level.  Otherwise, a swipe event will fire if the gesture ends a certain distance from its starting point.

These thresholds work well on a Nexus One, but we need to test on other devices to see how they might need to be adjusted.

Note: According to the docs, the "delta" value for the NS_SIMPLE_GESTURE_MAGNIFY event should be the total delta from the entire gesture, not the incremental delta from the last update.  This patch fixes this, although we don't currently use that value in the front-end.
Attachment #468745 - Attachment is obsolete: true
Attachment #468820 - Flags: review?(blassey.bugs)
so if the user swipes but never pinch zooms nothing happens during the gesture?
(In reply to comment #5)
> so if the user swipes but never pinch zooms nothing happens during the gesture?

We don't prevent the pinch events from firing during the swipe, so users might see the zoom level fluctuate while they are swiping.  In practice I don't find this noticeable for quick swipe gestures.

I suppose we could wait until pinchDelta passes the no-swipe threshold before starting the pinch zoom animation, but that might hurt the responsiveness of pinch zoom.
(In reply to comment #6)
> (In reply to comment #5)
> I suppose we could wait until pinchDelta passes the no-swipe threshold before
> starting the pinch zoom animation, but that might hurt the responsiveness of
> pinch zoom.

I wasn't suggesting that, but actually the opposite. I was thinking that its not a good experience if a user is making the swipe gesture and sees no response from the app until the its done. On the other hand I guess its a pretty explicit gesture. Perhaps something like contrails would help.
Comment on attachment 468817 [details] [diff] [review]
mobile-browser patch v2

You might want to file a followup bug and use a // BUG comment to make sure we revert to the commands when tiles are killed.
Attachment #468817 - Flags: review?(mark.finkle) → review+
Filed followup bug 590535 and added a comment.  mobile-browser patch pushed:
http://hg.mozilla.org/mobile-browser/rev/40d9e8b7a575
Depends on: 590565
I copied the up/down gestures from Mac OS X, but this doesn't make sense for Fennec.  Fennec's swipe gestures should be reversed, to match the single-finger panning.  This also agrees with what Madhava wrote at https://wiki.mozilla.org/Mobile/Projects/Multitouch -

* Swipe down: Go to top of page.
* Swipe up: Go to bottom of page.
Attachment #469083 - Flags: review?(mark.finkle)
Attachment #469083 - Flags: review?(mark.finkle) → review+
I have two concerns with this patch. The first is that we send pinch gesture events during a swipe gesture until the very end (when the gesture ends). It seems like we could determine during the update phase that its a swipe gesture and start sending a swipe gesture update event.

The second concern is related but is directed at our crack UX team. The way the platform is sending events, it doesn't seem like its possible for the front end to give any feedback to the user while they're performing a swipe event. Is this okay?
Agree with the first concern - zooming a page during a multitouch event would be disconcerting, I think.

For the second concern, by "while" do you mean "as the gesture is being performed"?
(In reply to comment #13)
> For the second concern, by "while" do you mean "as the gesture is being
> performed"?

yea, no feedback to the user while the user is performing the gesture.
I think that's OK. Not sure if we want to do feedback on gesture completion. Android offers a sort of "notification text" thing (I see it when I save a draft in email) that might be useful to:

 - describe the gesture just performed (ie: "Back")
 - indicate that the gesture wasn't recognized (ie: "lolwut?")
Attached patch Android patch v3Splinter Review
(In reply to comment #12)
> I have two concerns with this patch. The first is that we send pinch gesture
> events during a swipe gesture until the very end (when the gesture ends). It
> seems like we could determine during the update phase that its a swipe gesture
> and start sending a swipe gesture update event.

Okay, the patch now sends the swipe gesture as soon as it's detected, stopping the pinch gesture while it's still in progress.  It's definitely nicer this way.
 
(In reply to comment #13)
> Agree with the first concern - zooming a page during a multitouch event would
> be disconcerting, I think.

It's really not, in practice, especially now that we stop the zooming as soon as the swipe threshold is reached.  If you pinch in our out enough to change the zoom level by a lot, then we don't treat it as a swipe anyway.

(In reply to comment #15)
> I think that's OK. Not sure if we want to do feedback on gesture completion.
> Android offers a sort of "notification text" thing (I see it when I save a
> draft in email) that might be useful to:
> 
>  - describe the gesture just performed (ie: "Back")
>  - indicate that the gesture wasn't recognized (ie: "lolwut?")

I don't think any notification is necessary.  These gestures are pretty instantaneous.  You move your fingers about an inch (takes less than half a second) and before they even leave the screen we have jumped to the top or bottom of the page, or started navigating back/forward.

These are quick swipe gestures, not the sort of complex "draw a letter S" gestures that typically occur in a separate mode with on-screen feedback.
Attachment #468820 - Attachment is obsolete: true
Attachment #469271 - Flags: review?(blassey.bugs)
Attachment #468820 - Flags: review?(blassey.bugs)
Comment on attachment 469271 [details] [diff] [review]
Android patch v3

one nit, mGestureCanceled is a bit misleading. How about mGestureFinished?
Attachment #469271 - Flags: review?(blassey.bugs) → review+
(In reply to comment #17)
> Comment on attachment 469271 [details] [diff] [review]
> Android patch v3
> 
> one nit, mGestureCanceled is a bit misleading. How about mGestureFinished?

Changed to mGestureFinished and pushed:
http://hg.mozilla.org/mozilla-central/rev/cacebec5b70d

Opened bug 590565 for Qt widget implementation.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
PRETTY. verified FIXED on build:

Mozilla/5.0 (Android; Linux armv71; Nokia N900; en-US; rv:2.0b5pre) Gecko/20100826 Namoroka/4.0b5pre Fennec/2.0a1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=12739 added to regression test this bug
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.