Closed Bug 1395928 Opened 7 years ago Closed 4 years ago

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

Categories

(Toolkit :: Form Autofill, defect, P2)

Product:
Toolkit
Component:
Form Autofill
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox80 --- verified

People

(Reporter: Gabi, Assigned: abr)

References

(Blocks 6 open bugs, Regressed 1 open bug,
URL
)

Details

(Keywords: site-compat, Whiteboard: [cc-autofill-mvp])

Attachments

(3 files)

issue gif
(deleted), image/gif
Details
Bug 1395928 - Fire "blur" and "focus" event for each filling fields during filling form.
(deleted), text/x-review-board-request
Details
Bug 1395928: Call `focus()` on fields prior to autofilling them r?zbraniecki!
(deleted), text/x-phabricator-request
Details
Attached image issue gif (deleted) —
[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
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.
Assignee: nobody → selee
Whiteboard: [form autofill:MVP]
Summary: {Form Autofill] Exclamation signs are displayed after using shipping fields are auto filled → [Form Autofill] Exclamation signs are displayed after using shipping fields are auto filled
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.
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.
URL: https://www.macys.com
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
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.
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 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.
Attachment #8904206 - Flags: review?(lchang)
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?
Attachment #8904206 - Flags: review?(lchang)
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 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.
(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.
See Also: → https://github.com/whatwg/html/issues/3016
Keywords: site-compat
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.
Attachment #8904206 - Flags: review?(MattN+bmo)
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.
Attachment #8904206 - Flags: review?(lchang)
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.
Flags: needinfo?(bugs)
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?
Flags: needinfo?(bugs)
...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.
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.
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?
Flags: needinfo?(rbyers)
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
Could you file a spec bug about this? https://whatwg.org/newbug
Hi Smang, dtapuska updates the behavior of Chrome[1]. May I know if you have any concern of this implementation? Thanks.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Move it out of the MVP scope because the spec needs to be clarified first.
Blocks: fx-form-autofill
Whiteboard: [form autofill:MVP] → [form autofill]
Component: Form Manager → Form Autofill
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
Flags: needinfo?(rbyers)
Assignee: selee → nobody

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

Keywords: regression
Keywords: regression

Etsy is an example of site that requires a blur

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

Blocks: 1618937
Priority: P2 → P3
Whiteboard: [form autofill] → [cc-autofill-mvp] [form autofill]
Blocks: 1645733
Summary: [Form Autofill] Exclamation signs are displayed after using shipping fields are auto filled → [Form Autofill] Form autofill doesn't always generate events web content expects
Blocks: 1424119, 1645738
Priority: P3 → P2
Whiteboard: [cc-autofill-mvp] [form autofill] → [cc-autofill-mvp]
Assignee: nobody → adam
Status: NEW → ASSIGNED
Pushed by adam@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/43af894a68e0 Call `focus()` on fields prior to autofilling them r=zbraniecki

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

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?

Flags: needinfo?(adam)

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

Flags: needinfo?(adam)
Flags: qe-verify+

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

Status: RESOLVED → VERIFIED
status-firefox80: fixed → verified
Flags: qe-verify+
Regressions: 1651945
Blocks: 1691418
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: