1365895 - When a select element is autofilled, it should fire events properly

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Scott Wu [:scottwu]]

Assigning a new value to a select element doesn't trigger a "change" event, and manually sending one with dispatchEvent could not make the event "trusted".

We could create a method that simulates user changing select element, similar to setUserInput for input element.

Comment 1

[Scott Wu [:scottwu]]

Actually, since we are calling dispatchEvent with chrome privilege, the event is "trusted", but it's still not exactly the same as ones fired when the change is initiated by a user. Also, an "input" event is fired before "change" does.

Comment 2

[Scott Wu [:scottwu]]

Attachment: Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 3

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 4

[Matthew N. [:MattN]]

LGTM so requesting DOM peer review.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review150112

::: dom/webidl/HTMLSelectElement.webidl:73
(Diff revision 2)
>    [ChromeOnly]
>    AutocompleteInfo getAutocompleteInfo();
>    [ChromeOnly]
>    attribute DOMString previewValue;
> +  [ChromeOnly]
> +  void fireOnInputAndOnChange();

Event names don't start with "on".
So the method name should be fireInputAndChangeEvents() or some such.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review150114

::: browser/extensions/formautofill/FormAutofillHandler.jsm:109
(Diff revision 2)
>                break;
>              }
> -            // TODO: Using dispatchEvent does not 100% simulate select change.
> -            //       Should investigate further in Bug 1365895.
>              option.selected = true;
> -            element.dispatchEvent(new Event("input", {"bubbles": true}));
> +            element.fireOnInputAndOnChange();

So does this not work since we're using Event interface from a chrome JS global?
What if events were created using element.ownerDocument.createElement() ?


(and this comment is for the existing code. MozReview is behaving oddly here.)

Comment 7

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review150538

The code looks good but can you add a test that `input` and `change` fire on the <select>?


Similar tests for <input>:
https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html#52

https://dxr.mozilla.org/mozilla-central/rev/5801aa478de12a62b2b2982659e787fcc4268d67/browser/extensions/formautofill/test/unit/test_autofillFormFields.js#289

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review150114

> So does this not work since we're using Event interface from a chrome JS global?
> What if events were created using element.ownerDocument.createElement() ?
> 
> 
> (and this comment is for the existing code. MozReview is behaving oddly here.)

Thanks for pointing this out. Yes it does work properly now when I use `new element.ownerDocument.defaultView.UIEvent / Event`. Doing it from C++ was unneccessary.

Comment 10

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 11

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review152746

::: toolkit/modules/tests/modules/MockDocument.jsm:26
(Diff revision 4)
>      let parser = Cc["@mozilla.org/xmlextras/domparser;1"].
>                   createInstance(Ci.nsIDOMParser);
>      parser.init();
>      let parsedDoc = parser.parseFromString(aContent, aType);
>  
>      // Assign onwerGlobal to documentElement as well for the form-less

Can you fix the ownerGlobal typo while you're here?

::: toolkit/modules/tests/modules/MockDocument.jsm:30
(Diff revision 4)
>      for (let element of parsedDoc.forms) {
>        this.mockOwnerDocumentProperty(element, parsedDoc, aDocumentURL);
>        this.mockOwnerGlobalProperty(element);
> +      for (let field of element) {

Can you rename `element` to `form` to make this more clear?

::: toolkit/modules/tests/modules/MockDocument.jsm:33
(Diff revision 4)
> +      for (let field of element) {
> +        this.mockOwnerGlobalProperty(field);
> +      }

Can you enumerate over form.elements rather than the element directly as I don't even know what that does.

Comment 13

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 14

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 15

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

https://reviewboard.mozilla.org/r/146130/#review152746

> Can you fix the ownerGlobal typo while you're here?

Sure thing.

> Can you rename `element` to `form` to make this more clear?

Good idea.

> Can you enumerate over form.elements rather than the element directly as I don't even know what that does.

Got it. Thanks!

Comment 16

[Scott Wu [:scottwu]]

Comment on attachment 8874766 [details]
Bug 1365895 - Select element fires events properly when autofilled.

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

Comment 17

[Scott Wu [:scottwu]]

Fixed eslint errors and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ed622e4719e9fe5ef440aa7d420bcfccfe3fb7e

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b02f4c0422d
Select element fires events properly when autofilled. r=MattN

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/0b02f4c0422d