Last Comment Bug 755240 - Scrolling axis lock is unbreakable
: Scrolling axis lock is unbreakable
Status: RESOLVED FIXED
[make axis lock preffable][mentor=kat...
: uiwanted
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 23
Assigned To: justin.busby
:
Mentors:
: 756002 851636 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-15 04:44 PDT by Gian-Carlo Pascutto [:gcp]
Modified: 2013-04-18 02:04 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
Add prefs pane for pan mode and implement "sticky" panning (14.07 KB, patch)
2013-04-11 08:59 PDT, justin.busby
gpascutto: feedback+
Details | Diff | Splinter Review
Addressed issues from initial review (13.27 KB, patch)
2013-04-11 10:00 PDT, justin.busby
bugmail: review-
bugmail: feedback+
Details | Diff | Splinter Review
Address issues from review and removed ux code (left strings etc in place for hidden config) (19.00 KB, patch)
2013-04-12 02:56 PDT, justin.busby
no flags Details | Diff | Splinter Review
ux code split from original patch (1.37 KB, patch)
2013-04-12 02:56 PDT, justin.busby
no flags Details | Diff | Splinter Review
Removed all ux elements and left in the prefs setting in the js. (15.47 KB, patch)
2013-04-12 05:24 PDT, justin.busby
bugmail: feedback+
Details | Diff | Splinter Review
Made changes with the exception of the PANNING_HOLD_LOCK vs PANNING (15.93 KB, patch)
2013-04-12 07:10 PDT, justin.busby
bugmail: review+
Details | Diff | Splinter Review
Changed PANNING TO PANNING_HOLD as per review. (16.08 KB, patch)
2013-04-14 06:41 PDT, justin.busby
bugmail: review+
Details | Diff | Splinter Review

Description Gian-Carlo Pascutto [:gcp] 2012-05-15 04:44:32 PDT
This is a more general regression of bug 732364.

Start scrolling in a pure vertical direction on Native Fennec. While you're going up/down, start making a more horizontal movement. Keep doing this until you get at an almost pure horizontal scroll (say 80deg angle from the vertical).

Expected result: You break out of the axis lock around very roughly 45deg.

Actual result: The axis lock is unbreakable, you will always keep having a pure vertical scroll even at extreme horizontal angles.

The same issue exists with a horizontal scroll.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-15 06:47:11 PDT
Are you sure this is a regression of bug 732364? The patches on that bug should only affect iframes/subdocuments. Is there a particular URL you are seeing this on, or does it happen on all URLs?
Comment 2 Gian-Carlo Pascutto [:gcp] 2012-05-15 06:51:50 PDT
Not sure that this is a regression on that bug, I just guessed from the description. This is on all sites.

It indeed looks like this is the counterpoint to 706891. Basically the behavior there is so rigid that it becomes counter-intuitive past a certain angle. I can see us using some hysteresis to keep the axis lock past 45 degrees, put keeping it at, say, 89 degrees, feels kinda strange.
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-15 13:44:30 PDT
(In reply to Gian-Carlo Pascutto (:gcp) from comment #0)
> Start scrolling in a pure vertical direction on Native Fennec. While you're
> going up/down, start making a more horizontal movement. Keep doing this
> until you get at an almost pure horizontal scroll (say 80deg angle from the
> vertical).

Just to make sure, this is without lifting your finger, right? If you lift your finger from the vertical pan and then start a new pan at 45 degrees or whatever it works as expected.
Comment 4 Gian-Carlo Pascutto [:gcp] 2012-05-15 13:48:00 PDT
This is without lifting a finger.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-15 14:17:53 PDT
:ibarlow, how do you feel about the current axis locking behaviour? Too much locking? The current behaviour isn't so much a regression as the intended behaviour from bug 706891, I believe.
Comment 6 Ian Barlow (:ibarlow) 2012-05-17 07:07:11 PDT
This behaviour is actually as designed, we should be locking the scroll axis until you lift your finger.

If you're scrolling up with your thumb, your thumb often draws a radius that can start vertically and end almost horizontally on larger phones. So we correct for that directional change by assuming the user still wants to be scrolling upwards.

I notice that stock browser unlocks the scrolling axis at a certain angle of scrolling, but this feels like a sub-optimal experience.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-05-17 07:23:06 PDT
*** Bug 756002 has been marked as a duplicate of this bug. ***
Comment 8 Gian-Carlo Pascutto [:gcp] 2012-05-17 08:31:32 PDT
After some more thought and some discussion with Chris Lord, I think the annoyingness of the behavior is also dependent on whether you're using a phone or a tablet.

On a phone, the scroll distances are small and you're most likely to use portrait mode. On a tablet, you can do some long scrolls and you're likely to be in landscape. So you do much more J or L-like scrolls on a tablet, and are more likely to get into situations where our current behavior feels strange.

At some point the action that is happening on the phone (err, tablet) has no relation to your actual input. I don't really like this.

Regarding the use case for thumb scrolling, can we make this dependent on the position of the input? If it's near the edge and especially near the corners, make the lock hard, if not, a bit more relaxed with some hysteresis. (Too complicated/fiddly?)
Comment 9 Ian Barlow (:ibarlow) 2012-05-17 08:33:59 PDT
> Regarding the use case for thumb scrolling, can we make this dependent on
> the position of the input? If it's near the edge and especially near the
> corners, make the lock hard, if not, a bit more relaxed with some
> hysteresis. (Too complicated/fiddly?)

Certainly worth trying!
Comment 10 Chris Peterson [:cpeterson] 2012-06-28 11:35:23 PDT
btw, I vote for *more aggressive* axis locking (like Safari on iOS).
Comment 11 David Keeler [:keeler] (use needinfo?) 2012-08-16 17:09:12 PDT
As it is now, to me the experience is a bit frustrating. I bet we could figure out how to handle the thumb scrolling case and still unlock when it's obvious that the user wants to pan the other axis without breaking contact with the screen.
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-08-16 19:40:33 PDT
You're welcome to give it a shot if you like, I can point you to the relevant code - it's reasonably well encapsulated in the mobile/android/base/ui/PanZoomController.java class.
Comment 13 Gian-Carlo Pascutto [:gcp] 2013-01-16 04:42:06 PST
>btw, I vote for *more aggressive* axis locking (like Safari on iOS).

What's the use case where this is needed?

Feedback on this issue from a user:
"I've got some other panning annoyances too. For some reason, it seems to lock onto an axis when panning. It happens CONSTANTLY that I want to go diagonally and it locks down hard on left/right scrolling. This browser annoys me greatly by such subtle stuff. I know what action I want to perform, I perform it and the browser goes "nu-uh, you didn't satisfy $predicate, fuck you, you're going left"

>If you're scrolling up with your thumb, your thumb often draws a radius that can start vertically and end 
>almost horizontally on larger phones

Aside from the other ideas here, this should be fixable by noticing for how long the diagonal pan is going on. If it's a reasonable distance, at an angle that's not at all related to the axis in which we're locked, I can't see much reason to maintain the axis lock.

I know this is finetuning, but panning is basic stuff and important to get right. If you tried to use the FirefoxOS browser that doesn't have an axis lock right now, you'll understand :)
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2013-01-16 07:34:18 PST
(In reply to Gian-Carlo Pascutto (:gcp) from comment #13)
> Aside from the other ideas here, this should be fixable by noticing for how
> long the diagonal pan is going on. If it's a reasonable distance, at an
> angle that's not at all related to the axis in which we're locked, I can't
> see much reason to maintain the axis lock.

Note that bug 706891 intentionally made the axis lock unbreakable although there's not whole lot of rationale there other than "it feels better". This seems like one of those things that everybody is going to disagree on because it's a personal preference.
Comment 15 Gian-Carlo Pascutto [:gcp] 2013-01-16 07:36:54 PST
>This seems like one of those things that everybody is going to disagree on because it's a personal preference.

Then, specifically given that people complain about this, maybe there should be a setting. But I'd prefer to see if any tweaking can make our default behavior better.
Comment 16 Kevin Brosnan [:kbrosnan] 2013-03-15 14:53:11 PDT
*** Bug 851636 has been marked as a duplicate of this bug. ***
Comment 17 justin.busby 2013-04-09 15:49:15 PDT
Hey, i'd like to take this on with the initial intention of adding some hysteresis and possibly adding options in the settings pane to control behaviour.
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-09 16:28:29 PDT
Great! If you haven't already, you should get a development environment up and running using the steps at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

Once you have that set up, the relevant code can be found in mobile/android/base/gfx/JavaPanZoomController.java and Axis.java; look for the AXIS_LOCK_ANGLE code in PanZoomController.java and go from there. Feel free to ask here or in IRC (irc.mozilla.org, #mobile) if you have any questions.
Comment 19 justin.busby 2013-04-09 16:32:10 PDT
Perfect, thanks. Got the code up and building today. Thanks for the pointers and I'll come and lurk in irc later.
Comment 20 justin.busby 2013-04-11 08:59:22 PDT
Created attachment 736322 [details] [diff] [review]
Add prefs pane for pan mode and implement "sticky" panning

Allow users to select their panning mode. Standard provides the existing axis-locking behavior. Free provides no axis-locking. Sticky provides the ability to break out of the axis locking.
Comment 21 Gian-Carlo Pascutto [:gcp] 2013-04-11 09:02:03 PDT
Comment on attachment 736322 [details] [diff] [review]
Add prefs pane for pan mode and implement "sticky" panning

Should set review to ? indicated you want someone to review it for you. Should indicate the module owner as the reviewer (though in this case I'll throw it to kats).
Comment 22 justin.busby 2013-04-11 09:03:17 PDT
Thanks for that - first time for me.
Comment 23 Gian-Carlo Pascutto [:gcp] 2013-04-11 09:10:29 PDT
Comment on attachment 736322 [details] [diff] [review]
Add prefs pane for pan mode and implement "sticky" panning

Review of attachment 736322 [details] [diff] [review]:
-----------------------------------------------------------------

Going to do a drive-by review while I'm here. Patch seems neat to me. Check your editor settings to show spurious white-space, it'll help keep the patches clean.

Let's see if kats has any more remarks.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +100,5 @@
>  
> +    private enum PanZoomMode {
> +        STANDARD,       /* Default axis locking mode that doesn't break out until finger release */
> +        FREE,           /* No locking at all */
> +        STICKY		/* Break out with hysterisys so that it feels as free as possible whilst locking */

hysteresis

@@ +147,5 @@
> +        mMode = PanZoomMode.STANDARD;
> +
> +        PrefsHelper.getPref("app.screen.panBehavior", new PrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, String value) {
> +                Log.e(LOGTAG, "Setting pan state to " + value);

This isn't an error, so Log.e seems inappropriate.

@@ +154,5 @@
> +                } else if(value.equals("free")) {
> +                    mMode = PanZoomMode.FREE;
> +                } else {
> +                    mMode = PanZoomMode.STICKY;
> +                } 

nit: spurious whitespace

@@ +636,5 @@
>              // up our velocity
>              return;
>          }
>          mLastEventTime = time;
> +        

nit: spurious whitespace

@@ +670,5 @@
>          mX.saveTouchPos();
>          mY.saveTouchPos();
>  
> +        for (int i = 0; i < event.getHistorySize(); i++) { 
> +            track(event.getHistoricalX(0, i), event.getHistoricalY(0, i), event.getHistoricalEventTime(i));

The indenting here got screwed up, and there's spurious whitespace.
Comment 24 justin.busby 2013-04-11 10:00:38 PDT
Created attachment 736357 [details] [diff] [review]
Addressed issues from initial review
Comment 25 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-11 12:41:02 PDT
Comment on attachment 736357 [details] [diff] [review]
Addressed issues from initial review

Review of attachment 736357 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome job on this patch, it looks really good! I have some comments below that should be relatively quick to fix.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +62,5 @@
>      // Angle from axis within which we stay axis-locked
>      private static final double AXIS_LOCK_ANGLE = Math.PI / 6.0; // 30 degrees
>  
> +    // Axis-lock breakout angle
> +    private static final double AXIS_BREAKOUT_ANGLE = Math.PI / 8;

I haven't tried using this patch, but this angle seems small to me. In my mind you'd only want to break out of e.g. a y-axis lock when you've moved past 30 or 40 degrees from the vertical axis. However if this feels fine to you then I'm not opposed to landing the patch with this value and then adjusting it later with more user feedback.

@@ +97,5 @@
>                          to the FLING state except that it must be stopped manually by the code that
>                          started it, and it's velocity can be updated while it's running. */
>      }
>  
> +    private enum PanZoomMode {

Call this AxisLockMode rather than PanZoomMode, as that is more indicative of what it represents. In fact throughout the patch (comments, variable names, etc.) try to refer to it as "axis lock mode" rather than "pan mode"

@@ +141,5 @@
>          registerEventListener(MESSAGE_ZOOM_RECT);
>          registerEventListener(MESSAGE_ZOOM_PAGE);
>          registerEventListener(MESSAGE_TOUCH_LISTENER);
>  
> +        PrefsHelper.setPref("app.screen.panBehavior", "standard");

Instead of setting the pref here (which will overwrite whatever the user has selected), you should set the default initial value in the file at mobile/android/app/mobile.js. The other pan-zoom related prefs all start with "ui.scrolling", so I would suggest calling this "ui.scrolling.axis_lock_mode" or something similar.

@@ +147,5 @@
> +        mMode = PanZoomMode.STANDARD;
> +
> +        PrefsHelper.getPref("app.screen.panBehavior", new PrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, String value) {
> +                if(value.equals("standard")) {

nit: Please insert a space between the "if" and the "(" here and throughout the rest of the patch as well.

@@ +643,5 @@
> +            float dy = mY.panDistance(y);
> +            double angle = Math.atan2(dy, dx); // range [-pi, pi]
> +            angle = Math.abs(angle); // range [0, pi]
> +
> +            if(Math.abs(dx)>10 || Math.abs(dy)>10) {

Please pull out the magic number "10" to a constant at the top of the file. You'll probably also want to make this number dependent on the device dpi so that it's not a millimeter on some devices and an inch on others. See PAN_THRESHOLD for an example of how to do this.

@@ +645,5 @@
> +            angle = Math.abs(angle); // range [0, pi]
> +
> +            if(Math.abs(dx)>10 || Math.abs(dy)>10) {
> +                if (angle > AXIS_BREAKOUT_ANGLE && angle < (Math.PI - AXIS_BREAKOUT_ANGLE)) {
> +                    if(!mY.scrollable()) {

I'd like to avoid using mY.scrollable() as a check here, because that can return false in cases other than axis lock (for example, if the document is too short to allow scrolling). I would rather structure the code more like so:

if (mState == PanZoomState.PANNING_LOCKED) {
    if (/* check angle for breaking y-axis scrolling */) {
        mY.setScrollingDisabled(false);
        setState(PanZoomState.PANNING);
    } else if (/* check angle for breaking x-axis scrolling */) {
        /* same as above but for mX */
    }
}

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +124,5 @@
>  
> +<!ENTITY pref_pan_behavior "Pan Behavior">
> +<!ENTITY pref_pan_behavior_standard "Standard">
> +<!ENTITY pref_pan_behavior_free "Free">
> +<!ENTITY pref_pan_behavior_sticky "Sticky">

We'll need to get UX approval for these strings and whether or not this should even be visible in the settings. Note that even if this is not visible in the settings, people can still change the pref value by going to about:config. For now I would suggest splitting the settings-pane part of the code into a separate patch and we can get UX feedback on it.
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-11 12:43:59 PDT
Ian, should axis locking behaviour be exposed via the settings? The patch above allows three axis locking modes: "standard", which is what we have now, "free", which never goes into axis lock, and "sticky", which allows axis locking but then also allows you to break out of the axis lock if you pan at a large enough angle. If this should be exposed in the settings, please let us know what strings we should use for the different options. If not, users will be able to set the pref via about:config.
Comment 27 justin.busby 2013-04-12 02:56:21 PDT
Created attachment 736732 [details] [diff] [review]
Address issues from review and removed ux code (left strings etc in place for hidden config)
Comment 28 justin.busby 2013-04-12 02:56:53 PDT
Created attachment 736733 [details] [diff] [review]
ux code split from original patch
Comment 29 justin.busby 2013-04-12 03:05:48 PDT
Note that I had to add some extra constants so that I can identify between different locked axis. Which is why this took a while longer than I expected to fix as I had introduced a fallthrough bug in a case statement and couldn't spot it last night.

I've left the angle as it is at the moment for breakout as it's very subjective and I would be interested to get feedback. Personally I feel like the axis locking should be as unobtrusive as possible. I'd like to be almost unaware that I have to break out of it whilst still doing the job of locking me into a column of text.

Also I only split out the preferences pane code into a separate patch. Is this the correct thing here or do you think I should spit out all the strings etc. too? Thinking about this I'm presuming that the ux guys would want all that separated?

Let me know either way and I'll get stuff fixed today.
Comment 30 Ian Barlow (:ibarlow) 2013-04-12 04:48:39 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26)
> Ian, should axis locking behaviour be exposed via the settings? The patch
> above allows three axis locking modes: "standard", which is what we have
> now, "free", which never goes into axis lock, and "sticky", which allows
> axis locking but then also allows you to break out of the axis lock if you
> pan at a large enough angle. If this should be exposed in the settings,
> please let us know what strings we should use for the different options. If
> not, users will be able to set the pref via about:config.

I don't want this in Settings -- this is an advanced feature that would likely confuse our less savvy users. I would suggest moving it to about:config.
Comment 31 justin.busby 2013-04-12 05:22:18 PDT
Cool - will update the patch to remove all ux stuff
Comment 32 justin.busby 2013-04-12 05:24:14 PDT
Created attachment 736768 [details] [diff] [review]
Removed all ux elements and left in the prefs setting in the js.

users will have to adjust this setting via about:config
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-12 06:05:17 PDT
Comment on attachment 736768 [details] [diff] [review]
Removed all ux elements and left in the prefs setting in the js.

Review of attachment 736768 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for removing the strings and settings stuff. The patch got a lot more complicated with the addition of the new states - I didn't realize that would be necessary in my previous comments but I see that they are :(. Still, good work on the patch! Just a couple of relatively small things to fix up I noted below and then we can land it.

Also, for the final patch, please include a commit message and your author information. See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in for some instructions on how to do that with Mercurial. Please format the commit message something like "Bug 755240 - Allow Fennec axis locking behaviour to be controlled by a pref.". The person landing the patch (probably me) will fill in the reviewer info.

::: mobile/android/app/mobile.js
@@ +508,5 @@
>  // used by update service to decide whether or not to
>  // automatically download an update
>  pref("app.update.autodownload", "wifi");
>  
> +pref("app.screen.panBehavior", "standard");

Please rename this pref to ui.scrolling.axis_lock_mode and move the line to be near the other ui.scrolling.* prefs.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +460,5 @@
>  
> +        case PANNING_HOLD_LOCKED_X:
> +            setState(PanZoomState.PANNING_LOCKED_X);
> +            track(event);
> +            break;

Change this break to a "return true". As-is it will return false.

@@ +620,5 @@
> +            if (!mX.scrollable() || !mY.scrollable()) {
> +                setState(PanZoomState.PANNING);
> +            } else if (angle < AXIS_LOCK_ANGLE || angle > (Math.PI - AXIS_LOCK_ANGLE)) {
> +                mY.setScrollingDisabled(true);
> +                setState(PanZoomState.PANNING_LOCKED_Y);

PANNING_LOCKED_Y (based on the comments above, and what I would intuitively expect) should be the state where panning is locked to the y-axis. This if condition is actually locking panning to the x-axis, because it is disabling scrolling along the y-axis. So I think this setState call should be setting PANNING_LOCKED_X as the state, and the one below this should set PANNING_LOCKED_Y. The code that checks these states will also need to be flipped.

@@ +696,4 @@
>              } else {
>                  // should never happen, but handle anyway for robustness
>                  Log.e(LOGTAG, "Impossible case " + mState + " when stopped in track");
> +                setState(PanZoomState.PANNING);

Not sure why you changed this setState call from PANNING_HOLD_LOCKED to PANNING. I think this code should still never get hit, so if you ran into a case where it is getting hit please let me know. Otherwise I think it still makes sense to leave it as PANNING_HOLD_LOCKED.
Comment 34 justin.busby 2013-04-12 06:34:44 PDT
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> Comment on attachment 736768 [details] [diff] [review]
> Removed all ux elements and left in the prefs setting in the js.
> 
> Review of attachment 736768 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +696,4 @@
> >              } else {
> >                  // should never happen, but handle anyway for robustness
> >                  Log.e(LOGTAG, "Impossible case " + mState + " when stopped in track");
> > +                setState(PanZoomState.PANNING);
> 
> Not sure why you changed this setState call from PANNING_HOLD_LOCKED to
> PANNING. I think this code should still never get hit, so if you ran into a
> case where it is getting hit please let me know. Otherwise I think it still
> makes sense to leave it as PANNING_HOLD_LOCKED.

Due to the previous change around using PANNING_LOCKED instead of the scrollable check I'd had to modify PANNING_LOCKED to include X and Y versions of the constant. PANNING_HOLD_LOCKED was dependant on PANNING_LOCKED in method track(MotionEvent event) which had to change to include X and Y and consequently updated the PANNING_HOLD_LOCKED constants to mirror this.

This means that the state of PANNING_HOLD_LOCKED without an axis no longer exists and I'd have had to select an axis for this error state, which doesn't make sense so I selected PANNING as the next most sensible error state.

These lines in track(MotionEvent event)

} else if (mState == PanZoomState.PANNING_LOCKED_X) {
  setState(PanZoomState.PANNING_HOLD_LOCKED_X);
} else if (mState == PanZoomState.PANNING_LOCKED_Y) {
  setState(PanZoomState.PANNING_HOLD_LOCKED_Y);

could be modified to set the original constant PANNING_HOLD_LOCKED without an axis and thus revert line 696 onward but I'm not sure it makes sense without an axis due to the dependency here.
Comment 35 u452416 2013-04-12 06:46:06 PDT
A video with the problem can be viewed on: vimeo.com/61922141
Comment 36 justin.busby 2013-04-12 07:10:38 PDT
Created attachment 736795 [details] [diff] [review]
Made changes with the exception of the PANNING_HOLD_LOCK vs PANNING

Final (possibly) patch generated with hg queues to include checkin comment and author info
Comment 37 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-12 07:41:42 PDT
Comment on attachment 736795 [details] [diff] [review]
Made changes with the exception of the PANNING_HOLD_LOCK vs PANNING

(In reply to justin.busby from comment #34)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33)
> > > +                setState(PanZoomState.PANNING);
> > 
> > Not sure why you changed this setState call from PANNING_HOLD_LOCKED to
> > PANNING. I think this code should still never get hit, so if you ran into a
> > case where it is getting hit please let me know. Otherwise I think it still
> > makes sense to leave it as PANNING_HOLD_LOCKED.
> 
> This means that the state of PANNING_HOLD_LOCKED without an axis no longer
> exists and I'd have had to select an axis for this error state, which
> doesn't make sense so I selected PANNING as the next most sensible error
> state.

Ah, yes - I don't know what I was thinking when I suggested using a state that doesn't exist any more :p. That being said I think PANNING_HOLD would be more appropriate here as the fallback, because we do know at this point that isStopped() is true and so the user is in a "hold" state. But it doesn't matter much as that code should never actually get exercised anyway. I'm fine with this landing as-is, or if you don't have any objections I can change it to PANNING_HOLD when I land the patch.

Also I'm not sure exactly how you exported the patch but usually when it's exported from mercurial the author info shows up in a line like so:

# User first last <email@host>

I can still use the info in the patch to land it but for future reference and/or bugs you work on it would be good to sort that out.
Comment 38 justin.busby 2013-04-14 06:40:06 PDT
Sorry - have a habit of over-explaining things :)

Also I didn't look at the context around the code for PANNING_HOLD_LOCKED so you are of course entirely right that PANNING_HOLD is more appropriate so I've made the fix.

I had originally used hg qnew to generate the patch and attached it from the patches directory which seems to have resulted in the non-standard comments. hq export qtip seems to have resulted in the expected headings I think.
Comment 39 justin.busby 2013-04-14 06:41:59 PDT
Created attachment 737255 [details] [diff] [review]
Changed PANNING TO PANNING_HOLD as per review.

Also fixed the export method of the patch to get the correct headers generated.
Comment 40 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-14 20:20:21 PDT
Comment on attachment 737255 [details] [diff] [review]
Changed PANNING TO PANNING_HOLD as per review.

Review of attachment 737255 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2013-04-14 20:23:06 PDT
I landed the patch on inbound; this bug will be marked fixed once it gets merged to mozilla-central by the sheriffs, and should be available in nightlies after that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb82c0a48e5

You did a great job on this bug; if you have time you should definitely try grabbing another bug that you find interesting :)
Comment 42 Ryan VanderMeulen [:RyanVM] 2013-04-15 09:17:12 PDT
https://hg.mozilla.org/mozilla-central/rev/dbb82c0a48e5

Note You need to log in before you can comment on or make changes to this bug.