We don't do this. We need to. Setting to block bug 761934 because it will mostly likely fix it. We need this for 750977 because our async pan/zoom controller needs to use touch events to compute the gestures it requires.

This is a basecamp blocker, for many different reasons! :)

Comment on attachment 642468 [details] [diff] [review] Forward touch events across processes Review of attachment 642468 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. There are just two caveats that you need to be aware of: 1. The strategy you have set up probably won't be enough to implement the behavior of preventDefault() per the spec*, unless Wes set up some magic for that. The spec says that a call to preventDefault on touchstart or the first touchmove should prevent any mouse events associated with that touch point. This means that a webpage can call preventDefault only on touchstart, which in this code will block mousedown, but won't block the following mousemoves/up to be dispatched (because preventDefault might not be called on them). (many webpages might not know or been relying on this though, so this is something to consider for your web-compat priorities) Probably the best way to handle this will be to always forward touch and mouse, and let the child process keep track of that and block dispatch of the mouse events in that case. (This would have the advantage that you wouldn't need to create and dispatch the phony nsMouseEvent manually). (It'd be worth to talk to Wes to see in which level he handled this on the platform.) *http://www.w3.org/TR/touch-events/ sec. 5.4 and 5.6 2. Note that with this patch you'll block all mouse events from being dispatched in the parent when the target is a remote frame. If there is UI code using mouse events instead of touch that will be a problem. Solving (1) with what I proposed would also solve this I think. The following are do-what-you-want-about-them nits: ::: content/events/src/nsEventStateManager.h @@ +447,5 @@ > mozilla::dom::TabParent *GetCrossProcessTarget(); > bool IsTargetCrossProcess(nsGUIEvent *aEvent); > > + bool DispatchCrossProcessEvent(nsEvent* aEvent, nsIFrameLoader* remote, > + nsEventStatus *aStatus); the change from void to bool is returned in HandleCrossProcessEvent but ultimately unused. Did you change it for future use/correctness? My initial idea was that HandleCrossProcessEvent would return true/false if it was given an event that is meant to be dispatched cross-process (i.e., similar to the purpose of CrossProcessSafeEvent now), not if it successfully did it, mainly because I didn't think that the Send* functions could fail in any way that would useful to handle. You can keep the change, I'm just explaining why it is the way it is at the moment. ::: dom/ipc/TabChild.cpp @@ +704,5 @@ > + refPoint = aEvent.touches[0]->mRefPoint; > + } > + > + nsMouseEvent event(true, msg, NULL, > + nsMouseEvent::eReal, nsMouseEvent::eNormal); instead of true you could use NS_IS_TRUSTED_EVENT, assuming the flags are correctly created at the origin you could also set, for completeness: event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH;

(In reply to Felipe Gomes (:felipe) from comment #4) > > 1. The strategy you have set up probably won't be enough to implement the > behavior of preventDefault() per the spec*, unless Wes set up some magic for > that. This implementation follows what the gonk widget backend does[1], so if this patch violates spec so does the current widget code. If we implement these semantics in Gecko proper, I would expect ConsumeNoDefault to be returned from the event dispatch, and both the gonk widget backend and the cross-process code will adhere to spec. > Probably the best way to handle this will be to always forward touch and > mouse, and let the child process keep track of that and block dispatch of > the mouse events in that case. This is excessively expensive, because it will require a round-trip through content to dispatch the synthesized mouse event. I would actually prefer to delegate this logic to the content process. > 2. Note that with this patch you'll block all mouse events from being > dispatched in the parent when the target is a remote frame. If there is UI > code using mouse events instead of touch that will be a problem. Solving (1) > with what I proposed would also solve this I think. > That should be fine, at least for b2g --- any web content loaded into the main process (for v1, the "system" app and browser app) will be touch-event-aware, and should use capturing listeners when they might want to take events away from remote content. > ::: content/events/src/nsEventStateManager.h > @@ +447,5 @@ > > mozilla::dom::TabParent *GetCrossProcessTarget(); > > bool IsTargetCrossProcess(nsGUIEvent *aEvent); > > > > + bool DispatchCrossProcessEvent(nsEvent* aEvent, nsIFrameLoader* remote, > > + nsEventStatus *aStatus); > > the change from void to bool is returned in HandleCrossProcessEvent but > ultimately unused. Did you change it for future use/correctness? > Just to make the code simpler, so that I can simply return from the case statements :). > My initial idea was that HandleCrossProcessEvent would return true/false if > it was given an event that is meant to be dispatched cross-process (i.e., > similar to the purpose of CrossProcessSafeEvent now), not if it successfully > did it, mainly because I didn't think that the Send* functions could fail in > any way that would useful to handle. > I think that's still basically true. Since we currently ignore the return value from HandleCrossProcessEvent(), I don't think it matters all that much currently. > You can keep the change, I'm just explaining why it is the way it is at the > moment. > Sure, will keep. Thanks for explaining. > ::: dom/ipc/TabChild.cpp > @@ +704,5 @@ > > + refPoint = aEvent.touches[0]->mRefPoint; > > + } > > + > > + nsMouseEvent event(true, msg, NULL, > > + nsMouseEvent::eReal, nsMouseEvent::eNormal); > > instead of true you could use NS_IS_TRUSTED_EVENT, assuming the flags are > correctly created at the origin > > you could also set, for completeness: > event.inputSource = nsIDOMMouseEvent::MOZ_SOURCE_TOUCH; Will make these changes. Thanks again for the quick review. [1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsAppShell.cpp#332

(In reply to Felipe Gomes (:felipe) from comment #4) > 1. The strategy you have set up probably won't be enough to implement the > behavior of preventDefault() per the spec*, unless Wes set up some magic for > that. The spec says that a call to preventDefault on touchstart or the first > touchmove should prevent any mouse events associated with that touch point. I believe this behavior is needed by web compatibility. I don't recall how it is done in Fennec.

I'll have to read this to know what you're doing, but in Fennec (and in every other mobile browser in the world) mouse events are only sent at the end of a touch series. i.e. after the user taps down and up, if preventDefault wasn't called we send mousemove, mousedown, mouseup, click (the mousemove could be sent on touchstart if you wanted, but might lead to strange behaviors on some sites, so might be better delayed by 50ms or so). Last I heard (from PPK), B2G was the only mobile browser not following this convention. I assumed at the time because touch events support was fairly new. Is that still going on? We prevent sending mouse events (but still send them for XUL Fennec) here: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1290 You should be able to trust the status returned from dispatching the event. PresShell keeps track of whether or not preventDefault can be called on this particular touch series, and keeps returning prevent default for all events in the set. (i.e. if the page calls preventDefault on touchstart, we will return status = nsEventStatus_eConsumeNoDefault for all subsequent events in the "set"). Likewise, a site calling preventDefault on the third touchmove event won't affect the state returned. The platform isn't smart enough (yet) to discern between calling preventDefault on the first finger in a two finger touch, and calling preventDefault on the second. Once you've called on either touch, we always return status = nsEventStatus_eConsumeNoDefault for all touch events until there are no fingers on the screen. Some optimizations there originally led to problems. Fixing them was non-trivial. "content" sends the preventDefault message back to java from nsWindow.cpp here: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1393 java handles the touch events here touchEventHandler.java here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java I can go into more details here or on IRC. Kats also wrote a lot of this nsWindow and TouchEventHandler code, so feel free to ask him.

(In reply to Wesley Johnston (:wesj) from comment #7) > The platform isn't smart enough (yet) to discern between calling > preventDefault on the first finger in a two finger touch, and calling > preventDefault on the second. Once you've called on either touch, we always > return status = nsEventStatus_eConsumeNoDefault for all touch events until > there are no fingers on the screen. I also spent some time looking at this behavior on various mobile browsers. They are not consistent. I feel like our current behavior is a little bit restrictive to web authors (I'm sure there's some weird behavior you can only get by allowing a second finger to pan but not the first), but also far more predictable. Call preventDefault and everything platformy stops.

Comment on attachment 642468 [details] [diff] [review] Forward touch events across processes Based on that, r-

Wes, I don't understand what the widget backend is responsible for, i.e. what core gecko *isn't* doing. Is that documented anywhere?

Not well. Sorry :( Widget backend is responsible for dispatching the touch events and monitoring the response to see if preventDefault was called. If preventDefault wasn't called, its responsible for doing any other work processing the events for gesture detection, scrolling, etc. Part of gesture detection could be dispatching mouse events if a tap is detected as well (i.e. making desktop websites compatible). However, on Android tap/longtap/doubletap/pinchzoom all use the system's detectors (pinchzoom is using a custom detector written in Java now) so that ends up being a little bit out of band of our widget stuff. Those system events are sent to Javascript in browser.js where they are handled. I'd love to move some of that into widget or platform if possible. Happy to help write some of that. http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/ui/PanZoomController.java#980

So I still don't understand ... what layer queues(?) the touch events and uses them to dispatch mouse events? Is that gecko or the widget backend?

Java sends a message to browser.js which dispatches the mouse events using DOMWindowUtils. In more detail, touch events and are sent from the Java UI thread to the Gecko thread (nsWindow.cpp widget code) here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/TouchEventHandler.java#199 Gecko monitors the events to see if preventDefault was called: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsWindow.cpp#1396 In the mean time, Java queues the touch events and waits to see if preventDefault is called. It also runs a timer so if we don't hear back from Gecko within some minimum time limit, we send the touches on anyway (i.e. we have to keep scrolling async even under bad conditions). If preventDefault is not called (or the timer fires), We process the events in Java which includes sending them to a Java gesture detector: http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/gfx/TouchEventHandler.java#243 Which (if it detects a tap) calls: http://mxr.mozilla.org/mozilla-aurora/source/mobile/android/base/ui/PanZoomController.java#994 which sends a message to browser.js: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3365 which fires mouse events for us. Better? To much?

I fear that as long as this is done in platform- and frontend-specific code, we're going to suffer from the problem of fragmentation of behavior you refer to. smaug, wes, since the gonk widget backend is already not adhering to spec, and that's an orthogonal problem, how about we take this patch as-is and fix both in a followup? The current patch won't regress behavior.

As far as I know, that patch isn't web compatible, so I don't understand why we should take it.

Apparently, neither is gonk's nsWindow.cpp.

The calculus to me is In current m-c - gonk/b2g isn't spec compliant with touch events - content processes don't receive touch events at all - we can't use async pan/zoom in gecko If we take this patch - gonk/b2g isn't spec compliant with touch events - content process will receive touch events, which un-breaks a lot of content - we can use async pan/zoom That's why I propose landing this patch now, and then fixing gonk/b2g in a separate bug.

To move our click handlers into java (and make touch handling slightly better), Fennec would require something like bug 547997 to land (i.e. redirect clicks to something close by).

oops. s/into java/out of javascript/. my 2 cents on landing, I have a feeling that if gonk isn't fixed here it won't be, but that's a decision for someone else to make I guess.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #17) > The calculus to me is > > In current m-c > - gonk/b2g isn't spec compliant with touch events > - content processes don't receive touch events at all > - we can't use async pan/zoom in gecko > > If we take this patch > - gonk/b2g isn't spec compliant with touch events > - content process will receive touch events, which un-breaks a lot of > content And break a lot of content

(In reply to Olli Pettay [:smaug] from comment #20) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #17) > > The calculus to me is > > > > In current m-c > > - gonk/b2g isn't spec compliant with touch events > > - content processes don't receive touch events at all > > - we can't use async pan/zoom in gecko > > > > If we take this patch > > - gonk/b2g isn't spec compliant with touch events > > - content process will receive touch events, which un-breaks a lot of > > content > > And break a lot of content No, it's *already* broken wrt spec compliance. That's my point. *No* touch events breaks more content than non-spec-compliant touch events.

(In reply to Wesley Johnston (:wesj) from comment #19) > oops. s/into java/out of javascript/. > > my 2 cents on landing, I have a feeling that if gonk isn't fixed here it > won't be, but that's a decision for someone else to make I guess. You impugn my honor, sir! It will absolutely be fixed!

Comment on attachment 642468 [details] [diff] [review] Forward touch events across processes Ok, let's take this for now to let b2g apps be oop, but someone needs to fix touch event for b2g properly.

The key piece of missing information here, per IRC chat, was this affects *all* cross-process content in b2g: "browser tabs" *and* apps. Sorry that wasn't clear.

I added FIXME/bug 774458 comments to widget/gonk/nsAppShell.cpp and TabChild.

Lost inbound roulette, backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed13a015623 ... for breaking generated dom-bindings code?!?

These patches require pulling in DOM headers to serialize nsTouchEvent (sigh). The even-serializing code is used for plugins, and on mac we have some Object-C++ code that ends up including this header. dombindings.h (pretty sure not autogenerated?) typedef's NameOps to NO, but ... NO is a constant/keyword in Objective-C++ :(. I'm changing the typedef to NP. It's not pretty, but a problem we were probably going to run into eventually.

On the test page at http://people.mozilla.com/~cjones/events.html, I see touch events delivered properly in fennec, but not even touchstart delivered on b2g. Hmmmm ....

The page at http://www.quirksmode.org/m/tests/touch.html# is also useful for testing (although its pretty confusing as well). You can hit record, tap the colored boxes at the right, and it will output events. Settings lets you turn things like preventDefault on and off.

smaug, wesj, I'm getting the following behavior - touchstart is delivered to DispatchCrossProcessEvent(), from nsEventStateManager::PostHandleEvent() - thereafter, only synthetic mouse events are delivered (except for the synthesized touchend) I think that makes sense, given the DOM spec. I looked at what preventDefault() seems to do in nsPresShell::DispatchTouchEvent(), for touchstart. It seems to set nsIPresShell::gPreventMouseEvents. If, in DispatchCrossProcessEvent(), I manually set gPreventMouseEvents=true when receiving touchstart, then I see touchstart (followed by a synthetic mouse event generated in the content process). But then I don't see *either* touchmove *or* synthetic mousemove, at all, in DispatchCrossProcessEvent(). It seems that nsEventStateManager::PostHandleEvent() isn't being called at all. If you guys have pointers on what to do here, would love to hear! :) I'm going to try hacking in a preventDefault=true in DispatchTouchEvent() when the target is <iframe remote>, and see if that "works". I sort of doubt it though ...

OK, found some more stuff ... for the first touchmove event, we have the right event target (the <iframe remote>) in DispatchTouchEvent(), but somehow we've lost that by the time we hit HandleCrossProcessEvent() ...

Aha! The event target appears to be set per nsDOMTouch, and HandleCrossProcessEvent() isn't checking that.

Sigh.

Oleg, heads up: it looks like the Qt builder doesn't like this patch https://tbpl.mozilla.org/php/getParsedLog.php?id=13596284&tree=Try I think we're going to need some |#undef signals| sprinkled around.

Comment on attachment 642889 [details] [diff] [review] part 0: Rename 'NO' typename because that's a constant in Objective-C++ bz suggests NO->NOp

Comment on attachment 642889 [details] [diff] [review] part 0: Rename 'NO' typename because that's a constant in Objective-C++ r=me with that

(In reply to Boris Zbarsky (:bz) from comment #38) > Comment on attachment 642889 [details] [diff] [review] > part 0: Rename 'NO' typename because that's a constant in Objective-C++ > > r=me with that s/NP/NOp/ locally.

Comment on attachment 642890 [details] [diff] [review] part 1.1: Dispatch touch events with touch points (to be folded into previous patch) ># HG changeset patch ># User Chris Jones <jones.chris.g@gmail.com> ># Date 1342514297 25200 ># Node ID bd9284d7f18e3f0b87afe525509216d160efe17d ># Parent 560298345ceadef460e2711f5c10987f00b96f83 >Bug 774139, part 1.1: Dispatch touch events with touch points. r=smaug > >diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp >--- a/content/events/src/nsEventStateManager.cpp >+++ b/content/events/src/nsEventStateManager.cpp >@@ -1731,50 +1731,79 @@ CrossProcessSafeEvent(const nsEvent& aEv > return false; > } > } > > bool > nsEventStateManager::HandleCrossProcessEvent(nsEvent *aEvent, > nsIFrame* aTargetFrame, > nsEventStatus *aStatus) { >- if (!CrossProcessSafeEvent(*aEvent)) { >+ if (*aStatus == nsEventStatus_eConsumeNoDefault || >+ !CrossProcessSafeEvent(*aEvent)) { > return false; > } >- nsIContent* target = mCurrentTargetContent; >- if (!target && aTargetFrame) { >- target = aTargetFrame->GetContent(); >+ >+ nsAutoTArray<nsCOMPtr<nsIContent>, 1> targets; >+ if (aEvent->eventStructType != NS_TOUCH_EVENT || >+ aEvent->message == NS_TOUCH_START) { >+ nsIContent* target = mCurrentTargetContent; >+ if (!target && aTargetFrame) { >+ target = aTargetFrame->GetContent(); >+ } >+ if (IsRemoteTarget(target)) { >+ targets.AppendElement(target); >+ } >+ } else { >+ nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aEvent); >+ const nsTArray<nsCOMPtr<nsIDOMTouch> >& touches = touchEvent->touches; >+ for (PRUint32 i = 0; i < touches.Length(); ++i) { >+ nsIDOMTouch* touch = touches[i]; >+ // NB: the |mChanged| check is an optimization, subprocesses can >+ // compute this for themselves. >+ if (!touch || !touch->mChanged) { >+ continue; >+ } >+ nsCOMPtr<nsPIDOMEventTarget> targetPtr; nsCOMPtr<nsIDOMEventTarget> targetPtr; >+ touch->GetTarget(getter_AddRefs(targetPtr)); >+ if (!targetPtr) { >+ continue; >+ } >+ nsCOMPtr<nsIContent> target = do_QueryInterface(targetPtr); >+ if (target && IsRemoteTarget(target) && >+ targets.NoIndex == targets.BinaryIndexOf(target)) { >+ targets.InsertElementSorted(target); Why sorted? Why binaryindexof?

Thanks smaug! (In reply to Olli Pettay [:smaug] from comment #40) > >+ nsCOMPtr<nsPIDOMEventTarget> targetPtr; > nsCOMPtr<nsIDOMEventTarget> targetPtr; > Done. > > >+ touch->GetTarget(getter_AddRefs(targetPtr)); > >+ if (!targetPtr) { > >+ continue; > >+ } > >+ nsCOMPtr<nsIContent> target = do_QueryInterface(targetPtr); > >+ if (target && IsRemoteTarget(target) && > >+ targets.NoIndex == targets.BinaryIndexOf(target)) { > >+ targets.InsertElementSorted(target); > Why sorted? Why binaryindexof? The elements of the array (remote targets) need to be unique for correctness. Any number of touches can point at the same target.

Switched away from sorted array and added comments.

Comment on attachment 643167 [details] [diff] [review] part 1.1: Dispatch touch events with touch points (to be folded into previous patch) , v2 >+ if (IsRemoteTarget(target) && >+ targets.NoIndex == targets.Contains(target)) { Obvious typo, --> !Contains(target) locally.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f58fbba2e13b https://hg.mozilla.org/integration/mozilla-inbound/rev/d1045701fee9

https://hg.mozilla.org/mozilla-central/rev/f58fbba2e13b https://hg.mozilla.org/mozilla-central/rev/d1045701fee9