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

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

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.

Comment 1

[Edouard Oger [:eoger]]

Attachment: bug-1159538.patch

Naive implementation.

Comment 2

[Matthew N. [:MattN]]

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`.

Comment 3

[Edouard Oger [:eoger]]

Attachment: bug-1159538.patch

Thanks for the review Matthew. Comments addressed and test added.

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ac26180f1a16

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ac26180f1a16

Comment 7

[Matthew N. [:MattN]]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8647284 [details] [diff] [review]
bug-1159538.patch

Sure, let's take it!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0a75dc413cff