544614 - Implement touch events

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

iPhone and Android support variations of touch events: touchstart, touchmove and touchend. They events basically mimic mousedown, mousemove and mouseup  - respectively. There is also a touchcancel

iPhone adds properties to the event object, but I don't think Android does. For more info, see:

compatibility
http://www.quirksmode.org/blog/archives/2010/02/the_touch_actio.html

demo
http://www.quirksmode.org/m/tests/drag.html

There are differences in the iPhone and Android approaches. We need some research to determine which approach is reasonable for Fennec.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We may not want to implement iPhone touch events (they have some problems and
aren't in anyway standard.)

WebApps WG will hopefully have a draft spec for touch and gestures
reasonable soon.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And I'd use something else than quirksmode as a source for event handling
related information :/

Comment 3

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

(In reply to comment #2)
> And I'd use something else than quirksmode as a source for event handling
> related information :/

Indeed. I am trying to find some official information on Android touch events. I should also dig-up some draft specs from WebApps WG too.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm not sure if this is feasible for the 4.0 timeframe, but I'll nom anyway.  Another testcase where the android browser's (didn't try mobilesafari) touch support works quite well is
 http://10k.aneventapart.com/Uploads/83/

What are the design aspects of this feature?  I can think of
 - security: have to leave URL bar dropped down or something (not sure here), without taking up too much screen real estate
 - perf: should only give content a shot at processing events if it's going to do something useful with them, since the round-trip from chrome->content->chrome can be arbitrarily long.  I'm not sure of the best way to do this; I guess we could wait for an addEventListener("touch-thing") before attempting forwarding (?).  A manifest-y <meta>-y type declaration seems better to me but I don't know a clean way to do that.
 - abuse: the gmail served to mobilesafari uses touch events to implement its own iframe panning, to "work around" lack of single-finger panning of subframes.  This is noticeably suckier than the built-in scrolling.  We'll have our own fast solution for this so I would consider gmail's use here to be bad.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> What are the design aspects of this feature?  I can think of
>  - security: have to leave URL bar dropped down or something (not sure here),
> without taking up too much screen real estate
I don't understand this.

>  - perf: should only give content a shot at processing events if it's going to
> do something useful with them, since the round-trip from
> chrome->content->chrome can be arbitrarily long.
Why you it be chrome->content->chrome? Why not just chrome->content?

>  I'm not sure of the best way
> to do this; I guess we could wait for an addEventListener("touch-thing") before
> attempting forwarding (?).
Yeah, we do that for many cases anyway, like mutation events,
MozAfterPaint etc.


About standardization; it is expected that Web Events WG 
http://www.w3.org/2010/webevents/charter/ will come up with a spec
for touch events.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> (In reply to comment #4)
> > What are the design aspects of this feature?  I can think of
> >  - security: have to leave URL bar dropped down or something (not sure here),
> > without taking up too much screen real estate
> I don't understand this.
> 

If web content can eat touch events, then the user can't know whether the frontend or web content is moving the pixels on screen.  Malicious sites could then exploit that ambiguity to scroll mock urlbar/sidebars onto screen that look like the frontend's and use those to consume "trusted" user input.  Web content could also quasi-DoS fennec by letting the frontend pan until the content fills the screen, and then thereafter sending touch events to /dev/null.  The UI would then no longer be able to pan.

I suspect some of these issues are similar to the DOM full-screen proposal.

> >  - perf: should only give content a shot at processing events if it's going to
> > do something useful with them, since the round-trip from
> > chrome->content->chrome can be arbitrarily long.
> Why you it be chrome->content->chrome? Why not just chrome->content?
> 

Panning has to start in the chrome process, both for responsiveness and because panning can move parts of the fennec UI into view.

Comment 7

[Benjamin Stover (:stechz)]

We could probably wait for 20 or so milliseconds before we do a chrome move. And once a chrome move has begun, we can send touch events over to content, and once one of the content handlers prevents default we can stop the panning capture and give it to content.

I don't know how to solve phishing problems with our chrome. Maybe we can solve this like we were going to for iframes: if a swipe begins within a certain border around the screen (maybe 20-50px wide), then it is always a chrome pan.

Comment 8

[Benjamin Stover (:stechz)]

To be clear, panning could be (rough outline):
1. Send "mousedown" event (and/or touch equivalent) to content
2. Scroll chrome if N msecs have elapsed or if content did not prevent default, whichever comes first.
3. Continue to send touch events to content, allowing it to prevent default. We don't actually wait for the touch events to come back in chrome, but if content calls ev.preventDefault() we stop as soon as we know.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't think that's a good idea.  If I understand the proposal correctly, it would require us to trade off bad behavior around chrome fighting with content over pans, against laggier panning to give content more time to take control.

Comment 10

[Benjamin Stover (:stechz)]

(In reply to comment #9)
> I don't think that's a good idea.  If I understand the proposal correctly, it
> would require us to trade off bad behavior around chrome fighting with content
> over pans, against laggier panning to give content more time to take control.

"bad behavior around chrome fighting with content over pans" <-- not sure what this means

But yes, beginning a pan could be slightly more laggy, but it would be a one-time cost per pan and we could keep the constant small so that it is not so noticeable.

The only other plan I can think of is to let the chrome pan until content grabs it (maybe this is what you meant?). This could lead to some jittery behavior that could be perceptively worse: imagine doing a swipe to get to the tabs. If content captures it, the sidebar would pull out a few pixels and then snap back. The glitchiness of this points out that content will lag a little, so it may be best to give content a few milliseconds to claim the pan. Even though starting a pan will realistically take a little longer, the user perception could be better because there are no visual artifacts.

Comment 11

[Benjamin Stover (:stechz)]

My gut says a small constant is best, but I doubt we will know until there's a proof of concept that we can all try. Matt says he is intersted.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Yes, we'd be forced to walk a line between lag and jitter.  I don't think it's a good idea to build that into our design.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Here's a strawman proposal
 - fennec doesn't allow pages to listen to touch events by default
 - if a page does addEventListener([some touch event]), have browser-content notify browser-chrome.  Content still can't listen to touch events.
 - browser-chrome draws a little ghosted-out hand icon (or whatever) at a fixed location in the UI.  This alerts the user that content wants to capture touch events.  Maybe it also throws up a temporary, door-hanger style notification saying that too.
 - content still can't listen to touch events
 - if the user clicks the hand icon, it un-ghosts.  Browser-chrome notifies browser-content that the touch-event permission is granted for the URL it thinks requested it (need to be careful about race conditions here).
 - content can listen to touch events, and capture them.  Touch events inferred by browser-chrome are immediately dispatched to content, no timeout.  Need a requester-check here too before dispatching.
 - at any time, the user can click the hand icon to again disable touch events

This system could maybe unify with vlad's addon that delivers mouse events.  We could use similar UI, but a "pointer" for mouse and hand for touch.  I'm not sure there's any point in allowing users to enable dispatch of touch events to pages that don't have listeners for them, but that's a possibility too.

We could potentially make touch-event permission some sort of site pref.

I /think/ this covers the security issue, because this system gives the guarantee (read as a logical implication)
  If web content can capture touch events, the hand icon is drawn unghosted by the fennec UI in a well-known location

IOW, the user always knows when when content /may be/ capturing touch events.  I'm not sure if this is a strong enough guarantee.

This should also give the user a way to assign blame to web content somewhat if having to round-trip input events hurts perf: "I clicked the hand and things got slow!".

It seems like this could maybe be implemented by an addon, but I don't know the DOM-fu for knowing when content registers to listen for event X.

Comment 14

[[:fabrice] Fabrice Desré]

> It seems like this could maybe be implemented by an addon, but I don't know the
> DOM-fu for knowing when content registers to listen for event X.

The service to use for this is nsIEventListenerService (http://mxr.mozilla.org/mozilla-central/source/content/events/public/nsIEventListenerService.idl) but I'm not sure it can efficiently just tell "that page has an element listening for touch event"

Comment 15

[Benjamin Stover (:stechz)]

I'm not a fan of adding such a UI to do touch events, and I hope we only add it as a last resort. Touch should just work, at least for most cases.

My favorite plan is still waiting a number of milliseconds to give the content process a chance to capture input, allowing an outside border (maybe with a certain required pan velocity) as a way to escape. Later, for extreme cases (video games that require fast swiping motions that could bring out the sidebar), webapps could request that it go in fullscreen mode. I'm much more comfortable with a fullscreen request because users are familiar with the idea and have had some training on it.

Phishing is of course an important problem, but I'm not sure we're going to solve it in this bug. It's not high priority to solve it right now.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: wip

This mostly works for a single finger touch event. Adds a border around the edge of the page where panning can't be cancelled. The event names match desktop.

Unfortunately they don't gain us much on sites currently in production, but I figured it would be nice to get the ball rolling a bit. I think we could do something simple like this for 4.0 without a whole lot of risk.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: rebased wip

Same as Wes's patch but updated to apply against mobile-browser tip.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: More WIP

More WIP. Applies on top of the patch in bug 648573. This is getting there, has a border around the edges for panning in sidebars. Also allows panning sidebars if any of the sidebars or urlbar are visible when panning starts. Doesn't feel great, but its a work in progress.

Still need to work out when to publish touchcancel events, and get hold of an iPhone to do some comparison testing. I'll toss a build up for testing in a bit.

Comment 20

[Wesley Johnston (:wesj)]

Build is here:

http://dl.dropbox.com/u/72157/fennec-touchevents.apk

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

How is the performance? Does event creation show up badly in the profiles?

Comment 22

[Matt Brubeck (:mbrubeck)]

It works!!  Though on Google Maps, I find that swiping quickly still results in sidebar panning even when it shouldn't (i.e., the touch is not near the border, and sidebars are not already showing).

This is the first browser to pass the first test submitted to the Web Events working group (by me).  :)
http://dvcs.w3.org/hg/webevents/raw-file/tip/test/touchevents/touch-area.html

If we write any other interop tests, it would be great if they could use the test harness located in that HG repo so they can become part of the conformance test suite.

Comment 23

[Wesley Johnston (:wesj)]

(In reply to comment #21)
> How is the performance? Does event creation show up badly in the profiles?

General panning on Google maps seems on par with Opera/Stock browser IMO. Haven't done any profiling yet. Do you have anything in particular you'd like us to look at? Alon's IPC profiling?

I should note that I am currently firing every mousemove to the child process on desktop regardless of if it is part of a drag, but I am only creating touchmove events in the child if touchstart/mousedown was fired. On devices, obviously all mousemoves are part of a drag.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: WIP v4

I feel like the escape border stuff is starting to get better, at least on Maps. Looking for feedback on the general approach and opinions on the feel of the whole thing. The build above has been updated with this patch (also fixed clicking, but still need to tweak event targeting I think).

My current escape border setup is:

1.) If a touch begins in the middle of the page (more than 35 pixels from the edge of the browser element/screen) the page can cancel panning by calling preventDefault on touchstart or touchmove (matches iPhone accoding the quirksmode).

2.) If the touch is on the top of bottom edge, I allow panning up or down. Similarly, right or left side can pan left/right and/or show sidebars

3.) If the touch starts with a sidebar open, I allow the sidebar to be hidden, but not reshown.

Alon and I have talked about making it so that the left and right escape bars are zero width, but the sidebars can be panned in if the urlbar is showing. That isn't implemented here, but I don't think it will be tough to set up a way that can be done by tweaking prefs.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: WIP v4

Whoops. Wrong file

Comment 27

[Matt Brubeck (:mbrubeck)]

Comment on attachment 526280 [details] [diff] [review]
WIP v4

The code looks good.  I'm still occasionally getting sidebars when I shouldn't, especially when scrolling quickly and repeatedly.  I assume this is because the Content:EventCanceled has not reached the parent process by the time it starts panning.

To fix this, we might want to set allowPanning to false in tapDown, and wait for a message from the content process (or maybe a timeout, as suggested in comment 8) before setting it to "true".  If necessary, we could buffer any mousemoves that happen while waiting for the message, and replay them afterward.

Comment 28

[Benjamin Stover (:stechz)]

Wes, this is great work! Thanks for prototyping this out. Now that we have something real, it's making me understand this problem better.

I'm worried about confusing the user with dragging. Sometimes dragging will move Fennec around, and sometimes it will interact with the webpage. Escape borders, waiting for the content process, and every other clever idea we have is just going to confuse the user. It will be hard to communicate to the user how to interact with the browser and how to interact with the webpage (the distinction is something that isn't obvious to most users anyway).

As a first step, let me propose something much more simple:
1) Let's get touch events working for everything besides dragging (leave touch move out). This solves a lot of use cases!
2) Let's put a fullscreen option in the menu area to allow dragging on the web page. Hitting back would exit fullscreen (this could even happen in another bug).

If this isn't enough, we could start experimenting with escape borders and timeouts and what not. What does everyone think?

Comment 29

[Benjamin Stover (:stechz)]

So there are discoverability issues with finding fullscreen, but I think we might be happier solving those than solving the issues with competing drag attention. There's things we can do (later) for discoverability:
1) Let the website ask for fullscreen
2) Show a fade-in message when we detect something that listens for touch moves about how to get into fullscreen.

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

This is sorta like the borders idea, but could we drop a notification box (or maybe its called a doorhanger, I get confused with the terminology) down from the top that says something to the effect of "the page is intercepting drag events, touch here to return control to Fennec"? I think that will be less confusing to users

Comment 31

[Benjamin Stover (:stechz)]

Hm. I think Brad is right, in that we want the escape hatch to be for escaping to Fennec instead of the web page.

Comment 32

[Matt Brubeck (:mbrubeck)]

Let's move discussion of the "escape hatch" UI to new bug 650339.

Note that even if we use a separate mode (like the "full screen" proposal), we still need to solve the problem of giving content enough of a chance to preventDefault before we decide whether to start panning.

Comment 33

[Benjamin Stover (:stechz)]

So, for this bug, remove the border escape hatch. And I've filed 650342 for the content process timeout, so we're almost ready for review on this bug!

Comment 34

[Wesley Johnston (:wesj)]

Attachment: Patch v1

So this is just kinda the basics here. Sends touch events. Cancels panning in the parent if preventDefault was called on touchstart or touchmove. No attempt at creating escape borders or any other escape mechanism, so you can and will get stuck by pages.

Luckily the race between panning in the parent and child means you occasionally can pan a bit in the parent.

Reminder, you need the patch in bug 648573, and we shouldn't probably check this in until we have implemented an escape hatch.

Comment 35

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

Whoops. Dropped a curly brace in a last second deletion apparently.

Comment 36

[Matt Brubeck (:mbrubeck)]

Comment on attachment 526408 [details] [diff] [review]
Patch v1.1

Very nice!

Only one issue: Regardless of which escape hatch we implement, I think we should continue to always allow chrome panning if a sidebar is visible (like in your previous patch).  With this patch, if you have a sidebar visible on a page like Google Maps that consumes all touchmove events, then there's no way to hide the sidebar and see the whole page.  I think this needs to be fixed before checkin.

>+++ b/mobile/chrome/content/input.js
>       if (dragData.isPan()) {
>+        this.sendMove(aEvent.clientX, aEvent.clientY, aEvent.target)

Nit: Missing semicolon.

Comment 37

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

(In reply to comment #36)

> Only one issue: Regardless of which escape hatch we implement, I think we
> should continue to always allow chrome panning if a sidebar is visible (like in
> your previous patch).  With this patch, if you have a sidebar visible on a page
> like Google Maps that consumes all touchmove events, then there's no way to
> hide the sidebar and see the whole page.  I think this needs to be fixed before
> checkin.

Makes sense.

Comment 38

[Benjamin Stover (:stechz)]

Comment on attachment 526408 [details] [diff] [review]
Patch v1.1

>   dragMove: function dragMove(dx, dy, scroller, aIsKinetic) {
>+    if (!this.allowPanning) {
>+      dx = 0;
>+      dy = 0;
>+    }

Why not early return false? Do we need to update the scrollbars?

>   tapSingle: function tapSingle(aX, aY, aModifiers) {
>     // Cancel the mouse click if we are showing a context menu
>     if (!ContextHelper.popupState)
>-      this._dispatchMouseEvent("Browser:MouseUp", aX, aY, { modifiers: aModifiers });
>+      this._dispatchMouseEvent("Browser:MouseClick", aX, aY, { modifiers: aModifiers });
>+  },

Good rename :)

This patch looks great, but obviously feels a little wrong with the sidebars right now. I'm fine with either landing this disabled or landing this with Matt's review comments above. I know you are working on tests. If they don't land here, please file a followup. The dragger code is complicated enough that it *really* needs some tests so we can comfortably refactor it.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Only change is in the MainDragge.dragMove function to address Matt's comments.

Comment 40

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests.

These check that
1.) touch events are dispatched in the child,
2.) ensure panning doesn't occur if preventDefault is called in the child
3.) sidebars can be panned away but not panned back in even if preventDefault

Don't pass for me without the patch in Bug 651429 (which is currently leaving a hanging mousedown which confuses this test).

Comment 41

[Wesley Johnston (:wesj)]

Attachment: Tests again

Whoops. Now without debugging code

Comment 42

[Benjamin Stover (:stechz)]

Comment on attachment 527204 [details] [diff] [review]
Patch v2

>+pref("dom.w3c_touch_events.timeout", 500);

Shouldn't be in this patch.

>   dragStart: function dragStart(clientX, clientY, target, scroller) {
>@@ -1174,43 +1175,48 @@ Browser.MainDragger.prototype = {
>     if (panOffset.x != 0 && !this._stopAtSidebar) {
>       this._stopAtSidebar = panOffset.x; // negative: stop at left; positive: stop at right
>       this._sidebarTimeout = setTimeout(function(self) {
>         self._stopAtSidebar = 0;
>         self._sidebarTimeout = null;
>       }, 350, this);
>     }
> 
>-    if (this._contentView && !this._contentView.isRoot()) {
>-      this._panContentView(this._contentView, doffset);
>-      // XXX we may need to have "escape borders" for iframe panning
>-      // XXX does not deal with scrollables within scrollables
>+    if (this.allowPanning) {
>+      if (this._contentView && !this._contentView.isRoot()) {
>+        this._panContentView(this._contentView, doffset);
>+        // XXX we may need to have "escape borders" for iframe panning
>+        // XXX does not deal with scrollables within scrollables
>+      }
>+  
>+      // Do content panning
>+      this._panContentView(getBrowser().getRootView(), doffset);
>     }
> 
>-    // Do content panning
>-    this._panContentView(getBrowser().getRootView(), doffset);
>-
>     if (this._hitSidebar && aIsKinetic)
>       return; // No kinetic panning after we've stopped at the sidebar.
> 
>-    // Any leftover panning in doffset would bring controls into view. Add to sidebar
>-    // away panning for the total scroll offset.
>-    let offsetX = doffset.x;
>-    if ((this._stopAtSidebar > 0 && offsetX > 0) ||
>-        (this._stopAtSidebar < 0 && offsetX < 0)) {
>-      if (offsetX != panOffset.x)
>-        this._hitSidebar = true;
>-      doffset.x = panOffset.x;
>-    } else {
>-      doffset.add(panOffset);
>+    if (this.allowPanning || panOffset.x != 0 || panOffset.y > 0) {

Why is panOffset.x != 0 and panOffset.y > 0? Because the URL bar only slides away with a positive offset?

Maybe a comment here?

>+      // Any leftover panning in doffset would bring controls into view. Add to sidebar
>+      // away panning for the total scroll offset.
>+      let offsetX = doffset.x;
>+      if ((this._stopAtSidebar > 0 && offsetX > 0) ||
>+          (this._stopAtSidebar < 0 && offsetX < 0)) {
>+        if (offsetX != panOffset.x)
>+          this._hitSidebar = true;
>+        doffset.x = panOffset.x;
>+      } else {
>+        doffset.add(panOffset);
>+      }
>+  
>+      Browser.tryFloatToolbar(doffset.x, 0);
>+
>+      this._panScroller(Browser.controlsScrollboxScroller, doffset);
>+      this._panScroller(Browser.pageScrollboxScroller, doffset);

All of the sudden, this code is so complicated that I'm having trouble understanding how all the pieces are interacting, unfortunately. The "stop at sidebars code" is now all mixed in with the "should I allow sidebars to scroll in" mixed in with the "should I allow content to pan", and the comments aren't helping me a lot.

Let's refactor this a bit while we're here, eh? We need drag mover to be more about the big picture of what's going on without getting into the details. We could benefit from some helper functions here. My suggestions:
* allowPanning should be called allowContentPanning, with a description that sidebars can still scroll out of view.
* factor out the allow scrollbars to slide in code that sets the timeout. Perhaps something like: "if (panOffset.x != 0) this.preventSidebarsScrollingIn()".
* factor out the pan content code into _panContent, which will handle all subscroll frames appropriately.
* there are some extraneous trailing spaces in dragMove.
* rename panOffset to panSidebarsAwayOffset.
* _hitSidebar && aIsKinetic needs to return false, not just return!

There's probably more we could do, but this would make me much happier.

Comment 43

[Matt Brubeck (:mbrubeck)]

Comment on attachment 527204 [details] [diff] [review]
Patch v2

>+    let point = content.document.createTouch(content, aElement, aData.messageId, aData.x, aData.y, aData.x, aData.y, aData.x, aData.y);

You should pass 1/1/0/0 for radiusX/radiusY/rotationAngle/force here.  (According to the draft spec, these are the default values if no value is known.)

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Now with a refactored dragMove section.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests.

For these to pass you must apply the patch in here as well as the patch in bug 648573. I am a bit nervous about these.

To get them to pass, I had to send a message up from the child saying that the loadFrameScript call had completed ("Test:ScriptLoaded"). I'm not sure if that's really a fix or if its a red herring. Without that they will sometimes run through the first test fine, but timeout during the second.

Comment 46

[Matt Brubeck (:mbrubeck)]

Comment on attachment 528216 [details] [diff] [review]
Patch v2.1

>+    let point = content.document.createTouch(content, aElement, aData.messageId,
>+                                             aData.x, aData.y, aData.x, aData.y, aData.x, aData.y,
>+                                             1, 1, 0, 0);

Instead of using aData.messageId for the identifier, you should just pass 0:
http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#widl-Touch-identifier

(Though I actually think we might want to change that part of the spec to be less specific...)

Comment 47

[Matt Brubeck (:mbrubeck)]

(In reply to comment #46)
> Instead of using aData.messageId for the identifier, you should just pass 0:
> http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#widl-Touch-identifier
> 
> (Though I actually think we might want to change that part of the spec to be
> less specific...)

Raised an issue to change the Touch Events spec:
http://www.w3.org/2010/webevents/track/issues/15

Comment 48

[Wesley Johnston (:wesj)]

Attachment: Patch v2.2

Updated with some comments mbrubeck made here and in Bug 650342.

Comment 49

[Wesley Johnston (:wesj)]

Attachment: Updated Tests

Updated to new changes

Comment 50

[Matt Brubeck (:mbrubeck)]

The W3C Web Events repo now has a few conformance tests that might be useful to anyone working on this code:
http://dvcs.w3.org/hg/webevents/raw-file/tip/test/touchevents/single-touch.html

For easier typing on mobile devices, there is a mirror at limpet.net/w3.

Comment 51

[Matt Brubeck (:mbrubeck)]

Comment on attachment 528385 [details] [diff] [review]
Patch v2.2

>+    let point = content.document.createTouch(content, aElement, 0,
>+                                             aData.x, aData.y, aData.x, aData.y, aData.x, aData.y,
>+                                             1, 1, 0, 0);
>+    let touches = content.document.createTouchList(point);
>+    evt.initTouchEvent(aName, true, true, content, 0, true, true, true, true, touches, touches, touches);

For the "touchend" event, the "touches" and "targetTouches" lists should be empty (because there are no more touch points on the screen).  Only the "changedTouches" list should include a Touch object.

I updated the Touch Events spec to clarify this, and added tests for it:
http://dvcs.w3.org/hg/webevents/rev/fec574167f19
http://dvcs.w3.org/hg/webevents/rev/a3bdd103c7d9

Comment 52

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

Comment on attachment 528385 [details] [diff] [review]
Patch v2.2

From my review in bug 650342:

Overall, I think some terminology is a bit confusing, especially coming into
this code late. To me, the content can try to capture the mouse input. Using "capture"
seems to make sense to me. Also, we should always frame the naming
convention in the same logic, so lets always talk about whether content has
captured the mouse, and not about whether chrome panning is disabled.

>diff --git a/mobile/chrome/content/browser.js b/mobile/chrome/content/browser.js

>+
>+  // allow content panning allows pages to override panning, but should

    // allows pages to override panning, but should

>+  // still allow the sidebars to be panned out of view
>+  this.allowContentPanning = true;

this.contentMouseCapture

>+    // If the sidbars are showing, we pan them out of the way before panning the content.
>+    // The panning distance that should be used for the scrollbars in is stored in panSidebarsAwayOffset,

"scrollbars" or "sidebars" ?

>+    // and subtracted doffset
>+    let panSidebarsAwayOffset = this._panControlsAwayOffset(doffset);

Not your code, but you are adding a few methods with similar names. _panControlsAwayOffset does not actually pan, while other _panXxx methods do.
Rename _panControlsAwayOffset -> _getSidebarOffset to keep naming sane

panSidebarsAwayOffset doesn't read well either. The "pan" prefix is too close to the method names. Change to sidebarOffset

>+    // If we started with one sidebar open, stop when we get to the other.
>+    if (panSidebarsAwayOffset.x != 0)
>+      this._preventSidebarsScrollingIn(panSidebarsAwayOffset);

_preventSidebarsScrollingIn isn't my favorite either. _blockSidebars

>+  _preventSidebarsScrollingIn: function md_preventSidebarsScrollingIn(aSidebarPanning) {

aSidebarPanning -> aSidebarOffset (it's the offset, "panning" is too overloaded)

>     messageManager.addMessageListener("Browser:Highlight", this);
>+    messageManager.addMessageListener("Browser:EventComplete", this);

I'm confused about "EventComplete". What event is complete? "EventComplete" might be too general of a name

>           sendAsyncMessage("FindAssist:Hide", { });
>           this._sendMouseEvent("mousemove", this._highlightElement, x, y);
>           this._sendMouseEvent("mousedown", this._highlightElement, x, y);
>-          this._sendMouseEvent("mouseup", this._highlightElement, x, y);
>+          this._sendMouseEvent("mouseup",   this._highlightElement, x, y);

Please don't line up arguments with lines above or below


Yes, I am being a pain in the ass about naming, but I don't want to create confusion in a few weeks after this lands such that code maintenance and bug fixing creates breakage. Overall the code looks good and tries to stay simple, YAY! Let's try to get the foundation of touch events landed with the most simple changes we can make. Complexity will kill us.

Comment 53

[Benjamin Stover (:stechz)]

Comment on attachment 528385 [details] [diff] [review]
Patch v2.2

r+ from me once mfinkle's comments are addressed. Looks good.

Comment 54

[Wesley Johnston (:wesj)]

Attachment: Patch v3

> I'm confused about "EventComplete". What event is complete? "EventComplete"
> might be too general of a name

I changed this to Browser:EventCapture and added a "panning" attribute to the json message. I'm hoping that if we (or extensions) want to capture input in other cases we can selectively disable/capture clicks, panning, other things?

Comment 55

[Matt Brubeck (:mbrubeck)]

Comment on attachment 528924 [details] [diff] [review]
Patch v3

>+    evt.initTouchEvent(aName, true, true, content, 0, true, true, true, true, touches, touches, touches);

This still needs to be fixed to use empty "touches" and "targetTouches" lists for "touchend" events.

Comment 56

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

Comment on attachment 528924 [details] [diff] [review]
Patch v3

>diff --git a/mobile/chrome/content/input.js b/mobile/chrome/content/input.js

>       if (dragData.isPan()) {
>+        this.sendMove(aEvent.clientX, aEvent.clientY, aEvent.target);
>         // Only pan when mouse event isn't part of a click. Prevent jittering on tap.

Add a blank line before the comment

Make sure Matt's change is included in the check-in patch too

r+

Comment 57

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

pushed with Matt's change:
http://hg.mozilla.org/mozilla-central/rev/9ed390664183

Comment 58

[Aakash Desai [:aakashd]]

verified FIXED on build (using maps.google.com and testcase in comment #0):

Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110502 Firefox/6.0a1 Fennec/6.0a1 ID:20110502051816

Comment 59

[Wesley Johnston (:wesj)]

Attachment: Tests

These passed for me 5/5 times on desktop, so I feel confident we'll never ever EVER see a random orange out of them.

Comment 60

[Ioana Chiorean]

TC https://litmus.mozilla.org/single_result.cgi?id=427110 fails due to this bug.