Closed Bug 540008 Opened 10 years ago Closed 9 years ago

stop kinetic scrolling when user taps screen


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)

Firefox 5


(Reporter: madhava, Unassigned)




(1 file, 3 obsolete files)

When a page is panning by, having been flicked, sometimes you want to stop the movement because you're going to fly right past the thing you want.  On most touch platforms, you do this by "pinning" the page in place by tapping (tapping or tapping+holding).

We should do this too.  I think we used to, in some earlier version of our kinetic panning behaviour.
Duplicate of this bug: 559633
Assignee: nobody → mbrubeck
Attached patch patch (obsolete) — Splinter Review
This does the trick.  It's identical to the old code, and I can't find any downside.
Attachment #439329 - Flags: review?(mark.finkle)
Comment on attachment 439329 [details] [diff] [review]

vingtetun pointed out that this causes jerkiness when swiping repeatedly to scroll a long ways.  Back to the drawing board.

New plan: stop kinetic scrolling only after the user's touch remains in one place for some amount of time.
Attachment #439329 - Flags: review?(mark.finkle)
Attached patch patch v2 (obsolete) — Splinter Review
This adds a slight delay before panning stops, and will not stop kinetic panning if there is more movement before the delay expires.
Attachment #439329 - Attachment is obsolete: true
Attachment #439360 - Flags: review?(mark.finkle)
Attached patch patch v3 (obsolete) — Splinter Review
Minor fix: Clear the timeout on reset, just in case end() gets called in some other way before the timeout fires.

We discussed another approach on IRC, where touching the screen would cause "friction" and slow the movement smoothly (instead of this patch's abrupt stop).  I think this patch is sufficient to address the problem, but the friction approach is worth experimenting with too.
Attachment #439360 - Attachment is obsolete: true
Attachment #439364 - Flags: review?(mark.finkle)
Attachment #439360 - Flags: review?(mark.finkle)
Comment on attachment 439364 [details] [diff] [review]
patch v3

>diff -r 86be5a52183e chrome/content/InputHandler.js

> KineticController.prototype = {
>   _reset: function _reset() {
>     if (this._timer != null) {
>       this._timer.cancel();
>       this._timer = null;
>     }
>+    if (this._brakesTimeout) {
>+      clearTimeout(this._brakesTimeout);
>+      delete this._brakesTimeout;
>+    }
>     this.momentumBuffer = [];

nit: Line breaks between the code blocks would help readability

>+  /** Stop panning very soon if no more movement is added. */
>+  brakesApplied: function brakesApplied() {
>+    let self = this;
>+    this._brakesTimeout = setTimeout(function() {
>+      self.end();
>+      delete self._brakesTimeout;
>+    }, kKineticBrakesDelay);

Other parts of InputHandler code use a timer instead of a setTimeout (or setInterval). The inconsistency has my OCD active, but I guess it's ok in this case. The brakesTimeout doesn't need to be an nsITimer.

Maybe the kinetic timer doesn't need to be an nsITimer either - but that is a separate bug.

I still need to test the patch.
Attached patch patch v4Splinter Review
No functional changes, just rebased to latest mobile-browser and blank lines added as suggested above.
Attachment #439364 - Attachment is obsolete: true
Attachment #463685 - Flags: review?(mark.finkle)
Attachment #439364 - Flags: review?(mark.finkle)
Comment on attachment 463685 [details] [diff] [review]
patch v4

It was a very nice feature and I would like to see it back.
Nice and simple patch.
Attachment #463685 - Flags: review+

But then backed out because of a tendency to stop panning unintentionally when running on device:

Increasing the delay did not fix the problem with the patch; at 150ms the patch still caused unintentional stops, but was almost useless for stopping panning.
Attachment #463685 - Flags: review?(mark.finkle)
Fixed by patches in bug 639677
Assignee: mbrubeck → nobody
Depends on: 639677
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
Fixed by bug 639677.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 5
verified FIXED On build (Checked on zoomed-in
Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110504 Firefox/6.0a1 Fennec/6.0a1 ID:20110504042024
Flags: in-litmus?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.