653990 - preventDefault on touchmove does not completely prevent panning

Status: VERIFIED FIXED

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

Description

[Matt Brubeck (:mbrubeck)]

Steps to reproduce (on Fennec nightly):
1. Open http://limpet.net/w3/touchevents/preventDefault.html
2. Hide the urlbar and sidebars.
3. Start a vertical drag gesture in the box that says "prevent move"

Expected results: The page does not pan.

Actual results: The page pans slightly, then stops.

This happens because the first pan is applied before the touchmove event is processed.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Wait until the first touchmove event is processed before re-enabling panning.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: misc bustage fixes (checked in)

This fixes two errors I found in related code:

* Missed one s/contentCanCaptureMouse/contentMightCaptureMouse/ which means that contentMightCaptureMouse is always set to "undefined" in the parent process. (Regression from bug 650339.)

* "json is not defined" exception in TouchEventHandler.receiveMessage in the case where mayHaveTouchEventListener is false, which means that the Browser:CaptureEvents message is not actually sent in this case.  (Error introduced in bug 653899.)

There's also an s/ContentTouchHandler/this/ which is not an error, just code cleanup.

Comment 3

[Wesley Johnston (:wesj)]

I'm still looking at the first patch. The patch looks fine, but I feel like we currently have to many similar variables here. contentMouseCapture, contentMightCaptureMouse, canCancelPan, and now preventPanning.

I'm going to play with combining them as much as possible into one multi-state variable.

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 529351 [details] [diff] [review]
misc bustage fixes (checked in)

http://hg.mozilla.org/mozilla-central/rev/081668165af3

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Updated to apply against master, and fixed a bug that broke the "touchend" event.

(In reply to comment #3)
> I'm still looking at the first patch. The patch looks fine, but I feel like we
> currently have to many similar variables here. contentMouseCapture,
> contentMightCaptureMouse, canCancelPan, and now preventPanning.
> 
> I'm going to play with combining them as much as possible into one multi-state
> variable.

Yeah, it seems like we might be better off explicitly modelling this as a state machine.  We might want to do that as a followup bug, though -- right now, this patch has a noticeable impact on pages like http://www.quirksmode.org/m/tests/drag.html where dragging the element left or right causes the toolbars to move onscreen.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #3)
> I'm still looking at the first patch. The patch looks fine, but I feel like we
> currently have to many similar variables here. contentMouseCapture,
> contentMightCaptureMouse, canCancelPan, and now preventPanning.
> 
> I'm going to play with combining them as much as possible into one multi-state
> variable.

I definitely agree that we have too many state variables. That's expected, imo, when building a new feature. I also agree with Matt that we can file a bug for followup work.

Now that touch-events are stablizing, we should look for any cleanup and refactoring work.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 529517 [details] [diff] [review]
patch v2

Sounds like a plan.

Comment 8

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/67bd71d5eae6

Comment 9

[Andreea Pod]

Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110504
Firefox/6.0a1 Fennec/6.0a1 
Device: LG Optimus 2X