788073 - Use platform touch redirection

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

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.

Comment 1

[Wesley Johnston (:wesj)]

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.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

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 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Wesley Johnston (:wesj)]

Filed bug 791831 to restore the haptic stuff.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Wesley Johnston (:wesj)]

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

Comment 7

[Wesley Johnston (:wesj)]

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.

Comment 8

[Wesley Johnston (:wesj)]

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

Comment 9

[Wesley Johnston (:wesj)]

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

Comment 10

[Andrew Overholt [:overholt]]

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)

Comment 11

[Wesley Johnston (:wesj)]

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.

Comment 12

[Timothy Nikkel (:tnikkel)]

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?

Comment 13

[Wesley Johnston (:wesj)]

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.

Comment 14

[Wesley Johnston (:wesj)]

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

Comment 15

[Wesley Johnston (:wesj)]

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

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch

Whoops. Wrong patch.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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?

Comment 19

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/bf050e52851f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Wesley Johnston (:wesj)]

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.

Comment 22

[Brian Nicholson (:bnicholson)]

Attachment: Use platform touch redirection

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.

Comment 23

[Brian Nicholson (:bnicholson)]

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

Comment 24

[Kartikaya Gupta (email:[e-mail])]

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 25

[Wesley Johnston (:wesj)]

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

Comment 26

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/fx-team/rev/c8e8e389d84f

Comment 27

[Brian Nicholson (:bnicholson)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c8e8e389d84f

Comment 30

[Kartikaya Gupta (email:[e-mail])]

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.