1536728 - Show a dismissed login capture doorhanger when a user edits a password field (before submission)

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Matthew N. [:MattN]]

Currently we wait until we see a form "submission"[1] before showing a doorhanger to save/update a login but it will be hard to detect all times that we should offer to save as there are many different ways that applications with JavaScript can do their log in process (e.g. some combinations of window.location.href = "…", document.cookie = "…", XHR/fetch, location.reload(), <form>, etc. [see deps of bug 1119035). Since we will never cover 100% of the cases where we should prompt, we can reduce the impact of missing a capture signal by allowing the user to save before they even "submit" by showing a dismissed doorhanger as soon as a password (or username?) field is edited.

Chrome Beta 73 already seems to have this feature for password field edits. You can test with attachment 8880688 [details]. As soon as you edit the password field, the key icon appears (the panel doesn't open automatically at that point).

Advantages over manual addition:

• Doesn't require re-typing or copying+pasting the username and password.
• Doesn't require the user to correctly identify the login frame's origin (e.g. if it doesn't match the top-level window's origin).
• Allows capturing other useful data about the form such as the formActionOrigin and other currently unused metadata such as the field IDs/names.

Potential issues:

• Don't make it too easy for others to see the plaintext password in the doorhanger. We already avoid the toggle when a Master Password is set for the security conscious. Should the icon persist for the same duration even without a capture signal (i.e. across origin navigations).
• Having the icon appear in its usual icon on the left would cause a shift of the identity block and URL, potentially a bit distracting. Chrome doesn't have this problem since it's on the right side of the URL bar.
• We probably should not show the doorhanger toggle for edits to autofilled passwords since that makes them too easily revealed to users. We should check how Chrome handles this. We could either not show the doorhanger when it was autofilled previously (we already have that some of that state) or we should hide the checkbox in that case.

[1] Not necessarily a <form> 'submit' event but possibly just a navigation away or some other signal.

Comment 1

[Matthew N. [:MattN]]

Could you explore the UX of this in case we have time for it? See the "Potential issues" section above, specifically about the icon causing a visual shift.

Comment 2

[rgaddis]

Having the icon appear in its usual icon on the left would cause a shift of the identity block and URL, potentially a bit distracting. Chrome doesn't have this problem since it's on the right side of the URL bar.

I'm not worried about the shift in the URL bar, should we need to trigger an ability to access the doorhanger. I think it's more important to keep the same location for the login doorhangers.

Bonus points if we toggle it to blue if a change has been detected (that hasn't yet been saved)

Comment 3

[Sam Foster [:sfoster] (he/him)]

Potential issues:

• Don't make it too easy for others to see the plaintext password in the doorhanger. We already avoid the toggle when a Master Password is set for the security conscious. Should the icon persist for the same duration even without a capture signal (i.e. across origin navigations).

Some %age of login forms cross from e.g. login.example.com to example.com. A smaller %age of those doesn't use a form submission or other signal we can detect to know a login has been submitted. I don't know what that %age is, so its hard to know how to handle the trade-off as we balance the concerns about exposing a password via the doorhanger vs. not giving the user the opportunity to persist the password. I think there's overlap here with how we handle generated passwords. We're not going to auto-save a login for password field edits, but borrowing some of the generated password doorhanger behavior seems like a good place to start with this.

• We probably should not show the doorhanger toggle for edits to autofilled passwords since that makes them too easily revealed to users. We should check how Chrome handles this. We could either not show the doorhanger when it was autofilled previously (we already have that some of that state) or we should hide the checkbox in that case.

Katie, do you have a preference here?

Also, should we have the same dismissed capture doorhanger behavior for private windows? Or skip this entirely.

Comment 4

[katieC]

Hey Sam, based on our conversation, we decided:
• remove show password toggle from doorhanger for autofill +edit (for pages where we can't detect fields)
• behaviour should be the same in Private and non-Private windows

Comment 5

[Sam Foster [:sfoster] (he/him)]

I think my general approach here is to repurpose the generatedPasswordFilledOrEdited stuff (on both the child and parent side.) We need the same child-side edit detection and messaging (though we don't need the blur/focus event handling for non-generated password edits as we aren't going to toggle password masking.) In the parent, we need to do a lot of the same work figuring out what kind of dismissed doorhanger - if any - we need to show.

I'll refactor without functional changes and ensure nothing regresses in the first patch, and implement the non-generated password modification doorhanger in a 2nd patch. This becomes pretty straightforward with the changes in 1388674, so I'll base on those patches.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - Refactor generatedPasswordFilledOrEdited to become usable for any password filled/edited event. r?MattN

• No functional changes.
• Rename *GeneratedPasswordFilledOrEdited to passwordFilledOrEdited and pass a 'generated' flag

Depends on D24556

Comment 7

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - Show dismissed save/update doorhanger when password value is changed. r?MattN

Depends on D54263

Comment 8

[katieC]

From Ryan, Comment 2

Bonus points if we toggle it to blue if a change has been detected (that hasn't yet been saved)

Sam, did we get this in as part of this fix?

Comment 9

[Matthew N. [:MattN]]

(In reply to katieC from comment #8)

From Ryan, Comment 2

Bonus points if we toggle it to blue if a change has been detected (that hasn't yet been saved)

Sam, did we get this in as part of this fix?

This bug hasn't been resolved yet but I also don't think we should do this (at least not in this bug) as it's changing the meaning of the blue colour… now it means that the login was automatically saved but the new meaning would be different.

Comment 10

[MarniePW [:marnie]]

Matt, can we get a final decision here and close if it makes sense? Thanks.

Comment 11

[Matthew N. [:MattN]]

(In reply to Marnie Pasciuto-Wood [:marnie] from comment #10)

Matt, can we get a final decision here and close if it makes sense? Thanks.

I'm guessing that was meant for the pwgen in private browsing bug which was already resolved… we decided to ship it.

Sam is working on this bug and it isn't blocked.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - (WIP) Show dismissed save/update doorhanger when password value is changed

Depends on D54263

Comment 13

[MarniePW [:marnie]]

Per :sfoster: "That patch is almost ready but we want a full cycle to shake out any issues. So, not 73." We'll try to get this into 74.

Comment 14

[MarniePW [:marnie]]

[Tracking Requested - why for this release]:

Comment 15

[MarniePW [:marnie]]

Sam, can I please get an update on how this is going? I know we wanted it to soak for awhile before releasing it.

Comment 16

[Sam Foster [:sfoster] (he/him)]

(In reply to Marnie Pasciuto-Wood [:marnie] from comment #15)

Sam, can I please get an update on how this is going? I know we wanted it to soak for awhile before releasing it.

I'm having to rewrite some of the patch as a couple of the test failures I was investigating pointed to real differences in how we have been handling password fill/edits vs. form submits - discrepancies which become important to eliminate with this new UX.

Comment 17

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - test-only changes to ensure we get typed input into forms. r?MattN

Depends on D54263

Comment 18

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - Consolidate the PasswordFilledOrEdited and FormSubmit logic and messages. r?MattN

Depends on D60117

Comment 19

[MarniePW [:marnie]]

[Tracking Requested - why for this release]:

Comment 20

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1536728 - Reset anchorElement (icon) extraAttr when showing notification icons. r?MattN

Depends on D54263

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80ffc576da4a
Refactor generatedPasswordFilledOrEdited to become usable for any password filled/edited event. r=MattN
https://hg.mozilla.org/integration/autoland/rev/fe18718bfeb3
Reset anchorElement (icon) extraAttr when showing notification icons. r=MattN
https://hg.mozilla.org/integration/autoland/rev/155ff3fe94c7
test-only changes to ensure we get typed input into forms. r=MattN
https://hg.mozilla.org/integration/autoland/rev/dc86932a5454
Consolidate the PasswordFilledOrEdited and FormSubmit logic and messages. r=MattN
https://hg.mozilla.org/integration/autoland/rev/3c6419676ae1
Show dismissed save/update doorhanger when password value is changed. r=MattN

Comment 22

[Andreea Pavel [:apavel]]

Backed out for failing browser_context_menu_iframe.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=3c6419676ae1fb1deaec286aeae46c729a9bd351&selectedJob=288633796

Tier1 failure: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288648153&repo=autoland&lineNumber=14826

Backout: https://hg.mozilla.org/integration/autoland/rev/f2088b5ae50d3e547b370309c9608188ed853e03

Comment 23

[Julien Cristau [:jcristau]]

Not sure I need to track this.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25ec19fcfaf1
Refactor generatedPasswordFilledOrEdited to become usable for any password filled/edited event. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8dd71d9df292
Reset anchorElement (icon) extraAttr when showing notification icons. r=MattN
https://hg.mozilla.org/integration/autoland/rev/83e3e7c24012
test-only changes to ensure we get typed input into forms. r=MattN
https://hg.mozilla.org/integration/autoland/rev/667758dfe3e4
Consolidate the PasswordFilledOrEdited and FormSubmit logic and messages. r=MattN
https://hg.mozilla.org/integration/autoland/rev/6fea28a1358f
Show dismissed save/update doorhanger when password value is changed. r=MattN

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/25ec19fcfaf1
https://hg.mozilla.org/mozilla-central/rev/8dd71d9df292
https://hg.mozilla.org/mozilla-central/rev/83e3e7c24012
https://hg.mozilla.org/mozilla-central/rev/667758dfe3e4
https://hg.mozilla.org/mozilla-central/rev/6fea28a1358f

Comment 26

[Sam Foster [:sfoster] (he/him)]

(In reply to Andreea Pavel [:apavel] from comment #22)

Backed out for failing browser_context_menu_iframe.js

Thanks for the backout. It looks like that test had been failing in previous try pushes but I missed it in the noise of other failures which did get addressed.

Comment 27

[Andreea Pavel [:apavel]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #26)

(In reply to Andreea Pavel [:apavel] from comment #22)

Backed out for failing browser_context_menu_iframe.js

Thanks for the backout. It looks like that test had been failing in previous try pushes but I missed it in the noise of other failures which did get addressed.

No problem.