1025483 - Autocompletion of passwords doesn't cause an 'input' event to be fired

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Piotr Maliński]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140608211622

Steps to reproduce:

I have a login/registration form managed by ember.js. It doesn't like autocompleate as it won't notice value changes caused by autocompletion. So the form has autocomplete="off" and that worked before Firefox 30 (Ubuntu 14.04). It's not for saving it so... it looks like a bug.




Actual results:

When I enter login it fills the password field with the password saved for given login.


Expected results:

When I enter login it should not fill the password for autocomplete="off".

And javascript (in this ember.js framework) should be able to notice value change of a form field made by autocompletion (which seems uncommon to have by browsers - if it's a browser bug).

Comment 1

[YF (Yang)]

bug 956906

Comment 2

[:Gijs (he/him)]

As noted, this was an intentional change. For more information, please read: https://bugzilla.mozilla.org/show_bug.cgi?id=956906#c100 . Amongst other important points, it notes that Chrome and IE already ignore the autocomplete="off" attribute for password fields.

However, there's a valid point here that we don't fire an 'input' event - we do for other fields than passwords, and we fire 'input' for "manual" input to the password field, so we should be consistent. Morphing this bug appropriately.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Shouldn't the autocomplete code fire the input event, then?

Comment 4

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> Shouldn't the autocomplete code fire the input event, then?

As far as I can tell, the autocomplete code should be calling setUserInput, and that should fire the input event. That's what got fixed in bug 320462 for the general case. I don't know why that's not happening in the case of passwords, but I intend to find out. Leaving needinfo for now, and I'll move the bug again if this ends up being toolkit's own fault...

Comment 5

[:Gijs (he/him)]

So yes, this is toolkit's fault. Specifically, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#698 gets called when the core code autocompletes a login value from the username field (at least in the case where I pick one from the autocomplete popup), and sets .value on the password box. AFAICT, if we change that to setUserInput, it should Just Work, but I've not tested this yet...

Comment 6

[:Gijs (he/him)]

Attachment: fire 'input' event for password autocompletes

Dolske, does this look right to you? I tested this and it seems to work on http://jsbin.com/lacig/1/edit - need to reload after submitting to get the result of the jsbin back in the right-hand pane; note that you need > 1 logins to test on this page, I couldn't get an event to log anything for either username or password, which I expect is because jsbin adds the listeners too late, but I admit I've not checked in detail...

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> Created attachment 8440422 [details] [diff] [review]
> fire 'input' event for password autocompletes
> 
> Dolske, does this look right to you? I tested this and it seems to work on
> http://jsbin.com/lacig/1/edit - need to reload after submitting to get the
> result of the jsbin back in the right-hand pane; note that you need > 1
> logins to test on this page.

Egh. Should have followed:

I couldn't get an event to log anything for filling in 1 login as the page loads, for either username or password, which I expect is because jsbin adds the listeners too late, but I admit I've not checked that in detail...

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8440422 [details] [diff] [review]
fire 'input' event for password autocompletes

Review of attachment 8440422 [details] [diff] [review]:
-----------------------------------------------------------------

Something like this which is subtle to detect by the average user should have a test.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +711,5 @@
>                        (usernameField.value != selectedLogin.username &&
>                         usernameField.value.toLowerCase() == selectedLogin.username.toLowerCase());
>      
>                  if (!disabledOrReadOnly && !userEnteredDifferentCase) {
>                      usernameField.value = selectedLogin.username;

So setting the username here already gets an 'input' event?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8440422 [details] [diff] [review]
fire 'input' event for password autocompletes

(In reply to Matthew N. [:MattN] from comment #8)
> Comment on attachment 8440422 [details] [diff] [review]
> fire 'input' event for password autocompletes
> 
> Review of attachment 8440422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Something like this which is subtle to detect by the average user should
> have a test.

I totally agree, but I guess I wasn't sure about the approach here. Perhaps feedback? would have been more appropriate.

> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm
> @@ +711,5 @@
> >                        (usernameField.value != selectedLogin.username &&
> >                         usernameField.value.toLowerCase() == selectedLogin.username.toLowerCase());
> >      
> >                  if (!disabledOrReadOnly && !userEnteredDifferentCase) {
> >                      usernameField.value = selectedLogin.username;
> 
> So setting the username here already gets an 'input' event?

No! This actually just proves that comment #7 is a load of tosh. :-)

I was only testing the autocomplete-triggered login input, in which case the autocomplete entry picking causes the user name to be filled in, and as that uses setUserInput, it fires an event. This line should be updated too. Which is odd because I thought I looked for similar .value assignments in the file. Not sure why I missed this. :-\

Thanks for the immediate feedback. I'll update the patch and look at writing a proper test.

Comment 10

[:Gijs (he/him)]

Attachment: fire 'input' event for password autocompletes

I honestly tried to use DOM Promises for the test here, but they didn't make the code any more readable (only having access to resolve/reject inside the closure is annoying for purposes of having a timeout at which point we stop waiting for stuff to happen), and so I went with this version instead.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8440429 [details] [diff] [review]
fire 'input' event for password autocompletes

Review of attachment 8440429 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Gijs!

r=me with some comments (assuming the test failed before the patch since I'm not on my work computer atm. to double-check).

::: toolkit/components/passwordmgr/test/test_input_events.html
@@ +1,5 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for input events in Login Manager</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  

Nit: trailing whitespace (probably copied and not your fault)

@@ +41,5 @@
> +  info("Got " + e.type + " event.");
> +  if (e.type == "load") {
> +    onloadFired = true;
> +  } else if (e.target.name == "uname") {
> +    usernameInputFired = true;

It would be nice to check ise(e.data, …); for the two input events to make sure it's correct. e.g. if we accidentally send more than one with different values or something.

@@ +61,5 @@
> +}
> +
> +window.onload = onNewEvent;
> +$_(1, "uname").oninput = onNewEvent;
> +$_(1, "pword").oninput = onNewEvent;

It seems like it may be possible for DOMFormHasPassword to be fired before this event listener is added since PostPasswordEvent happens in HTMLFormElement::AddElement. Is there a guarantee that the <script> will run before the DOMFormHasPassword is dispatched? I think you could move these lines to attributes on the <input>s if that's a problem. e.g. oninput="onNewEvent".

@@ +68,5 @@
> +  ok(usernameInputFired, "Username input event should have fired by now.");
> +  ok(passwordInputFired, "Password input event should have fired by now.");
> +  ok(onloadFired, "Window load event should have fired by now.");
> +  ok(false, "Not all events fired yet.");
> +  cleanup();

I think SimpleTest.registerCleanupFunction is preferred so it's guaranteed to get called if there is an exception during the test.

@@ +70,5 @@
> +  ok(onloadFired, "Window load event should have fired by now.");
> +  ok(false, "Not all events fired yet.");
> +  cleanup();
> +  SimpleTest.finish();
> +}, 20000);

Nit: this can probably be lowered to be closer to 5s or 10s

@@ +79,5 @@
> +<pre id="test"></pre>
> +</body>
> +</html>
> +
> +

Nit: extra blank line

Comment 12

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] from comment #11)
> Comment on attachment 8440429 [details] [diff] [review]
> fire 'input' event for password autocompletes
> 
> Review of attachment 8440429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch Gijs!
> 
> r=me with some comments (assuming the test failed before the patch since I'm
> not on my work computer atm. to double-check).

Yeah, I verified that it "used to" break.

> ::: toolkit/components/passwordmgr/test/test_input_events.html
> @@ +1,5 @@
> > +<!DOCTYPE HTML>
> > +<html>
> > +<head>
> > +  <title>Test for input events in Login Manager</title>
> > +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
> 
> Nit: trailing whitespace (probably copied and not your fault)

Oops. Yes, copied from one of the other tests...

> @@ +41,5 @@
> > +  info("Got " + e.type + " event.");
> > +  if (e.type == "load") {
> > +    onloadFired = true;
> > +  } else if (e.target.name == "uname") {
> > +    usernameInputFired = true;
> 
> It would be nice to check ise(e.data, …); for the two input events to make
> sure it's correct. e.g. if we accidentally send more than one with different
> values or something.

Checked e.target.value instead; e.data is undefined, e.detail seems to always be 0...

> 
> @@ +61,5 @@
> > +}
> > +
> > +window.onload = onNewEvent;
> > +$_(1, "uname").oninput = onNewEvent;
> > +$_(1, "pword").oninput = onNewEvent;
> 
> It seems like it may be possible for DOMFormHasPassword to be fired before
> this event listener is added since PostPasswordEvent happens in
> HTMLFormElement::AddElement. Is there a guarantee that the <script> will run
> before the DOMFormHasPassword is dispatched? I think you could move these
> lines to attributes on the <input>s if that's a problem. e.g.
> oninput="onNewEvent".

I'm not sure, but switched to attributes anyway.

> @@ +68,5 @@
> > +  ok(usernameInputFired, "Username input event should have fired by now.");
> > +  ok(passwordInputFired, "Password input event should have fired by now.");
> > +  ok(onloadFired, "Window load event should have fired by now.");
> > +  ok(false, "Not all events fired yet.");
> > +  cleanup();
> 
> I think SimpleTest.registerCleanupFunction is preferred so it's guaranteed
> to get called if there is an exception during the test.

Done.

> @@ +70,5 @@
> > +  ok(onloadFired, "Window load event should have fired by now.");
> > +  ok(false, "Not all events fired yet.");
> > +  cleanup();
> > +  SimpleTest.finish();
> > +}, 20000);
> 
> Nit: this can probably be lowered to be closer to 5s or 10s

I went with 10s because I've been fighting too much randomorange and am paranoid.

I'd be landing this right now but the tree is closed...

Meanwhile, Marco, can you add this to this iteration?

Comment 13

[:Gijs (he/him)]

Attachment: fire 'input' event for password autocompletes,

Patch for checkin.

Comment 14

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Justin Dolske [:Dolske]]

I meant to ask earlier -- what do other browsers do here? Do these events fire when a password is filled in?

[I don't think it matters greatly either way, but it would be nice to record for the record.]

Comment 18

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #17)
> I meant to ask earlier -- what do other browsers do here? Do these events
> fire when a password is filled in?
> 
> [I don't think it matters greatly either way, but it would be nice to record
> for the record.]

I just checked Chrome and IE11, and both fire an 'input' event.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83d14fd3d739

Comment 20

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/83d14fd3d739

Backed out for using the wrong bug number:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b563ec69fa02


and relanded DONTBUILD as bug 1025691

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0abd034712

Comment 21

[Michael Best]

First, it's great that this is finally being fixed in Firefox. Libraries such as Knockout and Angular have suffered by not being able to know when the username and password fields are filled in.

But this change has produced a side-effect: that whenever you tab out of the username field (if you've already selected an existing username), it fires a "DOMAutoComplete" event (although you haven't changed anything), which then fires the "input" event for both the username and password fields, although neither has changed.

Another note on this change: All other browsers also fire the "change" event when the username and password fields are auto-filled. It seems Firefox should do that same.

Comment 22

[Matthew N. [:MattN]]

Hi Michael, please file new bugs once a bug is fixed.

Comment 23

[Michael Best]

Okay. I've created Bug 1077304 and Bug 1077308.