Closed Bug 526839 Opened 13 years ago Closed 13 years ago

Smooth out panning while kinetic scrolling is happening

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0

People

(Reporter: stechz, Assigned: stechz)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #410575 - Flags: review?(21)
Attached patch Patch v2 (obsolete) — Splinter Review
Ha!  I found the mismatched end drag.  If kinetic panning never started, I didn't finish the drag.
Assignee: nobody → webapps
Attachment #410575 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #410583 - Flags: review?(21)
Attachment #410575 - Flags: review?(21)
Attached patch Patch v3Splinter Review
Switched to the old way of handling calling dragStop if kinetic panning isn't started.
Attachment #410583 - Attachment is obsolete: true
Attachment #410601 - Flags: review?(21)
Attachment #410583 - Flags: review?(21)
>-    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.
I think its the right thing to do so that we don't stop panning
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));
Attachment #410601 - Flags: review?(21) → review+
pushed http://hg.mozilla.org/mobile-browser/rev/3155f50ce1ce
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Target Milestone: --- → Post-B5
Component: General → Panning/Zooming
You need to log in before you can comment on or make changes to this bug.