I haven't tested the multitouch part of this patch but it might work.

Does Android have real gesture events too? Or would it make sense to keep those gesture events android/nsWindow.cpp seems to generate?

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.

Trivial patch to make single-touch events work just like mouse events. No multitouch gestures yet.

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.

This version of the patch sets the streamId correctly.

Fixed bitrot.

What is the status of this fix? Paul's FF4 demos depend on this. Thanks!

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.

Not going to be working on this. The patch here should be enough for someone else to run with after fennec 4.0 ships.

Same as mwu's last patch, but updated to tip.

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.

(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 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.

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.

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.

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!

This is the Android code to get touchevents from widget to platform.

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.

This was tracking-fennec 8+, was nuked when it was moved into beos for some reason.

re-assigning to wes, and re-marking it as tracking 8+

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.

Addresses feedback comments. Ready for first round of review (i hope!)

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.

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?

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.

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

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.

Forgot to qref... doh.

Figured I can do these in parallel, but if Olli has changes on the platform side this may need to change.

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.

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!

Oh... probably easier to just post the link if someone was interested: http://dl.dropbox.com/u/72157/touchremove.html

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?

(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.

(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 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.

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.

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.

BTW, PRBool/PR_TRUE/PR_FALSE are all deprecated and should be removed from new patches.

Also, nsCOMArray is deprecated. I suspect you want a nsTArray combined with a nsRefPtr to the concrete class.

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?

(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.

Whoops. Trying to juggle between birch and mc apparently fails me. This is the right patch (I hope).

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

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.

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.

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.

This is the part that prevents panning/clicking/doubletap/etc if the page calls preventDefault.

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.

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.

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...

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 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 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 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

Updated for comments. Carrying review

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.

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 on attachment 580362 [details] [diff] [review] Widget Patch 2/3 - Prevent Default stuff This patch appears to be the same as the last one?

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 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.

> 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?

Whoops that should be: if ((action & MotionEvent.ACTION_MASK) == MotionEvent.ACTION_DOWN) { Fixed locally.

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 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.

That's a syntax error and shouldn't compile.

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 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 :(

> 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

Updated with smaug's comments.

And that was the wrong patch (stupid birch/mc switchover...)

Martijn, you've done some touch event testing. Could you perhaps test the tryserver build? (see comment 75)

Comment on attachment 582449 [details] [diff] [review] Platform Patch Would be good to get feedback also from Matt.

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)

> 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.

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 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.

Moving the component and setting priority from bug 695207 to help with searches.

Does those changes means the pref 'dom.w3c_touch_events.enabled' could be turned on by default on all platforms?

No. We don't support W3C touch events elsewhere but on Android (after this bug is fixed)

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?

(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.

So I guess we either deal with the duplicated code paths or try to emulate TouchEvent somehow. Ah well.

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.

Unbitrotted platform patch (with the same problems of the oldone). Carrying forward r+.

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.

Figured I might as well just upload this. This is all of the platform patches (including the ifdef one) qfolded into one.

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 on attachment 588700 [details] [diff] [review] Platform patch fixes Few other things cropping up on try...

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 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 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

Yay. All green on try with these fixes (or at least... as green as try gets I think).

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 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?

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.

(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.

Trackpad-like use cases are very different from the touch events being added here.

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/

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).

Not sure what I was thinking when I started to fold these.

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.

sorry, looks incomplete, reopening.

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 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

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?

https://hg.mozilla.org/integration/mozilla-inbound/rev/0286995894b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/a63b9ee257e8

https://hg.mozilla.org/mozilla-central/rev/0286995894b7 https://hg.mozilla.org/mozilla-central/rev/a63b9ee257e8

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.

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?

(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 on attachment 589648 [details] [diff] [review] Folded platform patches [Triage Comment] Mobile only - approved for Aurora.

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 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 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

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 on attachment 589649 [details] [diff] [review] Widget Parts 1+2 [Triage Comment] Mobile only - approved for Beta 11.

https://hg.mozilla.org/releases/mozilla-beta/rev/68b1fb4e1ad7 https://hg.mozilla.org/releases/mozilla-beta/rev/9b40fd1685b8

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.

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

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.

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 :(

Ok, great, thank you!

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.

assigning to yvan for code review

(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.

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?

Compatibility with the current de facto standards on the mobile web was the driving factor.

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.