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

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: antti.i.jarvelin, Assigned: mbrubeck)

Tracking

Bug Flags:
in-testsuite ?

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Assignee: nobody → mbrubeck
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 1

8 years ago
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>
(Assignee)

Comment 2

8 years ago
Created attachment 476930 [details] [diff] [review]
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.
Attachment #476930 - Flags: review?(webapps)
(Assignee)

Comment 3

8 years ago
Created attachment 476961 [details]
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 on attachment 476930 [details] [diff] [review]
patch

Vivien should take a look too
Attachment #476930 - Flags: review?(21)
Created attachment 477094 [details]
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?
(Assignee)

Comment 6

8 years ago
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 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
Attachment #476930 - Flags: review?(21) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [fennec-checkin-postb1]
pushed:
http://hg.mozilla.org/mobile-browser/rev/148bdf7f208e

Can we turn those test cases into browser-chrome tests?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb1]
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
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
(Assignee)

Updated

8 years ago
Attachment #476930 - Flags: review?(webapps)
You need to log in before you can comment on or make changes to this bug.