Closed Bug 714789 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
fennec 11+ ---

People

(Reporter: aaronmt, Assigned: mbrubeck)

References

()

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → mbrubeck
Summary: Opting to open an image (link) in a new tab does nothing → Opting to open an image (link) and saving an image in a new tab does nothing
Priority: -- → P2
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...
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.
Status: NEW → ASSIGNED
Summary: Opting to open an image (link) and saving an image in a new tab does nothing → "Open in new link" appears on Amazon product image that is not a link (and does nothing)
Attached patch WIP (obsolete) — Splinter Review
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:mbrubeck@mozilla.com".

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.
Attached patch patchSplinter Review
Attachment #586186 - Attachment is obsolete: true
Attachment #586277 - Flags: review?(mark.finkle)
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?
Attachment #586277 - Flags: review?(mark.finkle) → review+
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 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.
Attachment #586277 - Flags: approval-mozilla-aurora?
Target Milestone: --- → Firefox 12
tracking-fennec: --- → 11+
Comment on attachment 586277 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #586277 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
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)
You need to log in before you can comment on or make changes to this bug.