Closed Bug 1077304 Opened 10 years ago Closed 10 years ago

autocomplete of passwords causes extra input events

Categories

(Toolkit :: Password Manager, defect)

33 Branch
defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox33 --- wontfix
firefox34 + fixed
firefox35 + fixed

People

(Reporter: mbest, Assigned: Gijs)

References

()

Details

(Keywords: regression, site-compat, testcase)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

With a username/password form, tab out of a username field that already has an existing username filled in.


Actual results:

It fires a "DOMAutoComplete" event, which (as of Bug 1025483) then fires the "input" event for both the username and password fields, although neither has changed.


Expected results:

No events (other than "blur") should be fired.
Component: Untriaged → Autocomplete
OS: Windows 7 → All
Product: Firefox → Toolkit
Hardware: x86_64 → All
See Also: → 1025483
I've set this to the autocomplete component because I believe the root of the bug is there--that it generates "DOMAutoComplete" events when it shouldn't.
See Also: → 1077308
Thanks for filing Michael. I think this will be easier to find in the Password Manager component and I think the change will be there or in Core. Toolkit::Autocomplete is more for the dropdown itself but this happens even if the dropdown isn't used.
Component: Autocomplete → Password Manager
Flags: firefox-backlog+
Keywords: site-compat
Flags: qe-verify?
Isn't this a regression?  Do we not need to get this fixed in 33?
Blocks: 1025483
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Boris Zbarsky [:bz] from comment #3)
> Isn't this a regression?

Not sure. Reading https://bug1025483.bugzilla.mozilla.org/attachment.cgi?id=8440684 , I would expect an event only if the username doesn't match the autocomplete username, and an event for the password field. Neither of that seems to match comment #0, which then makes me wonder how sure we are if this is a regression from bug 1025483.

> Do we not need to get this fixed in 33?

I think it's too late for that, the last beta has shipped. I'm also not sure how serious the web impact of extra (rather than lacking) events is.
There are two parts of this problem. The main one is that setUserInput fires an input event even if the value hasn't been changed. So while I'm very glad that we'll now get input events in response to the username and password being filled in, it's still not right to fire the event when nothing has changed.

The second part of the problem is that the autocomplete component fires a DOMAutoComplete event whenever you press the Tab key in an input field that might support autocomplete. Thus simply tabbing through the fields on a page fires spurious DOMAutoComplete events. The password manager component listens for this event and responds by filling in the username and password using setUserInput, thus firing the input events.
(In reply to Michael Best from comment #5)
> There are two parts of this problem. The main one is that setUserInput fires
> an input event even if the value hasn't been changed. So while I'm very glad
> that we'll now get input events in response to the username and password
> being filled in, it's still not right to fire the event when nothing has
> changed.

Right. I can fix the toolkit code to not call setUserInput if the input already contains the requisite value, and that will fix the web-visible part of this part of the problem. I would tend to agree that setUserInput shouldn't fire an event if the value is the same, but I don't know that we should fix that on branches; I'd be afraid of regressing something else.

Note also that while we all seem to intuitively expect no input event to fire when the value of the input box remains unchanged, Chrome, IE and Firefox all fire such an event if you copy the current value of the box, select all, and then paste the same value in, even though technically that doesn't change the value of the input, either...

Which is even more interesting because it seems all browser are thus not sticking to the spec here:
https://html.spec.whatwg.org/multipage/forms.html#event-input-input

choice excerpt:

"any time the user causes the element's value to change without an explicit commit action, the user agent must queue a task to fire a simple event that bubbles named input at the input element"
<snip>
"Some user interactions do not cause changes to the value, e.g. hitting the "delete" key in an empty text field, or replacing some text in the field with text from the clipboard that happens to be exactly the same text."

> The second part of the problem is that the autocomplete component fires a
> DOMAutoComplete event whenever you press the Tab key in an input field that
> might support autocomplete. Thus simply tabbing through the fields on a page
> fires spurious DOMAutoComplete events.

How sure are you about this part? AFAICT it's the blur handling that triggers this, and I don't see DOMAutoComplete events firing at all. My testcase could be wrong, of course... but I also only see code initializing DOMAutoComplete events here (and in some tests and metro/android code): http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#559 which seems to only be triggered from here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1304 , which I don't think fires every time focus changes...

> The password manager component
> listens for this event and responds by filling in the username and password
> using setUserInput, thus firing the input events.

It also listens for blur events, which are what causes the first part, in my understanding. Needinfo'ing Matt to see if I'm missing something, and bz to check my spec reading...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 35.3
Points: --- → 2
Flags: needinfo?(bzbarsky)
Flags: needinfo?(MattN+bmo)
Flags: in-testsuite?
Thanks for the detailed response. It's interesting that the browsers don't always stick to the spec. I suppose the user action of pasting might seem worth noting even if it doesn't change the value. But in this case, the user action is pressing Tab, which isn't supposed to change anything on its own.

> It also listens for blur events, which are what causes the first part, in my understanding. 
> Needinfo'ing Matt to see if I'm missing something, and bz to check my spec reading...

I've looked through the code as well and used a debugger to track down what happens. When the Tab key is pressed, HandleTab[1] is called, which then calls HandleEnter[2], which in turn calls EnterMatch[3], which does what you've described above.

I also saw by looking through the code that blur should do something, but in my tests, it doesn't do anything. Using the testcase you provided, you can verify that pressing tab (or shift-tab) from the username field will generate input events, but simple clicking out of the field won't.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#957
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#359
[3] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#283
In the normal autocomplete path (not using password manager), there is no extra input event because it skips calling setUserInput if the value hasn't changed: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1255
(In reply to Michael Best from comment #7)
> Thanks for the detailed response. It's interesting that the browsers don't
> always stick to the spec. I suppose the user action of pasting might seem
> worth noting even if it doesn't change the value. But in this case, the user
> action is pressing Tab, which isn't supposed to change anything on its own.

Well, it triggers autocomplete, but I agree that shouldn't cause input events to fire if we're not actually filling in login info. If I understand your comment #8 correctly, only the password manager gets that part wrong, right?

> > It also listens for blur events, which are what causes the first part, in my understanding. 
> > Needinfo'ing Matt to see if I'm missing something, and bz to check my spec reading...
> 
> I've looked through the code as well and used a debugger to track down what
> happens. When the Tab key is pressed, HandleTab[1] is called, which then
> calls HandleEnter[2], which in turn calls EnterMatch[3], which does what
> you've described above.
> 
> I also saw by looking through the code that blur should do something, but in
> my tests, it doesn't do anything. Using the testcase you provided, you can
> verify that pressing tab (or shift-tab) from the username field will
> generate input events, but simple clicking out of the field won't.

Hrmpf. Autocomplete not triggering for mouse input is sucky, but I guess that's a separate bug. :-\

(In reply to :Gijs Kruitbosch from comment #6)
> How sure are you about this part? AFAICT it's the blur handling that
> triggers this, and I don't see DOMAutoComplete events firing at all. My
> testcase could be wrong, of course...

This was prophetic, because I listened for "DOMAutocomplete" (note lack of capital "c") - fixing that made the code work. Thanks for responding so quickly and politely to my stupidities. :-)
This should work. I couldn't just update userEnteredDifferentCase because it's also false if userTriggered  is false. I tried to create a test and failed miserably; I couldn't get test-created KeyboardEvents with a tab keyCode to trigger the autocomplete code. Let me know if you think I need to try harder.
Attachment #8500429 - Flags: review?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Marco, can you add this to the spreadsheet?
Flags: needinfo?(mmucci)
Keywords: regression
Added to IT 35.3
Flags: needinfo?(mmucci)
Gijs - Do you know when this issue was introduced?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Lawrence Mandel [:lmandel] from comment #13)
> Gijs - Do you know when this issue was introduced?

By bug 1025483, so in 33. Sorry for the confusion.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: qe-verify? → qe-verify-
> It's interesting that the browsers don't always stick to the spec.

It's way simpler and faster to "fire input on paste" than "on paste, save the old value, then paste, then if the new value doesn't match the old value fire input".  So I'm not surprised no one is following the spec there.
Flags: needinfo?(bzbarsky)
> It's way simpler and faster to "fire input on paste"

Makes sense.
Comment on attachment 8500429 [details] [diff] [review]
fix password manager to not fire input events if not changing input field values,

I wondered whether it makes sense to return didFillForm = false if we don't actually change any of the values, but I suppose that's unlikely to occur much in practice, and I couldn't find any existing users of that return value anyways.

I don't see any ways that this change would cause problems with the rest of the autocomplete code (there don't seem to be any baked-in assumptions that input events get fired here).

It would really be ideal to get a test for this, so I think you should try harder :) Shouldn't you use synthesizeKey rather than firing your own KeyboardEvents?
Attachment #8500429 - Flags: review?(MattN+bmo) → review+
Same patch, now with test. I'm sorry about the setTimeout, but I can't find a better way to wait for the input event to have fired...
Attachment #8503051 - Flags: review?(gavin.sharp)
Attachment #8500429 - Attachment is obsolete: true
Attachment #8503051 - Flags: review?(gavin.sharp) → review+
https://hg.mozilla.org/mozilla-central/rev/12a7e4dd8949
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
34 is marked as affected and this fix has been on m-c/Aurora for 11 days. The patch looks to be pretty safe. Can you please submit an uplift request?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8503051 [details] [diff] [review]
fix password manager to not fire input events if not changing input field values,

Approval Request Comment
[Feature/regressing bug #]: bug 1025483
[User impact if declined]: extraneous input events due to autocomplete
[Describe test coverage new/current, TBPL]: has an automated test
[Risks and why]: fairly low, has an automated test and spent some time backing on nightly and aurora already
[String/UUID change made/needed]: no
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8503051 - Flags: approval-mozilla-beta?
Comment on attachment 8503051 [details] [diff] [review]
fix password manager to not fire input events if not changing input field values,

Beta+
Attachment #8503051 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.