Closed
Bug 1132211
Opened 10 years ago
Closed 9 years ago
Password manager would really like to know when <input type=password> is added to a page but outside a <form>
Categories
(Core :: DOM: Core & HTML, defect, P1)
Core
DOM: Core & HTML
Tracking
()
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: Dolske, Assigned: MattN)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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•10 years ago
|
||
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•10 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•10 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.)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Iteration: --- → 41.2 - Jun 8
Points: --- → 3
Comment 7•10 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-
Assignee | ||
Comment 8•9 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
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•9 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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•