526839 - Smooth out panning while kinetic scrolling is happening

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Patch v1

While kinetic scrolling, if I start a pan the scrolling immediately stops.  This patch allows scrolling to continue if it's in the same direction, and also contains yet another rewrite of the kinetic panning algorithm.  Instead of measuring the velocity at each given step, I calculate the total distance moved within a given interval and base kinetic speed on that value.  This makes the variance in kinetic scrolling speeds much smaller.  This prevents super-fast scrolling when there isn't enough mousemovement events.

The big change is in KineticController.start (another rewrite of kinetic panning).  I consider this risky.  Please play with it on the N810 and N900 as there may be unexpected regressions.

For code reviewers, a quick overview of changes:

>+// how much movement input to take before mouse up for calculating kinetic speed
>+const kSwipeLength = 160;

New constant / preference for the new algorithm.  A couple of other irrelevant preferences have been removed.  Yet others were tweaked to better match the new algorithm.

> @@ -479,18 +476,21 @@ MouseModule.prototype = {
>   _onMouseDown: function _onMouseDown(evInfo) {
>     this._owner.allowClicks();
>-    if (this._kinetic.isActive())
>-      this._kinetic.end();
>+    if (this._dragData.dragging) {
>+      // Somehow a mouse up was missed.
>+      let [sX, sY] = dragData.panPosition();
>+      this._doDragStop(sX, sY, !dragData.isPan());
>+    }

Kinetic panning is no longer ended if active.  Also, I put this check in to make sure dragStop is called if a mouseup is never received (I've seen some indefinite paused rendering, and I think this *may* be the culprit).

>   _doDragStart: function _doDragStart(event) {
>     let dragData = this._dragData;
>     dragData.setDragStart(event.screenX, event.screenY);
>-    this._dragger.dragStart(event.clientX, event.clientY, event.target, this._targetScrollInterface);
>+    this._kinetic.addData(0, 0);
>+    if (!this._kinetic.isActive())
>+      this._dragger.dragStart(event.clientX, event.clientY, event.target, this._targetScrollInterface);

Drag start now calls addData again.  addData now takes changes in x and y so that if locking happens at a different spot besides the start point, kinetic panning won't move over a few pixels to the left or right and show the sidebars.  This might also have some nice code cleanup effects since panning also takes dx/dy.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Patch v2

Ha!  I found the mismatched end drag.  If kinetic panning never started, I didn't finish the drag.

Comment 2

[Benjamin Stover (:stechz)]

Attachment: Patch v3

Switched to the old way of handling calling dragStop if kinetic panning isn't started.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

>-    if (this._kinetic.isActive())
>-      this._kinetic.end();

Ben, I was a bit hesitant about that. This means if we do a lot of quick pan we never refresh the canvas, and so the user never have a feedback of where he is.
But for sure this speed up panning.

Comment 4

[Stuart Parmenter]

I think its the right thing to do so that we don't stop panning

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 410601 [details] [diff] [review]
Patch v3

Code is ok , the only little nit is:
   var self = this;
   this._kinetic = new KineticController(
-    function _dragByBound(dx, dy) { return self._dragBy(dx, dy); },
-    function _dragStopBound() { return self._doDragStop(0, 0, true); }
+    Util.bind(this._dragBy, this),
+    Util.bind(this._kineticStop, this)
   );

I had to look at it twice before understanding that they were parameters of a function. maybe we could write something like that:

this._kinetic = new KineticController(Util.bind(this._dragBy, this),
                                      Util.bind(this._kineticStop, this));

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mobile-browser/rev/3155f50ce1ce

Comment 7

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b3pre) Gecko/20091113 Firefox/3.6b3pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091113 Firefox/3.7a1pre Fennec/1.0b5