775773 - Add "Copy Image Location" to long press context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

A long press on an image should open the context menu with a "Copy Image Location" option.

Comment 1

[Arkady Blyakher [:rkd]]

Attachment: copy the link of an image into the clipboard

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

While I know it's not my review, there are a couple changes I would like to this:

0) wesj moved some of this code around. Make sure you pull the latest changes and rebase.

1) The context menu string should be "Copy Image Location" as opposed to "Copy Image Link". If the image is a link, "Copy Link" will still appear. If it is not a link, seeing "Copy Image Link" can be ambiguous. The associated variable names should also be changed.

2) With the patch in bug 775766, I added a NativeWindow.contextmenus._copyStringToDefaultClipboard function which can be used here. It would good if you could either wait for those to land in inbound or apply them yourself and update your callback function.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 644414 [details] [diff] [review]
copy the link of an image into the clipboard

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

Looks good! I agree with mcomella's comments though.

::: mobile/android/chrome/content/browser.js
@@ +1140,5 @@
> +               this.imageLinkCopyableContext,
> +               function(aTarget) {
> +                 let url = aTarget.src;
> +                 let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].getService(Ci.nsIClipboardHelper);
> +                 clipboard.copyString(url);

If we've got new helper functions, let's use 'em.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +209,5 @@
>  # Context menu
>  contextmenu.openInNewTab=Open Link in New Tab
>  contextmenu.share=Share
>  contextmenu.copyLink=Copy Link
> +contextmenu.copyImageLink=Copy Image Link

I agree about the naming. "Copy Image Location" is what we have on desktop. I'm worried that's too long, but i can't think of anything smarter.... Changing the other names to match (i.e. imageLocationCopyableContext) would be good.

I hate that we've got so many context's now.... Maybe we can do something like imageContext.extend(function() smallFunction)... That's a different problem though.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

My patches were just pushed to inbound so you'll be able to use that helper function when you pull.

(In reply to Wesley Johnston (:wesj) from comment #3)
> I agree about the naming. "Copy Image Location" is what we have on desktop.
> I'm worried that's too long, but i can't think of anything smarter....

To break it up, we definitely need the word "Copy" and we need the word "Location" or some variant of it. You might be able to get away with removing "Image" ("Copy Location") but it gets ambiguous if it's an image link.

The best I can do is "Copy Image URL" but URL may be considered too technical.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch

Took into account the comments above and reorganized some other code elements.

Note this patch builds upon the changes in bug 776064 and bug 744662 so those should be pushed before this patch, in that order.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I should mention, I left it as "Copy Image Location" and would like to create a followup UX bug to consolidate and polish the context menu entries.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 654341 [details] [diff] [review]
Patch

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

I think this patch is fine. I hate adding to the context menus. mfinnkle, ian (if you're not gone yet) do we want this?

::: mobile/android/chrome/content/browser.js
@@ -430,5 @@
>          });
>        });
>  
> -    NativeWindow.contextmenus.add(Strings.browser.GetStringFromName("contextmenu.fullScreen"),
> -      NativeWindow.contextmenus.SelectorContext("video:not(:-moz-full-screen)"),

Don't delete this stuff. Its nice to keep the blame clean in hg if possible and you're just moving it below.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +209,5 @@
>  # Context menu
>  contextmenu.openInNewTab=Open Link in New Tab
> +contextmenu.copyLink=Copy Link
> +contextmenu.copyEmailAddress=Copy Email Address
> +contextmenu.copyPhoneNumber=Copy Phone Number

Same here. No need to move this stuff around.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch v2

Moved r+. As per wesj's comment, undid reorganization to better preserve hg history.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch v3

Moved r+. Accidentally left rkd's name on the patch.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=ab6bd050d093

Parent commit looks okay: https://tbpl.mozilla.org/?rev=eccef9b3f1f1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Michael Comella (:mcomella) from comment #10)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=ab6bd050d093

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d702ef4214a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d702ef4214a

Comment 13

[Aaron Train [:aaronmt]]

Care for channel uplift here?

Comment 14

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

(In reply to Aaron Train [:aaronmt] from comment #13)
> Care for channel uplift here?

Nope. It can ride the trains.

Comment 15

[Adrian Tamas (:AdrianT)]

Behavior covered by the existing test case in the Context Menu testsuite under the BFTs testrun: https://moztrap.mozilla.org/manage/cases/_detail/5513/