Closed Bug 1365895 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Form Manager, enhancement)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

Details

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

Attachments

(1 file)

Bug 1365895 - Select element fires events properly when autofilled.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
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.
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
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Attachment #8874766 - Flags: review?(MattN+bmo) → review?(bugs)
LGTM so requesting DOM peer 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 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 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 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 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!
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/0b02f4c0422d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: