Add a "Copy link location" action to the context menu for links

VERIFIED FIXED in Firefox 17

Status

()

Firefox for Android
General
P3
enhancement
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: mbrubeck, Assigned: mcomella)

Tracking

(Blocks: 1 bug, {mobile, polish, pp})

Trunk
Firefox 17
All
Android
mobile, polish, pp
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
+++ This bug was initially created as a clone of Bug #643191 +++

We added this to XUL Fennec, and we should add it to Fennec Native for the same reasons.
Duplicate of this bug: 769885
(Reporter)

Comment 2

5 years ago
This is a good bug to get you introduced a bit more to our JavaScript code.  The patch from bug 643191 will give you a good idea of how to implement it, though the NativeWindow contextmenu API is slightly different from the old XUL Fennec APIs.
Assignee: mbrubeck → michael.l.comella
I noticed from bug 664798 that "Copy email address", "Copy phone number" and "Copy image location" (if not more) were desired in the previous XUL build and I believe they were lost on the switch to the Native UI. Are these still desired? Should I file separate bugs for them (or do they already exist)?

Also, bug 718437 seems like a previously existing meta bug for this issue.
Blocks: 718437
Changed (In reply to Michael Comella (:mcomella) from comment #3)
> I noticed from bug 664798 that "Copy email address", "Copy phone number" and
> "Copy image location" (if not more) were desired in the previous XUL build
> and I believe they were lost on the switch to the Native UI. Are these still
> desired? Should I file separate bugs for them (or do they already exist)?

I changed bug 718437 into an official meta bug. Comments to this question should move to that bug's page.
Created attachment 643684 [details] [diff] [review]
Patch

I am unsure about a few things:

1) The name of the copy action in the context menu. "Copy Link" (which was in XUL) vs. "Copy Link Location" (which is specified in the bug). I chose the former since it was in production at one point.

2) The regex used in the Copyable context. I did not know which URI schemes we would want access to copy. I chose to omit "tel" and "mailto" as these would be covered by "Copy phone number" and "Copy email address" respectively (which are not currently implemented - see meta bug 718437). I am not sure if anything else should be omitted.
Attachment #643684 - Flags: review?(mark.finkle)
Comment on attachment 643684 [details] [diff] [review]
Patch

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    linkCopyableContext: {
>+      matches: function linkCopyableContextMatches(aElement) {
>+        let uri = NativeWindow.contextmenus._getLink(aElement);
>+        if (uri) {
>+          let scheme = uri.scheme;
>+          let dontCopy = /^(javascript|mailto|tel)$/;

Desktop allows javascript: links too, so we can too. Those are used for bookmarklets IIRC.
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js?force=1#359

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties
>+contextmenu.copyLink=Copy Link

 Yeah, "Copy Link" fits in better with our other actions too.

r+ with the javascript: change. Post a new patch for check-in with that change. Carry the r+ forward.
Attachment #643684 - Flags: review?(mark.finkle) → review+
Created attachment 643917 [details] [diff] [review]
Patch v2

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 643684 [details] [diff] [review]
> Patch
> 
> Desktop allows javascript: links too, so we can too. Those are used for
> bookmarklets IIRC.

Done. Moved r+ forward.
Attachment #643684 - Attachment is obsolete: true
Attachment #643917 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Comment 8

5 years ago
Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/770e6becb5e4
Keywords: checkin-needed

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/770e6becb5e4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Broken with Bug 766275. Will post patch shortly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 644515 [details] [diff] [review]
Patch v3

For breaking it, you get to review it. :P

You might want to double check Patch v2 for the other additions I reference, such as the string additions.
Attachment #644515 - Flags: review?(wjohnston)
Attachment #643917 - Attachment is obsolete: true
Comment on attachment 644515 [details] [diff] [review]
Patch v3

Review of attachment 644515 [details] [diff] [review]:
-----------------------------------------------------------------

Lol. Sorry again :(
Attachment #644515 - Flags: review?(wjohnston) → review+
No problem. I wish all my reviews were this fast! :)
Keywords: checkin-needed
Blocks: 775766
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe97ca5ab1c
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fbe97ca5ab1c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 16

5 years ago
Build ID: 17.0a2 (2012-10-08)Aurora Channel
          18.0b1 (2012-10-08) Nightly Channel
          19.0b1 (2012-10-09) Nightly Channel
Device: Samsung Galaxy Nexus
OS: Android 4.1

Copy link location is now added to the context menu for regular links.
Marking bug as verify fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.