Matthew N. [:MattN] Reporter
	Description • 10 years ago

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.

	Edouard Oger [:eoger] Assignee
	Comment 1 • 9 years ago

Naive implementation.

	Matthew N. [:MattN] 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`.

	Edouard Oger [:eoger] Assignee
	Comment 3 • 9 years ago

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

	Matthew N. [:MattN] 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.

	Pulsebot
	Comment 5 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 6 • 9 years ago

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

	Matthew N. [:MattN] 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

	Sylvestre Ledru [:Sylvestre]
	Comment 8 • 9 years ago

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

Sure, let's take it!

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

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