Closed
Bug 1159538
Opened 10 years ago
Closed 9 years ago
Allow per-site recipes to adjust the username/password field detection for on Username Input
Categories
(Toolkit :: Password Manager, defect, P1)
Toolkit
Password Manager
Tracking
()
People
(Reporter: MattN, Assigned: eoger)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
8.14 KB,
patch
|
MattN
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Bug 1120129 implemented login field overrides for filling and bug 1145754 did the same for capture. We should use recipes in onUsernameInput too or there will be a mismatch between autofill and filling due to a blur/autocomplete.
Flags: firefox-backlog+
Updated•10 years ago
|
Flags: qe-verify?
Reporter | ||
Updated•10 years ago
|
Flags: qe-verify? → qe-verify-
Updated•9 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Assignee | ||
Comment 1•9 years ago
|
||
Naive implementation.
Attachment #8645975 -
Flags: feedback?(MattN+bmo)
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8645975 [details] [diff] [review] bug-1159538.patch Review of attachment 8645975 [details] [diff] [review]: ----------------------------------------------------------------- Great! Thanks. Using a sync message is good enough for now. The dependency (bug 1166113) provides the ideal solution. All you need now is tests. It's probably easiest to add one to the bottom of toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html. You can factor out the cleanup function from https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/pwmgr_common.js?rev=54e05c26ea80#307 and use .add/.reset on the recipe parent. You'll probably need to add the recipe for a form then test it and immediately reset the recipe as the passwordSelector recipe isn't constrained to a <form> for more flexibility IIRC. ::: toolkit/components/passwordmgr/LoginManagerContent.jsm @@ +521,5 @@ > > log("onUsernameInput from", event.type); > > + let doc = acForm.ownerDocument; > + let win = doc.defaultView; Just inline this since it's used once @@ +522,5 @@ > log("onUsernameInput from", event.type); > > + let doc = acForm.ownerDocument; > + let win = doc.defaultView; > + let hostname = LoginUtils._getPasswordOrigin(doc.documentURI); Please don't mix up terminology when working with code dealing with security/origins. This is an origin, not a hostname and should be identified as such. You can name this `formOrigin` then… @@ +526,5 @@ > + let hostname = LoginUtils._getPasswordOrigin(doc.documentURI); > + > + let messageManager = messageManagerFromWindow(win); > + let recipesArray = messageManager.sendSyncMessage("RemoteLogins:findRecipes", { > + formOrigin: hostname, you only need `formOrigin,` on this line. @@ +528,5 @@ > + let messageManager = messageManagerFromWindow(win); > + let recipesArray = messageManager.sendSyncMessage("RemoteLogins:findRecipes", { > + formOrigin: hostname, > + })[0]; > + let recipes = new Set(recipesArray); `recipesArray` is already a Set now (due to a platform fix) so this isn't necessary anymore. Can you also fix the place you copied this from too? Rename `recipesArray` to `recipes`.
Attachment #8645975 -
Flags: feedback?(MattN+bmo) → feedback+
Reporter | ||
Updated•9 years ago
|
Assignee: MattN+bmo → edouard.oger
Iteration: 41.1 - May 25 → 43.1 - Aug 24
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox43:
--- → affected
Assignee | ||
Comment 3•9 years ago
|
||
Thanks for the review Matthew. Comments addressed and test added.
Attachment #8645975 -
Attachment is obsolete: true
Attachment #8647284 -
Flags: review?(MattN+bmo)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8647284 [details] [diff] [review] bug-1159538.patch Review of attachment 8647284 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Edouard! I made a fix to the test as it was causing a form submission and I added a test for the blur case too. ::: toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html @@ +179,5 @@ > + > + <!-- test for onUsernameInput recipe testing --> > + <form id="form11" action="https://autocomplete7" onsubmit="return false;"> > + <input type="text" name="1"> > + <input type="text" name="2"> I'm surprised this works since "2" isn't a password field. We shouldn't depend on this behaviour since it may change. @@ +835,5 @@ > + checkACForm("", ""); > + uname.value = "testuser10"; > + doKey("down"); > + checkACForm("testuser10", ""); // value shouldn't update > + doKey("return"); // not "enter"! This is causing a form submission (which leaves a doorhanger hanging around) instead of selecting the first entry in the dropdown because the dropdown never appears (and you aren't checking if a popup opens either). The reason autocomplete doesn't appear is because the username is only marked as a username field by _fillForm but since you're adding the recipe after _fillForm happens, field 1 never gets marked as a username field. To fix this and the previous concern, I change .type to "password" which triggers _fillForm again. I also wait for the popup.
Attachment #8647284 -
Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/ac26180f1a16
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8647284 [details] [diff] [review] bug-1159538.patch Approval Request Comment [Feature/regressing bug #]: Password manager improvements for Fx42 [User impact if declined]: autocomplete and autofill on blur won't work on some sites even if we have recipes to fix the heuristics [Describe test coverage new/current, TreeHerder]: New mochitests and some sites Mozillians regularly use have recipes. [Risks and why]: Low risk simply passing recipes to a function in a place which wasn't passing any. [String/UUID change made/needed]: None
Attachment #8647284 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Comment 8•9 years ago
|
||
Comment on attachment 8647284 [details] [diff] [review] bug-1159538.patch Sure, let's take it!
Attachment #8647284 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0a75dc413cff
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•