Allow per-site recipes to adjust the username/password field detection for onUsernameInput

RESOLVED FIXED in Firefox 42



4 years ago
4 years ago


(Reporter: MattN, Assigned: eoger)


(Blocks 1 bug)

Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox39 wontfix, firefox40 wontfix, firefox41 affected, firefox42 fixed, firefox43 fixed)



(1 attachment, 1 obsolete attachment)

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+
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Iteration: 40.3 - 11 May → 41.1 - May 25
Posted patch bug-1159538.patch (obsolete) — Splinter Review
Naive implementation.
Attachment #8645975 - Flags: feedback?(MattN+bmo)
Comment on attachment 8645975 [details] [diff] [review]

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 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+
Assignee: MattN+bmo → edouard.oger
Iteration: 41.1 - May 25 → 43.1 - Aug 24
Thanks for the review Matthew. Comments addressed and test added.
Attachment #8645975 - Attachment is obsolete: true
Attachment #8647284 - Flags: review?(MattN+bmo)
Comment on attachment 8647284 [details] [diff] [review]

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+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8647284 [details] [diff] [review]

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?
Comment on attachment 8647284 [details] [diff] [review]

Sure, let's take it!
Attachment #8647284 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.