Matthew N. [:MattN] Reporter
	Description • 10 years ago

Being able to edit usernames and passwords from the Saved Passwords dialog seems like a decent way to address shortcomings in the password manager and its interaction with websites.

Being able to edit usernames would help address the following in some cases:
* Bug 516037 and bug 499649 - where the case of the username is "wrong"
* Bug 585591 - where we detect the wrong field as the username (I'm actually not 100% sure since we also store the usernameField but I don't think we require it to be the same upon filling [see bug 623910].)

Being able to edit passwords would help address the following in some cases:
* When the site adds autocomplete=off after a password was already saved. There was no way to fix the old password. (I think this is fixed now)
* It provides a manual mass password reset functionality. e.g. go through all sites using your LDAP and paste the new password for each after a force LDAP password change.

	Mardeg
	Comment 2 • 10 years ago

Is this a dupe of bug 264201?

	Chris Karlof [:ckarlof]
	Comment 4 • 10 years ago

Blocked by UX design.

	Edwin Wong [:edwong]
	Comment 5 • 9 years ago

are we still blocked by UX?

	Matthew N. [:MattN] Reporter
	Comment 6 • 9 years ago

Well, I think there is UX defined for about:passwords at https://www.lucidchart.com/documents/edit/87ab1cc8-e708-49d3-8b91-6e2e6da346fb/9 but it's unclear to me whether we should do this before about:passwords exists on desktop (in the current dialog).

	:Paolo Amadini
	Comment 7 • 9 years ago

(In reply to Edwin Wong [:edwong] from comment #5)
> are we still blocked by UX?

I do have some new designs for in-content passwords UI, based on the recent research, that I'd like to discuss with Ryan, so there may be some UX developments there.

For the rest, as far as I can tell we're blocked on resourcing, as I'm the only Desktop developer specifically focused on the Logins front-end, and I'm working on the Fill Doorhanger right now.

	:Paolo Amadini
	Comment 8 • 9 years ago

(In reply to Matthew N. [:MattN] from comment #6)
> but it's unclear to me whether we should do this before
> about:passwords exists on desktop (in the current dialog).

I think Matt is right that we may also want to do this earlier, independently from the new UX.

	Surya Prakash
	Comment 9 • 9 years ago

Thanks for initiating this project. I am a UX designer too. Can I suggest any new design thoughts to this project? Possible?

	:Paolo Amadini
	Comment 10 • 9 years ago

(In reply to Surya Prakash from comment #9)
> Thanks for initiating this project. I am a UX designer too. Can I suggest
> any new design thoughts to this project? Possible?

Thanks for your interest! I'd say the simplest thing to do would be to attach mockups to this bug, or alternatively put a link to them in a comment. Don't worry too much about placing them in the correct bug at this point - we can always move things around as necessary. If this bug becomes focused on UX we can move the implementation to a different bug. In fact, we used to have a newsgroup for UX design but it's not been particularly active recently.

I'm CC'ing Ryan Feeley who curates UX for the Logins project.

	Ryan Feeley [:rfeeley]
	Comment 11 • 9 years ago

(In reply to Surya Prakash from comment #9)
> Thanks for initiating this project. I am a UX designer too. Can I suggest
> any new design thoughts to this project? Possible?

I am watching this bug Surya and am open to ideas. Please review the latest UX proposal, and feel free to suggest changes: http://is.gd/password_manager_ux

	Riadh Chtara[:rchtara] Assignee
	Comment 12 • 9 years ago

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 13 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Matthew N. [:MattN] Reporter
	Comment 14 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

https://reviewboard.mozilla.org/r/13375/#review12099

Cool, that was quick!

::: toolkit/components/passwordmgr/content/passwordManager.js:113
(Diff revision 2)
> +      table[row].timePasswordChanged = Date.now();

If I tab out of the editable password field I get:
TypeError: table\[row\] is undefined (passwordManager.js:113:40)
NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: \[JavaScript Error: "table\[row\] is undefined" \{file: "chrome://passwordmgr/content/passwordManager.js" line: 113\}\]'\[JavaScript Error: "table\[row\] is undefined" \{file: "chrome://passwordmgr/content/passwordManager.js" line: 113\}\]' when calling method: \[nsITreeView::setCellText\] (tree.xml:405:0)

and the field gets in a broken state while the focus ring moves to the buttons.

::: toolkit/components/passwordmgr/content/passwordManager.js:235
(Diff revision 2)
>    if (e.keyCode == KeyEvent.DOM_VK_DELETE
>  #ifdef XP_MACOSX
>        || e.keyCode == KeyEvent.DOM_VK_BACK_SPACE
>  #endif
>       ) {
>      DeleteSignon();

This now needs to be adjusted otherwise passwords get deleted when the user tries to backspace in the editable field.

We can probably check the event target?

::: toolkit/components/passwordmgr/content/passwordManager.xul:26
(Diff revision 2)
>      <key keycode="VK_ESCAPE" oncommand="window.close();"/>
>      <key key="&windowClose.key;" modifiers="accel" oncommand="window.close();"/>

I think this should also be changed to not close the window while editing. Normally ESCAPE is used to undo a change during inline editing and we should do that too IMO.

As we discussed, we'll need tests for this.

	Riadh Chtara[:rchtara] Assignee
	Comment 15 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 16 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 17 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 18 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Matthew N. [:MattN] Reporter
	Comment 19 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

https://reviewboard.mozilla.org/r/13375/#review12259

::: toolkit/components/passwordmgr/content/passwordManager.js:240
(Diff revision 6)
> +  if (signonsTree.editingRow == null && signonsTree.editingColumn == null) {

Use `signonsTree.getAttribute("editing")` here too but do an early return instead of wrapping everything in the `if`.

Also, didn't the stopPropagation/preventDefault work instead?

::: toolkit/components/passwordmgr/content/passwordManager.js:448
(Diff revision 6)
> +function closeWindow() {

This method doesn't always close the window so the name should be clarified. Maybe "escapeKeyHandler"?

::: toolkit/components/passwordmgr/content/passwordManager.js:450
(Diff revision 6)
> +  if (signonsTree.editingRow == null && signonsTree.editingColumn == null) {

It looks like `signonsTree.getAttribute("editing")` may be a bit clearer but please test it works.

Early return here instead too.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:10
(Diff revision 6)
> +  var rect = tbo.getCoordsForCellItem(row, aTree.columns[column], "text");
> +  var x = rect.x + rect.width / 2;
> +  var y = rect.y + rect.height / 2;

`let` is the new `var`

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:125
(Diff revision 6)
> +            let togglePasswords = pwmgrdlg.document.querySelector("#togglePasswords");
> +            var event = new Event('command');
> +            togglePasswords.dispatchEvent(event);

`let` is the new `var`

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:130
(Diff revision 6)
> +                  editPassword(
> +                      () => {
> +                        var event = new Event('command');

`let` is the new `var`

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:5
(Diff revision 6)
> +const timeInterval = 1000;

Nit: `const` names should be uppercase with underscores between words.

::: toolkit/components/passwordmgr/content/passwordManager.js:104
(Diff revision 6)
> +      if (value !== table[row].username) {

Use an early return here and for passwordCol

::: toolkit/components/passwordmgr/content/passwordManager.js:105
(Diff revision 6)
> +        let login = table[row].clone();

How about calling this `existingLogin`?

::: toolkit/components/passwordmgr/content/passwordManager.js:112
(Diff revision 6)
> +      if (value !== table[row].password && value !== "") {

I would rather we didn't duplicate the validation logic here: `&& value !== ""`. I believe modifyLogin will throw in this case and we should just handle that appropriately.

::: toolkit/components/passwordmgr/content/passwordManager.js:107
(Diff revision 6)
> +        table[row].timePasswordChanged = Date.now();

I think this makes sense UX-wise even though technically the password didn't change.

::: toolkit/components/passwordmgr/content/passwordManager.js:111
(Diff revision 6)
> +    } else if (col.id === "passwordCol") {
> +      if (value !== table[row].password && value !== "") {
> +        let login = table[row].clone();
> +        table[row].password = value;
> +        table[row].timePasswordChanged = Date.now();
> +        passwordmanager.modifyLogin(login, table[row]);
> +        signonsTree.treeBoxObject.invalidateRow(row);
> +      }
> -  }
> +    }

The comments from "userCol" also apply here.

Since these two blocks are doing the same thing, can you make a local helper for it to avoid duplication.

::: toolkit/components/passwordmgr/content/passwordManager.js:240
(Diff revision 6)
> +  if (signonsTree.editingRow == null && signonsTree.editingColumn == null) {

Early return

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:65
(Diff revision 6)
> +      function editUsername(callback) {
> +        is(Services.logins.findLogins({}, site, "", "").length, 1);
> +        let login = Services.logins.findLogins({}, site, "", "")[0];
> +        is(login.username, oldUsername, "Correct username saved");
> +        is(getUsername(0), oldUsername, "Correct username shown");
> +        pwmgrdlg.setTimeout(
> +            () => {
> +              synthesizeDblClick(signonsTree, 1, 0);
> +              pwmgrdlg.setTimeout(
> +                  () => {
> +                    EventUtils.sendString(newUsername, pwmgrdlg);
> +                    pwmgrdlg.setTimeout(
> +                        () => {
> +                          let signonsIntro = doc.querySelector("#signonsIntro");
> +                          EventUtils.sendMouseEvent({type: "click"}, signonsIntro, pwmgrdlg);
> +                          pwmgrdlg.setTimeout(
> +                              () => {
> +                                is(Services.logins.findLogins({}, site, "", "").length, 1);
> +                                let login = Services.logins.findLogins({}, site, "", "")[0];
> +                                is(login.username, newUsername, "Correct username updated");
> +                                is(getUsername(0), newUsername, "Correct username shown");
> +                                callback();
> +                          }, timeInterval);
> +                    }, timeInterval);
> +              }, timeInterval);
> +        }, timeInterval);
> +      }

I hate to say this but this amount of nesting makes it really hard to review and maintain. New browser chrome tests should be written with add_task and promises instead of using test() and callbacks. Can you refactor this using add_task and helpers returning promises/Tasks? See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/browser_notifications.js for an example.

	Riadh Chtara[:rchtara] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 22 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Riadh Chtara[:rchtara] Assignee
	Comment 23 • 9 years ago

https://reviewboard.mozilla.org/r/13375/#review12259

> I would rather we didn't duplicate the validation logic here: `&& value !== ""`. I believe modifyLogin will throw in this case and we should just handle that appropriately.

the problem is that if we do that and an exception was thrown, we need to cancel the changes that happened in table. Otherwise the password manager and the table could have diffrent logins which causes lot of isseus.

	Matthew N. [:MattN] Reporter
	Comment 24 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

https://reviewboard.mozilla.org/r/13375/#review12935

Thanks. I addressed the review comments for you.

::: toolkit/components/passwordmgr/content/passwordManager.js:82
(Diff revision 9)
> +    if (col.id === "userCol" || col.id === "passwordCol") {

== is preferred over ===. https://developer.mozilla.org/en-US/docs/User%3AGavinSharp_JS_Style_Guidelines#JavaScript_features

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:1
(Diff revision 9)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Tests are normally public domain and no longer need this header. See http://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:50
(Diff revision 9)
> +    // Wait for the double click to be received.
> +    pwmgrdlg.setTimeout(resolve, TIME_INTERVAL);

Can't you wait for the appropriate event instead? Or at least used waitForCondition… so the test runs faster in the common case? setTimeout is generally frowned upon in tests as it leads to intermittent failures.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:141
(Diff revision 9)
> +  yield* testLoginChange("http://c.tn/", "userC","passC", "usernameC", "passwordC");
> +  yield* testLoginChange("http://b.tn/", "userB","passB", "usernameB", "passwordB");
> +  yield* testLoginChange("http://a.tn/", "userA","passA", "usernameA", "passwordA");

Nit: missing spaces

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:104
(Diff revision 9)
> +  pwmgr = Cc["@mozilla.org/login-manager;1"].getService(Ci.nsILoginManager);
> +  pwmgr.removeAllLogins();

Services.logins.removeAllLogins();

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:34
(Diff revision 9)
> +  EventUtils.synthesizeMouse(aTree.body, x, y, { clickCount: 2 },
> +                             aTree.ownerDocument.defaultView);

Did `EventUtils.sendMouseEvent({type: "dblclick", button: 0},…)` not work?

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:103
(Diff revision 9)
> +  waitForExplicitFinish();

This shouldn't be necessary for a browser-chrome test using add_task.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_editing.js:59
(Diff revision 9)
> +    pwmgrdlg.onclick = resolve;

IIRC, there can only be one .onclick handler per element so this is fragile (and also not cleaning up after itself). Please use BrowserTestUtils.waitForEvent(…) in the future.

	Pulsebot
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/1b9e05ea0145

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 9 years ago

Backed out for browser_passwordmgr_editing.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3966699&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/590f20d4425c

	Pulsebot
	Comment 27 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/89aa9083a960

	Pulsebot
	Comment 28 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/0899dac8e4f3

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/89aa9083a960
https://hg.mozilla.org/mozilla-central/rev/0899dac8e4f3

	Riadh Chtara[:rchtara] Assignee
	Comment 30 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

Bug 1001765 - Make login credentials in Saved Passwords editable

	Matthew N. [:MattN] Reporter
	Comment 31 • 9 years ago

Comment on attachment 8634314 [details]
MozReview Request: Bug 1001765 - Make login credentials in Saved Passwords editable

I already landed https://hg.mozilla.org/mozilla-central/rev/89aa9083a960

	Kamil Jozwiak [:kjozwiak]
	Comment 32 • 9 years ago

Went through verification using the following m-c build:
- http://archive.mozilla.org/pub/firefox/nightly/2015-08-11-03-02-06-mozilla-central/

OS's Used:

- OSX 10.10.4 x64 -> PASSED
- Win 8.1 x64 (VM) -> PASSED
- Win 10 x64 -> PASSED
- Ubuntu 14.04.2 x64 (VM) -> PASSED

Cases Used:

* ensured when the password is hidden, "Edit Password" is disabled via the right click context menu
* ensured when the password is visible, "Edit Password" is enabled via the right click context menu
* ensured that double clicking on the username makes the username field editable
** ensured that "Undo, Cut, Copy, Paste, Delete, Select All" work when the username field is editable
* ensured that double clicking on the password makes the password field editable when the passwords are visible
** ensured that "Undo, Cut, Copy, Paste, Delete, Select All" work when the password field is editable
* ensured that "Copy Username, Edit Username, Copy Password, Edit Password" are working correctly via the right click context menu
* ensured that "Copy Username, Edit Username, Copy Password, Edit Password" are disabled when multiple entries are selected
* changed old users credentials to a new user and ensured that you can login using the new credentials
* ensured that you cannot leave the password field blank (Pressing ESC, Cut, Delete via the context menu)
* ensured that the original username is restored when it's been removed and the user presses "ESC"
* ensured when the passwords are visible, closing the password manager hides all the password when it's re-opened
** also ensured that the passwords are hidden when closing the password manager while one of the passwords is being edited
* ensured that you cannot have duplicate username entries for the same website
* ensured that selecting "Show Passwords" prompts the Master Password dialog if the feature is enabled
* ensured that selecting “Copy Password” under the context menu prompts the Master Password dialog if the feature is enabled
* ensured that all of the above cases also worked when viewing the password manager via "View Page Info -> Security -> View Saved Passwords"

Bugs Found:
- bug # 1194026 - username field stays selected when using the same username that already exists for the same site

I've also created the following One & Done task so our community of contributors can help us test:
- https://oneanddone.mozilla.org/en-US/tasks/138/ (Editing Username & Password's via the "Saved Passwords" dialog)

Let me know if there's anything obvious that I might have missed while going through testing/verification!