Closed Bug 1189428 Opened 5 years ago Closed 5 years ago

Make password manager editability more discoverable

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

Attachments

(2 files)

Bug 1001765 made the username and password fields editable via double-click (the default behaviour for editable trees) but this may not be very discoverable since people may have learned from the past when it wasn't editable and because it's not like the search keyword tree where the default is that the whole column is empty.

Some ideas:
* icon/overlay when hovering the 2 columns
* tooltip on the 2 columns
* header/footer text adjacent to the tree to mention editability
Flags: qe-verify-
Flags: firefox-backlog+
Another idea is to add entry/entries to the context menu. Currently we have "Copy username" and "Copy password". We could add "Edit username" & "Edit password".
(In reply to Ryan Feeley from comment #2)
> Created attachment 8644500 [details]
> I believe this at least calls for a contextual menu. Other affordances to
> follow.

What if the user right-clicks on the other columns e.g. Site? I wonder if it's a good idea to make it harder for the user to copy their password because they right-click on the username field. Especially before we show *** in the password column, this makes it harder to Copy Passwords since it would require showing the whole password column.
Flags: needinfo?(rfeeley)
Should we should the same four items no matter where they click?

Copy username (assuming there is a username)
Copy password
Edit username (assuming there is a username)
Edit password

Does that order make sense?
Flags: needinfo?(rfeeley)
(In reply to Ryan Feeley from comment #4)
> Should we should the same four items no matter where they click?

I think so. That's seems to be how I recall context menus in tree rows usually working. 

> Copy username (assuming there is a username)
> Copy password
> Edit username (assuming there is a username)
> Edit password
> 
> Does that order make sense?

I personally would have put the username ones together with a separator:

Copy username (assuming there is a username)
Edit username (assuming there is a username)
-------------
Copy password
Edit password

> (assuming there is a username)

Good idea. The current "Copy username" menu item isn't smart about this case. I think we should disable the username fields if it's empty.
Bug 1189428 - Add edit options to the password manager context menu. r=rittme,rchtara
Attachment #8645254 - Flags: review?(rchtara)
Attachment #8645254 - Flags: review?(bernardo)
Assignee: rfeeley → MattN+bmo
Points: --- → 3
Flags: qe-verify- → qe-verify+
Whiteboard: [ux]
Comment on attachment 8645254 [details]
MozReview Request: Bug 1189428 - Add edit options to the password manager context menu. r=rittme,rchtara

https://reviewboard.mozilla.org/r/15485/#review13849

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_contextmenu.js:43
(Diff revision 1)
> -            is(isMenuitemEnabled(), false, "Copy should be disabled");
> +            is(isMenuitemEnabled("copyusername"), false, "Copy Username should be disabled");

you should probably refactor those tests, there are similar:
You can probably create a function
checkMenuItems(expectCopyUsernameEnabled, expectEditUsernameEnabled, expectCopyPasswordEnabled, expectEditPasswordEnabled)
Attachment #8645254 - Flags: review?(rchtara)
https://reviewboard.mozilla.org/r/15485/#review13849

great job :). did you pushed to the try?
Let's wait for bernardo if he is able to find something else.
Comment on attachment 8645254 [details]
MozReview Request: Bug 1189428 - Add edit options to the password manager context menu. r=rittme,rchtara

https://reviewboard.mozilla.org/r/15485/#review13855

Ship It!
Attachment #8645254 - Flags: review?(bernardo) → review+
https://reviewboard.mozilla.org/r/15485/#review13849

LGTM!

> you should probably refactor those tests, there are similar:
> You can probably create a function
> checkMenuItems(expectCopyUsernameEnabled, expectEditUsernameEnabled, expectCopyPasswordEnabled, expectEditPasswordEnabled)

It could be refactored, but I would say we shouldn't loose time with this. It's not hard to read or anything worth the time.
Now that we want to draw more attention to this context menu, it would be nice to have bug 1116304 fixed at some point. In my screen it looks really big and unnatural.
Comment on attachment 8645254 [details]
MozReview Request: Bug 1189428 - Add edit options to the password manager context menu. r=rittme,rchtara

Bug 1189428 - Add edit options to the password manager context menu. r=rittme,rchtara
Attachment #8645254 - Flags: review?(rchtara)
(In reply to Bernardo Rittmeyer [:rittme] from comment #10)
> https://reviewboard.mozilla.org/r/15485/#review13849
> > you should probably refactor those tests, there are similar:
> > You can probably create a function
> > checkMenuItems(expectCopyUsernameEnabled, expectEditUsernameEnabled, expectCopyPasswordEnabled, expectEditPasswordEnabled)
> 
> It could be refactored, but I would say we shouldn't loose time with this.
> It's not hard to read or anything worth the time.

I refactored the individual lines to reduce some duplication but left each menu item as a separate call so I could keep the explanations for some.
Attachment #8645254 - Flags: review?(rchtara) → review+
Ok cool. 
MattN: thanks for giving me the first opportunity to review a firefox patch.
Bernardo: in windows its big too,  it's weird
https://hg.mozilla.org/mozilla-central/rev/3f90176ca5ca
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
QA Contact: kjozwiak
(In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #5)
> Copy username (assuming there is a username)
> Edit username (assuming there is a username)
Note that:
1. Able to edit username if there is no username
2. Able to edit/clear the username, but unable to edit/clear the password
44.0a1 (2015-10-14) Win 7
Flags: needinfo?(MattN+bmo)
(In reply to Paul Silaghi, QA [:pauly] from comment #17)
> (In reply to Matthew N. [:MattN] (PTO Oct. 8-12) from comment #5)
> > Copy username (assuming there is a username)
> > Edit username (assuming there is a username)
> Note that:
> 1. Able to edit username if there is no username
> 2. Able to edit/clear the username, but unable to edit/clear the password
> 44.0a1 (2015-10-14) Win 7

That's expected/correct. Thanks!
Flags: needinfo?(MattN+bmo)
Status: RESOLVED → VERIFIED
UA:"Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0 SeaMonkey/2.43a1"
ID:20160109003001 en-US
c-c:4f4f6a3674c9e16efacf62fd8963c8be5a31b07c
m-c:0f363ae95dc90d593394ef464aa500804c824962

On this SeaMonkey build, the Data Manager (which is what Tools → Password Manager → Manage Stored Passwords opens) is still in limbo, but the Firefox-like password manager can be opened by browsing to chrome://passwordmgr/content/passwordManager.xul and there, right-clicking any line opens a context menu which includes (among others) "Edit Password". Double-clicking a password will also reveal it and make it editable; loss of focus hides it again.

tl;dr: This fix works even on SeaMonkey, if you know how to use it.
P.S. Usernames can be edited in the same way, and both can be copied: the full context menu has:

Copy Username
Edit Username
-------------
Copy Password
Edit Password
Edit
Clone
Visit site(s)
You need to log in before you can comment on or make changes to this bug.