508906 - (multitouch) Experimental support for Touch events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Add support to send touch events to the DOM, so XUL and content can make use of raw touch events from the system. Naming the title as 'experimental' because there's no defined standard for such events yet.

I'm already working on this, filing bug to keep track of progress and WIP patches.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozTouch* events - WIP v1

First draft version with all the needed interfaces to support a new MozTouchEvent. The event is currently based off MouseEvent with a new property |streamId|, and supports the messages MozTouchDown, MozTouchUp and MozTouchRelease.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozTouch* events - WIP v2

New version, removes unecessary pre- and post- event handling which would throw extra unecessary events; enable correct X/Y point propagation

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Touch Input data - WIP v1

This patch enables receiving multi-touching messages from the OS, reads the data (message type, X/Y position, message type) and generates the events to dispatch to DOM.   With this chrome and content receives the generated MozTouchDown, MozTouchMove and MozTouchRelease events.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Couple of notes:

1. Safari on iPhone already has some API to do similar things:

http://www.sitepen.com/blog/2008/07/10/touching-and-gesturing-on-the-iphone/

2. We should have a wiki page with basic documentation of what we're proposing so that people can comment and take part on shaping the API

3. At some point we should get some W3C or whatwg involvement.

Comment 5

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

(In reply to comment #4)
> Couple of notes:
> 
> 1. Safari on iPhone already has some API to do similar things:
> 
> http://www.sitepen.com/blog/2008/07/10/touching-and-gesturing-on-the-iphone/
Sure, but that is non-standard and I don't think we want what iPhone has.
It has some problems.

(And we do have gestures already, just not exposed to web content yet.)

> 2. We should have a wiki page with basic documentation of what we're proposing
> so that people can comment and take part on shaping the API
Sure.
(I started to write a page, http://mozilla.pettay.fi/gestures/gestures.xml, though that doesn't have much content yet.)

> 3. At some point we should get some W3C or whatwg involvement.
I started a thread in [e-mail], but so far not much activity.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozTouch* events - WIP v3

Small update from previous version, screenX/Y values are now set

Comment 7

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

Comment on attachment 393953 [details] [diff] [review]
MozTouch* events - WIP v3

>+[scriptable, uuid(cb68e879-f710-415d-a871-9a540860df01)]
>+interface nsIDOMMozTouchEvent : nsIDOMMouseEvent
>+{
>+  readonly attribute unsigned long streamId;
>+
>+  void initMozTouchEvent(in DOMString typeArg,
>+                              in boolean canBubbleArg,
>+                              in boolean cancelableArg,
>+                              in nsIDOMAbstractView viewArg,
>+                              in long detailArg,
>+                              in long screenXArg,
>+                              in long screenYArg,
>+                              in long clientXArg,
>+                              in long clientYArg,
>+                              in boolean ctrlKeyArg,
>+                              in boolean altKeyArg,
>+                              in boolean shiftKeyArg,
>+                              in boolean metaKeyArg,
>+                              in unsigned short buttonArg,
>+                              in nsIDOMEventTarget relatedTargetArg,
>+                              in unsigned long streamIdArg);
>+};


Would it make sense to add size and pressure attributes, or basically can we get such
data from OS? nsIDOMNSMouseEvent has mozPressure already.
Also, some people don't want touch event to extend mouse event, but would prefer separate pointer event.
(Though, personally, I don't mind reusing mouse event interface.) But still, something to think about.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

> Would it make sense to add size and pressure attributes, or basically can we
> get such
> data from OS? nsIDOMNSMouseEvent has mozPressure already.

Win7 does support getting the size attribute, both the width and height of the touch point. Though it's up to the specific driver/device to make it available or not. (So we would need a special value for 'not available')

Unfortunately I couldn't find any references on the touch SDK regarding pressure. Anyway the pressure attribute should probably be part of the interface if other platforms decide to support it.


> Also, some people don't want touch event to extend mouse event, but would
> prefer separate pointer event.
> (Though, personally, I don't mind reusing mouse event interface.) But still,
> something to think about.

I started inheriting it from the mouse event for simplicity on the initial implementation, because I wanted the client/screen XY values and they were already there so I don't need to duplicate code. Although taking a new look I think that we wouldn't lose that if I inherit from DOMUIEvent. One thing that would be lost is the relatedTarget that is in MouseEvent_base, but I'm not sure what it means yet.
Anyway a pointer event interface would be extremely similar to the mouse event, so yeah, _for now_ I don't see a strong reason to not use it

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP document.multitouchData

With this patch both our touch gestures support and the touch events can coexist. It's up to the webpage to make the call if it wants regular panning or wants to receive the raw data on MozTouch* events.  It does this by setting the attribute document.multitouchData to true (MozTouch* events) or false (regular Firefox gestures)

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: document.multitouchData - WIP v2

Implemented the getter for the attribute and made some small cleanups.

The three current patches (MozTouch events, getting input data, document.multitouchData) are part of a queue and applying the three to current trunk makes it possible for regular webpages to set document.multitouchData = true and start receiving multitouch DOM events. I'll attach some demo pages using this feature.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Attachment: Unified patch - aug 21

Unified patch with current versions of the patch queue, rebased today.

Just as a note, the document.multitouchData was added to make it easier to test the examples and switch between multitouch modes, but we're not sure if this is the solution we want yet.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: 5 demos using MozTouch events

Zip file with 5 demos that uses the current events implemented in this bug.

- Tracking divs:
  keep moving <div> elements to each point of contact
- Touch canvas:
  simple drawing <canvas> that fades the path of each finger
- Crop:
  demonstrate creating a image crop area overlayed on a picture
- Resize:
  resize and move Firefox and Thunderbird logos at the same time
- Pong
  2 player pong game where the players can move the pads at the same time on the screen

Comment 13

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

(In reply to comment #11)
> Just as a note, the document.multitouchData was added to make it easier to test
> the examples and switch between multitouch modes, but we're not sure if this is
> the solution we want yet.
Could you explain why you need multitouchData?
You seem to use it to prevent mGesture.SetWinGestureSupport call.

Both touch events and gestures should be dispatched. JS could then prevent 
default handling of those events it wants to. (If this doesn't quite work, for 
example with text selection, that is something to fix.)

Comment 14

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #13)
> Could you explain why you need multitouchData?
> You seem to use it to prevent mGesture.SetWinGestureSupport call.
> 
> Both touch events and gestures should be dispatched. JS could then prevent 
> default handling of those events it wants to. (If this doesn't quite work, for 
> example with text selection, that is something to fix.)

This is basically a platform limitation right now. Windows 7 does not support receiving WM_GESTURE and WM_TOUCH at the same time, you have to choose between one or another, so that's what multitouchData does (it calls RegisterTouchWindow to start receiving WM_TOUCH and also prevents SetWinGestureSupport because it's related only to WM_GESTURE).

We'll have to think on a way around this and as of now I don't see any really good solution. As the page itself will make the final call between one or another (either by preventDefault or some other mechanism), I figured that for now having this attribute wouldn't be so distant from the final result.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

One misc comments: "touch down" and "touch release" is inconsistent; should be "touch down" and "touch up", or "touch press" and "touch release".  (I'd prefer down/up, to be consistent with mousedown/museup).

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Attachment: Unified patch - feb 16

Earlier this week I quickly unbitrotted this patch so the users who were using the experimental build do not need to use a really old build. It compiles and works to current m-c (from last week).

I will now make a better update to each of the patch queue parts and also take vlad's suggestion for the event name change, which definitely makes sense.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozTouch* Events - WIP v4

First patch in the queue.  Unbitrotted to m-c, and updated events' names to:
MozTouchDown
MozTouchUp
MozTouchMove

per vlad's suggestion.

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Attachment: Get touch input messages from Win7, WIP v2

Second patch in the queue. Unbitrotted to m-c

This one is responsible to get the touch messages from windows and generate the corresponding DOM events

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: document.multitouchData, WIP v3

Third patch in queue. Unbitrotted to m-c.  This one implements the current switch from touch gestures to raw events from the OS.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: Unified patch - feb 17

Re-posting unified patch. The one from yesterday missed the new files.  This one also already have the name change for the events.

(This took me longer than I hoped due to trying to recover the Mercurial skillz)

I will now inform some community members who were using old builds that they can update their code with this new patch

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Attachment: Unified patch - mar 19

Updated patch. This one have 3 fixes from the last one, and now it compiles on all platforms. I put it on try server yesterday and it's all green, so now there's a fresh new build here: http://bit.ly/cNmXIS
(Note that this build will probably crash on exit due to unrelated bug 553293. Soon when I finish some other changes on this patch I'll get new builds without that crash)

Comment 22

[:Felipe Gomes (needinfo for replies!)]

New build, without the crash-on-exit bug from the other one.
http://bit.ly/agyXeE

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozTouch* Events - WIP v5

Just posting a more recent version of this patch

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 446439 [details] [diff] [review]
MozTouch* Events - WIP v5

Requesting feedback from Vlad on the API we have currently implemented.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 446439 [details] [diff] [review]
MozTouch* Events - WIP v5

So this seems fine, though it seems like it would cause an event explosion on full-multitouch surface -- e.g. 4 people using 10 fingers each would cause 40 events to be fired as they're all moving around, even though it's unlikely that they would be sampled at different times.

What Android does is you get a touch event, which contains data for one or more pointers.  Each pointer has an associated pointer ID, which is constant (first finger down, second finger down, etc., with holes being reused).  The advantage there is that on any motion event, the state of all currently-down points can be queried.  This could look something like:

var index = ev.getIndexForPointer(0);  // get the stream index for the first pointer

ev.touchClientX[index] -- the clientX value of the first pointer
ev.clientX -- equivalent to the above, same as mouse coord

var numPointers = ev.getPointerCount();
for (var i = 0; i < numPointers; ++i) {
  var pointer = ev.getPointerForIndex(i);
  // stream index "i" refers to pointer "pointer"
  ...
}

If there is no index that refers to a particular pointer, then that pointer is "up"; otherwise it is "down".

I think the above makes it easier for apps to handle touch events, because they get all the data in one place; with the current API, apps would have to buffer this information themselves.

However, it's also possible to get to an API like the above using the data provided by the API that the patch implements, so maybe we should let someone just write a helper library to do that.  (Right?  I don't think I see anything preventing someone creating a 'complete touch picture' API using mozTouch.)  As long as the 'event explosion' isn't a problem, this should be ok...

Comment 27

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #26)
> Comment on attachment 446439 [details] [diff] [review]
> MozTouch* Events - WIP v5
> 
> So this seems fine, though it seems like it would cause an event explosion on
> full-multitouch surface -- e.g. 4 people using 10 fingers each would cause 40
> events to be fired as they're all moving around, even though it's unlikely that
> they would be sampled at different times.
> 
> What Android does is you get a touch event, which contains data for one or more
> pointers.  Each pointer has an associated pointer ID, which is constant (first
> finger down, second finger down, etc., with holes being reused).  The advantage
> there is that on any motion event, the state of all currently-down points can
> be queried.  This could look something like:
> 
> var index = ev.getIndexForPointer(0);  // get the stream index for the first
> pointer
> 
> ev.touchClientX[index] -- the clientX value of the first pointer
> ev.clientX -- equivalent to the above, same as mouse coord
> 
> var numPointers = ev.getPointerCount();
> for (var i = 0; i < numPointers; ++i) {
>   var pointer = ev.getPointerForIndex(i);
>   // stream index "i" refers to pointer "pointer"
>   ...
> }
> 
> If there is no index that refers to a particular pointer, then that pointer is
> "up"; otherwise it is "down".
> 
> I think the above makes it easier for apps to handle touch events, because they
> get all the data in one place; with the current API, apps would have to buffer
> this information themselves.
> 
> However, it's also possible to get to an API like the above using the data
> provided by the API that the patch implements, so maybe we should let someone
> just write a helper library to do that.  (Right?  I don't think I see anything
> preventing someone creating a 'complete touch picture' API using mozTouch.)  As
> long as the 'event explosion' isn't a problem, this should be ok...

Correct, it is possible to a client side helper library to handle the events and aggregate the information needed to keep track of all touch points (I did this for some of the demos code). The original goal was to have the events be very simple and let the client handle anything more complex as needed. However, if most apps would probably want to make use of all touch points data at once, it probably makes sense to provide some way to do that. 

One possibility would be to have a global object on the |window| that would contain all the data available. Another one would be to have a MozTouchSample event that would be called after each cycle of touch events, which would make it easier for the app to keep in sync with the sampling (which is not trivial if we only have individual events). Or we could not send individual events and just send this one, similar to Android (and avoid event explosion).

I find Android's way of getting the touch info a bit convoluted (two array indirections it seems?). Why not always access the touch pointers by the actual array index, and let the stream id be an attribute of the object? I guess they made this to facilitate tracking the same pointer, but I don't think it would be any harder to do it the other way too.  I do like the fact that the primary pointer is always at index 0.
(We need to be able to call preventDefault at some place if the app wants to avoid getting regular mouse handling for the primary pointer.)


About the event explosion, I don't know how to evaluate if it is a problem or not. FWIW, the Webkit API has the same problem, but they built a way to avoid it in some situations. The events have a "capturing" nature, in which the Move events are always sent to the original element that received the Down event. If two or more touch points are capturing to the same element (this can happen if, say, your page is made of a big <canvas>), the events can be coalesced to a single one with data from all of them.

Comment 28

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #27)
> Correct, it is possible to a client side helper library to handle the events
> and aggregate the information needed to keep track of all touch points (I did
> this for some of the demos code). The original goal was to have the events be
> very simple and let the client handle anything more complex as needed. However,
> if most apps would probably want to make use of all touch points data at once,
> it probably makes sense to provide some way to do that. 

I would think they would, or just the main mouse events.  Dunno.

> One possibility would be to have a global object on the |window| that would
> contain all the data available. Another one would be to have a MozTouchSample
> event that would be called after each cycle of touch events, which would make
> it easier for the app to keep in sync with the sampling (which is not trivial
> if we only have individual events). Or we could not send individual events and
> just send this one, similar to Android (and avoid event explosion).

The first two suggestions are ones we could implement after this; but changing the main touch API is something we should figure out before checking things in.  The global object has the problem that you'd need to provide the event target for each pointer as well.

> I find Android's way of getting the touch info a bit convoluted (two array
> indirections it seems?). Why not always access the touch pointers by the actual
> array index, and let the stream id be an attribute of the object? I guess they
> made this to facilitate tracking the same pointer, but I don't think it would
> be any harder to do it the other way too.  I do like the fact that the primary
> pointer is always at index 0.
> (We need to be able to call preventDefault at some place if the app wants to
> avoid getting regular mouse handling for the primary pointer.)

This is so that they can handle the case of pointers going up/down; they don't actually have any distinction between down/up/move, the only event is a "touch" event.  So, for example, if you have three fingers down, add a fourth, and then lift the third, you'll have three streams -- but they'll refer to pointers 0 1 3, not 0 1 2 like they did originally.  If you then add the third again, you'll have 4 streams, but they'll refer to pointers 0 1 3 2, in that order.  Thus the indirection.  There's no object for them to access to add that data in, all the data is just stored in simple arrays of each type.

> About the event explosion, I don't know how to evaluate if it is a problem or
> not. FWIW, the Webkit API has the same problem, but they built a way to avoid
> it in some situations. The events have a "capturing" nature, in which the Move
> events are always sent to the original element that received the Down event. If
> two or more touch points are capturing to the same element (this can happen if,
> say, your page is made of a big <canvas>), the events can be coalesced to a
> single one with data from all of them.

That sounds weird; so the touch events are implicitly captured by the element that responded to the first down event?  Sounds more like that they're coalesced in a way similar to the Android events; you can have touch events firing to different widgets in Android as well, and I believe they'll get separate touch event structures.  (And not totally global state, just the state of all pointers interacting with that event target.)

The only real problem that I see with the current approach here is the potential for event explosion.  Maybe that's not such a big deal though...

Comment 29

[:Felipe Gomes (needinfo for replies!)]

> > I find Android's way of getting the touch info a bit convoluted (two array
> > indirections it seems?). Why not always access the touch pointers by the actual
> > array index, and let the stream id be an attribute of the object? I guess they
> > made this to facilitate tracking the same pointer, but I don't think it would
> > be any harder to do it the other way too.  I do like the fact that the primary
> > pointer is always at index 0.
> > (We need to be able to call preventDefault at some place if the app wants to
> > avoid getting regular mouse handling for the primary pointer.)
> 
> This is so that they can handle the case of pointers going up/down; they don't
> actually have any distinction between down/up/move, the only event is a "touch"
> event.  So, for example, if you have three fingers down, add a fourth, and then
> lift the third, you'll have three streams -- but they'll refer to pointers 0 1
> 3, not 0 1 2 like they did originally.  If you then add the third again, you'll
> have 4 streams, but they'll refer to pointers 0 1 3 2, in that order.  Thus the
> indirection.  There's no object for them to access to add that data in, all the
> data is just stored in simple arrays of each type.

Oh I see, yeah without that it would not be possible to keep the correct association to each touch point. If we have the Down/Up events as well we don't need to do that, right? You can invalidate the pointer data on Up and reuse the ids sooner.

> 
> > About the event explosion, I don't know how to evaluate if it is a problem or
> > not. FWIW, the Webkit API has the same problem, but they built a way to avoid
> > it in some situations. The events have a "capturing" nature, in which the Move
> > events are always sent to the original element that received the Down event. If
> > two or more touch points are capturing to the same element (this can happen if,
> > say, your page is made of a big <canvas>), the events can be coalesced to a
> > single one with data from all of them.
> 
> That sounds weird; so the touch events are implicitly captured by the element
> that responded to the first down event?  Sounds more like that they're
> coalesced in a way similar to the Android events; you can have touch events
> firing to different widgets in Android as well, and I believe they'll get
> separate touch event structures.  (And not totally global state, just the state
> of all pointers interacting with that event target.)

Yeah it's weird, but to the best of my knowledge that's exactly what happens: the events are captured by the element that received the down event (the deepest one at X/Y, so you can listen for the event at any point in its ancestor chain as the event bubbles up).
Note that the capturing only happens to the same finger being tracked; if another finger touches the screen, it will be captured to the target it hits, so it's still possible that touch events are fired to different elements at the same time.
They also have various data structures to represent total global state and only shared state within the same target.


So on android, the motion events are coalesced by the same widget? If a pointer moves from one widget to another, its data will start being seem by the new widget and not by the previous one, right? Do they propagate to parent widgets? (If that even exists... it seems that this would require a data merge in that case)

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - MozTouch* Events - v1

Did clean-up of current patch, updated headers and removed unused function. This part implements the MozTouchDown/Move/Up events and should be good to go for review.

Comment 31

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - get input messages

(Continuing patch clean-up, this is the second part ready for review)

This part is responsible of getting the WM_TOUCH input messages from Windows and dispatching the appropriate MozTouch events to content. It also adds the method signatures for other win32 related functions (registering touch windows) that will be used in the next patch.

Now the patch has quicker event delivery by setting a flag that turns off palm rejection on the OS (some people asked on the Summit if it'd be possible to get the events quicker and this makes Win7 not buffer them).

Comment 32

[:Felipe Gomes (needinfo for replies!)]

Forgot to mention that I haven't taken out the manually defined constants yet because for some reason the build system is not picking them up from the .h files even though the right headers are being included. I'm investigating that.

Comment 33

[Jim Mathies [:jimm]]

>+PRBool nsWinGesture::RegisterTouchWindow(HWND hWnd)
>+{
>+  if (!registerTouchWindow)
>+    return PR_FALSE;
>+
>+  return registerTouchWindow(hWnd, TWF_WANTPALM);
>+}

Why TWF_WANTPALM vs.TWF_FINETOUCH?

>+ * Touch
>+ */
>+typedef struct _TOUCHINPUT {
>+  LONG      x;
>+  LONG      y;
>+  HANDLE    hSource;
..

Is this code is sufficiently munged so we don't run into copyright issues with the sdk?

Comment 34

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #33)
> >+PRBool nsWinGesture::RegisterTouchWindow(HWND hWnd)
> >+{
> >+  if (!registerTouchWindow)
> >+    return PR_FALSE;
> >+
> >+  return registerTouchWindow(hWnd, TWF_WANTPALM);
> >+}
> 
> Why TWF_WANTPALM vs.TWF_FINETOUCH?

The WANTPALM flag specifies that Windows shouldn't perform its palm-rejection algorithms. The doc says that Windows buffer events to do that and thus they're sent with some delay.
The FINETOUCH on the other hand stop coalescing events that are very similar to each other (they have a precision of 0.01 pixel). That precision is not needed (most events only treat things with pixel resolution), and that makes us receive a lot more WM_TOUCH messages which sends too many DOM Events without any new data.

> 
> >+ * Touch
> >+ */
> >+typedef struct _TOUCHINPUT {
> >+  LONG      x;
> >+  LONG      y;
> >+  HANDLE    hSource;
> ..
> 
> Is this code is sufficiently munged so we don't run into copyright issues with
> the sdk?

That will go away soon as a figure out why they aren't being included by the SDK headers.

Comment 35

[Jim Mathies [:jimm]]

(In reply to comment #34)
> (In reply to comment #33)
> > >+PRBool nsWinGesture::RegisterTouchWindow(HWND hWnd)
> > >+{
> > >+  if (!registerTouchWindow)
> > >+    return PR_FALSE;
> > >+
> > >+  return registerTouchWindow(hWnd, TWF_WANTPALM);
> > >+}
> > 
> > Why TWF_WANTPALM vs.TWF_FINETOUCH?
> 
> The WANTPALM flag specifies that Windows shouldn't perform its palm-rejection
> algorithms. The doc says that Windows buffer events to do that and thus they're
> sent with some delay.
> The FINETOUCH on the other hand stop coalescing events that are very similar to
> each other (they have a precision of 0.01 pixel). That precision is not needed
> (most events only treat things with pixel resolution), and that makes us
> receive a lot more WM_TOUCH messages which sends too many DOM Events without
> any new data.

Hmm, do you think the different behaviors between the two warrant setting this via a hidden pref? Not sure, just curious. We can always add it later I suppose if we find it useful.
 
> 
> > 
> > >+ * Touch
> > >+ */
> > >+typedef struct _TOUCHINPUT {
> > >+  LONG      x;
> > >+  LONG      y;
> > >+  HANDLE    hSource;
> > ..
> > 
> > Is this code is sufficiently munged so we don't run into copyright issues with
> > the sdk?
> 
> That will go away soon as a figure out why they aren't being included by the
> SDK headers.

That would be better if possible.

Comment 36

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

Comment on attachment 457503 [details] [diff] [review]
Part 1 - MozTouch* Events - v1


>+class nsDOMMozTouchEvent : public nsIDOMMozTouchEvent,
>+                           public nsDOMMouseEvent

Inherit nsDOMMouseEvent before nsIDOMMozTouchEvent



>+{
>+public:
>+  nsDOMMozTouchEvent(nsPresContext*, nsMozTouchEvent*);
Please give some parameter names


>+[scriptable, uuid(9b454391-0190-4313-a070-1e26e9bf6f31)]
>+interface nsIDOMMozTouchEvent : nsIDOMMouseEvent
>+{
>+  readonly attribute unsigned long streamId;
>+
>+  void initMozTouchEvent(in DOMString typeArg,
>+                              in boolean canBubbleArg,
>+                              in boolean cancelableArg,
>+                              in nsIDOMAbstractView viewArg,
>+                              in long detailArg,
>+                              in long screenXArg,
>+                              in long screenYArg,
>+                              in long clientXArg,
>+                              in long clientYArg,
>+                              in boolean ctrlKeyArg,
>+                              in boolean altKeyArg,
>+                              in boolean shiftKeyArg,
>+                              in boolean metaKeyArg,
>+                              in unsigned short buttonArg,
>+                              in nsIDOMEventTarget relatedTargetArg,
>+                              in unsigned long streamIdArg);
Align parameters

Comment 37

[Jim Mathies [:jimm]]

Comment on attachment 457780 [details] [diff] [review]
Part 2 - get input messages

+  if (mGesture.GetTouchInputInfo((HTOUCHINPUT)lParam, cInputs, pInputs)) {
+
+    for (PRUint32 i=0; i < cInputs; i++) {
+
+      PRUint32 msg = 0;
+      PRUint32 nativeMsgFlag = 0;

nits - lets tighten up that whitepace a bit. Also, |i = 0|, and do we need the initialization here? Looks like it could be

PRUint32 msg, nativeMsgFlag;

+      nsMozTouchEvent touchEvent(PR_TRUE, msg, this, pInputs[i].dwID);

Just curious, what is pInputs[i].dwID? Some sort of sequential number? Do we use it?

+      DispatchPendingEvents();

Do we need to dispatch pending paints here? If so, let's add a comment as to why.

r+ w/those comments answered/addressed.

Comment 38

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - widget code for touch registration

Jim, this adds the final part needed at the widget level for getting the touch input messages. As on win7 we need to register/unregister the native windows to receive wm_touch, we add these functions to be called. They should be a no-op on other platforms.

They'll be called by the focus manager and the event listener manager whenever touch event listeners are added on a webpage and/or the displayed webpage on the widget changes.

Comment 39

[:Felipe Gomes (needinfo for replies!)]

Replying to review questions:

(In reply to comment #37)
> Comment on attachment 457780 [details] [diff] [review]
> Part 2 - get input messages
> 
> +  if (mGesture.GetTouchInputInfo((HTOUCHINPUT)lParam, cInputs, pInputs)) {
> +
> +    for (PRUint32 i=0; i < cInputs; i++) {
> +
> +      PRUint32 msg = 0;
> +      PRUint32 nativeMsgFlag = 0;
> 
> nits - lets tighten up that whitepace a bit. Also, |i = 0|, and do we need the
> initialization here? Looks like it could be
> 
> PRUint32 msg, nativeMsgFlag;

Ok!

> 
> +      nsMozTouchEvent touchEvent(PR_TRUE, msg, this, pInputs[i].dwID);
> 
> Just curious, what is pInputs[i].dwID? Some sort of sequential number? Do we
> use it?

Yes, it is an identifier that is a random number, but is kept constant for the same finger while it is touching the screen. This information is needed by webapps that need to track the movement of each finger and interpret the data.

> 
> +      DispatchPendingEvents();
> 
> Do we need to dispatch pending paints here? If so, let's add a comment as to
> why.
> 

Hm I don't think we need that. I don't see any difference having that or not; I added it because it is called by most of other input-related messages, so I wasn't sure if it could be kept away. I think I'll remove it and it can be re-added later if it feels needed.

Comment 40

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - Touch event listener tracking

Olli, this is the part that was done on bug 554823 (I made some improvements since the last version over that bug)

Some comments about the patch:

- There was a problem with the mTouchState flag on nsPIDOMWindow. Sometimes (or always, not sure), the nsPIDOMWindow was reused when loading a new document on the same tab, which made the flag have the wrong state of the previous doc.  Now, I just removed that flag, and instead verify directly that the nsIDocument has touch event listeners. This actually simplified things a lot because we don't have to make any effort to keep the flag synced.

- There's two new functions, UpdateTouchState and MaybeUpdateTouchState, that I could have made it as a single function that takes a bool parameter (PRBool aIgnoreFocus or something), but the parameter would give the wrong impression on the call sites (e.g. "UpdateTouchState(PR_FALSE)" seems misleading to me), so that's why I used the _Maybe*_ idiom that is used on other places.

- This patch have dom/ and content/ parts, but I believe they're all related. Let me know if I should split it or get extra review from someone else on any part of it. Also, does this need sr?

Comment 41

[Jim Mathies [:jimm]]

Comment on attachment 458962 [details] [diff] [review]
Part 3 - widget code for touch registration

+    NS_IMETHOD RegisterTouchWindow() = 0;
+    NS_IMETHOD UnregisterTouchWindow() = 0;

// b7ec5f61-57df-4355-81f3-41ced52e8026
#define NS_IWIDGET_IID \
{ 0xb7ec5f61, 0x57df, 0x4355, \
  { 0x81, 0xf3, 0x41, 0xce, 0xd5, 0x2e, 0x80, 0x26 } }

You'll need to rev the uid of the widget interface for this at the top of the file. (Visual studio has a tool that's helpful for this: Tools -> Create GUID.)

>   NS_IMETHOD              GetToggledKeyState(PRUint32 aKeyCode, PRBool* aLEDState);
>+  
>+  NS_IMETHOD            RegisterTouchWindow();
>+  NS_IMETHOD            UnregisterTouchWindow();

second column spacing is off.

>+NS_METHOD nsBaseWidget::RegisterTouchWindow()
>+{
>+  return NS_OK;
>+}
>+
>+NS_METHOD nsBaseWidget::UnregisterTouchWindow()
>+{
>+  return NS_OK;
>+}
>+

Probably NS_ERROR_NOT_IMPLEMENTED would be more appropriate?

Comment 42

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

> You'll need to rev the uid of the widget interface for this at the top of the
> file. (Visual studio has a tool that's helpful for this: Tools -> Create GUID.)
http://mozilla.pettay.fi/cgi-bin/mozuuid.pl works too

Comment 43

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

(In reply to comment #40)
> - There was a problem with the mTouchState flag on nsPIDOMWindow. Sometimes (or
> always, not sure), the nsPIDOMWindow was reused when loading a new document on
> the same tab,
The outer window is reused, but when is the inner window reused?

Comment 44

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

Comment on attachment 458964 [details] [diff] [review]
Part 4 - Touch event listener tracking


>--- a/content/events/src/nsEventListenerManager.cpp
>+++ b/content/events/src/nsEventListenerManager.cpp
>@@ -486,16 +486,24 @@ nsEventListenerManager::AddEventListener
>       window->SetMutationListeners((aType == NS_MUTATION_SUBTREEMODIFIED) ?
>                                    kAllMutationBits :
>                                    MutationBitForEventType(aType));
>     }
>   } else if (aTypeAtom == nsGkAtoms::onMozOrientation) {
>     nsPIDOMWindow* window = GetInnerWindowForTarget();
>     if (window)
>       window->SetHasOrientationEventListener();
>+  } else if (aType >= NS_MOZTOUCH_DOWN && aType <= NS_MOZTOUCH_UP) {
>+    nsCOMPtr<nsINode> node = do_QueryInterface(mTarget);
>+    if (node) {
>+      nsIDocument* document = node->GetOwnerDoc();
>+      if (document) {
>+        document->IncreaseTouchEventListenerCount();
>+      }
>+    }
So what if the listener is added to window?
If you handled toucheventlistenercount in the inner window, 
you could use GetInnerWindowForTarget here.


>@@ -534,16 +542,27 @@ nsEventListenerManager::RemoveEventListe
>       nsRefPtr<nsEventListenerManager> kungFuDeathGrip = this;
>       mListeners.RemoveElementAt(i);
>       mNoListenerForEvent = NS_EVENT_TYPE_NULL;
>       mNoListenerForEventAtom = nsnull;
>       break;
>     }
>   }
> 
>+  if (aType >= NS_MOZTOUCH_DOWN && aType <= NS_MOZTOUCH_UP) {
>+    nsCOMPtr<nsINode> node = do_QueryInterface(mTarget);
>+    if (node) {
>+      nsIDocument* document = node->GetOwnerDoc();
>+      if (document) {
>+        document->DecreaseTouchEventListenerCount();
>+      }
>+    }
>+
>+  }
>+
This isn't quite enough. If an element which has touch event listener is removed from document (and deleted by cycle collector for example),
DecreaseTouchEventListenerCount won't be called.
>+void nsGlobalWindow::UpdateTouchState()
>+{
>+  nsIDocShell *docshell = GetDocShell();
>+  if (!docshell)
>+    return;
>+
>+  nsPresContext *prescontext = nsnull;
>+  docshell->GetPresContext(&prescontext);
>+  if (!prescontext)
>+    return;
>+
>+  nsCOMPtr<nsISupports> container = prescontext->GetContainer();
>+  if (!container)
>+    return;
>+
>+  nsCOMPtr<nsIBaseWindow> docShellWin = do_QueryInterface(container);
>+  if (!docShellWin)
>+    return;
I don't understand why you need to get prescontext from docshell and then
its container (which is in practice the same docshell).
And btw, you leak prescontext.

Comment 45

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - Touch event listener tracking v2

Updated patch with comments addressed and the changes we talked. Now I'm simply handling the touch event flag in the nsPIDOMWindow instead of the document, and it's a permanent flag.
The focus now follow changes to inner windows as well.

I had fixed the leak but that part was actually not needed, the global window already had a function to get the main widget.

Comment 46

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #41)
> Comment on attachment 458962 [details] [diff] [review]
> Part 3 - widget code for touch registration
> 
> +    NS_IMETHOD RegisterTouchWindow() = 0;
> +    NS_IMETHOD UnregisterTouchWindow() = 0;
> 
> // b7ec5f61-57df-4355-81f3-41ced52e8026
> #define NS_IWIDGET_IID \
> { 0xb7ec5f61, 0x57df, 0x4355, \
>   { 0x81, 0xf3, 0x41, 0xce, 0xd5, 0x2e, 0x80, 0x26 } }
> 
> You'll need to rev the uid of the widget interface for this at the top of the
> file. (Visual studio has a tool that's helpful for this: Tools -> Create GUID.)
> 
Done

> >   NS_IMETHOD              GetToggledKeyState(PRUint32 aKeyCode, PRBool* aLEDState);
> >+  
> >+  NS_IMETHOD            RegisterTouchWindow();
> >+  NS_IMETHOD            UnregisterTouchWindow();
> 
> second column spacing is off.
> 
Fixed

> >+NS_METHOD nsBaseWidget::RegisterTouchWindow()
> >+{
> >+  return NS_OK;
> >+}
> >+
> >+NS_METHOD nsBaseWidget::UnregisterTouchWindow()
> >+{
> >+  return NS_OK;
> >+}
> >+
> 
> Probably NS_ERROR_NOT_IMPLEMENTED would be more appropriate?

I've changed to NS_ERROR_NOT_IMPLEMENTED, it seems more appropriate. But on non-Windows platforms the EventListenerManager will call this function. It doesn't care about the return value though, so if there's no problem on returning NS_ERROR_NOT_IMPLEMENTED, it's fine.

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - widget code for touch registration v2

(Forgot to attach the patch. See comment above)

Comment 48

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

Comment on attachment 459611 [details] [diff] [review]
Part 4 - Touch event listener tracking v2

>diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
>--- a/content/base/public/nsIDocument.h
>+++ b/content/base/public/nsIDocument.h
>@@ -113,18 +113,18 @@ class Loader;
> namespace dom {
> class Link;
> class Element;
> } // namespace dom
> } // namespace mozilla
> 
> 
> #define NS_IDOCUMENT_IID      \
>-{ 0x19df0b2c, 0xf89a, 0x4c83, \
>-  { 0x82, 0x29, 0x3a, 0xe0, 0xb6, 0x42, 0x71, 0x9c } }
>+{ 0xd6f9bebe, 0xd54f, 0x4335, \
>+  { 0x86, 0x97, 0x35, 0xb0, 0xc9, 0x60, 0xbe, 0xcb } }

No need for this. You're not changing nsIDocument


>+++ b/dom/base/nsFocusManager.cpp
>@@ -1587,22 +1587,27 @@ nsFocusManager::Focus(nsPIDOMWindow* aWi
>   PRINTTAGF("**Element %s has been focused", aContent);
>   nsCOMPtr<nsIDocument> docm = do_QueryInterface(aWindow->GetExtantDocument());
>   if (docm)
>     PRINTTAGF(" from %s", docm->GetRootElement());
>   printf(" [Newdoc: %d FocusChanged: %d Raised: %d Flags: %x]\n",
>          aIsNewDocument, aFocusChanged, aWindowRaised, aFlags);
> #endif
> 
>-  // if this is a new document, update the parent chain of frames so that
>-  // focus can be traversed from the top level down to the newly focused
>-  // window.
>-  if (aIsNewDocument)
>+  if (aIsNewDocument) {
>+    // if this is a new document, update the parent chain of frames so that
>+    // focus can be traversed from the top level down to the newly focused
>+    // window.
>     AdjustWindowFocus(aWindow, PR_FALSE);
> 
>+    // Update the window touch registration to reflect the state of
>+    // the new document that got focus
>+    aWindow->UpdateTouchState();
>+  }
>+

I wish we could test this all somehow. But at least test manually that this works
with different kinds of way when a window gets focused: 
mouse click, tab navigation, activating a deactivated window, dragging something
to a window...

>+void nsGlobalWindow::MaybeUpdateTouchState()
>+{
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+
>+  nsCOMPtr<nsIDOMWindow> focusedWindow;
>+  fm->GetFocusedWindow(getter_AddRefs(focusedWindow));
>+
>+  if(this == focusedWindow) {
>+    UpdateTouchState();
>+  }
IIRC, focusmanager keeps reference to innerwindow, so you should probably 
forward this method and UpdateTouchStatte to inner using FORWARD_TO_INNER


>+}
>+
>+void nsGlobalWindow::UpdateTouchState()
>+{
>+  nsCOMPtr<nsIWidget> mainWidget = GetMainWidget();
>+  if (!mainWidget)
>+    return;
>+
>+  PRBool touchListeners = mInnerWindow ? mInnerWindow->HasTouchEventListeners()
>+                                       : mMayHaveTouchEventListener;
...in which case you wouldn't need to have the innerwindow check here.

Comment 49

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #48)
> Comment on attachment 459611 [details] [diff] [review]
> Part 4 - Touch event listener tracking v2
> 
> >diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h
> >--- a/content/base/public/nsIDocument.h
> >+++ b/content/base/public/nsIDocument.h
> >@@ -113,18 +113,18 @@ class Loader;
> > namespace dom {
> > class Link;
> > class Element;
> > } // namespace dom
> > } // namespace mozilla
> > 
> > 
> > #define NS_IDOCUMENT_IID      \
> >-{ 0x19df0b2c, 0xf89a, 0x4c83, \
> >-  { 0x82, 0x29, 0x3a, 0xe0, 0xb6, 0x42, 0x71, 0x9c } }
> >+{ 0xd6f9bebe, 0xd54f, 0x4335, \
> >+  { 0x86, 0x97, 0x35, 0xb0, 0xc9, 0x60, 0xbe, 0xcb } }
> 
> No need for this. You're not changing nsIDocument
>
Ops, leftover from previous version. Removed 


> >+void nsGlobalWindow::MaybeUpdateTouchState()
> >+{
> >+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> >+
> >+  nsCOMPtr<nsIDOMWindow> focusedWindow;
> >+  fm->GetFocusedWindow(getter_AddRefs(focusedWindow));
> >+
> >+  if(this == focusedWindow) {
> >+    UpdateTouchState();
> >+  }
> IIRC, focusmanager keeps reference to innerwindow, so you should probably 
> forward this method and UpdateTouchStatte to inner using FORWARD_TO_INNER

I've made this change and looks like it works. I still got do a few more tests on it. I think that UpdateTouchState can definitely use FORWARD_TO_INNER (and thus get rid of mInnerWindow check and HasTouchEventListeners function). I'm not completely sure about the MaybeUpdateTouchState though, as I think the focus manager sometimes gives the outer window. If that's not true I'll change both, otherwise I'll change UpdateTouchState.

> 
> I wish we could test this all somehow. But at least test manually that this
> works
> with different kinds of way when a window gets focused: 
> mouse click, tab navigation, activating a deactivated window, dragging
> something
> to a window...
> 

So I've been thinking about testing.. If we reintroduce that MozMultitouchData property as an readonly attribute, it'd be possible to have mochitests that changes the focus between touch/non-touch domwindows and verify if the property was correctly updated. Not sure though if it's a good idea to expose such property just because of tests.
FWIW I've tested this manually by clicking, keyboard navigation, tab switching,  and everything works so far.

Comment 50

[Jim Mathies [:jimm]]

>+  nsWindow* nsWin = GetNSWindowPtr(aWnd);
>+  nsWin->mGesture.RegisterTouchWindow(aWnd);
>+  return TRUE;

Let's check for null on those nsWindow ptrs before we use them. Also nix "ns" on the variable name:

nsWindow* win = GetNSWindowPtr(aWnd);
if (win)
  win->mGesture.RegisterTouchWindow(aWnd);
return TRUE;

Comment 51

[Jim Mathies [:jimm]]

Also, note, it won't break anything since we don't build on ce, but we should wrap use of mGesture in |#if !defined(WINCE)| / |#endif|.

Comment 52

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 5 - tests

Event creation/listening/bubbling tests

Comment 53

[:Felipe Gomes (needinfo for replies!)]

Attachment: All patches - for checkin

hg diff -r qparent with all nits fixed, not including part 5 - tests.

Comment 54

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

Comment on attachment 461583 [details] [diff] [review]
Part 5 - tests

Please use 2 space indentation consistently.
And using random() is a bit strange, but should work.

Comment 55

[:Felipe Gomes (needinfo for replies!)]

Attachment: All patches including tests - for checkin

Full patch including tests. I sent this to tryserver over the weekend and AFAICT everything seemed fine.

Comment 56

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/83236349154e

Comment 57

[Eric Shepherd [:sheppy]]

These events are now documented:

https://developer.mozilla.org/en/DOM/Touch_events

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMMozTouchEvent

And the Firefox 4 for developers page mentions them:

https://developer.mozilla.org/en/Firefox_4_for_developers#DOM.c2.a0changes

Comment 58

[Ben Bucksch (:BenB)]

Am I understanding the patches and comments correctly that this added the generic DOM API and Win7 platform support, but it won't work on any other OS?

X11 is soon starting to get multitouch support, I filed bug 711711 about using that.

Comment 59

[Jim Mathies [:jimm]]

felipe - do we have any win bugs filed on migrating to the w3c touch events implemented in bug 603008?

Comment 60

[:Felipe Gomes (needinfo for replies!)]

Jim: not that I know of. I haven't seem any bug filed for that

Comment 61

[Matt Brubeck (:mbrubeck)]

Note that the W3C Touch Events spec is currently blocked by some patent issues, and might need to change to something closer to the MozTouch or MSPointer APIs:
http://blog.jquery.com/2012/04/10/getting-touchy-about-patents/

Comment 62

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #61)
> Note that the W3C Touch Events spec is currently blocked by some patent
> issues, and might need to change to something closer to the MozTouch or
> MSPointer APIs:
> http://blog.jquery.com/2012/04/10/getting-touchy-about-patents/

Well that sort of throws a wrench in things doesn't it.