1178855 - Hide password visibility checkbox when users close the capture doorhanger

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P4)

Description

[Ryan Feeley [:rfeeley]]

When users close the capture password door hanger by clicking on the close icon or by selecting not now, the door hanger should close and the key icon should not appear. If there are saved logins for the site, the key icon can remain.

Comment 1

[Florian Quèze [:florian]]

(In reply to Ryan Feeley from comment #0)
> When users close the capture password door hanger by clicking on the close
> icon or by selecting not now, the door hanger should close and the key icon
> should not appear.

I think the behavior of "not now" in door hangers is always expected to be a way for the user to temporarily dismiss the panel without having to make a decision immediately, so I think what you see as a bug was actually by design.

But anyway, this isn't a 'Device Permissions' issue, moving to 'Password Manager'.

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> (In reply to Ryan Feeley from comment #0)
> > When users close the capture password door hanger by clicking on the close
> > icon or by selecting not now, the door hanger should close and the key icon
> > should not appear.
> 
> I think the behavior of "not now" in door hangers is always expected to be a
> way for the user to temporarily dismiss the panel without having to make a
> decision immediately, so I think what you see as a bug was actually by
> design.
> 

+1.  The user should be able to ignore the key temporarily and get back to it if they want to when they are finished with whatever they are reading on the page.

Comment 3

[Ryan Feeley [:rfeeley]]

That's what click elsewhere would continue to do. But "not now" or "x" is a signal IMHO of "not until next time" (not "leave me alone"). We need a way to expose the saved logins, and this is in the way.

Comment 4

[Tanvi Vyas[:tanvi]]

(In reply to Ryan Feeley from comment #3)
> That's what click elsewhere would continue to do. But "not now" or "x" is a
> signal IMHO of "not until next time" (not "leave me alone"). We need a way
> to expose the saved logins, and this is in the way.

Does that mean we are keeping the x?

Comment 5

[Ryan Feeley [:rfeeley]]

For now, yes.

Comment 6

[Ryan Feeley [:rfeeley]]

User testing last year confirmed a few things:
1. When users click "x" or "not now" they assume that the credentials are not saved. They assume it is a "close" action and not "collapse" action as it currently behaves. 
2. They do not notice the key icon after they have "closed" the save password door hanger
3. While they are on a site where they have decided not to save the login, exposing their credentials is only two-clicks away from someone with physical access.
4. Users like being able to show the password, but it needs to be more explicit, and not risk #3.

Therefore I would like to see this prioritized highly as it paves the way for future password management work.

Comment 7

[Tanvi Vyas[:tanvi]]

Making this block the meta bug for Password Manager Security, since 3 below could cause a local attacker (friend, family member, etc) to learn about a password that the user never wanted to save.

(In reply to Ryan Feeley [:rfeeley] from comment #6)
> 3. While they are on a site where they have decided not to save the login,
> exposing their credentials is only two-clicks away from someone with
> physical access.
> 4. Users like being able to show the password, but it needs to be more
> explicit, and not risk #3.
>

Comment 8

[Tanvi Vyas[:tanvi]]

Also marking for fxprivacy triage.  We can assign a priority in our Tuesday meeting.

Comment 9

[Edwin Wong [:edwong]]

I'm tracking this

Comment 10

[Ryan Feeley [:rfeeley]]

Attachment: dont-save.png

We have a lot of footage from user testing that proves that users assume when they click the X that it means "don't save" and not "collapse" as is the current behaviour. We really need to align with user expectation when it comes to security.

Comment 11

[Ryan Feeley [:rfeeley]]

Low effort to align the behavior with user expectations.

Comment 12

[Matthew N. [:MattN]]

Ryan, I think we talked before about addressing #3 by removing the checkbox (from bug 1217134) after dismissal (regardless of the dismissal method). This avoids interfering with the doorhanger plans and having this prompt have inconsistent UX compared to others.

Note that the visibility toggle in the doorhanger is Nightly-only because of follow-ups like this but I'm hoping to get those wrapped up soon so we can ship it.

Is my proposal to remove the checkbox after the initial dismissal fine for now until the larger doorhanger changes happen.

Comment 13

[Ryan Feeley [:rfeeley]]

OK. It still seems a bit odd to keep the key there when users think they have closed (and not collapsed) it, but this removes the password exposure concern.

How are you liking the SHOW functionality? I find myself using it quite a bit.

Comment 14

[Saad Quadri [:saadq]]

Attachment: Bug 1178855 - Hide password visibility toggle when capture doorhanger is closed.

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

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8761813 [details]
Bug 1178855 - Hide password visibility toggle when capture doorhanger is closed.

https://reviewboard.mozilla.org/r/58878/#review56258

Looks good! Please do a try push of mochitest-Browser before requesting checkin.

::: toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger.js:574
(Diff revision 1)
> +    info("Clicking on anchor to reshow popup.")
> +    let promiseShown = BrowserTestUtils.waitForEvent(panel, "popupshown");
> +    notif.anchorElement.click();
> +    yield promiseShown;
> +
> +    let passwordVisiblityToggle = panel.querySelector('#password-notification-visibilityToggle');

Nit: Use double-quotes

Comment 16

[Saad Quadri [:saadq]]

Comment on attachment 8761813 [details]
Bug 1178855 - Hide password visibility toggle when capture doorhanger is closed.

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

Comment 17

[Saad Quadri [:saadq]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dd3113ee2260
Hide password visibility toggle when capture doorhanger is closed. r=MattN

Comment 19

[Carsten Book [:Tomcat]]

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