Closed Bug 1077308 Opened 5 years ago Closed 5 years ago

Autocomplete of username/password should generate change event

Categories

(Toolkit :: Password Manager, defect)

33 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.1

People

(Reporter: mbest, Assigned: mbest)

References

Details

(Keywords: site-compat)

Attachments

(1 file, 2 obsolete files)

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:

Open a page with a username/password form for which you've already saved a password. Or select a saved username from the drop-down after the page is loaded.


Actual results:

The username and password are filled in and "input" events are fired, but not "change" events.


Expected results:

"change" events should be fired in addition to "input". This is what Internet Explorere and Chrome do.
See Also: → 1025483, 1077304
Flags: qe-verify?
Possibly it would make sense to rename "ShouldBlur" to "IsFocused", but the main logic of this patch is what is important.
Thanks for the patch Michael! Would you like feedback/review on the patch? If so, you should set the appropriate flag[1] on the attachment details page. I would suggest Olli Pettay from [2].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed
[2] https://wiki.mozilla.org/Modules/Core#Document_Object_Model
Flags: qe-verify? → qe-verify-
Attachment #8500102 - Flags: review?(bugs)
Comment on attachment 8500102 [details] [diff] [review]
If a field is auto-filled while not in focus, fire a change event immediately

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

The password manager test looks good to me. I'll let a DOM peer review content/ and whether this makes sense with web specs. The spec says:
"The change event fires when the value is committed, if that makes sense for the control, or else when the control loses focus. In all cases, the input event comes before the corresponding change event (if any)."

So this seems reasonable to me.

::: content/html/content/src/HTMLInputElement.cpp
@@ +2409,2 @@
>                                                static_cast<nsIDOMHTMLInputElement*>(this),
>                                                NS_LITERAL_STRING("input"), true,

Nit: fix the indentation to align the arguments.
Attachment #8500102 - Flags: review+
Comment on attachment 8500102 [details] [diff] [review]
If a field is auto-filled while not in focus, fire a change event immediately


>-  return nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
>+  nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
>                                               static_cast<nsIDOMHTMLInputElement*>(this),
>                                               NS_LITERAL_STRING("input"), true,
>                                               true);
Indent the rest of the params.
Attachment #8500102 - Flags: review?(bugs) → review+
Attachment #8500102 - Attachment is obsolete: true
Attachment #8500674 - Flags: review?(bugs)
I've updated the patch to fix the indentation.
Attachment #8500674 - Flags: review?(bugs) → review+
Try push: https://tbpl.mozilla.org/?tree=Try&rev=cacb25be87f8

Thanks
Assignee: nobody → mbest
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
Hi Michael,

The try server run is showing a test failure:
TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug388558.html | Change event dispatched when input element doesn't have focus. - got 2, expected 1

The test file is at:
https://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_bug388558.html?force=1
(content/html/content/test/test_bug388558.html)

You can run the test with:
./mach mochitest-plain content/html/content/test/test_bug388558.html

Can you try fix the test?
Flags: needinfo?(mbest)
Keywords: checkin-needed
Attachment #8500674 - Attachment is obsolete: true
Thanks for the feedback. I've updated the test.
Flags: needinfo?(mbest)
Keywords: checkin-needed
Did the updated patch get run through Try to verify that the test failure is fixed?
Keywords: checkin-needed
I'm not able to run through the whole test suite in my development environment (using the Firefox build VM). But the change I made does make the affected test pass.
Flags: needinfo?(MattN+bmo)
OK, thanks. I've pushed to Try server again: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a831e8f494d7

I also updated the commit message and timestamp in https://hg.mozilla.org/try/rev/0d79dd9a6338 so that can be used for the push to an integration branch.
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2be42fbc9d98
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Iteration: --- → 36.1
You need to log in before you can comment on or make changes to this bug.