Closed Bug 1536728 Opened 2 years ago Closed 10 months ago

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

Categories

(Toolkit :: Password Manager, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- disabled
firefox76 --- fixed

People

(Reporter: MattN, Assigned: sfoster)

References

(Depends on 5 open bugs, Blocks 2 open bugs, )

Details

(Keywords: parity-chrome, uiwanted, Whiteboard: [passwords:capture-UI])

Attachments

(5 files, 1 obsolete file)

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.

Keywords: uiwanted
See Also: → 1548855
Flags: qe-verify+

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.

Flags: needinfo?(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)

Flags: needinfo?(rgaddis)
Priority: P2 → P1
Depends on: 1585952
Blocks: 1585952
No longer depends on: 1585952

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.

Flags: needinfo?(kcaldwell)

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

Flags: needinfo?(kcaldwell)

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.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Depends on: 1388674
Blocks: 1598541
  • No functional changes.
  • Rename *GeneratedPasswordFilledOrEdited to passwordFilledOrEdited and pass a 'generated' flag

Depends on D24556

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?

Flags: needinfo?(sfoster)

(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.

Flags: needinfo?(sfoster)

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

Flags: needinfo?(MattN+bmo)

(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.

Flags: needinfo?(MattN+bmo)
Whiteboard: [passwords:capture-UI]

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.

[Tracking Requested - why for this release]:

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

Flags: needinfo?(sfoster)

(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.

Flags: needinfo?(sfoster)
Attachment #9110780 - Attachment description: Bug 1536728 - (WIP) Show dismissed save/update doorhanger when password value is changed. r?MattN → Bug 1536728 - (WIP) Show dismissed save/update doorhanger when password value is changed
Attachment #9116803 - Attachment is obsolete: true
Attachment #9110780 - Attachment description: Bug 1536728 - (WIP) Show dismissed save/update doorhanger when password value is changed → Bug 1536728 - Show dismissed save/update doorhanger when password value is changed. r?MattN
Blocks: 1612255
Blocks: 1612257
Blocks: 1612258

[Tracking Requested - why for this release]:

Attachment #9125996 - Attachment description: Bug 1536728 - Reset anchorElement (icon) extraAttr when hiding notification icons. r?MattN → Bug 1536728 - Reset anchorElement (icon) extraAttr when showing notification icons. r?MattN
Pushed by sfoster@mozilla.com:
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

Not sure I need to track this.

Pushed by sfoster@mozilla.com:
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

(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.

Flags: needinfo?(sfoster)

(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.

Depends on: 1618587
No longer depends on: formless-logins
Depends on: 1618696
Depends on: 1620482
Depends on: 1627651
Depends on: 1627658
Depends on: 1630194
Depends on: 1632405
Depends on: 1632432
Depends on: 1633383
You need to log in before you can comment on or make changes to this bug.