726615 - Support W3C touch event instead of MozTouch event

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[brandon.wallace]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

(Win7 desktop touch device with latest Firefox nightly - 13.0a1 (2012-02-12)

According to the MDN documentation: https://developer.mozilla.org/en/DOM/Touch_events

We should be using "touchstart", "touchend" etc to listen for touch events.


Actual results:

After much experimenting and head-scratching, and setting/unsetting dom.w3c_touch_events.enabled, I finally concluded that "touch*" is not implemented for the desktop browser and I must continue using the experimental MozTouch* events: https://developer.mozilla.org/en/DOM/Touch_events_(Mozilla_experimental)


Expected results:

touch* events should work, or maybe the note at the bottom of the MDN documentation should mention that the desktop browser still uses the deprecated events.

Comment 1

[Makoto Kato [:m_kato]]

MozTocuh is Windows platform only, we can remove it after replacing with touch event on Windows

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Part 1. touch event for Windows 7

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Part 2. Remove MozTouch event

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Part 1. touch event for Windows

Comment 5

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

Comment on attachment 618622 [details] [diff] [review]
Part 1. touch event for Windows

I don't have any touch enabled Win7 device, so I can't test this.
Does this handle multitouch properly. Also, please test touch event handling when
the page is zoomed in or out.

> 
>   if (mGesture.GetTouchInputInfo((HTOUCHINPUT)lParam, cInputs, pInputs)) {
>+    nsTouchEvent *touchMoveEvent = nsnull;
Could you use nsAutoPtr for this.

>+    nsEventStatus status;
Please initialize status to some value. I know the old code didn't do it,
but nsEventStatus_eIgnore should be ok.

I hope wesj could look at this too, since he implemented support for multitouch.

Comment 6

[Wesley Johnston (:wesj)]

I'm out of the office today. Will try to get tot his as soon as possible.

Comment 7

[brandon.wallace]

Comment on attachment 618622 [details] [diff] [review]
Part 1. touch event for Windows

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

I have a win7 touch screen and can test if/when this is rolled into a nightly build.

Should the MozTouch events be immediately removed though?  Shouldn't they go through some sort of deprecation period to give people time to move to the new API?

::: widget/windows/nsWindow.cpp
@@ +6160,5 @@
> +                           TOUCH_COORD_TO_PIXEL(pInputs[i].cyContact) / 2) :
> +                         nsIntPoint(1,1), /* unknown */
> +                       0.0f,
> +                       0.0f /* unknown */ );
> +

Shouldn't the "force" be set to 1.0 when unknown?

Comment 8

[brandon.wallace]

Actually, looks like the W3C spec says "force" should be 0 when unknown, but the MDN documentation says it should be 1.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 618622 [details] [diff] [review]
> Part 1. touch event for Windows
> 
> I don't have any touch enabled Win7 device, so I can't test this.
> Does this handle multitouch properly. Also, please test touch event handling
> when the page is zoomed in or out.

Olli, you might consider getting a samsung tablet with win7 which can be upgraded to win8. Some guidelines can be found here - 

https://wiki.mozilla.org/Firefox/Windows_8_Integration#Samsung_Series_7

Comment 10

[Jim Mathies [:jimm]]

I don't see much in the way of handling for these events in the desktop front end code. What's the best way to test to see if these events are working correctly?

Comment 11

[Wesley Johnston (:wesj)]

This demo by paul irish lets you draw with multiple fingers and works well for some simple testing:

http://paulirish.com/demo/multi

doesn't support things like force, rotation, or radius. I've got a test page locally somewhere that tried to measure those as well.

mbrubeck has a non-official test page for testing preventDefault behaviors:

http://limpet.net/w3/touchevents/preventDefault.html

There's also some manual tests that you can run from the W3:

https://dvcs.w3.org/hg/webevents/file/1950bf275667/tests/touch-events-v1/submissions/Mozilla

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 618622 [details] [diff] [review]
Part 1. touch event for Windows

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

I think brandon is right about the force, but looks good to me!

::: widget/windows/nsWindow.cpp
@@ +6163,5 @@
> +                       0.0f /* unknown */ );
> +
> +      if (msg == NS_TOUCH_START || msg == NS_TOUCH_END) {
> +        if (touchMoveEvent) {
> +          // need dispatch stocked touch move event

NIT: // dispatch stocked touch move event

@@ +6187,5 @@
> +        touchMoveEvent->touches.AppendElement(t);
> +      }
> +    }
> +
> +    // Dispatch remained touch event

NIT: // Dispatch remaining touch event

Comment 13

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

Is the patch ready to land?
Jimm, Felipe, could you test it a bit?

Comment 14

[Jim Mathies [:jimm]]

(In reply to Olli Pettay [:smaug] from comment #13)
> Is the patch ready to land?
> Jimm, Felipe, could you test it a bit?

Sure looks like it, it has all the reviews it needs. Makoto?

Comment 15

[Jim Mathies [:jimm]]

Attachment: Remove MozTouch event

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 660233 [details] [diff] [review]
Remove MozTouch event

ugh, sorry, loaded with "egg-info" files.

Comment 17

[Jim Mathies [:jimm]]

Attachment: Remove MozTouch event

Comment 18

[Jim Mathies [:jimm]]

try run results look good:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=89c495c2c52e

I'm going to do some hand testing of the events.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 660235 [details] [diff] [review]
Remove MozTouch event

This patch was never approved by anyone. It entails a complete back out of all MozTouch related event code.

Comment 20

[Jim Mathies [:jimm]]

Attachment: Remove MozTouch event

Sorry for the review spam, that wasn't the latest patch.

Comment 21

[Jim Mathies [:jimm]]

Attachment: unbitrotten widget patch (orig. was Part 1)

Comment 22

[Jim Mathies [:jimm]]

> This demo by paul irish lets you draw with multiple fingers and works well
> for some simple testing:
> 
> http://paulirish.com/demo/multi
> 

These builds work fine with this demo.

A few other notes:

- pinch and zoom in the browser work
- most of the demos on the web I found didn't work including all of these:
https://developer.mozilla.org/en-US/demos/tag/tech:multitouch/
- we seem to have a pretty bad conflict with selection and these events
- Unfortunately they didn't fix the clicking issue in the cutthrerope demo. I'll investigate in that bug.

Comment 23

[Jim Mathies [:jimm]]

Attachment: widget patch v.1

Comment 24

[Matt Brubeck (:mbrubeck)]

Comment on attachment 660505 [details] [diff] [review]
widget patch v.1

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

I'm happy to review the next version of this patch, but smaug should look at it too.

::: widget/windows/nsWindow.cpp
@@ +6208,5 @@
> +      uint32_t msg;
> +      if (pInputs[i].dwFlags & TOUCHEVENTF_DOWN) {
> +        msg = NS_TOUCH_START;
> +      } else if (pInputs[i].dwFlags & TOUCHEVENTF_UP) {
> +        msg = NS_TOUCH_END;

Looking at PresShell and at the Android widget code, it looks like we actually need a separate path for NS_TOUCH_END.  While NS_TOUCH_START and NS_TOUCH_MOVE events should include every active input in the "touches" list, NS_TOUCH_END events should include only the changed inputs.  (Sorry, this is contrary to what I told you earlier.  This is the case for widget-level nsTouchEvents, and is different for content-level nsIDOMTouchEvents.)

@@ +6212,5 @@
> +        msg = NS_TOUCH_END;
> +      } else if (pInputs[i].dwFlags & TOUCHEVENTF_MOVE) {
> +        msg = NS_TOUCH_MOVE;
> +      } else {
> +        continue;

I think this "continue" should be removed.  For START and MOVE events, the "touches" list should contain all of the inputs, not just ones that have changed.

@@ +6226,5 @@
> +      if (msg == NS_TOUCH_START || msg == NS_TOUCH_END) {
> +        // W3c spec states start/end event should contain moves from existing
> +        // identifier event streams. Pres shell will discard previous event
> +        // streams if we don't have matching identifies in start events.
> +        touchEventToSend->message = msg;

Is it possible that we will have two different "msg" values in a single WM_TOUCH message (e.g. NS_TOUCH_START on one input and NS_TOUCH_MOVE on another)?  If so, this code will ignore the MOVE event and send only a START event; shouldn't we send multiple events if there are multiple changed touches?  And if not, this code is unneeded since we already passed the message to the nsTouchEvent constructor.

Comment 25

[Wesley Johnston (:wesj)]

Just to make sure its clear, Matt is right. We can trivially fix presShell to not require all the touches on start and move if wanted. For end, I needed some way for presShell to know what was ending.

Comment 26

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

Comment on attachment 660395 [details] [diff] [review]
Remove MozTouch event

Assuming the commented out code in nsWindow.cpp will be enabled and modified
in the other patch, r=me

Comment 27

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #24)
> Looking at PresShell and at the Android widget code, it looks like we
> actually need a separate path for NS_TOUCH_END.  While NS_TOUCH_START and
> NS_TOUCH_MOVE events should include every active input in the "touches"
> list, NS_TOUCH_END events should include only the changed inputs.  (Sorry,
> this is contrary to what I told you earlier.  This is the case for
> widget-level nsTouchEvents, and is different for content-level
> nsIDOMTouchEvents.)

No problem, this should be easy to fix.

> 
> @@ +6212,5 @@
> > +        msg = NS_TOUCH_END;
> > +      } else if (pInputs[i].dwFlags & TOUCHEVENTF_MOVE) {
> > +        msg = NS_TOUCH_MOVE;
> > +      } else {
> > +        continue;
> 
> I think this "continue" should be removed.


This is filtering out potentially spurious data points. For example (TOUCHEVENTF_INRANGE and TOUCHEVENTF_PALM). 

http://msdn.microsoft.com/en-us/library/windows/desktop/dd317334%28v=vs.85%29.aspx

I haven't seen these come in in practive on my win8 tablet.

> 
> @@ +6226,5 @@
> > +      if (msg == NS_TOUCH_START || msg == NS_TOUCH_END) {
> > +        // W3c spec states start/end event should contain moves from existing
> > +        // identifier event streams. Pres shell will discard previous event
> > +        // streams if we don't have matching identifies in start events.
> > +        touchEventToSend->message = msg;
> 

> Is it possible that we will have two different "msg" values in a single
> WM_TOUCH message (e.g. NS_TOUCH_START on one input and NS_TOUCH_MOVE on
> another)?  If so, this code will ignore the MOVE event and send only a START
> event;

From our discussion yesterday that was what I though was expected. This was to fix the spurious touchend event presshell was generating. So for example:

Windows input data:

id, event
1, move
2, start

Currently, this would be sent as a NS_TOUCH_START.

id, event
1, move
1, move
2, start
2, move

Same here, with all four data points in the touches list.

So I guess there are still some open questions here about how widget layers should package up blocks of events to keep pres shell happy.

Comment 28

[Jim Mathies [:jimm]]

> So I guess there are still some open questions here about how widget layers
> should package up blocks of events to keep pres shell happy.

By "blocks of events" I really meant "blocks of active touch point data"

Comment 29

[Jim Mathies [:jimm]]

Attachment: widget patch v.2

wesj, feel free to chime in. I want to be sure we are sending a stream of event pres shell will be happy with. This patch appears to, and works on all the demos / tests I have.

Comment 30

[Jim Mathies [:jimm]]

One thing I was wondering about, while processing the Windows array I'm splitting touch-up points off for the touchEndEventToSend event. I wonder if these should also be appended to the touchEventToSend event, if it exists.

As I mentioned though all the demos I know of work fine so maybe this isn't needed.

Comment 31

[Matt Brubeck (:mbrubeck)]

Comment on attachment 660876 [details] [diff] [review]
widget patch v.2

(In reply to Jim Mathies [:jimm] from comment #30)
> One thing I was wondering about, while processing the Windows array I'm
> splitting touch-up points off for the touchEndEventToSend event. I wonder if
> these should also be appended to the touchEventToSend event, if it exists.

I think this is fine, but you might want to reverse the order of the two blocks here, so the NS_TOUCH_END event is dispatched first.  Then PresShell will remove that point from its tracking list and won't be expecting to see it in the next START or MOVE event.  (Otherwise the START event could trigger PresShell's EvictTouchPoint, which would generate its own NS_TOUCH_END event before yours is dispatched.  Though this only matters in practice if Windows ever sends a DOWN for one input and an UP for another in the same message.)

>+    // Dispatch touch start and move event if we have one.
>+    if (touchEventToSend) {
>+      DispatchEvent(touchEventToSend, status);
>+      delete touchEventToSend;
>+    }
>+
>+    // Dispatch touch end event if we have one.
>+    if (touchEndEventToSend) {
>+      DispatchEvent(touchEndEventToSend, status);
>+      delete touchEndEventToSend;
>+    }
>+  }

This patch looks good to me.  I'd like wesj's eyes on it too since I've never really worked with widget code.

Comment 32

[Jim Mathies [:jimm]]

wjohnston -> review ping? Sorry to nag but I'd like to promote this to aurora if possible, so I'd like to get it on mc soonish.

Comment 33

[Jim Mathies [:jimm]]

Final try run:
https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=57ac676f6daf

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf9991c8d97
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afbf5440fc8

Comment 34

[Jim Mathies [:jimm]]

MDN: 

https://developer.mozilla.org/en-US/docs/DOM/Touch_events
(compatibility section)

https://developer.mozilla.org/en-US/docs/DOM/Touch_events_%28Mozilla_experimental%29
(depreciated and support removed)

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/2afbf5440fc8
https://hg.mozilla.org/mozilla-central/rev/fcf9991c8d97

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 660876 [details] [diff] [review]
widget patch v.2

> +    // Dispatch touch start and move event if we have one.
> +    if (touchEventToSend) {
> +      DispatchEvent(touchEventToSend, status);
> +      delete touchEventToSend;
> +    }
> +
> +    // Dispatch touch end event if we have one.
> +    if (touchEndEventToSend) {
> +      DispatchEvent(touchEndEventToSend, status);
> +      delete touchEndEventToSend;
> +    }

Looks like this isn't safe. Any dispatched DOM events can cause destroying the widget.  You should check mOnDestroyCalled after calling DispatchEvent().

Comment 37

[Jim Mathies [:jimm]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> Comment on attachment 660876 [details] [diff] [review]
> widget patch v.2
> 
> > +    // Dispatch touch start and move event if we have one.
> > +    if (touchEventToSend) {
> > +      DispatchEvent(touchEventToSend, status);
> > +      delete touchEventToSend;
> > +    }
> > +
> > +    // Dispatch touch end event if we have one.
> > +    if (touchEndEventToSend) {
> > +      DispatchEvent(touchEndEventToSend, status);
> > +      delete touchEndEventToSend;
> > +    }
> 
> Looks like this isn't safe. Any dispatched DOM events can cause destroying
> the widget.  You should check mOnDestroyCalled after calling DispatchEvent().

Hmm, I can add that, will file in a follow up.

Comment 38

[Jim Mathies [:jimm]]

http://thenextweb.com/microsoft/2012/09/24/the-w3c-accepted-published-microsofts-pointer-events-submission/

Hmm, so should we file bugs on implementing this new spec? :)

http://www.w3.org/Submission/2012/SUBM-pointer-events-20120907/

Comment 39

[Wesley Johnston (:wesj)]

Yes. I think we want to support both (maybe not at the same time though or something?). Hopefully someday we can deprecate Apple's crazy event model for the more sane one. Details need to be worked out. Can you file something?

Probably more important for the metro work than for Android.

Comment 40

[brandon.wallace]

As a web developer, I like the MS spec better than this webkit-based spec.  Not terribly different than the deprecated MozTouch events :)

Comment 41

[Jim Mathies [:jimm]]

(In reply to Wesley Johnston (:wesj) from comment #39)
> Yes. I think we want to support both (maybe not at the same time though or
> something?). Hopefully someday we can deprecate Apple's crazy event model
> for the more sane one. Details need to be worked out. Can you file something?
> 
> Probably more important for the metro work than for Android.

Should we consider simply skipping the w3c events in desktop Firefox? It seems like this is going to confuse developers and break web sites as we migrate through the different event models to arrive at one.

Comment 42

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

It will take some time before pointer events spec is stable enough to implement, or to release.

Comment 43

[Loic]

Support of W3C touch event has broken many webpages (check the list of regressions). Maybe it would be good to disable it by default for the next Aurora merge.

Comment 44

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

Yes, we have to disable touch events.

Comment 45

[Matt Brubeck (:mbrubeck)]

Attachment: temporarily flip pref to disable touch events on Windows

Let's pref this off until we have a better handle on the regressions.  We'll want to land this on Aurora for Firefox 18.

Comment 46

[Loic]

(In reply to Matt Brubeck (:mbrubeck) from comment #45)
> Created attachment 672550 [details] [diff] [review]
> temporarily flip pref to disable touch events on Windows
> 
> Let's pref this off until we have a better handle on the regressions.  We'll
> want to land this on Aurora for Firefox 18.

I think it's the purpose of bug 798821, isn't it? ;)

Comment 47

[Matt Brubeck (:mbrubeck)]

Comment on attachment 672550 [details] [diff] [review]
temporarily flip pref to disable touch events on Windows

Oops, I missed that.

Comment 48

[Jim Mathies [:jimm]]

Anyone know the best way to get our evangelism team involved in this?

Once bug 798821 lands on mc and aurora, these problems will go away for a short period of time. However in researching Win8 hardware releases, it's pretty clear a large percentage of new laptops, convertibles, and tablets are going to support touch. Which means all the regressions we're seeing are going to come back over the next year or so for anyone purchasing a new device.

It looks like Chrome takes the same approach we are implementing in bug 798821, so Google might be interested in helping out as well.

Comment 49

[Frank Wein [:mcsmurf]]

I guess there's no doc (yet?) which I could point web designers to on what they need to change to fix their website? Or some doc that points out the common causes of this problem?

Comment 50

[Wesley Johnston (:wesj)]

There is https://developer.mozilla.org/en-US/docs/DOM/Touch_events but it doesn't include much/any info on handling touch and mouse at the same time (which should mean fewer changes to sites than authors currently use).

Comment 51

[Jim Mathies [:jimm]]

I guess all the blockers here should be closed as wontfix now?

Comment 52

[Frank Wein [:mcsmurf]]

I don't think so. All those broken websites will still have problems on Windows 8 tablets when using a mouse, no?

Comment 53

[Jim Mathies [:jimm]]

(In reply to Frank Wein [:mcsmurf] from comment #52)
> I don't think so. All those broken websites will still have problems on
> Windows 8 tablets when using a mouse, no?

Right, and we're not going to fix those problems through the browser. Site authors will need to address their broken detection. Hence - wontfix.

Comment 54

[Frank Wein [:mcsmurf]]

Right, we should move the bugs to Tech Evangelism product then (some bugs are already in the TE product) to keep track of this. Maybe we should also remove the "blocking" dependency on this bug, not sure.

Comment 55

[Jim Mathies [:jimm]]

(In reply to Frank Wein [:mcsmurf] from comment #54)
> Right, we should move the bugs to Tech Evangelism product then (some bugs
> are already in the TE product) to keep track of this. Maybe we should also
> remove the "blocking" dependency on this bug, not sure.

Sounds like a good plan. I'll create a new tracking bug in evang and move them over.

Comment 56

[Jim Mathies [:jimm]]

One thing we do need to look out for is bugs in our implementation. For example on the coffee cup demo in bug 794066 I experience some weird interaction between touch and selection. This may be the site, or it may be our implementation. Over time we'll have to sort mozilla bugs from site related bugs.

Comment 57

[juan becerra [:juanb]]

I tried several of the examples referenced to in the dependency bugs, and on Aurora the examples I tried are back to working as expected. I tested on Win7.

I also tried using a Win7 machine with a touchscreen (HP TouchSmart) with and without a mouse, but there is at least one example (bug 798935) where the foxnews videos play well on the touchscreen machine in fx16.0.2, but not in the latest Aurora. Changing the touch event pref from 2 to 0 makes the video play.

I don't know how popular these machines were, but when Aurora goes to release those users will be affected.

Comment 58

[Jim Mathies [:jimm]]

We have a telemetry ping for this. Across our entire Windows install base, touch support is a minimal 1.12%. On Windows 8 it's 4.4% over a rather small data set, which isn't surprising considering the os was just released a couple weeks ago.

Comment 59

[Jim Mathies [:jimm]]

argh, sorry.