1360370 - Field Prediction Heuristics for select element.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Sean Lee [:seanlee][:weilonge]]

This bug is for the implementation of predicting select element for country, state, etc.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review commit: https://reviewboard.mozilla.org/r/134496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/134496/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review139508

Does filling actually work with <select> after this patch? You should add mochitest-plain tests to confirm.

::: browser/extensions/formautofill/FormAutofillContent.jsm:410
(Diff revision 1)
> +    }
> +
>      // Collects root forms from inputs.
>      for (let field of doc.getElementsByTagName("input")) {

I think we should use `querySelectorAll("input, select")` and somehow share that logic in FormAutofillUtils (perhaps just a shared selector) since a few places use this. Then we can have one loop and don't need the helper. You would just need to change the  `mozIsTextField` check and move that to another helper in Utils e.g. `isFieldEligibleForAutofill`

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:96
(Diff revision 1)
> +    if (element instanceof Ci.nsIDOMHTMLInputElement) {
> +      if (!["text", "email", "tel"].includes(element.type)) {
> +        return null;
> +      }
> +    } else if (!(element instanceof Ci.nsIDOMHTMLSelectElement)) {
>        return null;

This should also be part of the common isFieldEligibleForAutofill utility method instead of spreading logic around.

::: browser/extensions/formautofill/test/unit/head.js:87
(Diff revision 1)
>  function runHeuristicsTest(patterns, fixturePathPrefix) {
>    Cu.import("resource://gre/modules/FormLikeFactory.jsm");
>    Cu.import("resource://formautofill/FormAutofillHeuristics.jsm");
>  
>    // TODO: "select" and "textarea" will be included eventually.
> -  const QUERY_STRING = ["input"];
> +  const QUERY_STRING = ["input", "select"];

This could also use the shared FormAutofillUtils selector ("input, select") instead of using an array

::: toolkit/modules/FormLikeFactory.jsm:50
(Diff revision 1)
>     * Create a FormLike object from an <input> in a document.
>     *
>     * If the field is in a <form>, construct the FormLike from the form.
>     * Otherwise, create a FormLike with a rootElement (wrapper) according to
>     * heuristics. Currently all <input> not in a <form> are one FormLike but this
>     * shouldn't be relied upon as the heuristics may change to detect multiple
>     * "forms" (e.g. registration and login) on one page with a <form>.
>     *
>     * Note that two FormLikes created from the same field won't return the same FormLike object.
>     * Use the `rootElement` property on the FormLike as a key instead.
>     *
>     * @param {HTMLInputElement} aField - a field in a document
>     * @return {FormLike}
>     * @throws Error if aField isn't a password or username field in a document
>     */
>    createFromField(aField) {
> -    if (!(aField instanceof Ci.nsIDOMHTMLInputElement) ||
> +    if ((!(aField instanceof Ci.nsIDOMHTMLInputElement) && !(aField instanceof Ci.nsIDOMHTMLSelectElement)) ||

The comments would also need to be updated and we should ensure this doesn't regress password manager. Please audit how this method is used there.

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review139508

Since field filling feature for <select> is not implemented yet, may we added the mochitest when implementing filling <select> feature? The current xpcshell test is covered <select> heuristics already.

> I think we should use `querySelectorAll("input, select")` and somehow share that logic in FormAutofillUtils (perhaps just a shared selector) since a few places use this. Then we can have one loop and don't need the helper. You would just need to change the  `mozIsTextField` check and move that to another helper in Utils e.g. `isFieldEligibleForAutofill`

`FormAutofillUtils.autofillFieldSelector` is used here. The helper is removed as well.

> This should also be part of the common isFieldEligibleForAutofill utility method instead of spreading logic around.

`FormAutofillUtils.isFieldEligibleForAutofill` and its related test are implemented in the part 1.

> This could also use the shared FormAutofillUtils selector ("input, select") instead of using an array

Use `FormAutofillUtils.autofillFieldSelector` in the patch already.

> The comments would also need to be updated and we should ensure this doesn't regress password manager. Please audit how this method is used there.

The uses of FormLikeFactory.createFromField[1] are most from LoginManagerContent.createFormField. <select> is excluded in LoginManagerContent.createFormField[2] already. IMO, the change in FormLikeFactory.createFromField seems safe to me because it did not change any code path of PasswordManager. However, `try` link[3] is still necessary to make sure it doesn't regress anything.

[1] http://searchfox.org/mozilla-central/search?q=createFromField&path=
[2] http://searchfox.org/mozilla-central/rev/8b70b0a5038ef9472fe7c53e04a70c978bb06aed/toolkit/components/passwordmgr/LoginManagerContent.jsm#1615
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c567dadffc71b8d32fa1fb06d01f00130b0302

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review commit: https://reviewboard.mozilla.org/r/137046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137046/

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review commit: https://reviewboard.mozilla.org/r/137048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137048/

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/1-2/

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865378 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137046/diff/1-2/

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865379 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137048/diff/1-2/

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/2-3/

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8865378 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

https://reviewboard.mozilla.org/r/137046/#review146300

::: browser/extensions/formautofill/FormAutofillUtils.jsm:30
(Diff revision 2)
> +    if (element.autocomplete == "off") {
> +      return false;
> +    }
> +
> +    if (element instanceof Ci.nsIDOMHTMLInputElement) {
> +      if (!["text", "email", "tel", "number"].includes(element.type)) {

Since this will be called often it would save on JS engine garbage to make the array a const outside of the method.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8865379 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

https://reviewboard.mozilla.org/r/137048/#review146302

::: toolkit/modules/FormLikeFactory.jsm:61
(Diff revision 2)
> -   * "forms" (e.g. registration and login) on one page with a <form>.
> +   * multiple "forms" (e.g. registration and login) on one page with a <form>.
>     *
>     * Note that two FormLikes created from the same field won't return the same FormLike object.
>     * Use the `rootElement` property on the FormLike as a key instead.
>     *
> -   * @param {HTMLInputElement} aField - a field in a document
> +   * @param {(HTMLInputElement|HTMLSelectElement)} aField - an <input> or <select> field in a document

Nit: You don't need the round braces inside the curly braces

::: toolkit/modules/FormLikeFactory.jsm:65
(Diff revision 2)
>    createFromField(aField) {
> -    if (!(aField instanceof Ci.nsIDOMHTMLInputElement) ||
> +    if ((!(aField instanceof Ci.nsIDOMHTMLInputElement) && !(aField instanceof Ci.nsIDOMHTMLSelectElement)) ||

OK, I guess you meant LoginFormFactory.createFromField is used by pwmgr so it should be fine.

::: toolkit/modules/FormLikeFactory.jsm:78
(Diff revision 2)
>        return this.createFromForm(rootElement);
>      }
>  
>      let doc = aField.ownerDocument;
>      let elements = [];
>      for (let el of rootElement.querySelectorAll("input")) {

This is obviously wrong since it doesn't include <select>

::: toolkit/modules/FormLikeFactory.jsm:104
(Diff revision 2)
>     * a page contains a login form and a checkout form which are "submitted"
>     * separately, and the username field is passed in, ideally this would return
>     * an ancestor Element of the username and password fields which doesn't
>     * include any of the checkout fields.
>     *
>     * @param {HTMLInputElement} aField - a field in a document

This should be updated to include HTMLSelectElement

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review146304

::: browser/extensions/formautofill/FormAutofillContent.jsm:429
(Diff revision 3)
>        let formHandler = new FormAutofillHandler(form);
>        formHandler.collectFormFields();
>        if (formHandler.fieldDetails.length < AUTOFILL_FIELDS_THRESHOLD) {
>          this.log.debug("Ignoring form since it has only", formHandler.fieldDetails.length,
>                         "field(s)");
>          return;
>        }
>  
>        this._formsDetails.set(form.rootElement, formHandler);
>        this.log.debug("Adding form handler to _formsDetails:", formHandler);

If this lands isn't it going to break the handler's `autofillFormFields` since it will try to call setUserInput on a <select>.

::: browser/extensions/formautofill/FormAutofillContent.jsm:441
(Diff revision 3)
> -      );
> +        if (!(ele instanceof Ci.nsIDOMHTMLSelectElement)) {
> +          this._markAsAutofillField(ele);
> +        }

I would slightly prefer if this was handled inside the `_markAsAutofillField` method

Comment 13

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865378 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

https://reviewboard.mozilla.org/r/137046/#review146300

> Since this will be called often it would save on JS engine garbage to make the array a const outside of the method.

Fixed! I also found that `["SCRIPT", "NOSCRIPT", "OPTION", "STYLE"]` in `extractLabelStrings` should be applied the same thing.
It's in Bug 1349490 - Part 1: Implement FormAutofillUtil.extractLabelStrings function.; r?MattN	
https://reviewboard.mozilla.org/r/128810/diff/25#index_header

Comment 14

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865379 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

https://reviewboard.mozilla.org/r/137048/#review146302

> This is obviously wrong since it doesn't include <select>

Sorry for missing this part.

Comment 15

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865378 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137046/diff/2-3/

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8865379 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137048/diff/2-3/

Comment 17

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/3-4/

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review146304

> If this lands isn't it going to break the handler's `autofillFormFields` since it will try to call setUserInput on a <select>.

Nice catch! I did see the error of calling setUserInput of <select>.
The new patch only allows <input> to be filled value.
Bug 1364823 is for filling value in <select>

> I would slightly prefer if this was handled inside the `_markAsAutofillField` method

I move this logic to _markAsAutofillField, and it will exclude any non-Input elements.

Comment 19

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/4-5/

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/5-6/

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review commit: https://reviewboard.mozilla.org/r/143368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143368/

Comment 22

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review commit: https://reviewboard.mozilla.org/r/143370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143370/

Comment 23

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871861 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143368/diff/1-2/

Comment 24

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871862 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143370/diff/1-2/

Comment 25

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/6-7/

Comment 26

[Sean Lee [:seanlee][:weilonge]]

Hey MattN,

I've updated the part 3 patch with the following xpcshell-test improvements:
browser/extensions/formautofill/test/unit/test_autofillFormFields.js
browser/extensions/formautofill/test/unit/test_collectFormFields.js
browser/extensions/formautofill/test/unit/test_getFormInputDetails.js

However, I would suggest to finish the related xpcshell-test/mochitest in Bug 1364823 since this patch is not related to the value filling feature for <select>.

Comment 27

[Sean Lee [:seanlee][:weilonge]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee69efb483fc29f1c2965aad6fa51d9a6642479

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8871861 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

https://reviewboard.mozilla.org/r/143368/#review148092

::: browser/extensions/formautofill/FormAutofillUtils.jsm:65
(Diff revision 2)
>          prefix: logPrefix,
>        });
>      });
>    },
>  
> +  ALLOW_TYPES: ["text", "email", "tel", "number"],

Nit: ALLOWED_TYPES

::: browser/extensions/formautofill/test/unit/test_isFieldEligibleForAutofill.js:6
(Diff revision 2)
> +  {
> +    document: `<input id="targetElement" type="text">`,
> +    fieldId: "targetElement",
> +    expectedResult: true,
> +  },

Please add an input testcase without a type attribute and ensure the result is true

::: browser/extensions/formautofill/test/unit/test_isFieldEligibleForAutofill.js:61
(Diff revision 2)
> +    Assert.equal(FormAutofillUtils.isFieldEligibleForAutofill(field),
> +                     testcase.expectedResult);

Nit: fix indentation

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8871862 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

https://reviewboard.mozilla.org/r/143370/#review148102

Make sure this doesn't cause failure with pwmgr tests. Un

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review148104

Explicitly having a test with multiple <select> would be nice (to ensure that we find <select> that aren't the argument to `createFromField`) but I think you're indirectly covering that.

Thanks

::: browser/extensions/formautofill/FormAutofillContent.jsm:492
(Diff revision 7)
> -    if (!field) {
> +    // Since Form Autofill popup is only for input element, any non-Input
> +    // element should be excluded here.
> +    if (!field || !(field instanceof Ci.nsIDOMHTMLInputElement)) {
>        return;
>      }

Why can't `isFieldEligibleForAutofill` be used here? That seems like it will be a common question so if there is a good reason it should be documented in the comment.

Comment 31

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871861 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143368/diff/2-3/

Comment 32

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871862 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143370/diff/2-3/

Comment 33

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/134496/diff/7-8/

Comment 34

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871861 [details]
Bug 1360370 - Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.;

https://reviewboard.mozilla.org/r/143368/#review148092

> Please add an input testcase without a type attribute and ensure the result is true

The following cases are new added:
<input id="targetElement">
<input id="targetElement" type="unknown">
<select id="targetElement" multiple></select>

Comment 35

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8871862 [details]
Bug 1360370 - Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.;

https://reviewboard.mozilla.org/r/143370/#review148102

After checking test link https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee69efb483fc29f1c2965aad6fa51d9a6642479 , there is no regression.

Comment 36

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8862641 [details]
Bug 1360370 - Part 3: Implement the heuristics of select element for country and state prediction.;

https://reviewboard.mozilla.org/r/134496/#review148104

The related tests are added in `browser/extensions/formautofill/test/fixtures/autocomplete_basic.html` to verify select[multiple] and input[type=""]

> Why can't `isFieldEligibleForAutofill` be used here? That seems like it will be a common question so if there is a good reason it should be documented in the comment.

The comment said the popup is only for input elements per our spec.
Is it necessary to mark a non-input element here?
IMO, we can change the function once there is any new tag to be supported. e.g. textarea

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4b39120da03
Part 1: Implement FormAutofillUtils.isFieldEligibleForAutofill.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/2568411b8a0a
Part 2: Support nsIDOMHTMLSelectElement in FormLikeFactory.createFromField.; r=MattN
https://hg.mozilla.org/integration/autoland/rev/0ab2933e41c3
Part 3: Implement the heuristics of select element for country and state prediction.; r=MattN

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4b39120da03
https://hg.mozilla.org/mozilla-central/rev/2568411b8a0a
https://hg.mozilla.org/mozilla-central/rev/0ab2933e41c3