Closed Bug 1121463 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox43 fixed, b2g-master verified)

RESOLVED FIXED
FxOS-S6 (04Sep)
Tracking Status
firefox43 --- fixed
b2g-master --- verified

People

(Reporter: cwiiis, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 15 obsolete files)

46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review
5.92 KB, patch
boris
: review+
Details | Diff | Splinter Review
3.18 MB, video/3gpp
Details
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.
See Also: → 1121468
No longer blocks: CopyPasteLegacy
(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.
(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.
Flags: needinfo?(pchang)
Tim, can you take a look here and see if there is an easy fix?
blocking-b2g: 2.2? → ---
(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?
Flags: needinfo?(pchang) → needinfo?(mtseng)
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).
Bug 1012662 provide new api about copy/paste. So we should able to use this feature to resolved this bug.
Depends on: 1012662
Flags: needinfo?(mtseng)
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
Depends on: 952456
This bug is dependent on Bug 952456, which implements gonk/nsClipboard.cpp for text/html and image/* mime types.
Depends on: 1186313
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.
Flags: needinfo?(timdream)
It's not in v2.2.
Flags: needinfo?(timdream)
Summary: Provide 'copy link/URL' option when long-pressing hyperlinks and images → Provide 'copy link/URL' option when long-pressing hyperlinks
Move the "copy image" change into Bug 952456.
Attachment #8642878 - Attachment is obsolete: true
Attachment #8642877 - Attachment is obsolete: true
Add a new BrowserElement API, copyUrl, for copying links
Attachment #8644232 - Attachment is obsolete: true
Attachment #8644827 - Attachment is obsolete: true
Attachment #8644829 - Attachment is obsolete: true
Attachment #8644902 - Attachment is obsolete: true
Attachment #8652202 - Flags: review?(kchen)
Attachment #8652205 - Flags: review?(kchen)
(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.
Could you use the same mechanism used by copy image?
Attachment #8653996 - Flags: review?(kchen)
Add a special menuitem, copy-link, to pass the url and do command.

Fix mochitest test fail
Attachment #8653996 - Attachment is obsolete: true
Attachment #8653996 - Flags: review?(kchen)
Attachment #8654472 - Flags: review?(kchen)
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.
Attachment #8654472 - Flags: review?(kchen) → feedback+
Add a special menuitem, copy-link, to pass the url and do command.
Attachment #8654472 - Attachment is obsolete: true
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.
Attachment #8654781 - Flags: review?(kchen)
Comment on attachment 8654781 [details] [diff] [review]
Support copy link on context menu (v6)

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

LGTM
Attachment #8654781 - Flags: review?(kchen) → review+
Add a special menuitem, copy-link, to pass the url and do command.

Fix mochitest fail
Attachment #8654781 - Attachment is obsolete: true
Attachment #8655229 - Flags: review+
Comment on attachment 8652205 [details] [review]
[gaia] BorisChiou:Bug1121463 > mozilla-b2g:master

I'm not qualified to review the gaia part.
Attachment #8652205 - Flags: review?(kchen) → review?(timdream)
Attachment #8652205 - Flags: review?(timdream) → review+
Keywords: checkin-needed
We have to merge gecko & gaia code together to pass the new test cases.
https://hg.mozilla.org/mozilla-central/rev/758345e52ad6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
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
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.