1395928 - [Form Autofill] Form autofill doesn't always generate events web content expects

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, defect, P2)

Description

[Gabi Cheta [:Gabi] Release Desktop QA]

Attachment: issue gif

[Affected versions]:
- beta 56.0b8
- latest Nightly 57.0a1

[Affected platforms]:
- Windows 10 x64, macOS 10.12.6, Ubuntu 16.04 x64

[Steps to reproduce]:
1. Modify extensions.formautofill.available to "on" in about:config (if necessary)
2. In about:preferences#privacy click on 'Saved Addresses' 
3. Create a new profile under Saved Addresses
4. Go to macys.com and add a random item to the bag
5. Proceed to checkout
6. Click on each field so the exclamation sign appears 
7. Click on a field to trigger autofill
8. Select the auto fill address
9. Observe the exclamation field signs after auto-filling the shipping form

[Expected result]:
- Exclamation field signs should not be displayed after shipping fields are autofilled

[Actual result]:
- Exclamation field signs are displayed after shipping fields are auto filled, clicking on each field validates it

[Regression range]: Not regression

Comment 1

[Ray Lin[:ralin]]

Two quick finds by looking at the gif
1. It seems that the input validation occurs when focus "blur", but we only dispatch "change" for each input after autofilling. I think that behavior is intended with autocomplete="off" which expect user don't rely on any autofill feature but fill out one by one.
2. The preview background is affected by the website style.

Comment 2

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

Attachment: Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 3

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

Even this WIP patch can fix this issue, using focus() or blur() function could be more reasonable as a user agent to simulate the user's behavior.
I am working on a second version patch to fix this with better focus/blur event behavior.

Comment 4

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 5

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 6

[Matthew N. [:MattN]]

What events does Chrome dispatch? We should match them for better web compatibility.

The spec isn't really clear on what events we should fire:
> The autocompletion mechanism must be implemented by the user agent acting as if the user had modified the control's data, and must be done at a time where the element is mutable (e.g. just after the element has been inserted into the document, or when the user agent stops parsing).

https://html.spec.whatwg.org/#autofill-processing-model:control's-data-4

If this doesn't work in Chrome then I think it's fine calling this a compat. bug on Macy's side and contacting them.

Comment 7

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

For a general case[1], Chrome files [focus,input,change,blur] events for non-focused element, and [focus,blur] event of the focused one are at begin and end instead.

[focus] @name
[input] @name
[change] @name
[blur] @name

[focus] @organization
[input] @organization
[change] @organization
[blur] @organization

[input] @street-address <<-- focused element
[change] @street-address

[focus] @address-level2
[input] @address-level2
[change] @address-level2
[blur] @address-level2

The same event sequence happens on macys.com. 

Let's make all fields in Macys' form is with red warning box. Even the form filled by autofill, the red box on focused element is still there. It can be removed after clicking outside the input. That means "blur" event for focused element is triggered, and I guess the blur handler in macys.com checks the content again.

[1] http://jsbin.com/qasuwutiho/1/edit?html,js,console,output

Comment 8

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

For Chrome's design, "focus" and "blur" event for the focused input is never fired. I suppose "input" and "change" should always be with "focus" and "blur" if we want to simulate the user's behavior. Chrome's design seems not possible to do the same behavior by human.

Comment 9

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

After checking the activeElement behavior of Chrome [1], `activeElement` is always the manually focused one.

[1] http://jsbin.com/zetefixama/1/edit?js,console,output

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review181632

As discussed in the meeting, let's use CustomEvent instead.

Comment 11

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 12

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 13

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review181634

Patch overall looks good except a few comments below.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:327
(Diff revision 5)
>          // are empty.
> -        if (element == focusedInput ||
> +        if (element == focusedInput) {
> -            (element != focusedInput && !element.value)) {
>            element.setUserInput(value);
>            this.changeFieldState(fieldDetail, "AUTO_FILLED");
> +        } else if (element != focusedInput && !element.value) {

Isn't `element != focusedInput` unnecessary here?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:328
(Diff revision 5)
> +          element.dispatchEvent(new element.ownerGlobal.UIEvent("focus", {bubbles: true}));
> +          element.setUserInput(value);
> +          this.changeFieldState(fieldDetail, "AUTO_FILLED");
> +          element.dispatchEvent(new element.ownerGlobal.UIEvent("blur", {bubbles: true}));

I remember you said you're going to use `FocusEvent` type, aren't you? In addition, the native `focus` and `blur` events should be `bubbles: false`. Do you intentionally use `true` here?

::: browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html:54
(Diff revision 5)
>          is(element.value, expectedvalue, "Checking " + element.name + " field");
>          resolve();
>        }, {once: true});
>      }),
> +    new Promise(resolve => {
> +      element.addEventListener("blur", function onInput() {

`onBlur()`.

How about checking `focus` event as well?

Comment 14

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review181666

::: browser/extensions/formautofill/FormAutofillHandler.jsm:327
(Diff revision 5)
>          // are empty.
> -        if (element == focusedInput ||
> +        if (element == focusedInput) {
> -            (element != focusedInput && !element.value)) {
>            element.setUserInput(value);
>            this.changeFieldState(fieldDetail, "AUTO_FILLED");
> +        } else if (element != focusedInput && !element.value) {

Yes, it's unnecessary.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:328
(Diff revision 5)
> +          element.dispatchEvent(new element.ownerGlobal.UIEvent("focus", {bubbles: true}));
> +          element.setUserInput(value);
> +          this.changeFieldState(fieldDetail, "AUTO_FILLED");
> +          element.dispatchEvent(new element.ownerGlobal.UIEvent("blur", {bubbles: true}));

Thanks for catching this. It should be `FocusEvent` and `{bubbles: false}`.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:328
(Diff revision 5)
> -        if (element == focusedInput ||
> +        if (element == focusedInput) {
> -            (element != focusedInput && !element.value)) {
>            element.setUserInput(value);
>            this.changeFieldState(fieldDetail, "AUTO_FILLED");
> +        } else if (element != focusedInput && !element.value) {
> +          element.dispatchEvent(new element.ownerGlobal.UIEvent("focus", {bubbles: true}));

I found this line make the `focus` behavior by user broken.

After autofilling a form, the focused element is the last filling element but the original focused one, and the original focused one can not be focused after clicking on it manually.

Comment 15

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 16

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

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

Comment 17

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

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review181666

> I found this line make the `focus` behavior by user broken.
> 
> After autofilling a form, the focused element is the last filling element but the original focused one, and the original focused one can not be focused after clicking on it manually.

We all agreed to apply the behavior in comment 7 in the earilier meeting, and there is no `focus` and `blur` for the focused element.
When I try this way in the patch, I found the focusing mechanism is broken without "focus" event for the focused one in Firefox, and dispatching a focus event can fix this issue. In other words, this `focus` event dispatching is not in our agreement in the earlier meeting.

Comment 18

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #6)
> The spec isn't really clear on what events we should fire:
> > The autocompletion mechanism must be implemented by the user agent acting as if the user had modified the control's data, and must be done at a time where the element is mutable (e.g. just after the element has been inserted into the document, or when the user agent stops parsing).
> 
> https://html.spec.whatwg.org/#autofill-processing-model:control's-data-4

I filed https://github.com/whatwg/html/issues/3016 on the HTML spec to get the events specified.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review182202

Clearing review as I think we shouldn't rush something into 56 and should consult with DOM, spec, and webcompat teams to figure out the right outcome.

In the meantime we could file a separate webcompat bug and contact Macy's to fix their site to (also?) listen for the `change` event.

Comment 20

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8904206 [details]
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.

https://reviewboard.mozilla.org/r/175986/#review182264

Clearing review as Matt mentioned above.

Comment 21

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

Hello Smaug,

In this bug, we would like to fire focus and blur events for each field during filling value.
In a pratical case, the blur event is for Macys.com to validate the filled value.
Since the spec did not describe the detail about the sequence of blur and focus or when to fire them, we would like to adopt the behavior of Chrome's behavior like below:
> For a general case[1], Chrome files [focus,input,change,blur] events for
> non-focused element, and [focus,blur] event of the focused one are at begin
> and end instead.
> 
> [focus] @name
> [input] @name
> [change] @name
> [blur] @name
> 
> [focus] @organization
> [input] @organization
> [change] @organization
> [blur] @organization
> 
> [input] @street-address <<-- focused element
> [change] @street-address
> 
> [focus] @address-level2
> [input] @address-level2
> [change] @address-level2
> [blur] @address-level2
> 
> The same event sequence happens on macys.com. 
> 
> Let's make all fields in Macys' form is with red warning box. Even the form
> filled by autofill, the red box on focused element is still there. It can be
> removed after clicking outside the input. That means "blur" event for
> focused element is triggered, and I guess the blur handler in macys.com
> checks the content again.
> 
> [1] http://jsbin.com/qasuwutiho/1/edit?html,js,console,output

Could you provide your suggestion about the blur and focus events? Thank you.

Comment 22

[Olli Pettay [:smaug]]

focus and blur shouldn't fire if focus doesn't actually change.
Does Chrome really fire focusin/focus and focusout/blur for elements which never had focus?
That sounds like a bug in Chrome.
If Chrome fires some synthetic focus event, what is document.activeElement while that event fires?

Or does Chrome perhaps do something different and moves focus over the elements? So it would actually initially focus some element, autofill it, and then moves focus elsewhere?

Comment 23

[Olli Pettay [:smaug]]

...since if it does actually focus some elements and then moves to other elements, that doesn't too much like a bug after all, but just implementation detail in their autofill.

Comment 24

[Ray Lin[:ralin]]

Hi :smaug

Here's the event order of auto-filling in Chrome: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/exported/WebFormControlElement.cpp?gsn=SetAutofillValue&l=95

I think it's quite similar to what Sean propose, with the only differences that we don't surround "change" event with synthetic keypress event. Not sure if any concerns doing that, but it's reasonable for me to fire focus/blur to simulate like as manually fill one by one. Thanks.

Comment 25

[Olli Pettay [:smaug]]

huh, that Chromium code looks horrible. Like, isn't that missing focusin/focusout events. Isn't document.activeElement pointing to different element while the focus is dispatched...
(but better to verify this by testing how it actually behaves)
We definitely don't want similar crazy behavior, but Chromium to fix their stuff and then spec some reasonable behavior.

Rick, sorry to ask you, but I couldn't see familiar names in the blame. Who might know Chromiums autofill?

Comment 26

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

Hi Smaug,

(In reply to Olli Pettay [:smaug] from comment #22)
> focus and blur shouldn't fire if focus doesn't actually change.
> Does Chrome really fire focusin/focus and focusout/blur for elements which
> never had focus?
> That sounds like a bug in Chrome.
> If Chrome fires some synthetic focus event, what is document.activeElement
> while that event fires?
After checking the activeElement behavior of Chrome [1], `activeElement` is always the manually focused one.

> 
> Or does Chrome perhaps do something different and moves focus over the
> elements? So it would actually initially focus some element, autofill it,
> and then moves focus elsewhere?
I don't think the focus in Chrome really moves in a couple of blur/focus event since `activeElement` is never changed during filling values. It's checked in the handler[1] of blur and focus.

BTW, I use the same test page[1] to test Safari's behavior. Safari does fire blur/focus event for the manually focused input, but it still keeps `activeElement` as the manually focused one.

P.S. When "manually focused input" mentioned, it means the input which is used to show the popup.

[1] http://jsbin.com/zetefixama/1/edit?js,console,output

Comment 27

[Olli Pettay [:smaug]]

Could you file a spec bug about this? https://whatwg.org/newbug

Comment 28

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

MattN had already filed one: https://github.com/whatwg/html/issues/3016

Comment 29

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

Hi Smang,

dtapuska updates the behavior of Chrome[1]. May I know if you have any concern of this implementation? Thanks.

Comment 30

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

[1] https://github.com/whatwg/html/issues/3016#issuecomment-332566851

Comment 31

[Olli Pettay [:smaug]]

Well, whatever we implement should be in some spec, or at least agreed on some spec bug.
I'm not really happy with Chrome's behavior. It feels rather random.

Comment 32

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

When we use `focus()` to change the focus in the autofilling behavior, we need to be careful that the screen can be scolling due to `focus()` function.
P.S. Just share the discussion for who is not following the github thread.

Comment 33

[Luke Chang [:lchang] (inactive)]

Move it out of the MVP scope because the spec needs to be clarified first.

Comment 34

[Rick Byers]

Sorry I missed the needinfo request here.  But I did get the right Chrome folks involved in the discussion back in Sept on https://github.com/whatwg/html/issues/3016

Comment 37

[Release mgmt bot [:sylvestre / :calixte / :marco for bugbug]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 38

[Matthew N. [:MattN]]

Etsy is an example of site that requires a blur

Comment 39

[Matthew N. [:MattN]]

https://store.us.asmodee.com/ is another affected page.

Comment 41

[Adam Roach [:abr]]

Attachment: Bug 1395928: Call `focus()` on fields prior to autofilling them r?zbraniecki!

Depends on D81791

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43af894a68e0
Call `focus()` on fields prior to autofilling them r=zbraniecki

Comment 43

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/43af894a68e0

Comment 44

[Timea Cernea [:tbabos]]

Hey Adam,
The case with the exclamation mark on Macy's or Etsy.com seems to be fixed (checked on Win10).
However, on the site mentioned (https://store.us.asmodee.com/) by Matt in Comment 39 Address Autofill doesn't work at all.

For the patch that landed with this, which cases should've been covered?

Comment 45

[Adam Roach [:abr]]

(In reply to Timea Cernea [:tbabos] from comment #44)

Hey Adam,
The case with the exclamation mark on Macy's or Etsy.com seems to be fixed (checked on Win10).
However, on the site mentioned (https://store.us.asmodee.com/) by Matt in Comment 39 Address Autofill doesn't work at all.

For the patch that landed with this, which cases should've been covered?

This patch only addresses the situations where:

• Form autofill already knows how to fill a form correctly, BUT
• The form either doesn't submit or the user is shown spurious errors due to autofilling

This patch fixes the website errors (e.g., the exclamation marks or refusal to submit a form). It does not change form autofill's ability to detect forms in any way. My guess regarding asmodee.com is that the site has changed its behavior such that autofill no longer detects its fields correctly.

Comment 46

[Cristian Fogel, QA [:cfogel]]

Verified with 80.0 on Windows 10, macOS 11.0 keeping in mind notes form the previous comments as well.