1020995 - Text selection action bar is invoked on long tapping a images in Google Images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2014-06-05-15-30-47.png

Steps to reproduce:
1. Search an image in google
2. Long tap on it

Expected results:
- The context menu should appear

Actual results:
- The action bar appears

Comment 1

[Aaron Train [:aaronmt]]

I've seen this too. It makes it difficult to interact with content like in Google Images when the browser keeps trying to text-select.

Comment 2

[Wesley Johnston (:wesj)]

Hmm.. This is a bit tricky. The issue here is that Google attaches a click handler to the div that's holding the image. We find that div when you tap and use it as our "highlightElement". When you long press, we also use that as our context menu element (i.e. we want the context menu to fire for whatever is highlighted, even if your finger isn't exactly on it):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2294

To fix this we really need to just use anyElementFromPoint. To prevent that from causing the highlighted element to not show a context menu, we can walk up and verify that its a child of highlightElement. Will try...

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 4

[:Margaret Leibovic]

Comment on attachment 8436138 [details] [diff] [review]
Patch

Review of attachment 8436138 [details] [diff] [review]:
-----------------------------------------------------------------

anyElementFromPoint and elementFromPoint are confusing, since anyElementFromPoint is the one that actually calls cwu.elementFromPoint! This logic is kinda tricky, but the comments help.

::: mobile/android/chrome/content/browser.js
@@ +2275,5 @@
>      },
>  
> +    _findTarget: function(x, y) {
> +      let isDescendant = function(parent, child) {
> +        var node = child;

Nit: let

@@ +2291,5 @@
> +      let target = BrowserEventHandler._highlightElement;
> +      let touchTarget = ElementTouchHelper.anyElementFromPoint(x, y);
> +
> +      // If we have a highlighted element that has a click handler, we want to ensure our target is inside it
> +      if (touchTarget && target && isDescendant(target, touchTarget)) {

If you wanted, you could omit the check for touchTarget and target here, since isDescendant will return false if either of those elements don't exist.

@@ +2294,5 @@
> +      // If we have a highlighted element that has a click handler, we want to ensure our target is inside it
> +      if (touchTarget && target && isDescendant(target, touchTarget)) {
> +        target = touchTarget;
> +      } else if (!target) {
> +        // Otherwise, lets try to find something clickable

Grammar nit: let's

Comment 5

[Wesley Johnston (:wesj)]

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

Comment 6

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

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

Comment 7

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
Device: Alcatel One Touch
OS: Android 4.1
Build: Firefox for Android 33.0a1 (2014-06-12)