639179 - Improve kinetic panning

Status: VERIFIED FIXED

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

Description

[Benjamin Stover (:stechz)]

This problem has been around for a while, but it hadn't been obvious until checkerboarding was a little smoother. We were ignoring drags made by the user until they hit kinetic panning.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: Consecutive pans are jumpy

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 517141 [details] [diff] [review]
Consecutive pans are jumpy


>+        this._dragBy(this.dX, this.dY);
>+        // dragBy will reset dX and dY values to 0.


I'd like the comment before the code

Comment 3

[Stuart Parmenter]

Comment on attachment 517141 [details] [diff] [review]
Consecutive pans are jumpy

Please don't land this without sorting out the speed issues

Comment 4

[Benjamin Stover (:stechz)]

Attachment: Part 1: Fix waitingForPaint bug when kinetic panning is active

Comment 5

[Benjamin Stover (:stechz)]

Attachment: Part 2: Carefully track values sent to dragger

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Part 3: Tapping during kinetic panning stops panning

Comment 7

[Benjamin Stover (:stechz)]

Attachment: Part 4: Panning speed determines initial kinetic speed

Comment 8

[Benjamin Stover (:stechz)]

Attachment: Part 5: Use exponential approximation for kinetic friction

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 524703 [details] [diff] [review]
Part 2: Carefully track values sent to dragger

Thanks for breaking up this patch into chunks I can understand even in my current sleep-deprived state. :)

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 524704 [details] [diff] [review]
Part 3: Tapping during kinetic panning stops panning

>+    if (!dragData.isPan()) {
>+      this._kinetic.end();
>       this._dragger.dragStop(0, 0, this._targetScrollInterface);
>       this._dragger = null;
>     } else if (dragData.isPan()) {

Change "else if" to just "else" (and maybe swap the order of the if/else blocks).

>+      if (Date.now() - this._dragStartTime > 300) {
>+        // XXX
>+        this._kinetic._velocity.set(0, 0);
>+      }

Remove the XXX comment and the curly braces.

>+      currentVelocityX = clampFromZero(this._velocity.x + this._acceleration.x * currentTime, 0, 6);
>+      currentVelocityY = clampFromZero(this._velocity.y + this._acceleration.y * currentTime, 0, 6);

This could be Util.clamp(value, -6, 6) instead of clampFromZero(value, 0, 6).

>+    this._velocity.x = clampFromZero((distanceX / swipeLength) + currentVelocityX, Math.abs(currentVelocityX), 6);
>+    this._velocity.y = clampFromZero((distanceY / swipeLength) + currentVelocityY, Math.abs(currentVelocityY), 6);

Please replace "6" with a named constant (in these lines and the previous ones).

>+  resume: function resume() {
>+    if (this._paused)
>+      this._startTimer();
>+  },

Is resume() actually needed?

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 524705 [details] [diff] [review]
Part 4: Panning speed determines initial kinetic speed

>+    this._velocity.x = clampFromZero((distanceX / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityX, Math.abs(currentVelocityX), 6);
>+    this._velocity.y = clampFromZero((distanceY / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityY, Math.abs(currentVelocityY), 6);

You could pull "Math.min(swipeLength, lastTime - mb[0].t))" out into a local variable.

Comment 12

[Benjamin Stover (:stechz)]

> Change "else if" to just "else" (and maybe swap the order of the if/else
> blocks).

OK.

> Remove the XXX comment and the curly braces.

Done.

> This could be Util.clamp(value, -6, 6) instead of clampFromZero(value, 0, 6).

Good call. This works for this code, but not for the later code. Changed here, but still need clampFromZero sadly.

> Please replace "6" with a named constant (in these lines and the previous
> ones).

Done.

> Is resume() actually needed?

Nope! Removed.

> >+    this._velocity.x = clampFromZero((distanceX / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityX, Math.abs(currentVelocityX), 6);
> >+    this._velocity.y = clampFromZero((distanceY / Math.min(swipeLength, lastTime - mb[0].t)) + currentVelocityY, Math.abs(currentVelocityY), 6);
> 
> You could pull "Math.min(swipeLength, lastTime - mb[0].t))" out into a local
> variable.

Done.

Comment 13

[Benjamin Stover (:stechz)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/5d997f85c1b9
http://hg.mozilla.org/mozilla-central/rev/195c8ad12184
http://hg.mozilla.org/mozilla-central/rev/e5f8db95d0c5
http://hg.mozilla.org/mozilla-central/rev/97a7a9f80725
http://hg.mozilla.org/mozilla-central/rev/fd6ad8e567c9

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out due to massive mobile unittest orange following cedar merge (as the most suspicious candidate):
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=fe3f7889918b

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Not sure if bug 652219 is related to the changes?

Comment 16

[Matt Brubeck (:mbrubeck)]

Re-landed on 2011-04-11:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=65740bbb1aa8

Comment 17

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Droid 2 
OS: Android 2.2

Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110506 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2