Closed
Bug 526839
Opened 14 years ago
Closed 14 years ago
Smooth out panning while kinetic scrolling is happening
Categories
(Firefox for Android Graveyard :: Panning/Zooming, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
fennec1.0
People
(Reporter: stechz, Assigned: stechz)
Details
Attachments
(1 file, 2 obsolete files)
10.24 KB,
patch
|
vingtetun
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Comment 2•14 years ago
|
||
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)
Comment 3•14 years ago
|
||
>- 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•14 years ago
|
||
I think its the right thing to do so that we don't stop panning
Comment 5•14 years ago
|
||
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+
Comment 6•14 years ago
|
||
pushed http://hg.mozilla.org/mobile-browser/rev/3155f50ce1ce
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 7•14 years ago
|
||
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
Updated•14 years ago
|
Target Milestone: --- → Post-B5
Updated•14 years ago
|
Component: General → Panning/Zooming
You need to log in
before you can comment on or make changes to this bug.
Description
•