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

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

unspecified
mozilla56
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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.
(Assignee)

Comment 1

8 months ago
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.
Summary: Make sure a trusted change event is dispatched when a select element is autofilled → When a select element is autofilled, it should fire events properly
(Assignee)

Updated

8 months ago
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8874766 - Flags: review?(MattN+bmo) → review?(bugs)
LGTM so requesting DOM peer review.

Comment 5

8 months ago
mozreview-review
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

8 months ago
mozreview-review
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.)
Attachment #8874766 - Flags: review?(bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8874766 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

7 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 18

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0b02f4c0422d
Select element fires events properly when autofilled. r=MattN
Keywords: checkin-needed

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0b02f4c0422d
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

7 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.