1121463 - Provide 'copy link/URL' option when long-pressing hyperlinks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Chris Lord [:cwiiis]]

Now that we have copy/paste support, copying the URL of a link or image should be added to the long-press menu. Currently, you have to visit a URL before you have the chance to copy it.

Perhaps this could be done via the 'Share Link' menu? That seems an obvious place for it, and would also provide the functionality in the system browser too. Not sure if this is the right component...

Suggesting blocking on 2.2, as this is really obvious, missing functionality of a copy/paste implementation.

Comment 1

[Peter Chang[:pchang]]

(In reply to Chris Lord [:cwiiis] from comment #0)
> Now that we have copy/paste support, copying the URL of a link or image
> should be added to the long-press menu. Currently, you have to visit a URL
> before you have the chance to copy it.
> 
> Perhaps this could be done via the 'Share Link' menu? That seems an obvious
> place for it, and would also provide the functionality in the system browser
> too. Not sure if this is the right component...
> 
> Suggesting blocking on 2.2, as this is really obvious, missing functionality
> of a copy/paste implementation.
As we discussed with EPM before, the image support is not in v2.2 scope, move this item to V3 planning.

Comment 2

[Chris Lord [:cwiiis]]

(In reply to peter chang[:pchang][:peter] from comment #1)
> (In reply to Chris Lord [:cwiiis] from comment #0)
> > Now that we have copy/paste support, copying the URL of a link or image
> > should be added to the long-press menu. Currently, you have to visit a URL
> > before you have the chance to copy it.
> > 
> > Perhaps this could be done via the 'Share Link' menu? That seems an obvious
> > place for it, and would also provide the functionality in the system browser
> > too. Not sure if this is the right component...
> > 
> > Suggesting blocking on 2.2, as this is really obvious, missing functionality
> > of a copy/paste implementation.
> As we discussed with EPM before, the image support is not in v2.2 scope,
> move this item to V3 planning.

Ok, image support isn't in scope - but hyperlinks? I really miss this functionality, and it's a shame to have to impose that a user has to start loading a URL (and then go through several steps) to copy a URL, rather than just being able to long-press it. Especially as loading a URL can cause the app that housed the link to be killed due to memory use.

Comment 3

[Gregor Wagner [:gwagner]]

Tim, can you take a look here and see if there is an easy fix?

Comment 4

[Peter Chang[:pchang]]

(In reply to Chris Lord [:cwiiis] from comment #2)
> (In reply to peter chang[:pchang][:peter] from comment #1)
> > (In reply to Chris Lord [:cwiiis] from comment #0)
> > > Now that we have copy/paste support, copying the URL of a link or image
> > > should be added to the long-press menu. Currently, you have to visit a URL
> > > before you have the chance to copy it.
> > > 
> > > Perhaps this could be done via the 'Share Link' menu? That seems an obvious
> > > place for it, and would also provide the functionality in the system browser
> > > too. Not sure if this is the right component...
> > > 
> > > Suggesting blocking on 2.2, as this is really obvious, missing functionality
> > > of a copy/paste implementation.
> > As we discussed with EPM before, the image support is not in v2.2 scope,
> > move this item to V3 planning.
> 
> Ok, image support isn't in scope - but hyperlinks? I really miss this
> functionality, and it's a shame to have to impose that a user has to start
> loading a URL (and then go through several steps) to copy a URL, rather than
> just being able to long-press it. Especially as loading a URL can cause the
> app that housed the link to be killed due to memory use.

Morris, any thought about hyperlink support on gecko side?

Comment 5

[mac]

Currently we have two lacks here:
- as the bug author wrote, copying URL
- but also it should be possible to copy text of the link, not only the effective URL, currently it is impossible to copy text of the link

In my opinion there should be two more options in context menu after long pressing on the link:
- copy URL
- copy text of the link

The second should be rethinked because I can imagine situation when user doesn't want to copy entire text of the link, but i.e. few words from it (a part of it).

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Bug 1012662 provide new api about copy/paste. So we should able to use this feature to resolved this bug.

Comment 7

[Boris Chiou [:boris]]

This bug is dependent on Bug 952456, which implements gonk/nsClipboard.cpp for text/html and image/* mime types.

Comment 8

[Boris Chiou [:boris]]

Attachment: [WIP] Part 1: Add "Copy Link" option (gaia) (v1)

Provide 'copy link/URL' option when long-pressing hyperlinks

Comment 9

[Boris Chiou [:boris]]

Attachment: [WIP] Part 2: Add "Copy Image" (v1)

Comment 10

[Boris Chiou [:boris]]

Attachment: Add "Copy Link" and "Copy Image" option (gaia) (v2)

Provide 'copy link/url' and "copy image" option when long-pressing hyperlinks

Comment 11

[Boris Chiou [:boris]]

Hi Tim,

Should we implement "Copy Image" in this bug? According to Peter's comment, it's not in V2.2 scope, but I think we should update this info now.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It's not in v2.2.

Comment 13

[Boris Chiou [:boris]]

Move the "copy image" change into Bug 952456.

Comment 14

[Boris Chiou [:boris]]

Attachment: Part 1: Support copy links (v1)

Comment 15

[Boris Chiou [:boris]]

Attachment: Part 2: Add copy images/links marionette test (v1)

Comment 16

[Boris Chiou [:boris]]

Attachment: [WIP] Support copy link (v1, gecko)

Comment 17

[Boris Chiou [:boris]]

Attachment: Part 1: Support copy links (v1) [gaia]

Comment 18

[Boris Chiou [:boris]]

Attachment: Part 2: Add copy links marionette test (v1) [gaia]

Comment 19

[Boris Chiou [:boris]]

Attachment: Part 2: Add copy links marionette test (v2) [gaia]

Comment 20

[Boris Chiou [:boris]]

Attachment: Part 3: Add copy images marionette test (v1) [gaia]

Comment 21

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v2)

Add a new BrowserElement API, copyUrl, for copying links

Comment 22

[GitHub Autolander Bot]

Attachment: [gaia] BorisChiou:Bug1121463 > mozilla-b2g:master

Comment 23

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56b4a211bc7e

Comment 24

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=162d71f91a23

Comment 25

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #21)
> Created attachment 8652202 [details] [diff] [review]
> Support copy link on context menu (v2)
> 
> Add a new BrowserElement API, copyUrl, for copying links

Hi Kanru,

I add a new BrowserElement API for this because we have to pass a url to doCommmand('copy') on child side. I am not sure adding a new API is suitable for this case. If you have a better idea, please feel free to give me advice. Thanks.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Could you use the same mechanism used by copy image?

Comment 27

[Boris Chiou [:boris]]

(In reply to Kan-Ru Chen [:kanru] from comment #26)
> Could you use the same mechanism used by copy image?

Sure, but I have to defined another callback function because the original callback, "contextMenuItemSelected", cannot pass the URL.

Comment 28

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v3)

Add a special menuitem, copy-link, to pass the url and do command.

Comment 29

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v4)

Add a special menuitem, copy-link, to pass the url and do command.

Comment 30

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=329120ebb54e

Comment 31

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v5)

Add a special menuitem, copy-link, to pass the url and do command.

Fix mochitest test fail

Comment 32

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=94ae1b1a6c50

Comment 33

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8654472 [details] [diff] [review]
Support copy link on context menu (v5)

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1243,5 @@
> +        let linkHtml = '<a href="' + data.json.content + '">' + data.json.content + '</a>';
> +        e.clipboardData.setData('text/html', linkHtml);
> +        e.preventDefault();
> +      });
> +      content.document.execCommand('copy');

Choose this._recvDoCommand() or content.document.execCommand() for both copy-image and copy-link but don't mix them.

@@ +1275,5 @@
>      }
> +    // "Copy Link" menu item
> +    if (copyableElements.link) {
> +      menuObj.items.push({id: 'copy-link', content: copyableElements.link});
> +    }

You don't need this additional "content" attribute right? Like copy-image could find the current EventTarget and use the image, copy-link should also be able to use the current EventTarget.

Comment 34

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v6)

Add a special menuitem, copy-link, to pass the url and do command.

Comment 35

[Boris Chiou [:boris]]

Comment on attachment 8654472 [details] [diff] [review]
Support copy link on context menu (v5)

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +1243,5 @@
> +        let linkHtml = '<a href="' + data.json.content + '">' + data.json.content + '</a>';
> +        e.clipboardData.setData('text/html', linkHtml);
> +        e.preventDefault();
> +      });
> +      content.document.execCommand('copy');

Sure, Please check my new patch. Thanks for your comments

@@ +1275,5 @@
>      }
> +    // "Copy Link" menu item
> +    if (copyableElements.link) {
> +      menuObj.items.push({id: 'copy-link', content: copyableElements.link});
> +    }

Yes, I agree.

Comment 36

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebdb31891f6f

Comment 37

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8654781 [details] [diff] [review]
Support copy link on context menu (v6)

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

LGTM

Comment 38

[Boris Chiou [:boris]]

Attachment: Support copy link on context menu (v7, carry kanru's r+)

Add a special menuitem, copy-link, to pass the url and do command.

Fix mochitest fail

Comment 39

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8652205 [details] [review]
[gaia] BorisChiou:Bug1121463 > mozilla-b2g:master

I'm not qualified to review the gaia part.

Comment 40

[Boris Chiou [:boris]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4888bd08bb00

Comment 41

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #40)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4888bd08bb00

After we merge the gecko code, The gij test of gaia auto tester would pass.(In reply to Boris Chiou [:boris] from comment #40)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4888bd08bb00

We have to merge gecko & gaia code together to pass the new test cases.

Comment 42

[Boris Chiou [:boris]]

We have to merge gecko & gaia code together to pass the new test cases.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/758345e52ad6

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/e2fab8f6ac345ecde10a1350e699be9ceb6987d6

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/758345e52ad6

Comment 46

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on the latest build of Flame KK 2.5 and Aires KK 2.5 by the STR in comment 0.

Actual results: It shows 'Copy Link' or "Copy Image" option when long-pressing hyperlinks or image in Browser, the link/url can be copied and pasted. 
See attachment: verified_Flame_v2.5.3gp
Reproduce rate: 0/10


Device: Flame KK 2.5 (Pass)
Build ID               20150913150208
Gaia Revision          4d9b996be4b1935651057d0651461c1a36d98a18
Gaia Date              2015-09-13 10:47:32
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac
Gecko Version          43.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150913.184300
Firmware Date          Sun Sep 13 18:43:14 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5 (Pass)
Build ID               20150913195924
Gaia Revision          4d9b996be4b1935651057d0651461c1a36d98a18
Gaia Date              2015-09-13 10:47:32
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac
Gecko Version          43.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20150913.192006
Firmware Date          Sun Sep 13 19:20:14 UTC 2015
Bootloader             s1

Comment 47

[Shally Li（leave from Mozilla）]

Attachment: verified_Flame_v2.5.3gp