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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Justin Dolske [:Dolske]]

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?

Comment 1

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Justin Dolske [:Dolske]]

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

Comment 4

[Justin Dolske [:Dolske]]

(NI myself to remember to update this.)

Comment 5

[Matthew N. [:MattN]]

Attachment: 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

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d51dec3a840d

Comment 11

[Matthew N. [:MattN]]

Thanks for the review

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d51dec3a840d