Password manager would really like to know when <input type=password> is added to a page but outside a <form>

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM: Core & HTML
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Dolske, Assigned: MattN)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
This is similar to bug 771331 (and bug 355063 on the front-end side).

Currently, the Firefox password manager only works on login fields that are within actual <form> elements. This isn't actually strictly required, it's just that it's the most common case and works most of the time. But we know some sites can have password fields outside of any <form> element, and a major focus of current password manager work is to make it more robust in areas we know are currently broken.

Password manager is largely driven by the DOMFormHasPassword event added in bug 771331 (an event that fires when a form element first has a password field added to it). That kicks off our whole process of filling in logins to a page. We'd like to have a similar event to fire when a password input is added to the document outside of any <form>, which currently doesn't fire that event.

I think attachment 651070 [details] [diff] [review] in the original bug might have done something like this? Not sure if that's exactly what we want, though.

One TBD issue is that with DOMFormHasPassword it was important to only fire it _once_ per form, so that forms with multiple password fields (e.g., a change-password field might have 3 password fields) didn't have password manager processing it multiple times. But when there's no <form>, we probably don't want the new event to fire just once per document. It seems far more likely that a webapp-like page would have an extremely long-loved document that we'd want to process multiple times (Eg, a user logs in, logs out, logs in as someone else, then changes their password.) Maybe the "once per" throttling should be time based? Once per X seconds?
(Reporter)

Comment 1

3 years ago
Created attachment 8588702 [details] [diff] [review]
Patch v.1 WIP

I guess bug 1120860 already found the relevant spot to trigger this, so this does the relevant parts that bug 771331 did. Functional, but no tests yet, and I'll assume I did at least one thing wrong since my C++ is rusty.
Attachment #8588702 - Flags: feedback?(bugs)

Comment 2

3 years ago
Comment on attachment 8588702 [details] [diff] [review]
Patch v.1 WIP

Could we not add mDocPasswordEventDispatcher, and just have some flag?
mDocPasswordEventDispatcher increases the size of HTMLInputElement 4 or 8 bytes, and I think there is plenty of space for bool flags.
Attachment #8588702 - Flags: feedback?(bugs) → feedback+
(Reporter)

Comment 3

3 years ago
Yeah. I was thinking we should remove this anyway, because I suspect it's (potentially) suppressing multiple events from multiple fields on the page. Pretty sure we'll want pwmgr to know about all the fields, although we'll have to figure out some of the throttling issues noted at the end of comment 0 (but any such throttling would, I think, need to be in the password manager anyway.)
(Reporter)

Comment 4

3 years ago
(NI myself to remember to update this.)
Flags: needinfo?(dolske)
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
Created attachment 8615152 [details]
MozReview Request: Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug

Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug
Attachment #8615152 - Flags: review?(bugs)
Comment on attachment 8588702 [details] [diff] [review]
Patch v.1 WIP

The patch I just attached should work for our needs and allow us to throttle in the consumer (bug 1168707).
Attachment #8588702 - Attachment is obsolete: true
Flags: needinfo?(dolske)
Iteration: --- → 41.2 - Jun 8
Points: --- → 3

Comment 7

3 years ago
Comment on attachment 8615152 [details]
MozReview Request: Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug

InputPasswordEventDispatcher doesn't provide any new functionality on top of AsyncEventDispatcher, so just use AsyncEventDispatcher.
Attachment #8615152 - Flags: review?(bugs) → review-
Comment on attachment 8615152 [details]
MozReview Request: Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug

Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug
Attachment #8615152 - Flags: review- → review?(bugs)

Comment 9

3 years ago
Comment on attachment 8615152 [details]
MozReview Request: Bug 1132211 - Dispatch an event when <input type=password> is added to a document (including outside of a form). r=smaug

I think you want to dispatch the event only if the element is in document.
So, in AfterSetAttr case don't use GetParent() but IsInComposedDoc()
and add similar check for the BindToTree() case too.
if (mType == NS_FORM_INPUT_PASSWORD && IsInComposedDoc()) {
  ...

and I guess in that case don't change the #ifdef EARLY_BETA_OR_EARLIER stuff.
Attachment #8615152 - Flags: review?(bugs) → review+
Thanks for the review
Assignee: dolske → MattN+bmo
https://hg.mozilla.org/mozilla-central/rev/d51dec3a840d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1230243
You need to log in before you can comment on or make changes to this bug.