Luke Chang [:lchang] (inactive) Assignee
	Description • 7 years ago

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.

	Luke Chang [:lchang] (inactive) Assignee
	Comment 2 • 7 years ago

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?

	Matthew N. [:MattN]
	Comment 3 • 7 years ago
mozreview-review

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)

	Luke Chang [:lchang] (inactive) Assignee
	Comment 4 • 7 years ago
mozreview-review-reply

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.

	Luke Chang [:lchang] (inactive) Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

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

	Luke Chang [:lchang] (inactive) Assignee
	Comment 7 • 7 years ago

> I forget when it changes (load or DOMContentLoaded)

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

	Matthew N. [:MattN]
	Comment 8 • 7 years ago
mozreview-review

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.

	Luke Chang [:lchang] (inactive) Assignee
	Comment 9 • 7 years ago
mozreview-review-reply

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.

	Matthew N. [:MattN]
	Comment 12 • 7 years ago
mozreview-review

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.

	Luke Chang [:lchang] (inactive) Assignee
	Comment 14 • 7 years ago

Matt, Thanks.

	Luke Chang [:lchang] (inactive) Assignee
	Comment 17 • 7 years ago

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.

	Pulsebot
	Comment 19 • 7 years ago

Pushed by lchang@mozilla.com:
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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 7 years ago
bugherder

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