596895 - Context menu option list displayed for a long tap on image with link varies depending on the tap location

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Antti Järvelin]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.10pre) Gecko/20100910 Ubuntu/10.04 (lucid) Namoroka/3.6.10pre
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b5pre) Gecko/20100825 Namoroka/4.0b5pre Fennec/2.0b1pre

Long tapping on an image containing a link produces different context menus depending on the tap position.  If a user taps in the bottom of the image, only the link related context menu is shown.  Tapping in the middle of the image produces a context menu containing options for both image and link elements.

Reproducible: Always

Steps to Reproduce:
1. Start Fennec, open a page containing an image with link
2. Do a long tap in the bottom of the image
3. Do a long tap in the middle of he image
4. Compare the context menus displayed after steps 2 and 3
Actual Results:  
The context menus opened in step 2 and 3 are different.

Expected Results:  
The context menus opened in step 2 and 3 are not different; In desktop Firefox the CSM content does not depend on the exact location of the "right click". Any location over an image produces the same CSM.

Comment 1

[Matt Brubeck (:mbrubeck)]

If you load the following data URL, then a long tap in the red area results in link options only, while a long tap in the blue area results in link and image options:

data:text/html,<a href="http://example.com" style="border:1px solid red"><img width=100 height=100 border=1 src="http://pavlovdotnet.files.wordpress.com/2008/10/fennec.png"></a>

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This is caused by some problems with ElementTouchHelper._isElementClickable:

* This function does not consider parent elements that are outside of the "touchable" rect, which is true for the test case on this page. (The image is a child of the anchor, but extends outside the anchor's border.)

* The logic for iterating through nodes and parents is wrong, I think.

This simplified version fixes those issues, and otherwise still works correctly as far as I can tell.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: test case

This code was first added for bug 559829.  Since the original steps to reproduce no longer work (google.com has changed its design), I used this test case to verify that bug 559829 is not regressed.

On mozilla-1.9.2 before bug 559829, clicking on "test" highlights the link underneath.

On mozilla-1.9.2 after bug 559829, clicking on "test" does not highlight the link underneath.

On mozilla-central with the patch from this bug, clicking on "test" does not highlight the link underneath.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 476930 [details] [diff] [review]
patch

Vivien should take a look too

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: testcase 2

I think this testcase is closer to the google testcase because one of the problem was that the click was first on a non clickable element (the div with padding/margin).

Does your code works with this testcase?

Comment 6

[Matt Brubeck (:mbrubeck)]

Yes, my patch works with testcase 2 - same behavior as current mobile-browser tip, or Fennec 1.1.

(There are some problems with the contextmenu appearing at incorrect times in that testcase, but that happens with and without my patch.  It will be fixed by bug 597286.)

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 476930 [details] [diff] [review]
patch

If it works on the testcase by clicking not on the link but on the space between the link and the border this works for me.

>diff -r e58418091651 chrome/content/content.js
>+  _isElementClickable: function _isElementClickable(aElement) {
>+    const selector = "a,:link,:visited,[role=button],button,input,select,textarea,label";
>+    for (let elem = aElement; elem; elem = elem.parentNode) {
>+      if (this._hasMouseListener(elem))
>+        return true;
>+      if (elem.mozMatchesSelector && elem.mozMatchesSelector(selector))
>+        return true;

if (this._hasMouseListener(elem) || elem.mozMatchesSelector(selector)) shoud be enough since mozMatchesSelector is a method of nsIDOMNSElement

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/148bdf7f208e

Can we turn those test cases into browser-chrome tests?

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED on builds (using attached test cases):
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100930 Namoroka/4.0b7pre Fennec/4.0b1pre