706891 - Investigate making the axis lock unbreakable in some circumstances

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Patrick Walton (:pcwalton)]

mbrubeck tells me that in XUL Fennec we made the axis lock unbreakable after panning more than 200px or so. We should perhaps investigate doing something like this; I tried it and liked the result.

Comment 1

[Martijn Wargers (dead)]

What do you mean with axis lock?

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-1-rename-Axis-Locking.patch

Bug 706891 part 1 - Rename Axis setLocked/mLocked to disableScrolling/mScrollingDisabled to clarify meaning.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 594929 [details] [diff] [review]
bug-706891-part-1-rename-Axis-Locking.patch

Bug 706891 part 1 - Rename Axis setLocked/mLocked to disableScrolling/mScrollingDisabled to clarify meaning.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-2-reduce-pan-threshold.patch

Bug 706891 part 2 - Reduce PAN_THRESHOLD from 1/10" to 1/16" and change units to dps.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-3-check-axis-lock.patch

Bug 706891 part 3 - When drag breaks through pan threshold, reposition touch origin and check for axis locking.

Comment 6

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

Nice work. Just a couple of comments:

1) I'd prefer renaming disableScrolling to setScrollingDisabled because it takes a boolean and disableScrolling(false) doesn't make much sense when you read it.

2) According to http://knol.google.com/k/suppresswarnings-annotation-in-java# Eclipse uses the fallthrough annotation for different things than javac, so those annotations might still be needed. I don't use eclipse so I can't say for sure but unless you've tried it I would err on the safe side and leave them in.

Also I have some WIP that adds a regression test for axis locking. I don't have a bug for it yet and it shouldn't be affected by this change, but just a heads-up.

Comment 7

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

I'll r+ these patches if you address the things I noted in comment #6.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-1-rename-Axis-Locking-v2.patch

* Renamed setLock/disableScrolling to setScrollDisabled.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-3-check-axis-lock-v2.patch

* Renamed setLock/disableScrolling to setScrollDisabled.

* I think it is safe to remove the @SuppressWarnings("fallthrough") annotations
because I added those fallthrough annotations when I was fixing Java warnings a
few weeks ago. So presumably Eclipse users had not been complaining before
then. :) Also, those annotations are no longer necessary because our
Makefile.in now suppresses fallthrough warnings.

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-1-rename-Axis-Locking-v2.patch

Adding r=kats to patch.

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 597095 [details] [diff] [review]
bug-706891-part-1-rename-Axis-Locking-v2.patch

Carry forward r=kats flag.

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-2-reduce-pan-threshold.patch

part-2 patch needs review.

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-2-reduce-pan-threshold.patch

part-2 patch needs review.

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-3-check-axis-lock-v2.patch

Adding r=kats to patch.

Carry forward r=kats flag.

Comment 15

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

Comment on attachment 597100 [details] [diff] [review]
bug-706891-part-2-reduce-pan-threshold.patch

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

r+ with the comment fixed

::: mobile/android/base/ui/PanZoomController.java
@@ +82,3 @@
>      // The distance the user has to pan before we recognize it as such (e.g. to avoid
> +    // 1-pixel pans between the touch-down and touch-up of a click). In units of dps.
> +    public static final float PAN_THRESHOLD = 1/16f * GeckoAppShell.getDpi();

I assume by "dps" you mean "device pixels"? It's a little ambiguous since the code says Dpi which means "dots per inch" so in that context "dp" just means "dots per". Long story short, the comment should be updated to say "pixels" or "device pixels" or something.

Comment 16

[Chris Peterson [:cpeterson]]

Attachment: bug-706891-part-2-reduce-pan-threshold-v2.patch

Change "dps" to "density-independent pixels" in comment.

Carry forward r=kats flag.

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8119075352ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/442ad20cca01
https://hg.mozilla.org/integration/mozilla-inbound/rev/21783313b2e1

Comment 18

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

Also landed https://hg.mozilla.org/integration/mozilla-inbound/rev/a9393768863d to disable the failing testFlingCorrectness

Comment 19

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

I disabled testFlingCorrectness for now because I think the patches are fine and the behaviour change they introduce is correct, but the test should be updated to match the new behaviour and re-enabled as soon as possible.

The specific behaviour change I think is causing the test failure is the fact that now when we break out of the panning threshold and starting panning the page, we reset the "start position" of the pan. This means for a 100-pixel motion event, we will only move the page by (100 - pan threshold) pixels. The test is expecting the full 100 pixels and so fails.

However IIRC this behaviour should also cause testPanCorrectness to fail and off the top of my head I can't see why it isn't failing. Something to investigate...

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/8119075352ef
https://hg.mozilla.org/mozilla-central/rev/442ad20cca01
https://hg.mozilla.org/mozilla-central/rev/21783313b2e1

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 22

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

Wow I must be blind. I only just noticed this changes mLocked from private to public along with the rename.

Comment 23

[Chris Peterson [:cpeterson]]

kats, oops. That is a mistake. I will fix this when I investigate the `testFlingCorrectness` test breakage.

Comment 24

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

Awesome, thanks.

Comment 25

[Doug Turner (:dougt)]

this bug should be marked fixed, right?

Comment 26

[Chris Peterson [:cpeterson]]

This bug has not been closed because it injected a Robocop test failure. That test has been disabled for now. Should we just open a new bug to track test failure separately? Then we can close this feature bug.

Comment 27

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

Marking this bug fixed; bug 727351 is open to track the test failure.