Closed Bug 1364818 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Form Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3] ETA:612)

Attachments

(1 file)

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.
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 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 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 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`.
> I forget when it changes (load or DOMContentLoaded)

BTW, it changes from "loading" to "interactive" upon "DOMContentLoaded".
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.
Attachment #8867609 - Flags: review?(MattN+bmo)
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 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.
Attachment #8867609 - Flags: review?(MattN+bmo) → review+
Matt, Thanks.
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
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.
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
https://hg.mozilla.org/mozilla-central/rev/e39dfa768be9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.