Closed
Bug 1077308
Opened 10 years ago
Closed 10 years ago
Autocomplete of username/password should generate change event
Categories
(Toolkit :: Password Manager, defect)
Tracking
()
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.
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Keywords: site-compat
Updated•10 years ago
|
Flags: qe-verify?
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Possibly it would make sense to rename "ShouldBlur" to "IsFocused", but the main logic of this patch is what is important.
Comment 3•10 years ago
|
||
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-
Assignee | ||
Updated•10 years ago
|
Attachment #8500102 -
Flags: review?(bugs)
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8500102 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8500674 -
Flags: review?(bugs)
Assignee | ||
Comment 7•10 years ago
|
||
I've updated the patch to fix the indentation.
Updated•10 years ago
|
Attachment #8500674 -
Flags: review?(bugs) → review+
Comment 8•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=cacb25be87f8 Thanks
Updated•10 years ago
|
Attachment #8500674 -
Flags: review+
Comment 9•10 years ago
|
||
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
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8500674 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Thanks for the feedback. I've updated the test.
Flags: needinfo?(mbest)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Did the updated patch get run through Try to verify that the test failure is fixed?
Keywords: checkin-needed
Assignee | ||
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
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
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2be42fbc9d98
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2be42fbc9d98
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Updated•10 years ago
|
Iteration: --- → 36.1
You need to log in
before you can comment on or make changes to this bug.
Description
•