Bug 780847 took our touch redirection code and moved it into the platform (almost exactly I think). We should stop using ours in places where we don't need anymore.

Note, as part of this I'd like to move our mouse event dispatching into our widget code. We also use this code for tap highlighting and context menus as well though. Basically we try to do the fluffing once, highlight that element, and then ensure that context menus and mouseevents all are directed at the same thing. I'm not sure how hard it will be to move that all to platform, or if we can do it in small pieces.

This seems to work well here. The platform "fluff" code is a bit funny. For TOUCH events, it looks for nearby elements with MOUSE listeners (or anchors or textboxes, etc) and redirects to them. Happily, that means that we should be able to use the target of touchstart for a lot of our mouse handling code. That's what I'm trying to do here, but there are likely some small changes to some algorithms. Still looking at this to make sure those are acceptable, but happy to have some more eyes. Eventually it would be good to just move our mouse handling and tap highlight code to platform as well. The default radius in the platform prefs is 1cm which is kinda huge. I'm using 4mm here which is pretty close to our old values. I'm also stealing some stuff from bug 708048 which isn't + yet. I removed our haptic feedback here because I don't want to keep these methods around just for that. I'm going to move it into platform code I think.

Comment on attachment 661850 [details] [diff] [review] Patch Review of attachment 661850 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to make sure this doesn't regress any of the already-fixed dependencies of bug 753525 but otherwise looks fine. ::: mobile/android/app/mobile.js @@ +415,5 @@ > +pref("ui.touch.radius.enabled", true); > +pref("ui.touch.radius.leftmm", 4); > +pref("ui.touch.radius.topmm", 4); > +pref("ui.touch.radius.rightmm", 4); > +pref("ui.touch.radius.bottommm", 4); In the old code the radius was skewed so that the "top" side had a larger radius than the "bottom" - do we want to preserve that behaviour? I think we probably do; making the topmm/bottommm 5mm and 3mm should do the trick. ::: mobile/android/chrome/content/browser.js @@ +1390,5 @@ > }, > > _sendToContent: function(aX, aY) { > + // _highlightElement should already be fluffed to find nearby clickable elements > + let rootElement = BrowserEventHandler._highlightElement; Assuming that after merging this with the patch from bug 708048 it will still look very similar to this (just s/rootElement/target/) @@ +1426,5 @@ > } > }, > > _show: function(aEvent) { > + BrowserEventHandler._cancelTapHighlight(); This was in the handleEvent function in bug 708048. I think both locations are equivalent, just want to make sure it's in exactly one of the them after merging. @@ +3413,5 @@ > handleEvent: function(aEvent) { > if (!BrowserApp.isBrowserContentDocumentDisplayed() || aEvent.touches.length > 1 || aEvent.defaultPrevented) > return; > > let closest = aEvent.target; I'd rather just use aEvent.target instead of closest in this function since we don't need to reassign it to anything now.

Filed bug 791831 to restore the haptic stuff.

Are you waiting for bug 791831 to be resolved before landing this?

Yeah. Otherwise, I'm dropping the haptic feedback bits. It sounds like Smaug would just be happy with a comment there now though. I'll upload a new patch for him

Haptic feedback is out. Working on landing this (and bug 708048) now. I'll spin through the fixed bugs in bug 753525 and make sure they still work.

Sorry for the insane delay. Finally landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a5dbfe84e178

And this broke a test that clicks on an edge very close to a clickable element. I'll fix the test, but backed out for now https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d611217c39

Any news here, Wes? As you know, this is blocking bug 774458 which is a B2G blocker. We're just looking for status updates. Thanks. (Also, please forgive me if you shouldn't be the assignee; it looks like you're working on it so I assigned you)

This is not a huge priority for mobile to get in. This should not be blocking B2G AFAIK. B2G needs to do its own gesture detection and fire mouse events when "click"-like gesture are detected (rather than all the time like it does right now). This patch will not help them fix that.

In bug 837242 we are planning to make a significant change to the platform touch handling code. So we should either push this bug forward or port that change to the android code so that android can benefit from the change as well. Unifying of course would be the most desirable. What's needed here, just fixing a test failure?

Yeah, this was just blocked on tests. Since none of the platforms using this are running many tests, I assume that's still the case. Let me try and unbitrot this today and I'll toss it to try.

Enabling platform support doesn't seem to work very well. At least on google I'm seeing it constantly find a listener on the <html> element itself and bail out of fluffing. Digging in a bit more so that I can file the right bug. Similar things happen on wikipedia, but I think they happen for Fennec as well...

Unbitrotted and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3cd519096392

This looks good on try actually. And its the beginning of a cycle, so seems like a good time to try this.

Whoops. Wrong patch.

Comment on attachment 785989 [details] [diff] [review] Patch Review of attachment 785989 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/app/mobile.js @@ +430,5 @@ > +pref("ui.touch.radius.enabled", true); > +pref("ui.touch.radius.leftmm", 4); > +pref("ui.touch.radius.topmm", 4); > +pref("ui.touch.radius.rightmm", 4); > +pref("ui.touch.radius.bottommm", 4); Shouldn't we keep the bias towards the top? B2G and all.js also have that bias, and I think it's a good idea. Ditto for mouse radius. ::: mobile/android/chrome/content/browser.js @@ +2052,5 @@ > // find and store the top most element this context menu is being shown for > // use the highlighted element if possible, otherwise look for nearby clickable elements > // If we still don't find one we fall back to using anything > + > + let target = BrowserEventHandler._highlightElement; Comment here needs updating @@ +2101,5 @@ > this._target = null; > BrowserEventHandler._cancelTapHighlight(); > > if (SelectionHandler.canSelect(target)) > + BrowserEventHandler._cancelTapHighlight(); There's a call to _cancelTapHighligh() two lines above. Is this still needed?

Something in this push caused robocop-2 perma-orange. Backed out. https://hg.mozilla.org/integration/fx-team/rev/1fb5d14e8348 https://tbpl.mozilla.org/php/getParsedLog.php?id=26227297&tree=Fx-Team

The screenshots from these failures just show a big black area where the link should be. I wonder if I deleted something that helps the viewport update correctly, but the tests pass fine on all my local machines.

Rebased the existing patch here and made a few changes, the biggest being the conversion of the radius prefs. Since we dropped the browser.ui.touch.* prefs, this uses the ui.touch.radius.* prefs and converts them to px values. FWIW, the radius getter is used only by getTouchRadius, which is used only by SelectionHandler.js: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectionHandler.js?rev=bf52063ed95f#1049 Also changed the mouse events to use the Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH flag as suggested in bug 1066157.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=ddab2783ef0e

Comment on attachment 8490562 [details] [diff] [review] Use platform touch redirection Review of attachment 8490562 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4872,5 @@ > break; > > case "Gesture:SingleTap": { > + // The target should already have been fluffed by the platform touch event code, and it > + // will be fluffed out again by the platform mouse event. This comment doesn't seem quite right. It's not clear what "target" is being referred to here. Certainly the x and y coordinates in |aData| have not been fluffed already. I'm guessing what you're trying to say here is that _highlightElement was chosen after fluffing the touch events that led to this SingleTap, and that the mouse events should find the same target because they will go through the fluffing algorithm again. @@ +4883,5 @@ > > + try { > + // If the element was previously focused, show the caret attached to it. > + let element = this._highlightElement; > + if (element && element == BrowserApp.getFocusedInput(BrowserApp.selectedBrowser)) { I think you need to get the value of BrowserApp.getFocused(..) before you dispatch the mouse events, because those mouse events will trigger a click which will change the focused element. That's what the old code was doing, and I think that behaviour should probably stay the same. @@ +5047,3 @@ > try { > + let cwu = win.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > + cwu.sendMouseEventToWindow(aName, aX, aY, 0, 1, 0, true, 0, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH); Unrelated to this change, but based on what happened in http://hg.mozilla.org/mozilla-central/rev/f890a8991588 (specifically the change to TabChild) I think the aIsSynthesized argument to this function should be set to false. That should go into a separate bug that blocks 891882 though.

Comment on attachment 8490562 [details] [diff] [review] Use platform touch redirection Review of attachment 8490562 [details] [diff] [review]: ----------------------------------------------------------------- Lets try! ::: mobile/android/chrome/content/browser.js @@ +4872,5 @@ > break; > > case "Gesture:SingleTap": { > + // The target should already have been fluffed by the platform touch event code, and it > + // will be fluffed out again by the platform mouse event. I wrote this. One worry I have is that we'll highlight something when you touch, then fire a click on something different when you lift your finger. BUT, that's actually the bug we're trying to fix as well (something moves/hides on touchevents and then expects the click to fire on the new object at that position). Hopefully we can let platform code handle the :active setting anyway and let them deal with that. In fact, it might be good to file a follow up to see what we need to do to make that happen....

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #24) > @@ +5047,3 @@ > > try { > > + let cwu = win.top.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils); > > + cwu.sendMouseEventToWindow(aName, aX, aY, 0, 1, 0, true, 0, Ci.nsIDOMMouseEvent.MOZ_SOURCE_TOUCH); > > Unrelated to this change, but based on what happened in > http://hg.mozilla.org/mozilla-central/rev/f890a8991588 (specifically the > change to TabChild) I think the aIsSynthesized argument to this function > should be set to false. That should go into a separate bug that blocks > 891882 though. Filed bug 1071197. (In reply to Wesley Johnston (:wesj) from comment #25) > Hopefully we can let platform code handle the :active setting anyway and let > them deal with that. In fact, it might be good to file a follow up to see > what we need to do to make that happen.... Filed bug 1071216.

On a nightly with this change I'm seeing some taps trigger two click events. I'll try to make a test case and file a new bug with it sometime tonight but just a heads-up.