464063 - Lock panning into horizontal or vertical based on initial movement

Status: VERIFIED FIXED

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

Description

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

Panning in Fennec can be a bit jittery. Safari on iPhone will lock the panning in to horizontal or vertical only, if the initial panning movement is horz or vert, respectively. This makes panning a lot smoother and minimizes "of course" panning.

If initial panning is not predominately horizontal or vertical, then no lock is used.

Comment 1

[Madhava Enros [:madhava]]

This is especially important when zoomed in, because there are no page borders to serve as guides (when the page fits the width of the screen, slight off course pans get auto-corrected by the page snapping into place).

We could get even more constraining and say that flicking the page can only work horizontally or vertically.  Movement in other directions only works as 1:1 dragging.

Comment 2

[Ben Combee (:bcombee)]

Grabbing this to work on!

Comment 3

[Ben Combee (:bcombee)]

Attachment: change InputHandler.js to lock large moves to X or Y axis

Comment 4

[(dormant account)]

Comment on attachment 367077 [details] [diff] [review]
change InputHandler.js to lock large moves to X or Y axis


>+    if (absX > 4 || absY > 4) {

It  doesn't feel like 4 pixels is enough. I keep accidentally getting locked in the wrong direction. How about 10? This should probably be a pref. I don't know the code, but looks good to me otherwise.

Comment 5

[(dormant account)]

Comment on attachment 367077 [details] [diff] [review]
change InputHandler.js to lock large moves to X or Y axis

There seems to be  bug with this patch applied. I can't pan to the bottom of w3.org

Comment 6

[Ben Combee (:bcombee)]

I don't reproduce the problem with w3.org in my own testing.  I did see a lot of delay in getting checkerboard areas to draw, but I was able to get to the bottom of the page.

Comment 7

[Ben Combee (:bcombee)]

Hmmm... actually, I see what you're saying -- the checkerboard thing is happening for me too.  It might have been a small fix that I slipped in there -- checking

Comment 8

[Ben Combee (:bcombee)]

OK, it looks like a bad interaction with kinetic scrolling -- I can't reproduce if I force that off.  I'm digging into the code more now.

Comment 9

[Ben Combee (:bcombee)]

Attachment: Try #2 -- fixes issue with kinetic scrolling, also removed uneeded Abs() calls

Comment 10

[Ben Combee (:bcombee)]

This patch also includes the content of the patch for bug 483182, so when committing, just resolve the conflict.

Comment 11

[Ben Combee (:bcombee)]

Attachment: Try #3 -- updated to work with top of trunk

Comment 12

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

Comment on attachment 367214 [details] [diff] [review]
Try #3 -- updated to work with top of trunk

8 pixels seems a bit too small, especially on a high DPI screen.

Comment 13

[Ben Combee (:bcombee)]

I'm working on an update for the patch -- two big changes are moving the lock threshold value into a preference and changing the code to watch all movement before the timeout for the purposes of detecting lock.  I'll also update the value from 8 to 24.

Comment 14

[Ben Combee (:bcombee)]

Attachment: Try #4 - boost lock radius to 12, make it a pref that can be changed

Comment 15

[Ben Combee (:bcombee)]

I still want to change the detection code to not just use the very first move.

Comment 16

[Ben Combee (:bcombee)]

Attachment: Try #5 - remove pref, use new technique for determining locking based on the components of the original pan detection

Comment 17

[Ben Combee (:bcombee)]

You probably need the patch from bug 368347 applied for this to apply cleanly

Comment 18

[(dormant account)]

Comment on attachment 368539 [details] [diff] [review]
Try #5 - remove pref, use new technique for determining locking based on the components of the original pan detection

Need to factor out the logic instead of copying it

Comment 19

[Ben Combee (:bcombee)]

Attachment: Tre #6: same algorithm as #5, but cleaned up code in InputHandler to avoid duplication between modules

Comment 20

[(dormant account)]

Comment on attachment 368943 [details] [diff] [review]
Tre #6: same algorithm as #5, but cleaned up code in InputHandler to avoid duplication between modules

Looks ok to me now, but I don't know this code.

>   this._browserCanvas = browserCanvas;
>+  this._dragData = new DragData;

I prefer having () after constructors.

Comment 21

[Ben Combee (:bcombee)]

Attachment: Try #6.1 -- just changed new calls to have () per taras comment and fixed a couple of inits that were missing from constructors

Comment 22

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

Comment on attachment 368950 [details] [diff] [review]
Try #6.1 -- just changed new calls to have () per taras comment and fixed a couple of inits that were missing from constructors

> function InputHandler() {
>   /* used to stop everything if mouse leaves window on desktop */
>   window.addEventListener("mouseout", this, true);
>+  window.addEventListener("mousedown", this, true);
>+  window.addEventListener("mouseup", this, true);
>+  window.addEventListener("mousemove", this, true);
>+  window.addEventListener("click", this, true);
> 
>   let stack = document.getElementById("browser-container");
>   stack.addEventListener("DOMMouseScroll", this, true);
> 
>   /* these handle dragging of both chrome elements and content */

Kill the comment or move it up to the "window.addEventListener" section

>+/**
>+ * Drag Data is used by both chrome and content input modules
>+ */
>+
>+function DragData() {
>+  this.dragStartTimeout = -1;
>+  this.reset();
>+}
>+
>+DragData.prototype = {
>+  reset: function reset() {
>+    this.dragging = false;
>+    this.sX = 0;
>+    this.sY = 0;
>+    this.clearDragStartTimeout();
>+    this.targetScrollbox = null;
>+    this.lockedX = null;
>+    this.lockedY = null;
>+  },
>+
>+  clearDragStartTimeout: function clearDragStartTimeout() {
>+    if (this.dragStartTimeout != -1)
>+      clearTimeout(this.dragStartTimeout);
>+    this.dragStartTimeout = -1;
>+  }
>+};
>+

Why not put the "setTimeout" method in here too? Wrap the dragStartTimeout completely.


>+  _dragRadius: 20,

Why not in DragData?

>+  _dragStartTimeoutLength: 200, /* 1/5th of a second */

Move to DragData and pass in it's constructor?

>+  _dragData: null,
>+  _clickEvents : [],

I was thinking about moving clickEvents[] into DragData too, but I guess it's only used in ChromeInputHandler

> 
>   handleEvent: function handleEvent(aEvent) {
>     switch (aEvent.type) {
>@@ -230,29 +230,66 @@ ChromeInputModule.prototype = {
>     this._dragData.reset();
>   },
> 
>+  _setLockedAxis: function _setLockedAxis(sX, sY) {
>+    let dragData = this._dragData;
>+
>+    // look at difference from stored coord to lock movement, but only
>+    // do it if initial movement is sufficient to detect intent
>+    let absX = Math.abs(dragData.sX - sX);
>+    let absY = Math.abs(dragData.sY - sY);
>+
>+    // lock panning if we move more than half of the drag radius and that direction
>+    // contributed more than 2/3rd to the radial movement
>+    if ((absX > (this._dragRadius / 2)) && ((absX * absX) > (2 * absY * absY))) {
>+      dragData.lockedY = dragData.sY;
>+      sY = dragData.sY;
>+    }
>+    else if ((absY > (this._dragRadius / 2)) && ((absY * absY) > (2 * absX * absX))) {
>+      dragData.lockedX = dragData.sX;
>+      sX = dragData.sX;
>+    }
>+
>+    return [sX, sY];
>+  },
>+
>+  _lockMouseMove: function _lockMouseMove(sX, sY) {
>+    let dragData = this._dragData;
>+    if (dragData.lockedX !== null)
>+      sX = dragData.lockedX;
>+    else if (dragData.lockedY !== null)
>+      sY = dragData.lockedY;
>+
>+    return [sX, sY];
>+  },
>+

These two methods could be in DragData (especially if dragRadius moves in there

> 
>+function KineticData() {
>+  this.kineticHandle = -1;
>+  this.reset();
>+}
>+
>+KineticData.prototype = {
>+  reset: function reset() {
>+    if (this.kineticHandle != -1) {
>+      window.clearInterval(this.kineticHandle);
>+      this.kineticHandle = -1;
>+    }
>+
>+    this.momentumBuffer = [];
>+    this.momentumBufferIndex = 0;
>+    this.lastTime = 0;
>+    this.kineticDuration = 0;
>+    this.kineticDirX = 0;
>+    this.kineticDirY = 0;
>+    this.kineticStep  = 0;
>+    this.kineticStartX  = 0;
>+    this.kineticStartY  = 0;
>+    this.kineticInitialVel = 0;
>+  }
>+};
>+

I would drop all the ".kinetic" prefixes (this.kineticDirX -> this.dirX)

Unless you want to consider making KineticData derive from DragData

> ContentPanningModule.prototype = {
>+  _dragRadius: 20,
>+  _dragStartTimeoutLength: 200, /* 1/5th of a second */

Same "move it in" thoughts here

> 
>+  _setLockedAxis: ChromeInputModule.prototype._setLockedAxis,
>+  _lockMouseMove: ChromeInputModule.prototype._lockMouseMove,

Moving into DragData means this goes away

r- until we talk this re-org over

Comment 23

[Ben Combee (:bcombee)]

I agree with all of Mark's comments except the rename of the kineticData members... there's too much code to change for that right now that has nothing to do with lock panning.  I think that can be refactored in a separate bug that enables kinetics in chrome content if needed.  My goal for moving that structure out of the ContentPanningModule was to fix a problem where any instances of CPM would be using the same kinetic data because that object was defined on the CPM prototype.

Comment 24

[Ben Combee (:bcombee)]

Attachment: Try #7 - updated with Mark's review suggestions

Comment 25

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

Comment on attachment 369089 [details] [diff] [review]
Try #7 - updated with Mark's review suggestions

Looks better, thanks

Comment 26

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

http://hg.mozilla.org/mobile-browser/rev/6771e1018025

Comment 27

[Serge Gautherie (:sgautherie)]

Where else do you want this to be landed?

Comment 28

[Ben Combee (:bcombee)]

checkin-needed wasn't removed at commit time

Comment 29

[Aakash Desai [:aakashd]]

verified FIXED (can only pan vertically and horizontally and diagonally only on initial movement) on builds:

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.2b1pre) Gecko/20091002
Fennec/1.0b4

and

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/200910002
Fennec/1.0b4

Comment 30

[Aakash Desai [:aakashd]]

I think there's a testcase on general panning, but it needs to be separated for vertical lock, horizontal lock and diagonal panning

Comment 31

[Aakash Desai [:aakashd]]

litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11532 has been created to regression test this bug.