Closed Bug 1020995 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

32 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox29 affected, firefox30 affected, firefox31 affected, firefox32 affected, firefox33 verified, fennec+)

RESOLVED FIXED
Firefox 33
Tracking Status
firefox29 --- affected
firefox30 --- affected
firefox31 --- affected
firefox32 --- affected
firefox33 --- verified
fennec + ---

People

(Reporter: TeoVermesan, Assigned: wesj)

Details

(Keywords: reproducible)

Attachments

(2 files)

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
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.
tracking-fennec: --- → ?
Keywords: reproducible
Summary: Action bar is invoked on long tapping a searched image → Text selection action bar is invoked on long tapping a images in Google Images
Assignee: nobody → wjohnston
tracking-fennec: ? → +
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...
Attached patch PatchSplinter Review
Attachment #8436138 - Flags: review?(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
Attachment #8436138 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/c43720109522
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Verified fixed on:
Device: Alcatel One Touch
OS: Android 4.1
Build: Firefox for Android 33.0a1 (2014-06-12)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: