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

Status: VERIFIED FIXED

Description

[Matt Brubeck (:mbrubeck)]

+++ 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.

Comment 2

[Matt Brubeck (:mbrubeck)]

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.

Comment 3

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 4

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 5

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 6

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

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.

Comment 7

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 8

[Matt Brubeck (:mbrubeck)]

Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/770e6becb5e4

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/770e6becb5e4

Comment 10

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Broken with Bug 766275. Will post patch shortly.

Comment 11

[Michael Comella (:mcomella) [needinfo or I won't see it]]

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.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 644515 [details] [diff] [review]
Patch v3

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

Lol. Sorry again :(

Comment 13

[Michael Comella (:mcomella) [needinfo or I won't see it]]

No problem. I wish all my reviews were this fast! :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe97ca5ab1c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbe97ca5ab1c

Comment 16

[Ioana Chiorean]

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.