Closed Bug 706891 Opened 9 years ago Closed 9 years ago

Investigate making the axis lock unbreakable in some circumstances

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 13
Tracking Status
blocking-fennec1.0 --- +
fennec 11+ ---

People

(Reporter: pcwalton, Assigned: cpeterson)

References

Details

(Keywords: uiwanted)

Attachments

(3 files, 7 obsolete files)

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.
What do you mean with axis lock?
Assignee: nobody → chrislord.net
Priority: -- → P2
tracking-fennec: --- → 11+
Assignee: chrislord.net → cpeterson
Bug 706891 part 1 - Rename Axis setLocked/mLocked to disableScrolling/mScrollingDisabled to clarify meaning.
Attachment #594929 - Flags: review?
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.
Attachment #594929 - Flags: review? → review?(doug.turner)
Bug 706891 part 2 - Reduce PAN_THRESHOLD from 1/10" to 1/16" and change units to dps.
Attachment #594930 - Flags: review?(doug.turner)
Bug 706891 part 3 - When drag breaks through pan threshold, reposition touch origin and check for axis locking.
Attachment #594931 - Flags: review?(doug.turner)
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.
Attachment #594929 - Flags: review?(doug.turner) → review?(bugmail.mozilla)
Attachment #594930 - Flags: review?(doug.turner) → review?(bugmail.mozilla)
Attachment #594931 - Flags: review?(doug.turner) → review?(bugmail.mozilla)
I'll r+ these patches if you address the things I noted in comment #6.
* Renamed setLock/disableScrolling to setScrollDisabled.
Attachment #594929 - Attachment is obsolete: true
Attachment #594929 - Flags: review?(bugmail.mozilla)
Attachment #596813 - Flags: review?(bugmail.mozilla)
* 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.
Attachment #594931 - Attachment is obsolete: true
Attachment #594931 - Flags: review?(bugmail.mozilla)
Attachment #596815 - Flags: review?(bugmail.mozilla)
Attachment #596813 - Flags: review?(bugmail.mozilla) → review+
Attachment #596815 - Flags: review?(bugmail.mozilla) → review+
Adding r=kats to patch.
Attachment #596813 - Attachment is obsolete: true
Comment on attachment 597095 [details] [diff] [review]
bug-706891-part-1-rename-Axis-Locking-v2.patch

Carry forward r=kats flag.
Attachment #597095 - Flags: review+
part-2 patch needs review.
Attachment #597098 - Flags: review?(bugmail.mozilla)
part-2 patch needs review.
Attachment #594930 - Attachment is obsolete: true
Attachment #597098 - Attachment is obsolete: true
Attachment #594930 - Flags: review?(bugmail.mozilla)
Attachment #597098 - Flags: review?(bugmail.mozilla)
Attachment #597100 - Flags: review?(bugmail.mozilla)
Adding r=kats to patch.

Carry forward r=kats flag.
Attachment #596815 - Attachment is obsolete: true
Attachment #597102 - Flags: review+
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.
Attachment #597100 - Flags: review?(bugmail.mozilla) → review+
Change "dps" to "density-independent pixels" in comment.

Carry forward r=kats flag.
Attachment #597100 - Attachment is obsolete: true
Attachment #597157 - Flags: review+
Keywords: checkin-needed
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...
Whiteboard: [inbound], [leave open after inbound merge]
Wow I must be blind. I only just noticed this changes mLocked from private to public along with the rename.
kats, oops. That is a mistake. I will fix this when I investigate the `testFlingCorrectness` test breakage.
Awesome, thanks.
this bug should be marked fixed, right?
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.
Blocks: 727351
Marking this bug fixed; bug 727351 is open to track the test failure.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-fennec1.0: --- → +
You need to log in before you can comment on or make changes to this bug.