1334663 - Allow users to copy saved credential site URL via right-click

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Description

[Ryan Feeley [:rfeeley]]

Attachment: actual.png

STEPS TO REPRODUCE
1. Have at least one saved login or password in Saved Logins…
2. Visit Preference/Options > Security > Saved Logins
3. Right-click on the site column of an existing login or password

EXPECTED RESULTS
- User is able to go to and copy site URL

ACTUAL RESULTS
- No site actions are possible

I'd like to propose that we add a top section to this dropdown that includes:

Copy Site URL
Go to Site URL
----------- < divider

Go to Site URL opens the URL in a new tab.

Comment 1

[Matthew N. [:MattN]]

Relevant code at:
* https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/toolkit/components/passwordmgr/content/passwordManager.js#659
* https://dxr.mozilla.org/mozilla-central/rev/25a94c1047e793ef096d8556fa3c26dd72bd37d7/toolkit/components/passwordmgr/content/passwordManager.xul#33

Comment 2

[Steve Jarvis [:sajarvis]]

Attachment: Bug 1334663 - Add ability to copy and launch URL from password manager

Review commit: https://reviewboard.mozilla.org/r/115614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115614/

Comment 3

[Steve Jarvis [:sajarvis]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115614/diff/1-2/

Comment 4

[Steve Jarvis [:sajarvis]]

Attachment: Missing window elements

Hey MattN,

I've been working on this and have opened a review with my progress so far (just for reference). I've hit an issue, though. With the additional entries in the menu, some objects in the password manager window fail to render, like text on the buttons and the saved sites/credentials themselves (this is shown in the attachment, and I do have a site saved there). 

There's no obvious error logged to the process stdout or to the devtools console. I'm stumped. Any idea what's causing this or how I could go about further debugging? 

I'm on Ubuntu 16.10.

Thanks.

Comment 5

[Matthew N. [:MattN]]

Hi Steve, are you using the Browser Console or Browser Toolbox? You won't see it in the Web Console.

https://developer.mozilla.org/en-US/docs/Tools/Browser_Console

Comment 6

[Steve Jarvis [:sajarvis]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #5)
> Hi Steve, are you using the Browser Console or Browser Toolbox? You won't
> see it in the Web Console.
> 
> https://developer.mozilla.org/en-US/docs/Tools/Browser_Console

Thanks MattN, I didn't actually know of the Browser Console (I'm still pretty new here) but that helped me find my error. 

Searching the toolbox/ code I find examples for opening a new tab in unit tests, calling something like "gBrowser.selectedTab = gBrowser.addTab(url)" to launch and focus a new tab. That seems to trace back to BrowserTestUtils.jsm.

The only example I found for non unit test javascript code (in this area of the tree at least) is launching tabs from other settings panels, and that's setting "href = url" on label elements, so I don't think appropriate for this context menu.

So, this seems a bit silly, but what's the prescribed way to open a new tab here?

Comment 7

[Matthew N. [:MattN]]

Hi Steve,

I also struggled with this recentlys ince toolkit/ is technically not supposed to depend on browser/ since toolkit is used by other applications such as Thunderbird which may not even have the ability to open a tab to a webpage. There are APIs to open new windows for arbitary  toolkit applications but not currently tabs. I would be fine with using the following:
```
Services.wm.getMostRecentWindow("navigator:browser").openUILinkIn("http://www.mozilla.org/", "tab")
```

but we would only want to show the "Go to Site URL" menu item if `Services.wm.getMostRecentWindow("navigator:browser")` is truthy i.e. it won't show in Thunderbird IIUC.

Please use the "need more information" option below the comment field when you have questions to ensure I see them. Thanks.

Comment 8

[Steve Jarvis [:sajarvis]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115614/diff/2-3/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review119480

Thanks. This is looking good. For the issues of getMostRecentWindow I'm going to think about a solution for this. It's late here now but in the meantime you can fix the other issues.

::: toolkit/components/passwordmgr/content/passwordManager.js:640
(Diff revision 3)
> +function CopySiteUrl() {
> +  // Copy selected site url to clipboard
> +  let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> +                  getService(Ci.nsIClipboardHelper);
> +  let row = signonsTree.currentIndex;
> +  let url = signonsTreeView.getCellText(row, {id : "siteCol" });

Nit: no space before the colon. (I see that the old code has the same problem but that should get fixed once that eslint rule is enabled later).

::: toolkit/components/passwordmgr/content/passwordManager.js:642
(Diff revision 3)
> +  let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> +                  getService(Ci.nsIClipboardHelper);
> +  let row = signonsTree.currentIndex;
> +  let url = signonsTreeView.getCellText(row, {id : "siteCol" });
> +  clipboard.copyString(url);
> +  Services.telemetry.getHistogramById("PWMGR_MANAGE_COPIED_SITE_URL").add(1);

I understand you probably did this for consistency (which is normally good) but I don't think it's worth the hassle of data privacy review to collect data on these new items since I don't see how it's very actionable. Can you remove the histogram/telemetry additions please?

::: toolkit/components/passwordmgr/content/passwordManager.js:677
(Diff revision 3)
>    signonsTree.startEditing(row, signonsTree.columns.getColumnFor(columnElement));
>  }
>  
> +function LaunchSiteInNewTab() {
> +  let row = signonsTree.currentIndex;
> +  let url = signonsTreeView.getCellText(row, {id : "siteCol" });

Ditto

::: toolkit/components/passwordmgr/content/passwordManager.js:680
(Diff revision 3)
> +  let window = Services.wm.getMostRecentWindow("navigator:browser");
> +  if (window) {

There is a case I didn't think about when I gave this solution: It's possible to have the password manager window open without a browser window (on OS X via Page Info) and in that case we would want to open a new window to load the site.

::: toolkit/components/passwordmgr/content/passwordManager.js:705
(Diff revision 3)
>    }
>  
>    let selectedRow = signonsTree.currentIndex;
>  
> +  // Don't display "Launch Site URL" if we're not in a browser.
> +  if (Services.wm.getMostRecentWindow("navigator:browser")) {

Hmm… I don't think this call is so cheap that we want to do it every time the popup opens as it may noticeably delay the rendering of the menu.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review119480

> There is a case I didn't think about when I gave this solution: It's possible to have the password manager window open without a browser window (on OS X via Page Info) and in that case we would want to open a new window to load the site.

Btw. the other code where I recently needed something similar from toolkit didn't have to handle this case since it was guaranteed to be running in a window. After spending a lot of time researching options I think the following will work.

Add this above "passwordManager.js" in passwordManager.xul:
```xml
#if defined(MOZ_BUILD_APP_IS_BROWSER)
  <script type="application/javascript" src="chrome://browser/content/utilityOverlay.js"/>
#endif
```

then the JS can just look like:
```js
window.openUILinkIn(url, "tab");
```

> Hmm… I don't think this call is so cheap that we want to do it every time the popup opens as it may noticeably delay the rendering of the menu.

With the above you can simply check
```js
if (window.openUILinkIn) {…
```
which will be much faster.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review120810

See my previous comments for some things to change.

Comment 12

[Steve Jarvis [:sajarvis]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115614/diff/3-4/

Comment 13

[Steve Jarvis [:sajarvis]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #11)
> Comment on attachment 8841389 [details]
> Bug 1334663 - Add ability to copy and launch URL from password manager
> 
> https://reviewboard.mozilla.org/r/115614/#review120810
> 
> See my previous comments for some things to change.

Thanks MattN, think I got everything addressed. And thanks particularly for finding that utilityOverlay.js solution. The review says "r? cleared", and I'm not sure what "cleared" part means, so I'm going to tag you here just in case that isn't working like I think it is.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review121318

Hi Steve,

This is great but I noticed the string doesn't match the suggestion from Ryan on the UX team (see below):


(In reply to Steve Jarvis [:sajarvis] from comment #13)
> The review says "r? cleared", and
> I'm not sure what "cleared" part means, so I'm going to tag you here just in
> case that isn't working like I think it is.

This is a confusing thing about mozreview. It doesn't seem to update that text when re-requesting review after a reviewed clears the flag. The important part which puts it in my queue is the "review? MattN" that shows on the attachments section of the bugzilla bug.

::: toolkit/components/passwordmgr/content/passwordManager.js:640
(Diff revision 4)
> +function CopySiteUrl() {
> +  // Copy selected site url to clipboard
> +  let clipboard = Cc["@mozilla.org/widget/clipboardhelper;1"].
> +                  getService(Ci.nsIClipboardHelper);
> +  let row = signonsTree.currentIndex;
> +  let url = signonsTreeView.getCellText(row, {id: "siteCol" });

Nit: I don't know what we'll decide on for eslint but you should be consistent about putting spaces inside curly braces for a single line object literal. You put a space after "siteCol" but not before `id`.

::: toolkit/components/passwordmgr/content/passwordManager.js:676
(Diff revision 4)
>    signonsTree.startEditing(row, signonsTree.columns.getColumnFor(columnElement));
>  }
>  
> +function LaunchSiteUrl() {
> +  let row = signonsTree.currentIndex;
> +  let url = signonsTreeView.getCellText(row, {id: "siteCol" });

Nit: same here

::: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd:49
(Diff revision 4)
> +<!ENTITY      launchSiteUrlCmd.label          "Launch Site">
> +<!ENTITY      launchSiteUrlCmd.accesskey      "L">

(Quoting Ryan Feeley [:rfeeley] from comment #0)
> Copy Site URL
> Go to Site URL
> ----------- < divider

Did you discuss the change from "Go to Site URL" to "Launch Site" with Ryan? I didn't look closely at the exact strings in prior revisions. I would rather we agree on a string before landing.

Comment 15

[Ryan Feeley [:rfeeley]]

I'm also OK with:

Copy URL
Visit URL

Would probably translate better.

Comment 16

[Steve Jarvis [:sajarvis]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review121318

> (Quoting Ryan Feeley [:rfeeley] from comment #0)
> > Copy Site URL
> > Go to Site URL
> > ----------- < divider
> 
> Did you discuss the change from "Go to Site URL" to "Launch Site" with Ryan? I didn't look closely at the exact strings in prior revisions. I would rather we agree on a string before landing.

I did not, I thought the original proposal was more just an idea, I didn't realize. Based on Ryan's last comment that translation might be simpler, I'll go with "Copy URL" and "Visit URL".

Comment 17

[Steve Jarvis [:sajarvis]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115614/diff/4-5/

Comment 18

[Matthew N. [:MattN]]

Steve, sorry to delay this but since there doesn't seem to be an obvious agreed-on string I think it's best to ask our copy writer since changing strings later is annoying. I'm now thinking that "URL" is too much jargon.

Michelle, can you please look at attachment 8831297 [details] and help decide on two new strings to add to the context menu for a saved login? We want to add a menu item to copy the URL from the "Site" column and another to open that URL in a new tab.

Proposed arrangement (the first two are the ones we're discussing).
---------------
Copy Site URL
Go to Site URL
---------------
Copy Username
Edit Username
---------------
Copy Password
Edit Password
---------------

Ideas for copying: 
* Copy Site URL
* Copy Site Location – aligns with our context menu on links which says "Copy Link Location"
* Copy Site
* Copy URL

Ideas for opening the site:
* Go to Site URL
* Launch Site
* Open Site in New Tab – aligns with our context menu on links which says "Open Link in New Tab"
* Open in New Tab – perhaps confusing since the username and password of the row won't have an effect on the result, only the URL will. i.e. if you have two saved logins for the same site this menu item will do the same thing regardless of the highlighted login for the site.
* Open URL

I think we avoid using the word "URL" where possible but I could be wrong. Note that the column heading is "Site".

Thanks

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

Clearing until we get copy feedback

Comment 20

[Steve Jarvis [:sajarvis]]

Matt, I emailed Michelle a couple weeks ago, but haven't heard back there either. Is there anyone else on the UX team that can give input on this?

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8841389 [details]
Bug 1334663 - Add ability to copy and launch URL from password manager

https://reviewboard.mozilla.org/r/115614/#review204278

Sory for the delay. I was hoping Michelle would have eventually replied. Let's just land this I guess.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f1108e504819bda504a11dcaa30a95824638a28

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14612501cb6
Add ability to copy and launch URL from password manager. r=MattN,rfeeley

Comment 23

[Dorel Luca [:dluca]]

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