1360374 - Apply the field prediction heuristics for the elements added after DOMContentLoaded

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

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

Since it's a common case that website inserts elements after the page loaded, the prediction heuristics should apply to these new added elements to make sure all interesting element support Form Autofill feature.

Comment 1

[Matthew N. [:MattN]]

We discussed replacing the DOMContentLoaded listener with a "focusin" listener to delay any work as long as possible and avoid a talos regression in tp5. This will block enabling the feature by default on Nightly since due to Talos.

Comment 2

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

Attachment: Bug 1360374 - Identify FormAutofill fields when a field is focused on.;

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

Comment 3

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

Comment on attachment 8864413 [details]
Bug 1360374 - Identify FormAutofill fields when a field is focused on.;

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

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8864413 [details]
Bug 1360374 - Identify FormAutofill fields when a field is focused on.;

https://reviewboard.mozilla.org/r/136102/#review139492

Thanks Sean! r=me assuming all formautofill tests pass.

::: browser/extensions/formautofill/FormAutofillContent.jsm:413
(Diff revision 2)
> +      if (this.getFormHandler(field)) {
> +        continue;
> +      }

Add a comment about what this is about since it's not the ideal situation since ideally we would want to handle dynamic additions and check that the actual "field", not "form" were already found. Example:

```
// For now skip consider fields in forms we've already seen before even if the specific field wasn't seen before.
// Ideally whether the field is already in the handler's form details would be considered.
```

Alternatively you could fix it to do the above.

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:40
(Diff revision 2)
>      switch (evt.type) {
> -      case "DOMContentLoaded": {
> -        let doc = evt.target;
> +      case "focusin": {
> +        let element = evt.target;

Can you ensure this code is covered by mochitests by commenting out the addEventListener and ensuring tests fails? If not, please add one.

Comment 5

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

Comment on attachment 8864413 [details]
Bug 1360374 - Identify FormAutofill fields when a field is focused on.;

https://reviewboard.mozilla.org/r/136102/#review139492

> Add a comment about what this is about since it's not the ideal situation since ideally we would want to handle dynamic additions and check that the actual "field", not "form" were already found. Example:
> 
> ```
> // For now skip consider fields in forms we've already seen before even if the specific field wasn't seen before.
> // Ideally whether the field is already in the handler's form details would be considered.
> ```
> 
> Alternatively you could fix it to do the above.

Thanks for the comments. I will be back here to improve it with more tests for single field change.

> Can you ensure this code is covered by mochitests by commenting out the addEventListener and ensuring tests fails? If not, please add one.

The mochitest fails after removing addEventListener("focusin", this); in FormAutofillFrameScript.

Comment 6

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

Comment on attachment 8864413 [details]
Bug 1360374 - Identify FormAutofill fields when a field is focused on.;

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

Comment 7

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

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03c0ea04be4e
Identify FormAutofill fields when a field is focused on.; r=MattN

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/03c0ea04be4e