769391 - Panning enter horizontal axis lock mode even if the page can't actually pan or scroll horizontally

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Load a Wikipedia page
2. Try panning the page horizontally. (The page won't move horizontally because it was designed to fit in the width of the screen.)
3. WITHOUT LIFTING YOUR FINGER, try panning vertically.

AR:
The page will not scroll vertically because it is locked in horizontal axis lock mode.

ER:
The page should not have entered horizontal axis lock mode because the page doesn't actually pan horizontally.

* To be clear, I am a big fan of axis locking, but we should not enter that mode if we can't actually scroll the page along that axis.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This would be a good first bug for contributors, the relevant code is in mobile/android/base/ui/PanZoomController.java and mobile/android/base/ui/Axis.java. The axis locking code will need to be tweaked so that we don't enter axis locking on an axis that can't be scrolled.

Comment 2

[Alexander Hagerman]

Attachment: WIP patch for Top Level Documents

This is a WIP patch for bug 769391 currently working on Top Level Documents.

Comment 3

[Alexander Hagerman]

Attachment: WIP patch for Top Level Documents

WIP Patch for bug 769391 working on Top Level Documents

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Just for context: the above WIP fixes the behaviour for top-level documents, but breaks axis locking on subdocuments. I'm helping Alex work on the patch further so that it doesn't break subdocument axis-locking.

Comment 5

[Alexander Hagerman]

Attachment: Attachment edits the previous if statement to consider the level (sub, top) of a document.

The new patch looks at the level of the document (sub, top) using scrollable() while also setting the axis locks according to the getViewportLength and getPageLength comparison information.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 661332 [details] [diff] [review]
Attachment edits the previous if statement to consider the level (sub, top) of a document.

Awesome! This patch is functionally correct, and seems to handle all of the cases properly. There's just a few cosmetic things I'd like you clean up to get it ready for landing.

># User AlexHagerman <[e-mail]>
>Bug 769391 - Changed PanZoomController.java to prevent the axis lock when on a page that fits the users screen vertically or horizontally.
>This was achieved by changing the if statement on line 495 of the startPanning method to consider the users viewport in relation to the
>generated page's length, to compliment the already calculated AXIS_LOCK_ANGLE; r = kats
>

For the commit message, the usual format is something like this:
-----
Bug xxxxxx - A concise description of the bug. r=reviewer

A longer description can optionally go here, separated from the bug number and
short description by an empty line. The longer description should be word-wrapped
to about 80 characters for easier readability.
-----

So in your case the first line should look something like "Bug 769391 - Prevent axis locking when on a page that fits the user's screen vertically or horizontally. r=kats" and then in the longer description you can put more detail if you feel like it. For this particular bug I don't think there's any real need for a longer description but if you feel there's something you'd like to call out in particular you can put it in.

>diff --git a/mobile/android/base/ui/Axis.java b/mobile/android/base/ui/Axis.java
>      */
>-    private boolean scrollable() {
>+    boolean scrollable() {
>         // If we're scrolling a subdocument, ignore the viewport length restrictions (since those

Just a note: this function has changed since you started this patch, in particular bug 783553 modified it. However your patch is still correct, it will just need to be resolved manually when landing. It's not a problem at all, and I'll do it when I land the final patch but I wanted to point it out because in some cases such changes can affect the correctness of your patch, and it's good practice to update your copy of the codebase, rebase your patch on top of it, and re-test it to make sure that it's still good.

>diff --git a/mobile/android/base/ui/PanZoomController.java b/mobile/android/base/ui/PanZoomController.java
>         mY.startTouch(y);
>-        mLastEventTime = time;
>-
>-        if (angle < AXIS_LOCK_ANGLE || angle > (Math.PI - AXIS_LOCK_ANGLE)) {
>+        mLastEventTime = time;  
>+   

So here you added some extra whitespace which should be removed for the final patch. There are space characters at the end of both the "mLastEventTime = time;" line as well as the empty line following it, and all of those spaces should be removed (but keep the empty line).

>+        if ( mX.scrollable() == false || mY.scrollable() == false) {

For this line, please remove the space between the opening parenthesis and the "mX". Also to conform to our coding style, please replace the "foo == false" checks with "!foo".

>+            mX.setScrollingDisabled(false);
>+            mY.setScrollingDisabled(false);
>+            setState(PanZoomState.PANNING);

Although this code works fine, it's actually not necessary to call setScrollingDisabled(false) on the two axes here. You'll note that a few lines up, mX.startTouch and mY.startTouch are called, and those functions will set scrolling to disabled anyway, so these two lines are redundant. I'd prefer that you take them out. You'll see that the "else" clause of this if statement does the same thing - it just sets the state to PANNING and doesn't bother calling setScrollingDisabled(false) because it would be redundant.

Once you have addressed the above issues, please upload a new patch and set the "review" flag to "?" and stick my name in the reviewer field. That'll be the official review request and I'll r+ it and land it assuming everything's fixed. Thanks again for working on this bug, I realize this code isn't the most straightforward to work through but you did a great job! :)

Comment 7

[Alexander Hagerman]

Attachment: Bug 769391 - Prevent axis locking when on a page that fits the user's screen vertically or horizontally. r=kats

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 661789 [details] [diff] [review]
Bug 769391 - Prevent axis locking when on a page that fits the user's screen vertically or horizontally. r=kats

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

The setState call still needs to be included in PanZoomController, and the patch commit message needs to be updated (you can do this with "hg qref -e" while the patch is the topmost applied patch in your local queue)

Comment 9

[Alexander Hagerman]

Attachment: Inappropriate Axis Lock Correction

Bug 769391 - Prevent axis locking when on a page that fits the user's screen vertically or horizontally. r=kats

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 661792 [details] [diff] [review]
Inappropriate Axis Lock Correction

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

Perfect! I'll land this on inbound in a few minutes.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

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

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf303be47402

Comment 13

[Cristian Nicolae (:xti)]

Vertical scroll is enabled if the horizontal one is locked. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-23)
Device: Galaxy Note
OS: Android 4.0.4