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)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.2 - Jun 8
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?
Attached patch Patch v.1 WIP (obsolete) (deleted) — Splinter Review
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 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+
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.)
(NI myself to remember to update this.)
Flags: needinfo?(dolske)
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P1
Blocks: 1168707
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 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 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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1191092
Depends on: 1230243
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: