Closed Bug 464063 Opened 12 years ago Closed 12 years ago

Lock panning into horizontal or vertical based on initial movement

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(fennec1.0b2+)

VERIFIED FIXED
Tracking Status
fennec 1.0b2+ ---

People

(Reporter: mfinkle, Assigned: bcombee)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Blocks: 477628
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0b2+
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.
Grabbing this to work on!
Assignee: nobody → combee
Status: NEW → ASSIGNED
Attachment #367077 - Flags: review?(tglek)
Attachment #367077 - Flags: superreview?(pavlov)
Attachment #367077 - Flags: review?(tglek)
Attachment #367077 - Flags: review+
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 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
Attachment #367077 - Flags: superreview?(pavlov) → superreview-
Attachment #367077 - Flags: review+ → review-
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.
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
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.
Attachment #367077 - Attachment is obsolete: true
Attachment #367213 - Flags: superreview?(pavlov)
Attachment #367213 - Flags: review?(tglek)
This patch also includes the content of the patch for bug 483182, so when committing, just resolve the conflict.
Attachment #367213 - Attachment is obsolete: true
Attachment #367213 - Flags: superreview?(pavlov)
Attachment #367213 - Flags: review?(tglek)
Attachment #367214 - Flags: review?(tglek)
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.
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.
Attachment #367214 - Attachment is obsolete: true
Attachment #367214 - Flags: review?(tglek)
I still want to change the detection code to not just use the very first move.
You probably need the patch from bug 368347 applied for this to apply cleanly
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
Attachment #368539 - Flags: review?(tglek) → review-
Attachment #368943 - Flags: review?(tglek)
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.
Attachment #368943 - Flags: review?(tglek) → review+
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
Attachment #368950 - Flags: review?(mark.finkle) → review-
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.
Attachment #368950 - Attachment is obsolete: true
Attachment #369089 - Flags: review?(mark.finkle)
Comment on attachment 369089 [details] [diff] [review]
Try #7 - updated with Mark's review suggestions

Looks better, thanks
Attachment #369089 - Flags: review?(mark.finkle) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mobile-browser/rev/6771e1018025
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Where else do you want this to be landed?
checkin-needed wasn't removed at commit time
Keywords: checkin-needed
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
Status: RESOLVED → VERIFIED
I think there's a testcase on general panning, but it needs to be separated for vertical lock, horizontal lock and diagonal panning
Flags: in-litmus?
Component: General → Panning/Zooming
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=11532 has been created to regression test this bug.
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.