Last Comment Bug 603008 - Support multitouch on Android
: Support multitouch on Android
Status: RESOLVED FIXED
[sec-assigned:yvan]
: dev-doc-complete, feature
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All Android
: P2 normal with 1 vote (vote)
: Firefox 12
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
: 695207 708942 718294 718390 721359 (view as bug list)
Depends on: multitouch 676275 719309 721076 721079 721080 721212 721359 721402 721484 721597 721712 721714 724001 728480 750448 1038930
Blocks: 689602 544614 631042 702492 708942 719647 745071
  Show dependency treegraph
 
Reported: 2010-10-08 15:31 PDT by Michael Wu [:mwu]
Modified: 2014-07-16 06:38 PDT (History)
52 users (show)
dveditz: sec‑review? (yvanboily+mozbugmail)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Less is more (7.27 KB, patch)
2010-10-08 15:31 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
mobile-browser patch (part 1): single-touch (1.80 KB, patch)
2010-10-08 15:54 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
Less is more, v2 (7.67 KB, patch)
2010-10-08 16:29 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
mobile-browser patch (WIP) (2.49 KB, patch)
2010-10-08 17:03 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
Less is more, v3 (11.88 KB, patch)
2010-10-09 21:05 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Less is more, v4 (11.88 KB, patch)
2010-10-18 11:16 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
unbitrotted (17.00 KB, patch)
2011-04-28 19:55 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
WIP Patch (46.06 KB, patch)
2011-06-17 09:56 PDT, Wesley Johnston (:wesj)
bugs: feedback+
Details | Diff | Splinter Review
WIP v2 (49.36 KB, patch)
2011-07-01 14:55 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch part 1/4 (34.22 KB, patch)
2011-07-26 19:03 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch part 2/4 - Android backend (21.09 KB, patch)
2011-07-26 19:04 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch part 3/4 - Mobile frontend (5.09 KB, patch)
2011-07-26 19:06 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v1 - 1/4 -> Platform changes (34.22 KB, patch)
2011-08-04 16:15 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Unbitrotted WIP platform and widget stuff (74.99 KB, patch)
2011-10-26 11:08 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (57.98 KB, patch)
2011-11-03 14:54 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch 1/2 Platform stuff (35.16 KB, patch)
2011-11-04 15:34 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch 1/2 Platform stuff (35.16 KB, patch)
2011-11-04 15:35 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch 2/2 - Android widget stuff (23.22 KB, patch)
2011-11-04 15:36 PDT, Wesley Johnston (:wesj)
blassey.bugs: review+
Details | Diff | Splinter Review
Patch 1/2 Platform stuff v2 (35.02 KB, patch)
2011-11-04 17:33 PDT, Wesley Johnston (:wesj)
bugs: review-
Details | Diff | Splinter Review
WIP Widget (49.84 KB, patch)
2011-11-11 17:29 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
WIP (36.25 KB, patch)
2011-11-14 18:13 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
WIP (66.84 KB, patch)
2011-11-17 17:19 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
WIP2 (68.77 KB, patch)
2011-11-17 17:48 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v2 (82.99 KB, patch)
2011-12-01 02:13 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
WIP Widget (63.05 KB, patch)
2011-12-06 09:16 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Part 1/3 (45.53 KB, patch)
2011-12-07 09:03 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Part 2/3 - Prevent panning/clicking/etc. in java (16.24 KB, patch)
2011-12-07 09:04 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Part 3/3 - Look for touch listeners (13.14 KB, patch)
2011-12-07 09:06 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Patch 1/3 - Dispatching touch events (46.37 KB, patch)
2011-12-08 12:01 PST, Wesley Johnston (:wesj)
blassey.bugs: review+
Details | Diff | Splinter Review
Widget Patch 2/3 - Prevent Default stuff (19.88 KB, patch)
2011-12-08 12:03 PST, Wesley Johnston (:wesj)
bugmail: feedback-
Details | Diff | Splinter Review
Widget Patch 3/3 - Touch Listeners (12.71 KB, patch)
2011-12-08 12:04 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Platform Patch (83.04 KB, patch)
2011-12-08 12:26 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Patch 1/3 - Dispatching touch events (46.37 KB, patch)
2011-12-09 04:12 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
Details | Diff | Splinter Review
Widget Patch 2/3 - Prevent Default stuff (19.88 KB, patch)
2011-12-09 04:15 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Patch 3/3 - Touch Listeners (12.71 KB, patch)
2011-12-09 04:17 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Patch 2/3 - Prevent Default stuff (15.59 KB, patch)
2011-12-09 05:39 PST, Wesley Johnston (:wesj)
bugmail: review-
Details | Diff | Splinter Review
Widget Patch 3/3 - Touch Listeners (13.17 KB, patch)
2011-12-09 05:40 PST, Wesley Johnston (:wesj)
mark.finkle: review+
bugs: review+
Details | Diff | Splinter Review
Widget Patch 1/3 - Dispatching touch events (46.10 KB, patch)
2011-12-09 05:41 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
Details | Diff | Splinter Review
Platform Patch (83.05 KB, patch)
2011-12-09 05:42 PST, Wesley Johnston (:wesj)
bugs: review-
Details | Diff | Splinter Review
Widget Patch 2/3 - Prevent Default stuff (15.91 KB, patch)
2011-12-09 08:03 PST, Wesley Johnston (:wesj)
bugmail: review+
Details | Diff | Splinter Review
Platform Patch (65.85 KB, patch)
2011-12-16 17:35 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Widget Patch 3/3 - Touch Listeners (7.60 KB, patch)
2011-12-16 17:36 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Platform Patch (74.23 KB, patch)
2011-12-16 17:38 PST, Wesley Johnston (:wesj)
bugs: review-
Details | Diff | Splinter Review
Platform Patch (73.64 KB, patch)
2011-12-19 17:32 PST, Wesley Johnston (:wesj)
bugs: review+
Details | Diff | Splinter Review
Platform patch fixes (4.03 KB, patch)
2012-01-14 18:52 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Platform Patch (92.37 KB, patch)
2012-01-14 18:53 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
Details | Diff | Splinter Review
IFDEF platform pieces (62.05 KB, patch)
2012-01-14 18:56 PST, Wesley Johnston (:wesj)
bugs: review-
Details | Diff | Splinter Review
Folded platform patches (92.37 KB, patch)
2012-01-14 19:10 PST, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Platform patch fixes (4.03 KB, patch)
2012-01-15 20:30 PST, Wesley Johnston (:wesj)
bugs: review+
Details | Diff | Splinter Review
IFDEF platform pieces (62.05 KB, patch)
2012-01-15 23:19 PST, Wesley Johnston (:wesj)
bugs: review+
Details | Diff | Splinter Review
Folded platform patches (92.91 KB, patch)
2012-01-18 14:27 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Widget Parts 1+2 (60.67 KB, patch)
2012-01-18 14:29 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Widget Patch 3/3 - Touch Listeners (14.51 KB, patch)
2012-01-18 14:30 PST, Wesley Johnston (:wesj)
wjohnston2000: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Fixes for widget (6.51 KB, patch)
2012-01-23 08:14 PST, Wesley Johnston (:wesj)
blassey.bugs: review-
Details | Diff | Splinter Review
Fixes for widget (14.56 KB, patch)
2012-01-24 08:43 PST, Wesley Johnston (:wesj)
blassey.bugs: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Michael Wu [:mwu] 2010-10-08 15:31:25 PDT
Created attachment 481952 [details] [diff] [review]
Less is more

I haven't tested the multitouch part of this patch but it might work.
Comment 1 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-10-08 15:48:53 PDT
Does Android have real gesture events too?
Or would it make sense to keep those gesture events android/nsWindow.cpp
seems to generate?
Comment 2 Matt Brubeck (:mbrubeck) 2010-10-08 15:51:48 PDT
Android does not have native gesture events.

It might make sense to keep sending MozSimpleGesture events from nsWindow, as long as they don't interfere with the MozTouch events.  Or we can implement gestures in the frontend instead.  We might want to do that anway, because things like pinch-zoom on a touchscreen can benefit from more information than MozSimpleGestureMagnify provides.
Comment 3 Matt Brubeck (:mbrubeck) 2010-10-08 15:54:07 PDT
Created attachment 481958 [details] [diff] [review]
mobile-browser patch (part 1): single-touch

Trivial patch to make single-touch events work just like mouse events.  No multitouch gestures yet.
Comment 4 Michael Wu [:mwu] 2010-10-08 16:29:05 PDT
Created attachment 481967 [details] [diff] [review]
Less is more, v2
Comment 5 Matt Brubeck (:mbrubeck) 2010-10-08 17:03:32 PDT
Created attachment 481975 [details] [diff] [review]
mobile-browser patch (WIP)

This is a very stupid patch just to prove that the events are received correctly.  It implements pinch zooming (no swipe gestures).  It makes a lot of assumptions about the events based on the current Android implementation.
Comment 6 Michael Wu [:mwu] 2010-10-09 21:05:52 PDT
Created attachment 482109 [details] [diff] [review]
Less is more, v3

This version of the patch sets the streamId correctly.
Comment 7 Michael Wu [:mwu] 2010-10-18 11:16:31 PDT
Created attachment 484029 [details] [diff] [review]
Less is more, v4

Fixed bitrot.
Comment 8 Thomas Arend [:tarend] 2011-01-04 10:05:40 PST
What is the status of this fix? Paul's FF4 demos depend on this. Thanks!
Comment 9 Matt Brubeck (:mbrubeck) 2011-01-04 10:22:32 PST
The main bug related to touch events on mobile (bug 544614) is currently blocking-fennec:2.0-, although it has gone back and forth in the past.  We have some open design questions (see bug 531974 and bug 544614) to be resolved before this is exposed to web content.  Because this is 2.0- no one is actively working on it, although I am planning to do some prototyping work if I have extra time available.
Comment 10 Michael Wu [:mwu] 2011-02-07 14:32:16 PST
Not going to be working on this. The patch here should be enough for someone else to run with after fennec 4.0 ships.
Comment 11 Matt Brubeck (:mbrubeck) 2011-04-28 19:55:53 PDT
Created attachment 529024 [details] [diff] [review]
unbitrotted

Same as mwu's last patch, but updated to tip.
Comment 12 Wesley Johnston (:wesj) 2011-06-17 09:56:57 PDT
Created attachment 540074 [details] [diff] [review]
WIP Patch

WIP. This works to fire off multi-touch events from the Android to the parent process, but isn't "done" yet. I still needs to finish up things like event targeting (both firing the touch events on multiple targets if necessary, and setting targetTouches correctly on touch events), and preventing mouse events when preventDefault is called. But I think I'm at the point where getting some feedback would be helpful.

I realise the spec isn't exactly clear on all of this, and we will likely need to compare more with Apple, Google, Opera, etc implementations in order to make these things correct, but wanted to keep moving here as well.

Main question I wanted feedback on relates to caching. Every touchevent has a touches, changedTouches, and targetTouches attribute. Changed reflects touches that are different in this event, and target is touches that were originally targetted at this target. In order to get things like changed and targetTouches correct, we need to cache information about the previous events somewhere. So I've added a gCaptureTouchList hashtable in presshell to do that.

I know this current implementation isn't perfect, but wanted feedback on the approach, plus any big hairy things you wanted to bring up.
Comment 13 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-21 09:32:35 PDT
(In reply to comment #12)
> I realise the spec isn't exactly clear on all of this, and we will likely
> need to compare more with Apple, Google, Opera, etc implementations in order
> to make these things correct, but wanted to keep moving here as well.
yeah, handling of various touch lists needs testing.


Looking at the patch now. Sorry for the delay.
Comment 14 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-06-21 10:06:22 PDT
Comment on attachment 540074 [details] [diff] [review]
WIP Patch

># HG changeset patch
># User Michael Wu <mwu@mozilla.com>
># Date 1304045693 25200
># Node ID a9b198946e19e516b933dc0761061b9351c666e1
># Parent c88622da09eee89044ba85eaeb54559be43916eb
>[mq]: mwu-multitouch
>
>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -569,6 +569,13 @@
>     { nsGkAtoms::onMozTouchMove,                NS_MOZTOUCH_MOVE, EventNameType_None, NS_MOZTOUCH_EVENT },
>     { nsGkAtoms::onMozTouchUp,                  NS_MOZTOUCH_UP, EventNameType_None, NS_MOZTOUCH_EVENT },
> 
>+    { nsGkAtoms::ontouchstart,                  NS_TOUCH_START, EventNameType_None, NS_TOUCH_EVENT },
>+    { nsGkAtoms::ontouchmove,                   NS_TOUCH_MOVE, EventNameType_None, NS_TOUCH_EVENT },
>+    { nsGkAtoms::ontouchend,                    NS_TOUCH_END, EventNameType_None, NS_TOUCH_EVENT },
>+    { nsGkAtoms::ontouchenter,                  NS_TOUCH_ENTER, EventNameType_None, NS_TOUCH_EVENT },
>+    { nsGkAtoms::ontouchleave,                  NS_TOUCH_LEAVE, EventNameType_None, NS_TOUCH_EVENT },
>+    { nsGkAtoms::ontouchcancel,                 NS_TOUCH_CANCEL, EventNameType_None, NS_TOUCH_EVENT },
>+

Why do you add there here?
touch events are added to the list using touchEventArray.
>+nsIntPoint
>+nsDOMTouch::GetPagePoint()
>+{
Could we actually calculate all the Get***Point values when creating touch.
I think that would match webkit's behavior.
(I know you've probably just copied old code from mouseevents)


>+    for (PRUint32 i = 0; i < touches.Length(); i++) {
>+      if (touches[i]->target == mEvent->target)
>+        targetTouches.AppendElement(touches[i]);
Coding style is
if (expr) {
  stmt;
}

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -909,7 +909,17 @@
I wish the patch would have been created with -U 8 -p

>   static nsPoint GetEventCoordinatesRelativeTo(const nsEvent* aEvent,
>                                                nsIFrame* aFrame);
> 
>+  static nsPoint GetEventCoordinatesRelativeTo(const nsIntPoint aPoint,
>+                                               nsIWidget* aWidget,
>+                                               nsIFrame* aFrame);
>+

Why do you need the version which takes nsIWidget?
Couldn't you get the widget from nsIFrame?


> #define NS_MOZTOUCH_EVENT_START      4400
>-#define NS_MOZTOUCH_DOWN             (NS_MOZTOUCH_EVENT_START)
>+#define NS_MOZTOUCH_UP               (NS_MOZTOUCH_EVENT_START)
> #define NS_MOZTOUCH_MOVE             (NS_MOZTOUCH_EVENT_START+1)
>-#define NS_MOZTOUCH_UP               (NS_MOZTOUCH_EVENT_START+2)
>+#define NS_MOZTOUCH_DOWN             (NS_MOZTOUCH_EVENT_START+2)
Why this change?


>+class nsTouch
>+{
>+public:
>+  nsTouch()
>+    : changed(PR_FALSE),
>+      target(nsnull)
>+  { }
>+  nsIntPoint point;
>+  int id;
>+  PRBool changed;
>+  float pressure;
>+  float orientation;
>+  nsIntPoint radius;
>+  nsCOMPtr<nsPIDOMEventTarget> target;
>+};
Please initialize member variables in the ctor


>+class nsTouchEvent : public nsInputEvent
>+{
>+public:
>+  nsTouchEvent(PRBool isTrusted, PRUint32 msg, nsIWidget* w)
>+    : nsInputEvent(isTrusted, msg, w, NS_TOUCH_EVENT),
>+      touches(nsnull)
>+  {
>+  }
>+  ~nsTouchEvent()
>+  {
>+    for (PRUint32 i = 0; i < touches.Length(); i++) {
>+      delete touches[i];
>+    }
>+    touches.Clear();
>+  }
>+
>+  nsTArray<nsTouch*> touches;
>+};
I wonder if you could use nsTArray<nsAutoPtr<nsTouch> > here.
Or, other option is to just have nsTArray<nsRefPtr<nsDOMTouch> >, if that is
possible.


But yeah, looks ok.
Comment 15 Wesley Johnston (:wesj) 2011-07-01 14:55:59 PDT
Created attachment 543521 [details] [diff] [review]
WIP v2

This doesn't compile if I remember right, but has some targetting fixed. Namely, I think touchdown events were originally only targetted at the document. I fixed that, and then after we call EventDispatcher->DispatchEvent I am trying to take the event->target and store it. When touchmove's happen I iterate through the array of target's we have seen and fire touchmove on all of them (this isn't working right) yet. On touchup, I find the original touchdown target for this touch and fire the events on that.

Last thing to do after that is just some checks I wanted to put it in case we have nsITouch's in our hash table that aren't being cleaned up. And most of the cleanup mentioned by Olli above.

I'm out next week, but will try to get back on this when I get back. Wanted to put this up in case someone else has time to work on it.
Comment 16 Wesley Johnston (:wesj) 2011-07-15 09:28:06 PDT
Marking dev-doc-needed so that MDC can track when this is finally done.

On the progress front, still digging through targeting. Apparently we target mouseevents by using the coordinates of the event itself. For multitouch, on touchstart I think I need to determine which is the new touch and use its coordinates for targeting.

I've already modified this so that on touchend, the widget code only sends us the touch that ended and we append (for the queue of active touches) everything else. I think I may do the same thing for touchdown so that I don't have to dig through the queue on every touchdown and figure out what is new.
Comment 17 Wesley Johnston (:wesj) 2011-07-26 19:03:54 PDT
Created attachment 548658 [details] [diff] [review]
Patch part 1/4

Everythings starting to come up milhouse here. This implements the platform backend for touchevents. Mostly done. Need to address the feedback comments and just do all around cleanup/testing. Happy to get more feedback if there is any!
Comment 18 Wesley Johnston (:wesj) 2011-07-26 19:04:49 PDT
Created attachment 548660 [details] [diff] [review]
Patch part 2/4 - Android backend

This is the Android code to get touchevents from widget to platform.
Comment 19 Wesley Johnston (:wesj) 2011-07-26 19:06:49 PDT
Created attachment 548661 [details] [diff] [review]
Patch part 3/4 - Mobile frontend

This attaches to the mobile front to (currently) take touchevents and redispatch GestureEvents. Its directly copied from the old platform code, but needs a few fixes to deal with cancelling mouseevents fired by the initial mousedown.

This is still not forwarding these events to content. That would be Part 4/4... or we can put that in a separate bug.
Comment 20 Kevin Brosnan [:kbrosnan] 2011-07-29 17:40:56 PDT
This was tracking-fennec 8+, was nuked when it was moved into beos for some reason.
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2011-07-29 17:42:33 PDT
re-assigning to wes, and re-marking it as tracking 8+
Comment 22 Wesley Johnston (:wesj) 2011-08-03 09:57:24 PDT
Adding depends on bug 676275 (support newer sdks) since there are newer multitouch apis (off the top of my head i remember orientation of touches, as well as separate x and y radius stuff) in new sdks that would be useful.
Comment 23 Wesley Johnston (:wesj) 2011-08-04 16:15:29 PDT
Created attachment 550873 [details] [diff] [review]
Patch v1 - 1/4 -> Platform changes

Addresses feedback comments. Ready for first round of review (i hope!)
Comment 24 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-08-05 10:19:49 PDT
Comment on attachment 550873 [details] [diff] [review]
Patch v1 - 1/4 -> Platform changes

Could you investigate if just having nsDOMTouch could be possible.
It is a bit ugly to copy all the data from nsTouch to nsDOMTouch.
Comment 25 Oleg Romashin (:romaxa) 2011-09-08 00:33:44 PDT
btw, do we handle here multitarget case? for example 2 divs each one touched in multitouch case... and in that case we could have both divs moved at the same time?
Comment 26 Wesley Johnston (:wesj) 2011-10-26 11:08:26 PDT
Created attachment 569731 [details] [diff] [review]
Unbitrotted WIP platform and widget stuff

Sorry. Have been busy and haven't had time to put into this. Sounds like someone from B2G may be able to grab this?

I had removed all of my nsTouch stuff and had this compiling. Was then crashing whenever you touched the screen (it looked like when I tried to get the id of the touch).

This is very very very quickly unbitrotted and checked to make sure it compiled. Not installed and tested at all, but wanted to put it up if someone else was going to grab this. It includes both platform and widget stuff because i accidentally intermixed the two and haven't had a chance to pull them apart again.
Comment 27 Wesley Johnston (:wesj) 2011-11-03 14:54:21 PDT
Created attachment 571790 [details] [diff] [review]
Patch v2

So this seems to work fairly well (for me) passes the single and multi-touch tests up for the charter (although they're very simple, and we do fail one for not having a relatedTarget attribute on these?), works with a few simple demos I found around the web that aren't to webkit specific to be useless (including jquery mobile ones). 

Unfortunately, it doesn't pan Google Maps. (lots of "JavaScript Error: "a.target is null" {file: "http://maps.gstatic.com/cat_js/intl/en_us/mapfiles/378b/maps2/%7Bmain,mod_util,mod_mmh,mod_mmpc,mod_mobpnl,mod_rst%7D.js" line: 1948}" errors). So I'm going to plug away at it for another day. Get rid of some of the useless logging I have scattered around. Try to add some more comments so its all not too mysterious, and write some more tests to make sure mouse events are being prevented correctly, etc. Hopefully not have to dig into the maps source to hard.

Olli, if you want to start glancing at this, I'm hoping to have it up for review... soon.

For anyone who wants to play with it, Native UI build is at http://people.mozilla.com/~wjohnston/fennec_touch.apk
Comment 28 Wesley Johnston (:wesj) 2011-11-04 15:34:09 PDT
Created attachment 572107 [details] [diff] [review]
Patch 1/2 Platform stuff

So my problem with GoogleMaps turned out to be that I wasn't setting the target of each touch correctly. Now fixed (here and in the touchevents spec) along with a few other little quirks. Google Maps pans well. Bing supports two finger pinch zoom and works well (except requiring a UA tweak to even get to it...). All the other demos and tests I've managed to fine seem to work.

I moved mTarget to be a WeakReference, so that if the page removes the node while the touch is in progress we just start returning nothing? We'll also maybe hit problems dispatching I expect... Not sure that's right there. Also not sure what to do with the NS_IMPL_CYCLE_COLLECTION stuff at the top of nsDOMTouchEvent.cpp.
Comment 29 Wesley Johnston (:wesj) 2011-11-04 15:35:06 PDT
Created attachment 572110 [details] [diff] [review]
Patch 1/2 Platform stuff

Forgot to qref... doh.
Comment 30 Wesley Johnston (:wesj) 2011-11-04 15:36:39 PDT
Created attachment 572111 [details] [diff] [review]
Patch 2/2 - Android widget stuff

Figured I can do these in parallel, but if Olli has changes on the platform side this may need to change.
Comment 31 Wesley Johnston (:wesj) 2011-11-04 15:38:04 PDT
Sorry for the bug spam, but also though I'd note that this removes our support for MozGesture (for now). The frontend patch I have up reimplements that in JS. I figured we can do that if we want it back, but happy to hear better ideas.
Comment 32 Wesley Johnston (:wesj) 2011-11-04 17:33:33 PDT
Created attachment 572138 [details] [diff] [review]
Patch 1/2 Platform stuff v2

Removed the weakptrs. Quickly. Feel free to tell me I did all of this horribly wrong. I have a test page for that I'll put up here for fun too!
Comment 33 Wesley Johnston (:wesj) 2011-11-04 17:34:42 PDT
Oh... probably easier to just post the link if someone was interested:

http://dl.dropbox.com/u/72157/touchremove.html
Comment 34 Brad Lassey [:blassey] (use needinfo?) 2011-11-04 22:01:34 PDT
Comment on attachment 572111 [details] [diff] [review]
Patch 2/2 - Android widget stuff

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

::: embedding/android/GeckoEvent.java
@@ +187,5 @@
> +            }
> +        }
> +    }
> +
> +    public void addMotionPoint(int i, int j, MotionEvent event) {

nit, use more descriptive variable names than i and j

@@ +192,5 @@
> +        mPoints[i] = new Point((int)event.getX(j), (int)event.getY(j));
> +        mPointIndicies[i] = event.getPointerId(j);
> +        // getToolMajor, getToolMinor and getOrientation are API Level 8 features
> +
> +        if (Build.VERSION.SDK_INT >= 9) {

if they're level 8 features, then you should test for 8, not 9

@@ +194,5 @@
> +        // getToolMajor, getToolMinor and getOrientation are API Level 8 features
> +
> +        if (Build.VERSION.SDK_INT >= 9) {
> +            mPointRadii[i] = new Point((int)event.getToolMajor(j),
> +                                       (int)event.getToolMinor(j));

this seems a bit odd to me. The major and minor lengths may not be on the x or y axis. In order to translate into x and y coords, you'll have to take tilt into account

Not sure what the right thing to do is though, where is this information used? Is it used in x,y coords? or do we just get a radius in the end?

::: widget/src/android/nsWindow.cpp
@@ +802,5 @@
> +            nsIntPoint point(0,0);
> +            nsTArray<nsIntPoint> points = ae->Points();
> +            if (points.Length() > 0) {
> +                point = points[0];
> +            }

AndroidGeckoEvent.cpp ensures there are 2 points for a SIZE_CHANGED event, right? So let's just have an assertion in debug builds and skip this check for release. So, just:

nsIntPoint point = ae->Points()[0];

@@ +825,5 @@
> +            if (points.Length() > 1) {
> +                point = points[1];
> +            }
> +            int newScreenWidth = point.x;
> +            int newScreenHeight = point.y;

same as above

@@ +860,4 @@
>                  else
>                      obs->RemoveObserver(notifier, "ipc:content-created");
>              }
> +            break;

this is a fairly significant change. Is the missing break a bug? or was this intentional?

@@ +869,5 @@
> +                nsIntPoint pt(0,0);
> +                nsTArray<nsIntPoint> points = ae->Points();
> +                if (points.Length() > 0) {
> +                    pt = points[0];
> +                }

a motion event is guaranteed to have at least one point, please just use a debug assert here

@@ +884,3 @@
>                  if (target) {
> +                    target->OnMultitouchEvent(ae);
> +                    if (ae->Count() < 2)

A motion event can have at most 2 points right? So why test for that?
Comment 35 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-05 05:34:14 PDT
(In reply to Wesley Johnston (:wesj) from comment #31)
> Sorry for the bug spam, but also though I'd note that this removes our
> support for MozGesture (for now).
On Mobile? I hope this doesn't remove support for gesture events on OSX/Win7.
Comment 36 Wesley Johnston (:wesj) 2011-11-05 08:00:45 PDT
(In reply to Olli Pettay [:smaug] from comment #35)
> (In reply to Wesley Johnston (:wesj) from comment #31)
> > Sorry for the bug spam, but also though I'd note that this removes our
> > support for MozGesture (for now).
> On Mobile? I hope this doesn't remove support for gesture events on OSX/Win7.

Oh yeah. Didn't mean to scare anyone. Removes gesture events on Android only (and only for now, we'll probably have to re add them for things like pinch zoom).
Comment 37 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-11 13:57:00 PST
Comment on attachment 572138 [details] [diff] [review]
Patch 1/2 Platform stuff v2

> TOUCH_EVENT(touchstart,
>-            NS_USER_DEFINED_EVENT,
>+            NS_TOUCH_START,
>             EventNameType_All,
>-            NS_INPUT_EVENT)
>+            NS_TOUCH_EVENT)
> TOUCH_EVENT(touchend,
>-            NS_USER_DEFINED_EVENT,
>-            EventNameType_All,
>+            NS_TOUCH_END,
>+            NS_TOUCH_EVENT,
>             NS_INPUT_EVENT)
Hmm, something wrong with this.
You remove EventNameType_All and put back NS_TOUCH_EVENT?

>+nsIntPoint
>+nsDOMTouch::GetClientPoint(nsPresContext* aPresContext, nsIntPoint aPoint)
>+{
>+  if (!aPresContext) {
>+    return mClientPoint;
>+  }
>+  nsPoint pt(0, 0);
>+  nsIPresShell* shell = aPresContext->GetPresShell();
>+  if (!shell) {
>+    return nsIntPoint(0, 0);
>+  }
You could define pt after the |if|


>+  nsIFrame* rootFrame = shell->GetRootFrame();
>+  if (rootFrame)
>+    pt = nsLayoutUtils::GetEventCoordinatesRelativeTo(aPoint, rootFrame);
if (expr) {
  stmt;
}


>+nsDOMTouchList::nsDOMTouchList(nsCOMArray<nsIDOMTouch> &aTouches)
>+{
>+  for (PRInt32 i = 0; i < aTouches.Count(); i++) {
>+    mPoints.AppendObject(aTouches[i]);
>+  }
  mPoint.AppendObjects(aTouches); ?


>@@ -273,22 +328,73 @@
> NS_IMETHODIMP
> nsDOMTouchEvent::GetTouches(nsIDOMTouchList** aTouches)
> {
>-  NS_IF_ADDREF(*aTouches = mTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aTouches);
>+  nsRefPtr<nsDOMTouchList> t;
>+  if (!mTouches && mEvent) {
>+    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+    if (mEvent->message == NS_TOUCH_END || mEvent->message == NS_TOUCH_CANCEL) {
>+      // for touchend events, remove any changed touches from the touches array
>+      nsCOMArray<nsIDOMTouch> unchangedTouches;
>+      nsCOMArray<nsIDOMTouch> touches = touchEvent->touches;
>+      for (PRUint32 i = 0; i < touches.Count(); i++) {
>+       if (!touches[i]->changed)
>+        unchangedTouches.AppendObject(touches[i]);
>+      }
Missing a space before |if| and the next line.
if (expr) {
  stmt;
}


> NS_IMETHODIMP
> nsDOMTouchEvent::GetTargetTouches(nsIDOMTouchList** aTargetTouches)
> {
>-  NS_IF_ADDREF(*aTargetTouches = mTargetTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aTargetTouches);
>+  if (!mTargetTouches && mEvent) {
>+    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+    nsCOMArray<nsIDOMTouch> targetTouches;
>+    nsCOMArray<nsIDOMTouch> touches = touchEvent->touches;
>+    for (PRUint32 i = 0; i < touches.Count(); i++) {
I'd prefer ++i;
Same also elsewhere in the patch.


>+      // for touchend/cancel events, don't append to the target list if this is a
>+      // touch that is ending
>+      if (( mEvent->message != NS_TOUCH_END &&
Extra space after ((


>+            mEvent->message != NS_TOUCH_CANCEL )
Extra space before )

>+          || !touches[i]->changed) {
>+
>+        nsCOMPtr<nsPIDOMEventTarget> targetPtr;
nsIDOMEventTarget

>+        touches[i]->GetTarget(getter_AddRefs(targetPtr));
I wonder if nsIDOMTouch could have some C++ method which returns target without addrefing.

>+
>+        if (targetPtr == mEvent->target) {
>+          targetTouches.AppendObject(touches[i]);
>+        }
>+      }
>+    }
>+    nsRefPtr<nsDOMTouchList> t = new nsDOMTouchList(targetTouches);
>+    mTargetTouches = t.forget().get();
>+  }
>+  return CallQueryInterface(mTargetTouches, aTargetTouches);
> }


> nsDOMTouchEvent::GetChangedTouches(nsIDOMTouchList** aChangedTouches)
> {
>-  NS_IF_ADDREF(*aChangedTouches = mChangedTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aChangedTouches);
>+  if (!mChangedTouches && mEvent) {
>+    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+    nsCOMArray<nsIDOMTouch> changedTouches;
>+    nsCOMArray<nsIDOMTouch> touches = touchEvent->touches;
>+    for (PRUint32 i = 0; i < touches.Count(); i++) {
>+       if (touches[i]->changed)
>+        changedTouches.AppendObject(touches[i]);
if (expr) {
  stmt;
}

>+[scriptable, builtinclass, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
Thank you!

>+++ b/layout/base/nsLayoutUtils.cpp
>@@ -932,11 +932,20 @@
>                   aEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT &&
>                   aEvent->eventStructType != NS_GESTURENOTIFY_EVENT &&
>                   aEvent->eventStructType != NS_MOZTOUCH_EVENT &&
>+                  aEvent->eventStructType != NS_TOUCH_EVENT &&
>                   aEvent->eventStructType != NS_QUERY_CONTENT_EVENT))
>     return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
> 
>   const nsGUIEvent* GUIEvent = static_cast<const nsGUIEvent*>(aEvent);
>-  if (!GUIEvent->widget)
I don't understand this change. Is it really ok to not have !GUIEvent->widget check?


>@@ -5940,8 +5953,51 @@
I'm missing the context here. is the patch not created with -P ?


>+          // Apparently we're adding touches to the queue without removing them
>+          // and LEAKING. Touches should be removed on touchend
When can we leak?



>+static PLDHashOperator
>+EvictOldTouchPoints(const PRUint32& aKey, nsCOMPtr<nsIDOMTouch>& aData, void *userArg)
>+{
>+  PRTime now = PR_Now();
>+  if (aData->timestamp - now > 10000) {
>+    return PL_DHASH_REMOVE;
>+  }
>+  return PL_DHASH_NEXT;
>+}
what on earth is this?


>+    if (gCaptureTouchList.Count() > 20) {
>+      // throw away captured touch points that are older than one minute
>+      gCaptureTouchList.Enumerate(&EvictOldTouchPoints, nsnull);
>+    }
Doesn't make sense. Can we not get right events from OS or is the implementation buggy to
not releasing old touch points? Can we get some event from OS when there are no touches and
release everything at that point?


>+
>+        // touch events should fire on all targets
>+        // XXX - Should/can we just iterate over changed touches here?
That sounds right. Please check what the spec says or what other implementation do.
I'm pretty sure the spec isn't clear enough in this case.


>+        if (aEvent->message == NS_TOUCH_MOVE) {
>+          nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
>+          nsCOMArray<nsIDOMTouch> touches = touchEvent->touches;
>+          for (PRUint32 i = 0; i < touches.Count(); i++) {
>+            nsDOMTouch *touch = static_cast<nsDOMTouch*>(touches[i]);
>+            if (touch) {
>+              nsCOMPtr<nsPIDOMEventTarget> targetPtr;
>+              touch->GetTarget(getter_AddRefs(targetPtr));
>+              if (targetPtr) {
>+                aEvent->target = targetPtr;
>+                nsEventDispatcher::Dispatch(targetPtr, mPresContext,
>+                                            aEvent, nsnull, aStatus, &eventCB);
>+              }
>+            }
>+          }
>+        }
You can't really dispatch the same aEvent multiple times, since some call (preventDefault() for example) may have changed its flags.
So, you need to copy it for each touch.




>+        else if (mCurrentEventContent) {
>+          // touchevents need to have the target attribute set on each touch
>+          if (aEvent->message == NS_TOUCH_START) {
>+            nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
>+            nsCOMArray<nsIDOMTouch> touches = touchEvent->touches;
>+            for (PRUint32 i = 0; i < touches.Count(); i++) {
>+              nsDOMTouch *touch = static_cast<nsDOMTouch*>(touches[i]);
>+              if (touch->changed) {
>+                touch->SetTarget(mCurrentEventContent);
>+              }
>+            }
>+          }
Might be nice to move this touch event handling to its own method and modify the generic code path
as little as possible.

>+      // on touch start, we store the target for future touchmove dispatching
>+      if (nsEventStatus_eConsumeNoDefault == *aStatus &&
>+          ( aEvent->message == NS_TOUCH_START ||
>+           ( aEvent->message == NS_TOUCH_MOVE && touchIsNew) )) {
extra spaces after ( and before )


>+        // calling preventDefault on touchstart or the first touchmove for a
>+        // point prevents mouse events 
The "first" part is insane, but that is what other implementations do, IIRC, and
what the spec says.


>+        gPreventMouseEvents = true;
>+      }
>+
>       nsContentUtils::SetIsHandlingKeyBoardEvent(wasHandlingKeyBoardEvent);
> 
>       // 3. Give event to event manager for post event state changes and
>@@ -6457,6 +6658,30 @@
>   }
>   return rv;
> }
>+ 
>+PRBool
bool

>+PresShell::TouchChanged(nsIDOMTouch* aTouch1, nsIDOMTouch* aTouch2) {
{ should be in the next line
Could the method name be something like "IsEqualTouch".


>+  ~nsTouchEvent()
>+  {
>+    touches.Clear(); 
Why is Clear() needed? It should happen after the dtor has run.
Could you add MOZ_COUNT_CTOR/DTOR to nsTouchEvent


Looking pretty good. Memory handling needs to be fixed. No strange timing if just possible.
Comment 38 Wesley Johnston (:wesj) 2011-11-11 17:29:12 PST
Created attachment 573972 [details] [diff] [review]
WIP Widget

This is a WIP updated to build against the new birch stuff and address blassey's comments. Sorta works to prevent panning. I added a timeout on the first action_move event. If the content process doesn't answer in time, we'll allow panning. Timeout is pretty fast (100ms) right now, and in the mean time we still allow events to go the the PanZoomController. It just doesn't do anything. I think that means that when those 100ms are up we move as if you had never prevented anything to begin with. Seems to work well here.

Needs cleanup. Probably broke some important things like clicking. I will probably prioritize the platform side over this for now though because it involves changes to Gecko we will need in for the platform freeze.
Comment 39 Wesley Johnston (:wesj) 2011-11-14 18:13:00 PST
Created attachment 574498 [details] [diff] [review]
WIP

Putting this up because I promised mwu I'd get him something. This applies cleanly on birch and mozilla-central for me.

Fixed most of the nits. Still trying to decide/figure out what to do about the memory issues. And writing some tests.

I'm not sure that this will help with panning in B2G. Certainly you can use touchevents for panning, and being able to detect multitouch will let you determine whether to pan or pinch zoom. But Fennec did the same thing formerly using mouse events + a filter in widget/src/android/nsWindow.cpp to NOT send mouse events when multitouch was happening.
Comment 40 Michael Wu [:mwu] 2011-11-15 11:27:13 PST
BTW, PRBool/PR_TRUE/PR_FALSE are all deprecated and should be removed from new patches.
Comment 41 Michael Wu [:mwu] 2011-11-15 11:51:45 PST
Also, nsCOMArray is deprecated. I suspect you want a nsTArray combined with a nsRefPtr to the concrete class.
Comment 42 Wesley Johnston (:wesj) 2011-11-17 17:19:25 PST
Created attachment 575343 [details] [diff] [review]
WIP

No idea why, but I have some obsession with uploading these WIP Patches. This implements a whole lot of the changes requested from mwu and smaug. Need to make another few passes through to make sure I've gotten everything. Maybe look at using nsTArray<nsDOMTouch> rather than the super awesome nsTArray<nsCOMPtr<nsIDOMTouch>> I've got now. It also adds a bunch of tests. I'll probably pull those into their own file for review. Still rebasing the widget code to test this with.

One thing I realized writing the tests is that the way I am preventing mouse events on preventDefault is a bit funny. If you call preventDefault, presShell will start eating all mouse events until the next touchstart fires. That means, if for some reason another touchstart never fires (maybe you switch to kbm mode on a desktop?) we won't fire any mouse events. I'd like to just eat them until touchend, but we also need to fire the final mouseup (which can also cause a click to fire) after we've fired touchend.

I think the right solution is to move the prevent logic into widget? We can return a status on the touch events that says basically, "Don't fire anything else related to this event." I'll have to preserve that state in presShell so that calling preventDefault on just touchstart (or first touchmove) will continue to return that status flag for all touchevents until (the last?) touchend.

I don't like depending on a correct widget implementation to match the spec, but I'm not sure if there's a better way?
Comment 43 Michael Wu [:mwu] 2011-11-17 17:22:30 PST
(In reply to Wesley Johnston (:wesj) from comment #42)
> I don't like depending on a correct widget implementation to match the spec,
> but I'm not sure if there's a better way?

It's already a problem, so you wouldn't be making it any worse. We already require preventDefault to be explicitly handled to determine whether a keypress should be fired after a keydown.

Of course, it would be nice if widgets didn't need to have to work so hard.
Comment 44 Wesley Johnston (:wesj) 2011-11-17 17:48:26 PST
Created attachment 575349 [details] [diff] [review]
WIP2

Whoops. Trying to juggle between birch and mc apparently fails me. This is the right patch (I hope).
Comment 45 Michael Wu [:mwu] 2011-11-18 10:43:44 PST
Comment on attachment 575349 [details] [diff] [review]
WIP2

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

Just making a few comments here since I have to look at the patch anyway. All minor things. De Morgan's law helps minimize indentation levels.

::: content/events/src/nsDOMTouchEvent.cpp
@@ +260,5 @@
>  {
>    if (aEvent) {
>      mEventIsInternal = false;
> +
> +    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);

Isn't aEvent already a nsTouchEvent*?

@@ +337,5 @@
>  nsDOMTouchEvent::GetTouches(nsIDOMTouchList** aTouches)
>  {
> +  NS_ENSURE_ARG_POINTER(aTouches);
> +  nsRefPtr<nsDOMTouchList> t;
> +  if (!mTouches && mEvent) {

if (mTouches || !mEvent)
  return CallQueryInterface(mTouches, aTouches);

Same for the other two functions below.

@@ +352,5 @@
> +      t = new nsDOMTouchList(unchangedTouches);
> +    } else {
> +      t = new nsDOMTouchList(touchEvent->touches);
> +    }
> +    mTouches = t.forget().get();

I suspect you can set mTouches directly. Similar things apply to the next two functions.

::: content/events/src/nsDOMTouchEvent.h
@@ +62,5 @@
> +      mIdentifier = aIdentifier;
> +      mPagePoint = nsIntPoint(aPageX, aPageY);
> +      mScreenPoint = nsIntPoint(aScreenX, aScreenY);
> +      mClientPoint = nsIntPoint(aClientX, aClientY);
> +      mRefPoint = nsIntPoint(0,0);

Space after the comma.

@@ +97,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_CLASS(nsDOMTouch)
>    NS_DECL_NSIDOMTOUCH
> +  void InitializePoints(nsPresContext* aPresContext, nsEvent* aEvent) {
> +    if (!mPointsInitialized) {

if (mPointsInitialized)
  return;

::: dom/base/nsDOMWindowUtils.cpp
@@ +591,5 @@
> +  nsTouchEvent event(true, msg, widget);
> +  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
> +  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
> +  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
> +  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;

We actually don't need to do the ? true : false; dance these days - the bool type take care of that for us, though I hear MSVC gets upset if you aren't explicit. Either way, the standard way to convert to bool is !! if you don't have an actual bool type.

@@ +602,5 @@
> +
> +  event.touches.SetCapacity(count);
> +  PRInt32 appPerDev = presContext->AppUnitsPerDevPixel();
> +  for (int i = 0; i < count; ++i) {
> +    nsIntPoint pt(0,0);

space after the comma.

::: layout/base/nsPresShell.cpp
@@ +5736,5 @@
> +{
> +  nsIWidget *widget = nsnull;
> +  // is there an easier/better way to dig out the widget?
> +  nsCOMPtr<nsINode> node(do_QueryInterface(aTouch->GetTarget()));
> +  if (node) {

if (!node)
  return PL_DHASH_NEXT;

@@ +5738,5 @@
> +  // is there an easier/better way to dig out the widget?
> +  nsCOMPtr<nsINode> node(do_QueryInterface(aTouch->GetTarget()));
> +  if (node) {
> +    nsIDocument* doc = node->GetCurrentDoc();
> +    if (doc) {

if (!doc)
  return PL_DHASH_NEXT;

and so on

@@ +5758,5 @@
> +    event.isAlt = false;
> +    event.isMeta = false;
> +    event.widget = widget;
> +    event.time = PR_IntervalNow();
> +    event.touches.SetCapacity(1);

AppendElement automatically makes the array bigger if necessary.

@@ +6547,5 @@
> +        bool haveChanged = false;
> +        for (PRUint32 i = 0; i < touches.Length(); ++i) {
> +          nsIDOMTouch *touch = touches[i];
> +          nsDOMTouch *domtouch = static_cast<nsDOMTouch*>(touch);
> +          if (touch) {

if (!touch)
  continue;

@@ +6554,5 @@
> +            touch->message = aEvent->message;
> +
> +            nsCOMPtr<nsIDOMTouch> oldTouch;
> +            gCaptureTouchList.Get(id, getter_AddRefs(oldTouch));
> +            if (oldTouch) {

if (!oldTouch)
  continue;

@@ +6700,5 @@
> +    nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent);
> +    nsTArray<nsCOMPtr<nsIDOMTouch>> touches = touchEvent->touches;
> +    for (PRUint32 i = 0; i < touches.Length(); ++i) {
> +      nsIDOMTouch *touch = touches[i];
> +      if (touch && touch->changed) {

if (!touch || !touch->changed)
  continue;

@@ +6704,5 @@
> +      if (touch && touch->changed) {
> +        // copy the event
> +        nsCOMPtr<nsPIDOMEventTarget> targetPtr;
> +        touch->GetTarget(getter_AddRefs(targetPtr));
> +        if (targetPtr) {

if (!targetPtr)
  continue;

::: widget/public/nsGUIEvent.h
@@ +1541,5 @@
> +class nsTouchEvent : public nsInputEvent
> +{
> +public:
> +  nsTouchEvent(nsTouchEvent *aEvent)
> +    :nsInputEvent(aEvent->flags & NS_EVENT_FLAG_TRUSTED ? true : false,

!! or nothing
Comment 46 Wesley Johnston (:wesj) 2011-12-01 02:13:34 PST
Created attachment 578211 [details] [diff] [review]
Patch v2

I think this has all the changes requested by smaug and mwu as well as the new tests.

I removed the yelling when we get to many touches. Instead, if widget sends a touchstart event with a touch point in it, but there are still touch points in our queue, I'm firing touchend events for all the points in the queue (maybe that should be touchcancel?), and sending an NS_WARNING. 

I also removed the code the prevents mouse events and instead I'm just sending a status of nsEventStatus_eConsumeNoDefault to widget iff preventDefault was called on touchstart/first touchmove for all subsequent touches until the final touchend in this set.

Still working on a revised version of our widget code.
Comment 47 Wesley Johnston (:wesj) 2011-12-06 09:16:53 PST
Created attachment 579330 [details] [diff] [review]
WIP Widget

Saving my place.

This works fairly well though. I noticed that at times I am not getting touch events in nsWindow.cpp until several seconds after they are dispatched. Some digging shows that's because we aren't "coalescing" paint events any more, which also means we aren't coalescing most touchmove events (since we usually have move-draw-move-draw-move-draw and we had special code in place to coalesce the two). Not getting the events means we can't tell if the page called prevent default, and we'll occasionally allow panning for a few seconds before suddenly stopping it (I'm not sure if the page should snap back at that point or what). Rummer is someone has a patch to return the coalescing behavior.

I also started putting in a notification to be fired when a page registers touch event listeners. Hopefully we can use it and only introduce this (potential) panning delay on pages that need it.
Comment 48 Wesley Johnston (:wesj) 2011-12-07 09:03:23 PST
Created attachment 579712 [details] [diff] [review]
Widget Part 1/3

Splitting this out into pieces for review. Works well except that I've killed both clicking and doubletap zoom. Hopefully I can clear those up and put this up tomorrow.
Comment 49 Wesley Johnston (:wesj) 2011-12-07 09:04:46 PST
Created attachment 579713 [details] [diff] [review]
Widget Part 2/3 - Prevent panning/clicking/etc. in java

This is the part that prevents panning/clicking/doubletap/etc if the page calls preventDefault.
Comment 50 Wesley Johnston (:wesj) 2011-12-07 09:06:03 PST
Created attachment 579715 [details] [diff] [review]
Widget Part 3/3 - Look for touch listeners

This part creates a notification if the page has touch listeners, and uses it to decide whether or not to wait for touch events before panning.
Comment 51 Wesley Johnston (:wesj) 2011-12-08 12:01:56 PST
Created attachment 580134 [details] [diff] [review]
Widget Patch 1/3 - Dispatching touch events

These patches together work well for me, but I'm seeing crashes in XUL Fennec, with not much useful info in the logcat. Need to build debug.
Comment 52 Wesley Johnston (:wesj) 2011-12-08 12:03:38 PST
Created attachment 580135 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

Prevent default stuff. Was planning to get a review from you katz. Before I do, maybe you can give me any feedback you have on the general idea? This is a bit racy...
Comment 53 Wesley Johnston (:wesj) 2011-12-08 12:04:15 PST
Created attachment 580136 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners
Comment 54 Wesley Johnston (:wesj) 2011-12-08 12:26:50 PST
Created attachment 580141 [details] [diff] [review]
Platform Patch

Whoops. Did I accidentally remove my last patch here? This is the platform bit. Has a few fixes for some little glitches I found writing widget stuff, but mostly intact.
Comment 55 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-08 12:53:27 PST
Comment on attachment 580135 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

In general this approach seems kind of brittle to me. I'd prefer doing it the other way. In LayerController, intercept touch events coming from LayerView, and when you see a touch-down, send it to gecko and start queuing up additional touch events coming in from the LayerView until either (a) the timeout expires or (b) you get a preventPanning() call from nsWindow. If (a) happens, then continue with the normal processing of the touch events, and if (b) happens, then only send the events to gecko.

The way you're doing it now seems more brittle because it slices into PanZoomController and allows partial execution of that code, which is likely going to break hidden assumptions in the PZC code and lead to hard-to-find bugs.

The approach I'm suggesting is (I think) cleaner, although it might require some rearranging of the onTouchEvent functions in LayerController and LayerView to make sure that queuing/resuming of events is possible there. What do you think?
Comment 56 Wesley Johnston (:wesj) 2011-12-08 14:31:12 PST
Comment on attachment 580134 [details] [diff] [review]
Widget Patch 1/3 - Dispatching touch events

Sending the review to blassey so he can start looking at it while I clean up this XUL-fennec crash.
Comment 57 Brad Lassey [:blassey] (use needinfo?) 2011-12-08 20:47:31 PST
Comment on attachment 580134 [details] [diff] [review]
Widget Patch 1/3 - Dispatching touch events

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

::: embedding/android/GeckoEvent.java
@@ +95,5 @@
>      public static final int IME_RANGE_UNDERLINE = 1;
>      public static final int IME_RANGE_FORECOLOR = 2;
>      public static final int IME_RANGE_BACKCOLOR = 4;
>  
> +    public static final float RADIANS_TO_DEGREES = (float)(180.0/Math.PI);

shouldn't be needed, we have Math.toDegrees() available

@@ +193,5 @@
> +        mPoints[index] = new Point((int)Math.round(geckoPoint.x), (int)Math.round(geckoPoint.y));
> +        mPointIndicies[index] = event.getPointerId(eventIndex);
> +        // getToolMajor, getToolMinor and getOrientation are API Level 9 features
> +        if (Build.VERSION.SDK_INT >= 9) {
> +            mOrientations[index] = event.getOrientation(eventIndex)*RADIANS_TO_DEGREES;

use Math.toDegrees()
http://developer.android.com/reference/java/lang/Math.html#toDegrees%28double%29

comments go for both GeckoEvent.java's

::: widget/src/android/Makefile.in
@@ +56,5 @@
>  endif
>  
> +ifdef MOZ_ONLY_TOUCH_EVENTS
> +DEFINES += -DMOZ_ONLY_TOUCH_EVENTS
> +endif

use ACDEFINE() in configure.in so you don't have to do this

::: widget/src/android/nsWindow.cpp
@@ +898,5 @@
> +            nsTArray<nsIntPoint> points = ae->Points();
> +            point = points[0];
> +
> +            int nw = point.x;
> +            int nh = point.y;

I think I'd rather you just use points[0].x and points[0].y here

@@ +917,5 @@
>              }
>  
> +            point = points[1];
> +            int newScreenWidth = point.x;
> +            int newScreenHeight = point.y;

and points[1].x and points[1].y here

@@ +981,5 @@
> +                    if (!preventDefaultActions && ae->Count() == 2) {
> +                        target->OnGestureEvent(ae);
> +                    }
> +#ifndef MOZ_ONLY_TOUCH_EVENTS
> +                    printf_stderr("wesj send mouse events?");

get rid of this
Comment 58 Wesley Johnston (:wesj) 2011-12-09 04:12:44 PST
Created attachment 580361 [details] [diff] [review]
Widget Patch 1/3 - Dispatching touch events

Updated for comments. Carrying review
Comment 59 Wesley Johnston (:wesj) 2011-12-09 04:15:38 PST
Created attachment 580362 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

I like the idea kats, and so I implemented it!

We have to copy the motion events in order to keep android from overwriting them each time around. Otherwise this seems to work fine.
Comment 60 Wesley Johnston (:wesj) 2011-12-09 04:17:13 PST
Created attachment 580363 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

This keeps us from doing any of this stuff on pages that have no registerd touch listeners. Asking for review from mfinkle for the fennec-ish stuff, and smaug for the touch event listener notification.
Comment 61 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-09 05:32:52 PST
Comment on attachment 580362 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

This patch appears to be the same as the last one?
Comment 62 Wesley Johnston (:wesj) 2011-12-09 05:39:40 PST
Created attachment 580383 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

Crud. Got my birch and mc patches confused. Thanks.

Sorry for the bugspam, but I'll post updated versions of the other patches as well.
Comment 63 Wesley Johnston (:wesj) 2011-12-09 05:40:38 PST
Created attachment 580384 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners
Comment 64 Wesley Johnston (:wesj) 2011-12-09 05:41:30 PST
Created attachment 580385 [details] [diff] [review]
Widget Patch 1/3 - Dispatching touch events
Comment 65 Wesley Johnston (:wesj) 2011-12-09 05:42:11 PST
Created attachment 580386 [details] [diff] [review]
Platform Patch
Comment 66 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-09 07:04:02 PST
Comment on attachment 580383 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

Overall pretty good, but r- for the threading issue below.

>+++ b/mobile/android/base/gfx/LayerController.java

>+import org.mozilla.gecko.GeckoAppShell;
>+import org.mozilla.gecko.GeckoEvent;
>+import java.util.Timer;
>+import java.util.TimerTask;
>+import java.util.Date;

GeckoAppShell and Date imports not needed.

>-        if (mPanZoomController.onTouchEvent(event))
>-            return true;
>+        int action = event.getAction();
>+        switch (action & MotionEvent.ACTION_MASK) {
>+            case MotionEvent.ACTION_DOWN: {
>+                preventPanning(true);
>+            }
>+        }

You could use an if instead of a switch here, unless you anticipate needing to add more cases (which I don't think will happen because this is based on the touch events spec).

>+            case MotionEvent.ACTION_MOVE: {
>+                if (!inTouchSession && allowDefaultTimer == null) {
>+                    inTouchSession = true;
>+                    allowDefaultTimer = new Timer();
>+                    allowDefaultTimer.schedule(new TimerTask() {
>+                        public void run() {
>+                            preventPanning(false);
>+                        }
>+                    }, PREVENT_DEFAULT_TIMEOUT);

The preventPanning call should always happen on the UI thread or you're gonna end up with races/unsynchronized access. The timer will operate on another background thread, so you should post another runnable back to the UI thread to do the preventPanning call, or use something like the GeckoAsyncTask with a Thread.sleep() in the doInBackground(), or something else.

>+            case MotionEvent.ACTION_UP: {
>+                inTouchSession = false;
>+            }

This should also happen on a touch cancel event for robustness. Also, I'm not sure what the spec says about multitouch, but you probably need a check here for getPointerCount() == 1 to avoid terminating the session too early. Actually the same applies for the ACTION_DOWN case above, I think you want to not do anything with that if getPointerCount() > 1.

>+        }
>+        return !allowDefaultActions;
>+    }
>+
>+    private boolean allowDefaultActions = true;
>+    private Timer allowDefaultTimer =  null;
>+    private boolean inTouchSession = false;

I'd prefer if these were moved up with the other declarations at the top of the file, for consistency with Java style.

>+++ b/mobile/android/base/gfx/LayerView.java

>+    private LinkedList<MotionEvent> mEventQueue = new LinkedList<MotionEvent>();

Please move declaration up. Also add a comment that it should only be touched from the UI thread.

>     nsEventStatus status;
>     DispatchEvent(&event, status);
>+    if (status == nsEventStatus_eConsumeNoDefault) {
>+        AndroidBridge::Bridge()->PreventPanning();
>+        return true;
>+    }
>     return false;

Does this only happen for down events? We shouldn't call preventPanning if the page does preventDefault on a motion or up event.
Comment 67 Wesley Johnston (:wesj) 2011-12-09 08:03:46 PST
Created attachment 580419 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

> The preventPanning call should always happen on the UI thread or you're
> gonna end up with races/unsynchronized access.

I just posted to the mController.

> not sure what the spec says about multitouch, but you probably need a check
> here for getPointerCount() == 1 to avoid terminating the session too early.
> Actually the same applies for the ACTION_DOWN case above, I think you want
> to not do anything with that if getPointerCount() > 1.

Little known fact, but ACTION_DOWN and ACTION_UP are actually only set where there's one touch. Multiple touches will have ACTION_POINTER_DOWN/UP set. I left this for now, but we can add the extra check if you want.

> Does this only happen for down events? We shouldn't call preventPanning if
> the page does preventDefault on a motion or up event.

So pages can only prevent panning on touchdown or the first touchmove. If they do, Gecko will return ConsumeNoDefault for that touch and all subsequent touches that happens. If the page calls preventDefault on a touchmove after the first (or on touchend) Gecko will still just return Ignore.

That means this currently sends a whole lot of preventPanning calls to Gecko, which usually just clears the queue. We could try and clean that up, but I wasn't in the hurry to optimize there right now. Maybe we need to though?
Comment 68 Wesley Johnston (:wesj) 2011-12-09 08:35:21 PST
Whoops that should be:

if ((action & MotionEvent.ACTION_MASK) == MotionEvent.ACTION_DOWN) {

Fixed locally.
Comment 69 Kartikaya Gupta (email:kats@mozilla.com) 2011-12-09 08:41:54 PST
Comment on attachment 580419 [details] [diff] [review]
Widget Patch 2/3 - Prevent Default stuff

>-            return true;
>+        int action = event.getAction();
>+        if (action & MotionEvent.ACTION_MASK) {

Missing "== MotionEvent.ACTION_DOWN"

Rest looks good. Good to know about that ACTION_POINTER_DOWN business, that means some code in PZC that I wrote a while back is technically incorrect.

r+ with above fix.
Comment 70 Michael Wu [:mwu] 2011-12-13 13:13:36 PST
Comment on attachment 580386 [details] [diff] [review]
Platform Patch

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

::: widget/public/nsGUIEvent.h
@@ +1558,5 @@
> +  {
> +    MOZ_COUNT_DTOR(nsTouchEvent);
> +  }
> +
> +  nsTArray<nsCOMPtr<nsIDOMTouch>> touches;

I think some compilers may get upset if you have a >> there, so I think it's more common to see:

nsTArray<nsCOMPtr<nsIDOMTouch> > touches;

But I don't know enough C++ to comment much on this.
Comment 71 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-13 23:13:08 PST
That's a syntax error and shouldn't compile.
Comment 72 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-14 09:11:32 PST
Comment on attachment 580384 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

>* * *
>hg qdiff
>Stuff I forgot
>
>diff --git a/dom/base/nsPIDOMWindow.h b/dom/base/nsPIDOMWindow.h
>--- a/dom/base/nsPIDOMWindow.h
>+++ b/dom/base/nsPIDOMWindow.h
>@@ -45,20 +45,23 @@
> #include "nsIDOMLocation.h"
> #include "nsIDOMXULCommandDispatcher.h"
> #include "nsIDOMElement.h"
> #include "nsIDOMEventTarget.h"
> #include "nsIDOMDocument.h"
> #include "nsCOMPtr.h"
> #include "nsEvent.h"
> #include "nsIURI.h"
>+#include "nsIObserverService.h"
>+#include "nsServiceManagerUtils.h"
> 
> #define DOM_WINDOW_DESTROYED_TOPIC "dom-window-destroyed"
> #define DOM_WINDOW_FROZEN_TOPIC "dom-window-frozen"
> #define DOM_WINDOW_THAWED_TOPIC "dom-window-thawed"
>+#define DOM_TOUCH_LISTENER_ADDED "dom-touch-listener-added"
> 
> class nsIPrincipal;
> 
> // Popup control state enum. The values in this enum must go from most
> // permissive to least permissive so that it's safe to push state in
> // all situations. Pushing popup state onto the stack never makes the
> // current popup state less permissive (see
> // nsGlobalWindow::PushPopupControlState()).
>@@ -443,16 +446,31 @@ public:
>   }
>   
>   /**
>    * Call this to indicate that some node (this window, its document,
>    * or content in that document) has a touch event listener.
>    */
>   void SetHasTouchEventListeners()
>   {
>+    if (!mMayHaveTouchEventListener) {
>+      nsPIDOMWindow *win;
>+      if (IsOuterWindow()) {
>+        win = GetCurrentInnerWindow();
>+      } else {
>+        win = this;
>+      }
>+
>+      nsCOMPtr<nsIObserverService> observerService =
>+        do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>+      if (observerService) {
>+        observerService->NotifyObservers(win, DOM_TOUCH_LISTENER_ADDED, nsnull);
>+      }
>+    }
Please implement the method in nsGlobalWindow so that not every user of nsPIDOMWindow
needs to include all the observer/service manager stuff.


>+    } else if (aTopic == "dom-touch-listener-added") {
>+      let browser = BrowserApp.getBrowserForWindow(aSubject);
>+      if (!browser)
>+        return;
>+
>+      let tab = BrowserApp.getTabForBrowser(browser);
>+      if (!tab)
>+        return;

I don't know this code at all, but I assume you've tested that 
the right thing is done when touch events are used in content iframes



I can't/shouldn't review the android/java part
Comment 73 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-14 09:37:45 PST
Comment on attachment 580386 [details] [diff] [review]
Platform Patch

>@@ -1360,22 +1385,16 @@ const char* nsDOMEvent::GetEventName(PRU
>   case NS_SIMPLE_GESTURE_ROTATE_UPDATE:
>     return sEventNames[eDOMEvents_MozRotateGestureUpdate];
>   case NS_SIMPLE_GESTURE_ROTATE:
>     return sEventNames[eDOMEvents_MozRotateGesture];
>   case NS_SIMPLE_GESTURE_TAP:
>     return sEventNames[eDOMEvents_MozTapGesture];
>   case NS_SIMPLE_GESTURE_PRESSTAP:
>     return sEventNames[eDOMEvents_MozPressTapGesture];
>-  case NS_MOZTOUCH_DOWN:
>-    return sEventNames[eDOMEvents_MozTouchDown];
>-  case NS_MOZTOUCH_MOVE:
>-    return sEventNames[eDOMEvents_MozTouchMove];
>-  case NS_MOZTOUCH_UP:
>-    return sEventNames[eDOMEvents_MozTouchUp];
Why this change? You aren't removing the older Win7 touch event handling elsewhere.


>+nsDOMTouch::GetScreenPoint(nsPresContext* aPresContext, nsEvent* aEvent, nsIntPoint aPoint)
>+{
>+  nsIntPoint screenOffset = ((nsGUIEvent*)aEvent)->widget->WidgetToScreenOffset();
Please use C++ casting


>+nsDOMTouch::GetClientPoint(nsPresContext* aPresContext, nsEvent* aEvent, nsIntPoint aPoint)
>+{
>+  if (!aPresContext)
>+    return mClientPoint;
if (expr) {
  stmt;
}
Same also elsewhere.

>+  nsIntPoint outpt(nsPresContext::AppUnitsToIntCSSPixels(pt.x),
>+                    nsPresContext::AppUnitsToIntCSSPixels(pt.y));
extra space before nsPresContext::AppUnitsToIntCSSPixels(pt.y));



>+nsDOMTouch::GetPagePoint(nsPresContext* aPresContext, nsEvent* aEvent, nsIntPoint aPoint)
>+{
>+  nsIntPoint pagePoint = GetClientPoint(aPresContext, aEvent, aPoint);
>+
>+  // If there is some scrolling, add scroll info to client point.
>+  if (aPresContext && aPresContext->GetPresShell()) {
>+    nsIPresShell* shell = aPresContext->GetPresShell();
>+    nsIScrollableFrame* scrollframe = shell->GetRootScrollFrameAsScrollable();
>+    if (scrollframe) {
>+      nsPoint pt = scrollframe->GetScrollPosition();
>+      pagePoint += nsIntPoint(nsPresContext::AppUnitsToIntCSSPixels(pt.x),
>+                              nsPresContext::AppUnitsToIntCSSPixels(pt.y));
>+    }
>+  }
So this is same as what nsDOMUIEvent::GetPagePoint() does. Could you move the code to some helper method? Maybe to nsDOMEvent
Also, could you please handle mPrivateDataDuplicated case, so that after the event has been dispatched, pageX/Y etc have still the right values.

> nsDOMTouchEvent::GetTargetTouches(nsIDOMTouchList** aTargetTouches)
> {
>-  NS_IF_ADDREF(*aTargetTouches = mTargetTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aTargetTouches);
>+  if (mTargetTouches || !mEvent)
>+    return CallQueryInterface(mTargetTouches, aTargetTouches);
This crashes if mTargetTouches and mEvent are null.
Call* methods aren't null-safe, do_* methods are
mEvent shouldn't be ever null, so you could add NS_ENSURE_STATE(mEvent);

>+
>+  nsTArray<nsCOMPtr<nsIDOMTouch>> targetTouches;
space between the two > arrows. Same also elsewhere.

>+  nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+  nsTArray<nsCOMPtr<nsIDOMTouch>> touches = touchEvent->touches;
>+  for (PRUint32 i = 0; i < touches.Length(); ++i) {
>+    // for touchend/cancel events, don't append to the target list if this is a
>+    // touch that is ending
>+    if ((mEvent->message != NS_TOUCH_END &&
>+          mEvent->message != NS_TOUCH_CANCEL)
>+        || !touches[i]->changed) {
|| should be in the previous line


>+  void InitializePoints(nsPresContext* aPresContext, nsEvent* aEvent) {
{ to the next line


>+    if (mPointsInitialized)
>+      return;
as mentioned before, the patch should use the style
if (expr) {
  stmt;
}


>+
>+    mPagePoint = GetPagePoint(aPresContext, aEvent, mRefPoint);
>+    mScreenPoint = GetScreenPoint(aPresContext, aEvent, mRefPoint);
>+    mClientPoint = GetClientPoint(aPresContext, aEvent, mRefPoint);
>+    mPointsInitialized = true;
>+  }
>+  void SetTarget(nsIDOMEventTarget *aTarget) {
{ to the next line

>   nsMouseScrollEvent event(true, msg, widget);
>-  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
>-  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
>-  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
>-  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;
>+  event.isShift = !!(aModifiers & nsIDOMNSEvent::SHIFT_MASK);
>+  event.isControl = !!(aModifiers & nsIDOMNSEvent::CONTROL_MASK);
>+  event.isAlt = !!(aModifiers & nsIDOMNSEvent::ALT_MASK);
>+  event.isMeta = !!(aModifiers & nsIDOMNSEvent::META_MASK);
So here you change code to use !!


>+  nsTouchEvent event(true, msg, widget);
>+  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
>+  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
>+  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
>+  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;
And here you add code which uses ?:
Please be consistent, and in this case it would make more sense to use
the existing ?: style.

>+PresShell::TouchesAreEqual(nsIDOMTouch* aTouch1, nsIDOMTouch* aTouch2)
>+{
>+  float pressure1, pressure2;
>+  float orientation1, orientation2;
>+  PRInt32 radiusX1, radiusY1, radiusX2, radiusY2;
>+  aTouch1->GetForce(&pressure1);
>+  aTouch1->GetRotationAngle(&orientation1);
>+  aTouch1->GetRadiusX(&radiusX1);
>+  aTouch1->GetRadiusY(&radiusY1);
>+  aTouch2->GetForce(&pressure2);
>+  aTouch2->GetRotationAngle(&orientation2);
>+  aTouch2->GetRadiusX(&radiusX2);
>+  aTouch2->GetRadiusY(&radiusY2);
>+  return aTouch1->mRefPoint != aTouch2->mRefPoint ||
>+         (pressure1 != pressure2) ||
>+         (orientation1 != orientation2) ||
>+         (radiusX1 != radiusX2) || (radiusY1 != radiusY2);
>+}
Could this be a (C++) helper method in nsIDOMTouch object?




I wish I had an Android device which can run Fennec-trunk, but tablets aren't supported atm :(
Comment 74 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-16 12:17:46 PST
*** Bug 708942 has been marked as a duplicate of this bug. ***
Comment 75 Wesley Johnston (:wesj) 2011-12-16 17:35:03 PST
Created attachment 582444 [details] [diff] [review]
Platform Patch

> Why this change? You aren't removing the older Win7 touch event handling
> elsewhere.

Just a mistake. Thanks.
 

> So this is same as what nsDOMUIEvent::GetPagePoint() does. Could you move
> the code to some helper method? Maybe to nsDOMEvent
> Also, could you please handle mPrivateDataDuplicated case, so that after the
> event has been dispatched, pageX/Y etc have still the right values.

I moved all three getPagePoint, getClientPoint, and getScreenPoint to be static methods in nsDOMEvent. I was attempting to pass mPrivateDataDuplicated to it so that it could return if necessary, but I gave up and just moved the checks back into nsDOMUIEvent.
 
> This crashes if mTargetTouches and mEvent are null.
> Call* methods aren't null-safe, do_* methods are
> mEvent shouldn't be ever null, so you could add NS_ENSURE_STATE(mEvent);

I don't think I needed this? Just switched to NS_ENSURE_STATE(mEvent).
 
> Could this be a (C++) helper method in nsIDOMTouch object?

Done.

> I wish I had an Android device which can run Fennec-trunk, but tablets
> aren't supported atm :(

Tablet support isn't great, but its not bad atm. Good enough to run tests on these if you want (and since tablets typically support more touches, you can test a bit more too). Portrait mode works better than landscape. I can provide a build if you want (or I just sent off a try run):

https://tbpl.mozilla.org/?tree=Try&rev=675c29c4d7a3
Comment 76 Wesley Johnston (:wesj) 2011-12-16 17:36:47 PST
Created attachment 582446 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

Updated with smaug's comments.
Comment 77 Wesley Johnston (:wesj) 2011-12-16 17:38:51 PST
Created attachment 582449 [details] [diff] [review]
Platform Patch

And that was the wrong patch (stupid birch/mc switchover...)
Comment 78 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-19 02:04:35 PST
Martijn, you've done some touch event testing. Could you perhaps test the tryserver build?
(see comment 75)
Comment 79 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-19 02:07:20 PST
Comment on attachment 582449 [details] [diff] [review]
Platform Patch

Would be good to get feedback also from Matt.
Comment 80 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-19 03:09:22 PST
Comment on attachment 582449 [details] [diff] [review]
Platform Patch

 >+nsIntPoint nsDOMEvent::GetScreenCoords(nsPresContext* aPresContext,
>+                                       nsEvent* aEvent,
>+                                       nsIntPoint aPoint)
>+{
nsIntPoint should be on its own line.


>+  if (!aEvent || 
>+       (aEvent->eventStructType != NS_MOUSE_EVENT &&
>+        aEvent->eventStructType != NS_POPUP_EVENT &&
>+        aEvent->eventStructType != NS_MOUSE_SCROLL_EVENT &&
>+        aEvent->eventStructType != NS_MOZTOUCH_EVENT &&
>+        aEvent->eventStructType != NS_DRAG_EVENT &&
>+        aEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT)) {
>+    return nsIntPoint(0, 0);
>+  }
Does this not need to have NS_TOUCH_EVENT ?
Tests are missing screenX/Y, I think

>+nsIntPoint nsDOMEvent::GetClientCoords(nsPresContext* aPresContext,
>+                                       nsEvent* aEvent,
>+                                       nsIntPoint aPoint,
>+                                       nsIntPoint aDefaultPoint)
>+{
>+  if (!aEvent ||
>+      (aEvent->eventStructType != NS_MOUSE_EVENT &&
>+       aEvent->eventStructType != NS_POPUP_EVENT &&
>+       aEvent->eventStructType != NS_MOUSE_SCROLL_EVENT &&
>+       aEvent->eventStructType != NS_MOZTOUCH_EVENT &&
>+       aEvent->eventStructType != NS_TOUCH_EVENT &&
>+       aEvent->eventStructType != NS_DRAG_EVENT &&
>+       aEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT) ||
... This one does have NS_TOUCH_EVENT


>+bool nsDOMTouch::Equals(nsIDOMTouch* aTouch)
bool should be in the previous line.

> NS_IMETHODIMP
> nsDOMTouchEvent::GetTargetTouches(nsIDOMTouchList** aTargetTouches)
> {
>-  NS_IF_ADDREF(*aTargetTouches = mTargetTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aTargetTouches);
>+  NS_ENSURE_STATE(mEvent);
>+
>+  nsTArray<nsCOMPtr<nsIDOMTouch> > targetTouches;
>+  nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+  nsTArray<nsCOMPtr<nsIDOMTouch> > touches = touchEvent->touches;
>+  for (PRUint32 i = 0; i < touches.Length(); ++i) {
>+    // for touchend/cancel events, don't append to the target list if this is a
>+    // touch that is ending
>+    if ((mEvent->message != NS_TOUCH_END &&
>+          mEvent->message != NS_TOUCH_CANCEL) || !touches[i]->changed) {
extra space before mEvent


>+      nsIDOMEventTarget* targetPtr = touches[i]->GetTarget();
>+      if (targetPtr == mEvent->target) {
>+        targetTouches.AppendElement(touches[i]);
>+      }
>+    }
>+  }
>+  mTargetTouches = new nsDOMTouchList(targetTouches);
>+  return CallQueryInterface(mTargetTouches, aTargetTouches);
> }
So this ends up creating new nsDOMTouchList all the time. Doesn't sounds right.
Shouldn't there be
if (mTargetTouches) {
  return CallQueryInterface(mTargetTouches, aTargetTouches);
}
somewhere in the beginning of the method..

> 
> NS_IMETHODIMP
> nsDOMTouchEvent::GetChangedTouches(nsIDOMTouchList** aChangedTouches)
> {
>-  NS_IF_ADDREF(*aChangedTouches = mChangedTouches);
>-  return NS_OK;
>+  NS_ENSURE_ARG_POINTER(aChangedTouches);
>+  NS_ENSURE_STATE(mEvent);
>+
>+  nsTArray<nsCOMPtr<nsIDOMTouch> > changedTouches;
>+  nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(mEvent);
>+  nsTArray<nsCOMPtr<nsIDOMTouch> > touches = touchEvent->touches;
>+  for (PRUint32 i = 0; i < touches.Length(); ++i) {
>+    if (touches[i]->changed) {
>+      changedTouches.AppendElement(touches[i]);
>+    }
>+  }
>+  mChangedTouches = new nsDOMTouchList(changedTouches);
>+  return CallQueryInterface(mChangedTouches, aChangedTouches);
> }
Same here


>@@ -452,10 +454,10 @@
>   nsMouseEvent event(true, msg, widget, nsMouseEvent::eReal,
>                      contextMenuKey ?
>                        nsMouseEvent::eContextMenuKey : nsMouseEvent::eNormal);
>-  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
>-  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
>-  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
>-  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;
>+  event.isShift = !!(aModifiers & nsIDOMNSEvent::SHIFT_MASK);
>+  event.isControl = !!(aModifiers & nsIDOMNSEvent::CONTROL_MASK);
>+  event.isAlt = !!(aModifiers & nsIDOMNSEvent::ALT_MASK);
>+  event.isMeta = !!(aModifiers & nsIDOMNSEvent::META_MASK);
No need for this change.



>   event.button = aButton;
>   event.widget = widget;
> 
>@@ -549,6 +551,76 @@
>   return widget->DispatchEvent(&event, status);
> }
> 
>+
>+NS_IMETHODIMP
>+nsDOMWindowUtils::SendTouchEvent(const nsAString& aType,
>+                                 PRUint32 *identifiers,
>+                                 PRInt32 *xs,
>+                                 PRInt32 *ys,
>+                                 PRUint32 *rxs,
>+                                 PRUint32 *rys,
>+                                 float *rotationAngles,
>+                                 float *forces,
>+                                 PRUint32 count,
>+                                 PRInt32 aModifiers,
>+                                 bool aIgnoreRootScrollFrame)
Please use Mozilla coding style for parameter names:
aName


>+  nsTouchEvent event(true, msg, widget);
>+  event.isShift = (aModifiers & nsIDOMNSEvent::SHIFT_MASK) ? true : false;
>+  event.isControl = (aModifiers & nsIDOMNSEvent::CONTROL_MASK) ? true : false;
>+  event.isAlt = (aModifiers & nsIDOMNSEvent::ALT_MASK) ? true : false;
>+  event.isMeta = (aModifiers & nsIDOMNSEvent::META_MASK) ? true : false;
>+  event.widget = widget;
>+  event.time = PR_IntervalNow();
PR_Now() ?


> [scriptable, uuid(36adf309-e5c4-4912-9152-7fb151dc754a)]
> interface nsIDOMWindowUtils : nsISupports {
update uuid


>+  void sendTouchEvent(in AString aType,
>+                      [array, size_is(count)] in PRUint32 identifiers,
>+                      [array, size_is(count)] in PRInt32 xs,
>+                      [array, size_is(count)] in PRInt32 ys,
>+                      [array, size_is(count)] in PRUint32 rxs,
>+                      [array, size_is(count)] in PRUint32 rys,
>+                      [array, size_is(count)] in float rotationAngles,
>+                      [array, size_is(count)] in float forces,
>+                      in PRUint32 count,
>+                      in long aModifiers,
>+                      [optional] in boolean aIgnoreRootScrollFrame);
Interesting use of array and count :) Didn't know it was possible to use
same size for many arrays.


>-[scriptable, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
>+[scriptable, builtinclass, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
> interface nsIDOMTouch : nsISupports {
>   readonly attribute long              identifier;
>   readonly attribute nsIDOMEventTarget target;
>@@ -56,6 +60,14 @@
>   readonly attribute long              radiusY;
>   readonly attribute float             rotationAngle;
>   readonly attribute float             force;
>+  %{C++
>+    nsCOMPtr<nsIDOMEventTarget> mTarget;
>+    nsIDOMEventTarget *GetTarget() { return mTarget; }
>+    void SetTarget(nsIDOMEventTarget *target) { mTarget = target; }
>+    nsIntPoint mRefPoint;
>+    bool changed;
>+    PRUint32 message;

Please be consistent with naming.
mVariableName.

(I know nsEvent structs don't use m-prefix)

>+nsLayoutUtils::GetEventCoordinatesRelativeTo(const nsEvent* aEvent,
>+                                             const nsIntPoint aPoint,
>+                                             nsIFrame* aFrame)
>+{
>+  if (!aFrame)
>     return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);


if (expr) {
  stmt;
}


> 
>+  const nsGUIEvent* GUIEvent = static_cast<const nsGUIEvent*>(aEvent);
>+
>   /* If we walk up the frame tree and discover that any of the frames are
>    * transformed, we need to do extra work to convert from the global
>    * space to the local space.
>    */
>   nsIFrame* rootFrame = aFrame;
>+
>+  nsIWidget* widget = GUIEvent->widget;
>+  if (!widget) {
>+    return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
>+  }
Why you move this here? Do this when you have if (!aFrame)



>   bool transformFound = false;
> 
>   for (nsIFrame* f = aFrame; f; f = GetCrossDocParentFrame(f)) {
>@@ -959,8 +977,7 @@
>     return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
> 
>   nsPoint widgetToView = TranslateWidgetToView(rootFrame->PresContext(),
>-                               GUIEvent->widget, GUIEvent->refPoint,
>-                               rootView);
>+                               widget, aPoint, rootView);
> 
>   if (widgetToView == nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE))
>     return nsPoint(NS_UNCONSTRAINEDSIZE, NS_UNCONSTRAINEDSIZE);
>@@ -980,7 +997,8 @@
>   /* Otherwise, all coordinate systems are translations of one another,
>    * so we can just subtract out the different.
>    */
>-  return widgetToView - aFrame->GetOffsetToCrossDoc(rootFrame);
>+  nsPoint offset = aFrame->GetOffsetToCrossDoc(rootFrame);
>+  return widgetToView - offset;
> }
> 
> nsIFrame*
>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h
>--- a/layout/base/nsLayoutUtils.h
>+++ b/layout/base/nsLayoutUtils.h
>@@ -432,6 +432,10 @@
>   static nsPoint GetEventCoordinatesRelativeTo(const nsEvent* aEvent,
>                                                nsIFrame* aFrame);
> 
>+  static nsPoint GetEventCoordinatesRelativeTo(const nsEvent* aEvent,
>+                                               const nsIntPoint aPoint,
>+                                               nsIFrame* aFrame);
>+
This needs documentation. How is the new method different from the old one.
What is aPoint?


>+EvictOldTouchPoints(const PRUint32& aKey, nsCOMPtr<nsIDOMTouch>& aTouch, void *userArg)
>+{
>+  nsIWidget *widget = nsnull;
>+  // is there an easier/better way to dig out the widget?
>+  nsCOMPtr<nsINode> node(do_QueryInterface(aTouch->GetTarget()));
>+  if (!node) {
>+    return PL_DHASH_NEXT;
>+  }
>+  nsIDocument* doc = node->GetCurrentDoc();
>+  if (!doc) {
>+    return PL_DHASH_NEXT;
>+  }
>+  nsIPresShell *presShell = doc->GetShell();
>+  if (!presShell) {
>+    return PL_DHASH_NEXT;
>+  }
>+  nsIFrame* frame = presShell->GetRootFrame();
>+  if (!frame) {
>+    return PL_DHASH_NEXT;
>+  }
>+  nsPoint *pt = new nsPoint(aTouch->mRefPoint.x, aTouch->mRefPoint.y);
>+  widget = frame->GetView()->GetNearestWidget(pt);
>+  if (!widget) {
>+    return PL_DHASH_NEXT;
>+  }
>+  nsTouchEvent event(true, NS_TOUCH_END, widget);
>+  event.isShift = false;
>+  event.isControl = false;
>+  event.isAlt = false;
>+  event.isMeta = false;
>+  event.widget = widget;
>+  event.time = PR_IntervalNow();
>+  event.touches.AppendElement(aTouch);
>+
>+  nsEventStatus status;
>+  widget->DispatchEvent(&event, status);
>+  return PL_DHASH_NEXT;
>+}
I think you should use EvictOldTouchPoints (call it perhaps GetOldTouchPoints) to collect touch objects to a list (nsCOMArray<nsIDOMTouch> for example)
and then iterate through the list and dispatch events.
Dispatching events at somewhat random time, like here during hashtable enumeration, is scary. Keep in mind that dispatching an event
may destroy worlds (i.e. close windows)
Comment 81 Wesley Johnston (:wesj) 2011-12-19 17:32:50 PST
Created attachment 583033 [details] [diff] [review]
Platform Patch

> Why you move this here? Do this when you have if (!aFrame)

I'm not exactly sure what you meant here, but I moved this earlier in the function?

> This needs documentation. How is the new method different from the old one.
> What is aPoint?

I added some documentation comment, but I'm not sure what more to write beyond this is a point that you want to get its coordinates relative to a frame and the widget associated with this event? I don't have a strong idea of what either of those even are (and no one I talk to does either).

> I think you should use EvictOldTouchPoints (call it perhaps
> GetOldTouchPoints) to collect touch objects to a list
> (nsCOMArray<nsIDOMTouch> for example)
> and then iterate through the list and dispatch events.
> Dispatching events at somewhat random time, like here during hashtable
> enumeration, is scary. Keep in mind that dispatching an event
> may destroy worlds (i.e. close windows)

Done. We actually already have an enumerator that adds everything in the hash table to another array, so I used it.
Comment 82 Wesley Johnston (:wesj) 2011-12-19 17:37:58 PST
pushed to try again too. a bit worried that i'll see leaks, but good to find them now

https://tbpl.mozilla.org/?tree=Try&rev=138f1f3ed937
Comment 83 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-20 08:04:11 PST
Comment on attachment 583033 [details] [diff] [review]
Platform Patch

>+nsDOMEvent::GetScreenCoords(nsPresContext* aPresContext,
>+                            nsEvent* aEvent,
>+                            nsIntPoint aPoint)
>+{
>+  if (!aEvent || 
>+       (aEvent->eventStructType != NS_MOUSE_EVENT &&
>+        aEvent->eventStructType != NS_POPUP_EVENT &&
>+        aEvent->eventStructType != NS_MOUSE_SCROLL_EVENT &&
>+        aEvent->eventStructType != NS_MOZTOUCH_EVENT &&
>+        aEvent->eventStructType != NS_TOUCH_EVENT &&
>+        aEvent->eventStructType != NS_DRAG_EVENT &&
>+        aEvent->eventStructType != NS_SIMPLE_GESTURE_EVENT)) {
>+    return nsIntPoint(0, 0);
>+  }
You should also check that aPresContext isn't null

>+function checkTouch(aFakeTouch, aTouch) {
>+  is(aFakeTouch.identifier, aTouch.identifier, "Touch has correct identifier");
>+  is(aFakeTouch.target, aTouch.target, "Touch has correct target");
>+  is(aFakeTouch.page.x, aTouch.pageX, "Touch has correct pageX");
>+  is(aFakeTouch.page.y, aTouch.pageY, "Touch has correct pageY");
>+  is(aFakeTouch.page.x + window.mozInnerScreenX, aTouch.screenX, "Touch has correct screenX");
>+  is(aFakeTouch.page.y + window.mozInnerScreenY, aTouch.screenY, "Touch has correct screenY");
>+  is(aFakeTouch.page.x, aTouch.clientX, "Touch has correct clientX");
>+  is(aFakeTouch.page.y, aTouch.clientY, "Touch has correct clientY");
>+  is(aFakeTouch.radius.x, aTouch.radiusX, "Touch has correct radiusX");
>+  is(aFakeTouch.radius.y, aTouch.radiusY, "Touch has correct radiusY");
>+  is(aFakeTouch.rotationAngle, aTouch.rotationAngle, "Touch has correct rotationAngle");
>+  is(aFakeTouch.force, aTouch.force, "Touch has correct force");
>+}
Add screenX/Y tests


>+EvictTouchPoint(nsCOMPtr<nsIDOMTouch>& aTouch)
>+{
>+  nsIWidget *widget = nsnull;
nsCOMPtr please, so that widget stays alive when dispatching event.
Comment 84 Wesley Johnston (:wesj) 2012-01-05 09:30:26 PST
*** Bug 695207 has been marked as a duplicate of this bug. ***
Comment 85 Wesley Johnston (:wesj) 2012-01-05 09:32:37 PST
Moving the component and setting priority from bug 695207 to help with searches.
Comment 86 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-01-09 04:57:52 PST
Does those changes means the pref 'dom.w3c_touch_events.enabled' could be turned on by default on all platforms?
Comment 87 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-09 05:02:50 PST
No. We don't support W3C touch events elsewhere but on Android (after this bug is fixed)
Comment 88 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-09 11:21:15 PST
The pref makes development harder for pages that try to work across both touch and mouse interfaces.  Gaia (b2g UI), for example, normalizes all mouse events to touch events in order to simplify input-processing code.  With this pref disabled though, |document.createEvent('touchevent')| throws NS_ERROR_DOM_NOT_SUPPORTED_ERR, which doesn't really make sense to me.  We still can support *creating* touch events everywhere, even if Gecko would never deliver them to pages.

addEventListener("touchstart", ...) still succeeds, which is odd to me.  It seems better for authors to have that call fail if we didn't support touch events.  But DOM event semantics are not my specialty :).

Can we find a happy solution to this problem, or is this behavior in the spec?
Comment 89 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-09 11:33:04 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #88)
> The pref makes development harder for pages that try to work across both
> touch and mouse interfaces.  Gaia (b2g UI), for example, normalizes all
> mouse events to touch events in order to simplify input-processing code. 
> With this pref disabled though, |document.createEvent('touchevent')| throws
> NS_ERROR_DOM_NOT_SUPPORTED_ERR, which doesn't really make sense to me.  We
> still can support *creating* touch events everywhere, even if Gecko would
> never deliver them to pages.
No. "TouchEvent" in window or document.createEvent("TouchEvent") etc 
are used as a feature detection whether the browser supports touch events.

If gaia sends Touch events to web content, then it of course needs to enable that pref.

> addEventListener("touchstart", ...) still succeeds, which is odd to me. 
"touchstart" is just event type. The event you get in the listener could be for example
a key event named "touchstart"


> It
> seems better for authors to have that call fail if we didn't support touch
> events. 
No. That is not how DOM events work. You can always add a listener for any event type.
event.type doesn't say anything about the interface event implements.

> Can we find a happy solution to this problem, or is this behavior in the
> spec?
Not exposing TouchEvent and other relevant APIs when it isn't actually supported is a web compatibility requirement.
Comment 90 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-09 11:40:34 PST
So I guess we either deal with the duplicated code paths or try to emulate TouchEvent somehow.  Ah well.
Comment 91 Wesley Johnston (:wesj) 2012-01-14 18:52:49 PST
Created attachment 588700 [details] [diff] [review]
Platform patch fixes

Found some problems in the platform patch. This fixes them. Tests pass locally, and mostly on try (waiting for more results). Also adds a few tests I meant to add but forgot about.
Comment 92 Wesley Johnston (:wesj) 2012-01-14 18:53:40 PST
Created attachment 588701 [details] [diff] [review]
Platform Patch

Unbitrotted platform patch (with the same problems of the oldone). Carrying forward r+.
Comment 93 Wesley Johnston (:wesj) 2012-01-14 18:56:15 PST
Created attachment 588702 [details] [diff] [review]
IFDEF platform pieces

This ifdefs (with ifdef ANDROID) all of the platform changes so that we can potentially move this forward to Aurora. I made no changes to the underlying patches. I'm not sure what I need to do for review with this.

In addition, I can put up a patch that has all of this qfolded if that makes it easier to verify that there are no changes to the non-Android platform. Not sure what people will want...? Happy to have extra eyes looking for mistakes.
Comment 94 Wesley Johnston (:wesj) 2012-01-14 19:10:41 PST
Created attachment 588703 [details] [diff] [review]
Folded platform patches

Figured I might as well just upload this. This is all of the platform patches (including the ifdef one) qfolded into one.
Comment 95 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-14 21:34:14 PST
Comment on attachment 580384 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>+            } else if (event.equals("Tab:HasTouchListener")) {
>+                Log.i(LOGTAG, "Tab has touch listeners");
>+                int tabId = message.getInt("tabID");
>+                Tab tab = Tabs.getInstance().getTab(tabId);
>+                tab.setHasTouchListeners(true);
>+                if (Tabs.getInstance().isSelectedTab(tab)) {
>+                    mMainHandler.post(new Runnable() {
>+                        public void run() {
>+                            mLayerController.setWaitForTouchListeners(true);
>+                        }
>+                    });
>+                }

You need to put the isSelectedTab inside the runnable to protect against a race. Make "tab" be final, so you can use it in the runnable.

>         GeckoAppShell.registerGeckoEventListener("AgentMode:Changed", GeckoApp.mAppContext);
>+        GeckoAppShell.registerGeckoEventListener("Tab:HasTouchListener", GeckoApp.mAppContext);

>         GeckoAppShell.unregisterGeckoEventListener("AgentMode:Changed", GeckoApp.mAppContext);
>+        GeckoAppShell.unregisterGeckoEventListener("Tab:HasTouchListener", GeckoApp.mAppContext);

Put these with the other "Tab:Xxx" lines. (just my OCD)

r+ with comments addressed
Comment 96 Wesley Johnston (:wesj) 2012-01-14 21:36:39 PST
Comment on attachment 588700 [details] [diff] [review]
Platform patch fixes

Few other things cropping up on try...
Comment 97 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-14 21:36:55 PST
Comment on attachment 582446 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

This patch has much of the same code in it as the other "Widget 3/3" patch and my comments are the same as well.

Which is the one you intend to use?
Comment 98 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-14 21:40:09 PST
Comment on attachment 588702 [details] [diff] [review]
IFDEF platform pieces

I really can't review any of this code. The idea is good: use #ifdef to make this code safer for landing on aurora. I think smaug need to review this.
Comment 99 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-15 03:13:17 PST
Comment on attachment 588702 [details] [diff] [review]
IFDEF platform pieces


>-[scriptable, builtinclass, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
>+[scriptable, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
You should keep builtinclass.

So, touch interfaces are currently enabled, AFAIK, only on Android, and while running mochitests.
So most the ifdefs are not needed. Only the cases when mouseevent or uievent handling is changed
might need ifdef
Comment 100 Kevin Brosnan [:kbrosnan] 2012-01-15 08:25:18 PST
*** Bug 718294 has been marked as a duplicate of this bug. ***
Comment 101 Wesley Johnston (:wesj) 2012-01-15 20:30:07 PST
Created attachment 588808 [details] [diff] [review]
Platform patch fixes

Yay. All green on try with these fixes (or at least... as green as try gets I think).
Comment 102 Wesley Johnston (:wesj) 2012-01-15 23:19:47 PST
Created attachment 588819 [details] [diff] [review]
IFDEF platform pieces

This should just ifdef the pieces where I've changed our Screen/Client/Page coordinate calculations, nsLayoutUtils, and the changes in nsPresShell::DispatchEvent. Also turns off the tests on non-Android platforms since the touch events will not pass through nsPresShell correctly.

I also cleaned up a stray function declaration left behind from an old version.
Comment 103 Andreas Gal :gal 2012-01-16 01:34:30 PST
Comment on attachment 588819 [details] [diff] [review]
IFDEF platform pieces

We would like to have touch events for Gonk (B2G) as well. ifdef android seems pretty ugly. How about ifdef HAS_TOUCH?
Comment 104 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-16 05:42:25 PST
*** Bug 718390 has been marked as a duplicate of this bug. ***
Comment 105 Wesley Johnston (:wesj) 2012-01-16 10:30:27 PST
I'm fine with that. The idea is just to be able to push this Nightly, test for a bit, push to Aurora, and then remove the ifdefs from Nightly.
Comment 106 Jeff Griffiths (:canuckistani) (:⚡︎) 2012-01-16 10:49:38 PST
(In reply to Andreas Gal :gal from comment #103)
> Comment on attachment 588819 [details] [diff] [review]
> IFDEF platform pieces
> 
> We would like to have touch events for Gonk (B2G) as well. ifdef android
> seems pretty ugly. How about ifdef HAS_TOUCH?

An additional use case is multi-touch input devices ( eg the Apple trackpad ) attached to a desktop. I've seen demos of multi-touch enabljed via npapi plugins with the predictably crashy results.
Comment 107 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-16 11:06:40 PST
Trackpad-like use cases are very different from the touch events being added here.
Comment 108 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-01-18 10:33:19 PST
Comment on attachment 588819 [details] [diff] [review]
IFDEF platform pieces


>-
>-[scriptable, builtinclass, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
>+[scriptable, uuid(98bc0f7d-5bff-4387-9c42-58af54b48dd5)]
You need to keep this as builtinclass

s/ANDROID/MOZ_TOUCH/
Comment 109 Wesley Johnston (:wesj) 2012-01-18 14:27:51 PST
Created attachment 589648 [details] [diff] [review]
Folded platform patches

Awesome. All set up to check this in. I switched to #ifdef MOZ_TOUCH, but only enabled it for ANDROID (I don't want to mess up something in B2G if you guys aren't ready?). mbrubeck (and anyone else who wants) is going to give a build a once over tonight to give feedback. Build is at:

http://people.mozilla.org/~wjohnston/fennec-touch.apk

(or with this and the following he can build himself).
Comment 110 Wesley Johnston (:wesj) 2012-01-18 14:29:20 PST
Created attachment 589649 [details] [diff] [review]
Widget Parts 1+2

Not sure what I was thinking when I started to fold these.
Comment 111 Wesley Johnston (:wesj) 2012-01-18 14:30:24 PST
Created attachment 589651 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners
Comment 112 Wesley Johnston (:wesj) 2012-01-18 15:23:03 PST
pushed the platform piece at least:

https://hg.mozilla.org/integration/mozilla-inbound/rev/593594a73dd0

still looking at the widget stuff (which has some bugginess) and trying to decide if its ready to land or not.
Comment 113 Marco Bonardo [::mak] 2012-01-19 02:54:37 PST
https://hg.mozilla.org/mozilla-central/rev/593594a73dd0
Comment 114 Marco Bonardo [::mak] 2012-01-19 02:55:37 PST
sorry, looks incomplete, reopening.
Comment 115 Wesley Johnston (:wesj) 2012-01-23 08:14:31 PST
Created attachment 590723 [details] [diff] [review]
Fixes for widget

I realized I needed to test XUL Fennec before I pushed this. Found some errors. This should apply on top of the Widget Part 1 stuff.
Comment 116 Brad Lassey [:blassey] (use needinfo?) 2012-01-23 22:20:05 PST
Comment on attachment 590723 [details] [diff] [review]
Fixes for widget

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

::: embedding/android/GeckoAppShell.java
@@ +1791,5 @@
>          GeckoNetworkManager.getInstance().disableNotifications();
>      }
> +
> +    public static void preventPanning() {
> +    }

what's the point of of this function?

::: embedding/android/GeckoEvent.java
@@ +152,5 @@
>          mMetaState = m.getMetaState();
>  
>          switch (mAction & MotionEvent.ACTION_MASK) {
> +            // for these events, we assure that the point at index 0
> +            // is the changed event

this seems like an over complication. Why not just add mPointerIndex to the event?

::: mobile/android/base/GeckoEvent.java
@@ +160,5 @@
>          mMetaState = m.getMetaState();
>  
>          switch (mAction & MotionEvent.ACTION_MASK) {
>              // these events will only send a single touch point, the one that
>              // was lifted or the one that was cancelled

this comment isn't true
Comment 117 Wesley Johnston (:wesj) 2012-01-24 08:43:37 PST
Created attachment 591118 [details] [diff] [review]
Fixes for widget

Included a pointerIndex field. I added the preventPanning function because it is implemented in Native Fennec and XUL Fennec fails when try to attach it via JNI. This seemed like the simplest fix. I added a comment explaining that its only used in Native Fennec. We can do something fancier if you want?
Comment 120 Wesley Johnston (:wesj) 2012-01-26 08:01:50 PST
Comment on attachment 589648 [details] [diff] [review]
Folded platform patches

Requesting Aurora Approval for this patch

User impact if declined: No way to interact gesturally with pages. Currently that behavior is preffed off, so right now the impact is minimal.

Testing completed (on m-c, etc.): This has been on mc since 1-18. AFAIK, no regressions have been found on Desktop or Mobile.

Risk to taking this patch (and alternatives if risky): We took care to #ifdef the parts of this patch that affected things outside mobile so that there would be no impact to the desktop product. The things that aren't ifdef'd are paths that desktop should not hit, especially since the pref for touch events is off there, making it impossible to dispatch them. The risk is low, but higher than some of our other mobile-only patches.

Just two days ago I landed a series of widget patches that depend on this on m-c. We need to be able to uplift those patches to aurora, if for no other reason that to minimize differences between aurora and mc for people landing patches on both. Doing that would be easiest if we uplift this patch as well.

Alternatively, we could modify the widget patches in order to minimize the differences between mc and aurora (but there would still need to be some minor ones, likely in nsWindow.cpp::DispatchMotionEvent() and land them that way.
Comment 121 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-01-27 04:06:06 PST
In an Aurora build of 2012-01, I see currently in about:config:
dom.w3c_touch_events.enabled true
dom.w3c_touch_events.safetyX 5
dom.w3c_touch_events.safetyY 20

Is that expected? Or is that cruft from the XUL version of Fennec?
Comment 122 Matt Brubeck (:mbrubeck) 2012-01-27 10:02:01 PST
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #121)
> In an Aurora build of 2012-01, I see currently in about:config:
> dom.w3c_touch_events.enabled true
> dom.w3c_touch_events.safetyX 5
> dom.w3c_touch_events.safetyY 20
> 
> Is that expected? Or is that cruft from the XUL version of Fennec?

That's cruft copied from XUL Fennec.  Those prefs have no effect on Aurora right now.
Comment 123 Alex Keybl [:akeybl] 2012-01-27 16:19:42 PST
Comment on attachment 589648 [details] [diff] [review]
Folded platform patches

[Triage Comment]
Mobile only - approved for Aurora.
Comment 124 Wesley Johnston (:wesj) 2012-01-28 07:13:24 PST
Comment on attachment 589649 [details] [diff] [review]
Widget Parts 1+2

User impact if declined: No multitouch
Testing completed (on m-c, etc.): Has been on mc since 1/24
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only. Disabled for now.
Comment 125 Wesley Johnston (:wesj) 2012-01-28 07:15:02 PST
Comment on attachment 589651 [details] [diff] [review]
Widget Patch 3/3 - Touch Listeners

User impact if declined: No multitouch
Testing completed (on m-c, etc.): On mc since 1/24
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only. Currently disabled
Comment 126 Wesley Johnston (:wesj) 2012-01-28 07:15:40 PST
Comment on attachment 591118 [details] [diff] [review]
Fixes for widget

User impact if declined: No multitouch
Testing completed (on m-c, etc.): On mc since 1/24
Risk to taking this patch (and alternatives if risky): Low risk. Mobile only. Currently disabled
Comment 127 Brad Lassey [:blassey] (use needinfo?) 2012-01-30 11:35:25 PST
landed the platform patches, not market 11 fixed because there are 3 more to go

https://hg.mozilla.org/releases/mozilla-aurora/rev/95c01032a28f
Comment 128 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-30 14:23:45 PST
*** Bug 721359 has been marked as a duplicate of this bug. ***
Comment 129 Alex Keybl [:akeybl] 2012-02-02 06:58:31 PST
Comment on attachment 589649 [details] [diff] [review]
Widget Parts 1+2

[Triage Comment]
Mobile only - approved for Beta 11.
Comment 131 Wesley Johnston (:wesj) 2012-02-28 11:02:51 PST
I realize I never really had the security team look over this stuff, and I got a bit worried that I'd missed some iframe sneakiness. Asking them to make sure they've looked at it.
Comment 132 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-02-28 18:38:25 PST
but is marked sec-review-needed, we will cover this in our next security triage and decide what needs to be done from our side
Comment 133 Timothy Nikkel (:tnikkel) 2012-02-29 19:50:47 PST
Is there a reason that nsLayoutUtils::GetEventCoordinatesRelativeTo(const nsEvent* aEvent, nsIFrame* aFrame) can't just be a wrapper for nsLayoutUtils::GetEventCoordinatesRelativeTo(const nsEvent* aEvent, const nsIntPoint aPoint, nsIFrame* aFrame) instead of duplicating it almost exactly? The code is a little bit delicate and I'd really prefer not to have two copies sitting around waiting to get out of sync.
Comment 134 Wesley Johnston (:wesj) 2012-02-29 20:45:00 PST
Nope. I need to remove all the #ifdef MOZ_TOUCH pieces entirely. We should be fine. They are only there so we could safely move this forward to Aurora and Beta if necessary. I'll push a patch with that change to try tomorrow (but 722965) and if its good put it up for review.

Slipped my mind. Sorry :(
Comment 135 Timothy Nikkel (:tnikkel) 2012-02-29 23:16:35 PST
Ok, great, thank you!
Comment 136 Wesley Johnston (:wesj) 2012-03-01 15:08:25 PST
Bug 722965 is for removing these ifdefs. I'm building here. Assuming that goes well i'll have a try run notify there and upload the patch. Assuming that goes well, I'll toss it back to you smaug.
Comment 137 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-03-02 14:26:47 PST
assigning to yvan for code review
Comment 138 Ian Melven :imelven 2012-03-02 14:37:43 PST
(In reply to Curtis Koenig [:curtisk] from comment #137)
> assigning to yvan for code review

Yvan, i've talked briefly about this with Wes, we discussed events crossing domain boundaries as a possible problem.
Comment 139 Jim Mathies [:jimm] 2012-04-13 06:48:59 PDT
I've filed bug 745071 for win8 metro, but based on a comment from mbrubeck in bug 508906, I'm not sure which event system to go with. What was our reasoning for implementing the w3c events? Are we feeling upbeat about the outcome of the patent issues?
Comment 140 Wesley Johnston (:wesj) 2012-04-13 07:49:07 PDT
Compatibility with the current de facto standards on the mobile web was the driving factor.
Comment 141 Eric Shepherd [:sheppy] 2012-04-19 13:26:04 PDT
Noted support for this has arrived for Firefox Android:

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

Documented nsIDOMWindowUtils.sendTouchEvent():

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#sendTouchEvent%28%29

Multi-touch support (and the nsIDOMWindowUtils change) noted on Firefox 12 for developers.

Note You need to log in before you can comment on or make changes to this bug.