1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Luke Chang [:lchang] (inactive)]

The form autofill popup won't appear on an input which is auto-focused by the website.

Bug 1341582 has fixed this issue once but bug 1360374 regressed it. It happens because "focusin" event will be fired prior to "DOMContentLoaded" when an input has "autofocus" attribute and there might be only one input rendered at that time. Thus, autofill feature won't be triggered since there are less than 3 inputs identified.

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 2

[Luke Chang [:lchang] (inactive)]

Hi Matt,

I'd like to defer "identifyAutofillFields" if an input is focused prior to "DOMContentLoaded". I think it can also reduce the loading time. What do you think?

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review142394

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:66
(Diff revision 1)
> +        let info = documentInfo.get(doc) || {};
> +
>          if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
>            return;
>          }
> -        FormAutofillContent.identifyAutofillFields(element.ownerDocument);
> +        if (!info.DOMContentLoaded) {

Did you consider document.readyState instead? I forget when it changes (load or DOMContentLoaded)

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review142394

> Did you consider document.readyState instead? I forget when it changes (load or DOMContentLoaded)

I totally forgot `readyState`. I'll try and update you soon.

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review142394

> I totally forgot `readyState`. I'll try and update you soon.

Changed to `readyState`. Wondering if we can further get rid of `DOMContentLoaded` and `WeakMap`.

Comment 7

[Luke Chang [:lchang] (inactive)]

> I forget when it changes (load or DOMContentLoaded)

BTW, it changes from "loading" to "interactive" upon "DOMContentLoaded".

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review142792

I have some suggestions/ideas to help avoid the complexity this is adding.

::: commit-message-e66de:1
(Diff revision 2)
> +Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused. r=MattN

Please add an automated test since it will be easy to unintentionally regress this again…

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:28
(Diff revision 2)
>   *
>   * NOTE: Declares it by "var" to make it accessible in unit tests.
>   */
>  var FormAutofillFrameScript = {
>    init() {
> +    addEventListener("DOMContentLoaded", this);

I'm worried that this will regress performance again.   Can't we jsut add the listener in the readyState = "loading" case and not have "focusBeforeDOMContentLoaded" state?

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:60
(Diff revision 2)
> +        let doc = element.ownerDocument;
> +
>          if (!(element instanceof Ci.nsIDOMHTMLInputElement)) {
>            return;
>          }
> -        FormAutofillContent.identifyAutofillFields(element.ownerDocument);
> +

Would use setTimeout/requestIdleCallback here be enough and allow use to avoid the DCL listener? I guess it's possible that the page is still not loaded but is the problem just that the page isn't loaded because some other state isn't ready OR is it that we want the rest of the "form" to be available to we have the correct field number threshold?

Will fixing https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/browser/extensions/formautofill/FormAutofillContent.jsm#413-418 help?

Having an automated test will make proposing solutions more clear.

Comment 9

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review142792

> I'm worried that this will regress performance again.   Can't we jsut add the listener in the readyState = "loading" case and not have "focusBeforeDOMContentLoaded" state?

Makes sense. I'll try this way.

> Would use setTimeout/requestIdleCallback here be enough and allow use to avoid the DCL listener? I guess it's possible that the page is still not loaded but is the problem just that the page isn't loaded because some other state isn't ready OR is it that we want the rest of the "form" to be available to we have the correct field number threshold?
> 
> Will fixing https://dxr.mozilla.org/mozilla-central/rev/0f4df67c5f162e00d6f52825badf468aefbfba19/browser/extensions/formautofill/FormAutofillContent.jsm#413-418 help?
> 
> Having an automated test will make proposing solutions more clear.

The problem this time is that `identifyAutofillFields` happens before the form is fully rendered so we don't have the correct field number to pass the threshold. The `setTimeout` should work but might not be sufficient as pages might be loaded very slowly. Also, the fix you mentioned might not work for this issue because `identifyAutofillFields` won't be triggered agian unless we re-focus the input.

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

https://reviewboard.mozilla.org/r/139172/#review149546

This will conflict with Ray's preview patch btw.

Comment 13

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 14

[Luke Chang [:lchang] (inactive)]

Matt, Thanks.

Comment 15

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 16

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 17

[Luke Chang [:lchang] (inactive)]

I removed `ParentUtils.cleanUpAddress();` in "formautofill_parent_utils.js" because it caused `enabledStatus` to be `false` by default, which doesn't align the real world. Also, it seems to me that we don't really need to clean up in the beginning of tests.

Comment 18

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8867609 [details]
Bug 1364818 - [Form Autofill] popup won't apply to an auto-focused input until it's refocused.

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e39dfa768be9
[Form Autofill] popup won't apply to an auto-focused input until it's refocused. r=MattN

Comment 20

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

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