Closed Bug 521108 Opened 15 years ago Closed 15 years ago

Improve pan axis locking

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
fennec1.0b5

People

(Reporter: stechz, Assigned: stechz)

References

Details

(Keywords: polish)

Attachments

(1 file, 1 obsolete file)

On devices, even pressing your finger or stylus down in one place can cause the axis to lock.  This is probably caused by some noise that occurs when pressure is put down on the screen.  Waiting for a threshold before panning and locking could help with this.  The threshold should probably be measured in millimeters instead of pixels.
tracking-fennec: --- → ?
Blocks: 504390
Priority: -- → P1
I see this issue where the url bar floats on the screen when trying to pan vertically after a **very** small horizontal jitter.
OS: Mac OS X → All
Hardware: x86 → All
Remove the initial "jump" once we've passed the click threshold.  Thanks to this, increase of tap radius to significantly help with clicking on links and double tapping.  Also makes panning a little smoother.

For locking, I've tried a new idea: allow reverting of our locking decision after we make it.  This keeps small pans easy to move around horizontally and vertically and at the same time allows us to be much more aggressive about locking for large swipes.  Now if absy is one pixel greater than absx, the x axis is locked.  The result is much less accidental sidebar peeking on swipes while not sacrificing other kinds of pans.

Jay and Madhava have tested and think they are pretty big improvements.
Assignee: nobody → webapps
Attachment #409247 - Flags: review?(21)
Comment on attachment 409247 [details] [diff] [review]
Significant improvement to locking and tap radius

>   /**
>    * Inform our dragger of a dragStart and update kinetic with new data.
>    */
>   _doDragStart: function _doDragStart(event) {
>     let dragData = this._dragData;
>-
>-    // addData must be called before setDragStart. Otherwise addData may end kinetic panning and
>-    // thus accidentally end the drag we've just begun.
>-    this._kinetic.addData(event.screenX, event.screenY);
>     dragData.setDragStart(event.screenX, event.screenY);
>-
>     this._dragger.dragStart(event.clientX, event.clientY, event.target, this._targetScrollInterface);
>   },

Since you remove this._kinetic.addData you should probably update the function comment

 
>     // If now a pan, mark previous position where panning was.
>     if (this._isPan) {
>+      let absX = Math.abs(this._originX - sX);
>+      let absY = Math.abs(this._originY - sY);
>+
>+      // After the first lock, see if locking decision should be reverted.
>+      if (this.lockedX && absX > 3 * absY)
>+        this.lockedX = null;
>+      else if (this.lockedY && absY > 3 * absX)
>+        this.lockedY = null;

I like the idea of being able to unlock but I'm not sure of the method here.
If I understand well this code, that means that we are trying to unlock mostly based on the initial starting point of the pan, which means that if the user pan quickly on Y on the first pan he can't really unlock the X axis after (and inversely).

And in contrary if he does a little pan at the start, unlocking is too easy.

Maybe it is possible to do something with prevPanX/prevPanY instead of _originX/originY here?


>+      // Check if axes should be locked.
>+      if (!this.locked) {

Not important but I think the comment can be safely remove (Ok I have to admit than I'm not a fan of comments :))

>+        // look at difference from origin coord to lock movement, but only
>+        // do it if initial movement is sufficient to detect intent
>+
>+        // divide possibilty space into eight parts.  Diagonals will allow
>+        // free movement, while moving towards a cardinal will lock that
>+        // axis.  We pick a direction if you move more than twice as far
>+        // on one axis than another, which should be an angle of about 30
>+        // degrees from the axis
>+
>+        let absX = Math.abs(this._originX - sX);
>+        let absY = Math.abs(this._originY - sY);

absX/absY have already been declared with the same value before
Attachment #409247 - Flags: review?(21) → review-
> Since you remove this._kinetic.addData you should probably update the function
comment

Good catch!  FWIW addData is removed there because axis locking now takes place at different coordinates than the beginning of the drag, so with the line kinetic scrolling could make the sidebar poke out.

> I like the idea of being able to unlock but I'm not sure of the method here.  If I understand well this code, that means that we are trying to unlock mostly
based on the initial starting point of the pan, which means that if the user
pan quickly on Y on the first pan he can't really unlock the X axis after (and
inversely).

This behavior is on purpose.  For large swipes we normally don't care about unlocking an axis.  In fact, if a vertical swipe becomes somewhat diagonal we want to make sure that it doesn't start panning in the wrong direction.  Usually a large swipe is followed by a tap release, so free movement afterwards isn't a concern.

> And in contrary if he does a little pan at the start, unlocking is too easy.

I had to make unlocking pretty easy for little pans since locking is so aggressive at first.  Before the patch, if I just wanted to touch down, wait a second, and then pan in a direction, most times the axis was already locked by fat-finger noise.

There could be room for improvement: maybe if unlocking was based on mousemove speed?  That way swipes are still aggressively locked but we don't feel constricted with small pans.

Definitely play with it on the N900: although the algorithm might sound strange, it feels much better than before in practice.  I'll address the other code review issues.
Attachment #409247 - Attachment is obsolete: true
Attachment #409712 - Flags: review?(21)
Comment on attachment 409712 [details] [diff] [review]
Code review changes


> This behavior is on purpose.  For large swipes we normally don't care about
> unlocking an axis.  In fact, if a vertical swipe becomes somewhat diagonal we
> want to make sure that it doesn't start panning in the wrong direction. 
> Usually a large swipe is followed by a tap release, so free movement afterwards
> isn't a concern.

Right, I'm still not sure to be a huge fan of that but if Madhava, Jay and you think that's ok it sounds right for me.
Attachment #409712 - Flags: review?(21) → review+
we want to test this for b5

pushed:
https://hg.mozilla.org/mobile-browser/rev/4e666c4e1399
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → B5
verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091104 Firefox/3.6b2pre Fennec/1.0b5

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091104 Firefox/3.7a1pre Fennec/1.0b5
Status: RESOLVED → VERIFIED
Component: General → Panning/Zooming
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: