Use platform touch redirection

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: wesj, Assigned: bnicholson)

Tracking

(Blocks 1 bug)

unspecified
Firefox 35
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Comment 1

7 years ago
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.
Reporter

Updated

7 years ago
Blocks: 774458
Reporter

Comment 2

7 years ago
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #661850 - Flags: review?(bugmail.mozilla)
Blocks: 712772
No longer depends on: 712772
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.
Attachment #661850 - Flags: review?(bugmail.mozilla) → review+
Reporter

Comment 4

7 years ago
Filed bug 791831 to restore the haptic stuff.
Are you waiting for bug 791831 to be resolved before landing this?
Reporter

Comment 6

7 years ago
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
Reporter

Comment 7

7 years ago
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.
Reporter

Comment 8

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

Comment 9

7 years ago
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)
Assignee: nobody → wjohnston
Flags: needinfo?(wjohnston)
Reporter

Comment 11

7 years ago
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.
Flags: needinfo?(wjohnston)
Reporter

Updated

7 years ago
Blocks: 819119
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?
Flags: needinfo?(wjohnston)
Reporter

Comment 13

6 years ago
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.
Flags: needinfo?(wjohnston)
Reporter

Comment 14

6 years ago
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...
Reporter

Updated

6 years ago
Depends on: 901117
Reporter

Comment 15

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

Comment 16

6 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
This looks good on try actually. And its the beginning of a cycle, so seems like a good time to try this.
Attachment #661850 - Attachment is obsolete: true
Attachment #785944 - Flags: review?(bugmail.mozilla)
Reporter

Comment 17

6 years ago
Posted patch Patch (obsolete) — Splinter Review
Whoops. Wrong patch.
Attachment #785944 - Attachment is obsolete: true
Attachment #785944 - Flags: review?(bugmail.mozilla)
Attachment #785989 - Flags: review?(bugmail.mozilla)
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?
Attachment #785989 - Flags: review?(bugmail.mozilla) → review+
Reporter

Comment 21

6 years ago
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.
Assignee

Comment 22

5 years ago
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.
Assignee: wjohnston → bnicholson
Attachment #785989 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8490562 - Flags: review?(wjohnston)
Attachment #8490562 - Flags: review?(bugmail.mozilla)
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.
Attachment #8490562 - Flags: review?(bugmail.mozilla) → review+
Reporter

Comment 25

5 years ago
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....
Attachment #8490562 - Flags: review?(wjohnston) → review+
Assignee

Comment 27

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/c8e8e389d84f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Assignee

Updated

5 years ago
Duplicate of this bug: 1066157
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.
No longer depends on: 1071758
Depends on: 1078029
You need to log in before you can comment on or make changes to this bug.