714789 - "Open in new link" appears on Amazon product image that is not a link (and does nothing)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aaron Train [:aaronmt]]

STR:

1. Load bug URL (http://goo.gl/cVJim)
2. Tap and hold on produce image, select: 'Open Link in a New Tab'

Expected to see the image open in a new tab.

--
Samsung Nexus S (Android 4.0.3)
Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20120103 Firefox/12.0a1 Fennec/12.0a1

Comment 1

[Matt Brubeck (:mbrubeck)]

This image is not a link, so the expected behavior is that "Open link in new tab" should not appear in the context menu.  Investigating to see why it is...

Comment 2

[Matt Brubeck (:mbrubeck)]

This doesn't appear to affect just any random image; working on a reduced test case to see why this Amazon page in particular has this bug.

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

The problem is that ":not([href=''])" is not selective enough; we also want to exclude anchors with no href element at all (as in the Amazon page) or with href values like "mailto:[e-mail]".

This patch copies the contextmenu logic from XUL Fennec, and fixes some bugs in the existing _getLinkURL method.  It also causes the "Save Image" context menu not to appear on the Amazon page for some reason; investigating that now.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 5

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

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


>+    linkOpenableContext: {
>+      matches: function linkOpenableContextMatches(aElement) {
>+        if (aElement.nodeType == Ci.nsIDOMNode.ELEMENT_NODE &&
>+            ((aElement instanceof Ci.nsIDOMHTMLAnchorElement && aElement.href) ||
>+            (aElement instanceof Ci.nsIDOMHTMLAreaElement && aElement.href) ||
>+            aElement instanceof Ci.nsIDOMHTMLLinkElement ||
>+            aElement.getAttributeNS(kXLinkNamespace, "type") == "simple")) {
>+          let uri;

Would aElement.mozMatchesSelector make this any cleaner?

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/909cbc7fa325

> Would aElement.mozMatchesSelector make this any cleaner?

I'd decided to leave this as-is.  Doing this it selectors gets ugly if you want to check all the for both missing *and* empty attributes ("a[href]:not([href='']), link[href]:not([href=''])"), not to mention dealing with namespaces correctly.  Checking interfaces instead lets us access properties like .href without worrying about exceptions.  Also, this code is tested from XUL Fennec so I'm more confident in it.

Comment 7

[Matt Brubeck (:mbrubeck)]

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

[Approval Request Comment]
Native-Android-only patch that fixes a bug where the wrong context menu items are displayed for some elements.

Comment 8

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Mobile only - approved for Aurora.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/909cbc7fa325
https://hg.mozilla.org/releases/mozilla-aurora/rev/538c968a8486

Comment 10

[Carla Nadastean]

Verified with:
12.0a1 (2012-01-09) Device: LG Optimus 2X Android(2.2)
11.0a2 (2012-01-09) Device: LG Optimus 2X Android(2.2)

"Open link in new tab" is no longer displayed on the Amazon product image, since is not a link.

Comment 11

[Carla Nadastean]

In addition to Comment10 :
Verified with:
12.0a1 (2012-01-11) Device: LG Optimus 2X Android(2.2)
11.0a2 (2012-01-11) Device: LG Optimus 2X Android(2.2)