1266575 - [E10s] click event is not dispatched to <select> when clicking on opened popup

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

https://bug1265968.bmoattachments.org/attachment.cgi?id=8744065 is a testcase.
Chrome and Edge at least seem to get the click. And non-e10s FF.

Didn't test what is the .target of the event. But I think click should be dispatched, and possibly mousedown/up.

Comment 1

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

Some of this stuff was discussed @ https://bugzilla.mozilla.org/show_bug.cgi?id=1090602

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey smaug, is there any new information on top of what was already discussed in bug 1090602? This got WONTFIX'd over there, with a nod of approval by web-compat...

Comment 3

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

That bug focus mostly on mousemoves and mouseovers and such, at least that is how I read it. click is way more important.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Ah, I remember now. Bug 1090602 was mostly concerned with events being fired on the <option> elements, and not the <select>.

That's what the analysis in bug 1090602 comment 27 is about.

I think that firing a click event on the <select> shouldn't be too difficult - we already do the change event.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Yoink

Comment 6

[Mike Conley (:mconley) (:⚙️)]

So what is the desired behaviour here, exactly? On my OS X machine, here's what I get when I use the mouse to select an item from the list:

== Firefox Nightly (e10s enabled) ==

* Change event

== Firefox Nightly (e10s disabled) ==

* Change event
* Click event on the <option> that was selected

== Safari 9.1 (10601.5.17.4) ==

* Input event on the <select>
* Change event
* Click event, targeted at the <select>

== Chrome 50.0.2661.102 (64-bit) ==

* Input event on the <select>
* Change event


So, unsurprisingly, there's some pretty wild inconsistency here.

Unfortunately, due to the challenges I already faced in bug 1090602, firing events on the <option> elements isn't going to be trivial, since the content process isn't convinced that the select dropdown is actually open, so is refusing to dispatch the events properly.

So what do we want to do here?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Roping in miketaylr who might have an opinion on this too.

Comment 8

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

note, input events are being implemented in other bug. (they are relatively new addition to the spec), so they are not about this bug.

On Linux I certainly do get click on Chrome and non-e10s FF.
There I get mousedown... change... mouseup...click.

I would expect to see at least click when closing the dropdown by selecting an option using mouse.
And to get click there should be mousedown/up.

Does anyone have Edge to test?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

The reason I'm not seeing mousedown and mouseup is because the page at https://bug1265968.bmoattachments.org/attachment.cgi?id=8744065 only logs click, input and change events.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test case

Comment 11

[Mike Conley (:mconley) (:⚙️)]

ni'ing myself to test my machine with Edge on it when I get back to the office.

Comment 12

[Mike Taylor [:miketaylr]]

Testing with https://bug1266575.bmoattachments.org/attachment.cgi?id=8752271, Chrome/Win 10 I get:

click (when opening the select)
change (when selecting an option)
click

I get the same thing in Edge (only difference being that their click events are PointerEvent of type click, while chrome fires MouseEvent of type click).

(And yeah, I think we need to support click on <select> to be consistent with other browsers.)

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Taylor [:miketaylr] from comment #12)
> Testing with
> https://bug1266575.bmoattachments.org/attachment.cgi?id=8752271, Chrome/Win
> 10 I get:
> 
> click (when opening the select)
> change (when selecting an option)
> click
> 
> I get the same thing in Edge (only difference being that their click events
> are PointerEvent of type click, while chrome fires MouseEvent of type click).
> 
> (And yeah, I think we need to support click on <select> to be consistent
> with other browsers.)

Interesting. While you're there, can you try with the attachment I put on this bug? That'll include the mousedown and mouseup events.

I'll do the same with Chrome, Safari and Firefox (both e10s and non-e10s), and then I _think_ we'll have a fuller picture of what's going on.

Comment 14

[Mike Taylor [:miketaylr]]

Sure thing. On Win 10:

Chrome Win10:
mousedown (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)
input (currentTarget: null, target: select)
change (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)

Edge: (currentTarget: select, target: select)
mousedown (currentTarget: select, target: select)
mouseup (currentTarget: select, target: select)
click (currentTarget: select, target: select)
mousedown (currentTarget: select, target: select)
mouseup (currentTarget: select, target: select)
change (currentTarget: select, target: select)
click (currentTarget: select, target: option)

I'll leave Fx testing to you. ^_^

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Firefox Nightly (e10s disabled):
mousedown (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)
mousedown (currentTarget: null, target: option)
mouseup (currentTarget: null, target: option)
change (currentTarget: null, target: select)
click (currentTarget: null, target: option)


Firefox Nightly (e10s enabled):
mousedown (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)
change (currentTarget: select, target: select)


Chrome 50.0.2661.102 (64-bit)
mousedown (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)
input (currentTarget: null, target: select)
change (currentTarget: null, target: select)


Safari 9.1 (10601.5.17.4)
mousedown (currentTarget: null, target: select)
input (currentTarget: null, target: select)
change (currentTarget: null, target: select)
mouseup (currentTarget: null, target: select)
click (currentTarget: null, target: select)


So, great. Pretty inconsistent - even between OS's (look at how Chrome differs between OS X and Windows...). What should we do?

Comment 16

[Mike Taylor [:miketaylr]]

Woof. Let's see what HTML says,

https://html.spec.whatwg.org/#the-select-element

"If the multiple attribute is absent, and the element is not disabled....Upon this option element being picked... and before the relevant user interaction event is queued (e.g. before the click event)... send select update notifications.

https://html.spec.whatwg.org/#send-select-update-notifications

And send select update notifications is defined as firing a bubbling "input" event at select, then firing a bubbling "change" event at the select.

So ignoring mousedown and mouseup, it sounds like the spec is suggesting an order of input, change, click. 

(It looks like Chrome on Win 10 gets closest.)

smaug, does that seem correct?

Comment 17

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

That sounds about right (though there is https://github.com/whatwg/html/issues/1092).
The most important part in this bug is that it seems that desktop browsers tend to dispatch click but we don't always do that. We dispatch click when opening the dropdown, but not when closing it, at least on linux.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Okay, so the plan of record here is to fire the following after changes are made to <select>:

* input
* change
* click

Correct? Unless I hear otherwise, I'll assume this is the case. Patch coming up.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

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

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1266575 - Regression test. r?felipe

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

Comment 21

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

I'd assume there to be mousedown/up dispatched so that ESM ends up generating click based on those.
If one uses DOMWindowUtils' sendMouseEvent, event dispatching goes through presshell and ESM, so click should be dispatched automatically.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

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

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

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

Comment 24

[Neil Deakin]

> let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor)
>                        .getInterface(Ci.nsIDOMWindowUtils);
>           let rect = this.element.getBoundingClientRect();
>           dwu.sendMouseEvent("mousedown", rect.left, rect.top, 0, 1, 0, true);
>           dwu.sendMouseEvent("mouseup", rect.left, rect.top, 0, 1, 0, true);

Wouldn't this send a mousedown/mouseup event as well? What if you didn't select the option with the mouse?

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to Neil Deakin from comment #24)
> > let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor)
> >                        .getInterface(Ci.nsIDOMWindowUtils);
> >           let rect = this.element.getBoundingClientRect();
> >           dwu.sendMouseEvent("mousedown", rect.left, rect.top, 0, 1, 0, true);
> >           dwu.sendMouseEvent("mouseup", rect.left, rect.top, 0, 1, 0, true);
> 
> Wouldn't this send a mousedown/mouseup event as well? What if you didn't
> select the option with the mouse?

Yes, we get the mousedown and mouseup events in that case as well.

Interestingly, in Chrome (at least on Windows), they fire a mouseup and click event if the keyboard is used to select an item from the <select> popup.

As far as I can tell, there's nothing really in the spec that makes it clear what we should do in the keyboard (or heck, even in the mouse) case, beyond the input and change events.

Perhaps what would be ideal is if I just find a way of mimicking what non-e10s Firefox already does for <select>'s for both mouse and keyboard. I may have found a way around the issues I was having in bug 1090602.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

https://reviewboard.mozilla.org/r/52828/#review50074

::: toolkit/modules/SelectContentHelper.jsm:19
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "setTimeout",
> +                                  "resource://gre/modules/Timer.jsm");

not used?

::: toolkit/modules/SelectContentHelper.jsm:24
(Diff revision 2)
> +// A process global state for whether or not content thinks
> +// that a <select> dropdown is open or not. This is managed
> +// entirely within this module, and is read-only accessible
> +// via SelectContentHelper.open.
> +var gOpen = false;

can you explain, in the comment or here in the bug, why adding this gOpen state tracker was necessary in the fixing of this bug?

It looks like the correct thing to do but I'm curious what problem this causes related to this particular bug

Comment 27

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

https://reviewboard.mozilla.org/r/52830/#review50080

Comment 28

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/52828/#review50074

> not used?

Good call.

> can you explain, in the comment or here in the bug, why adding this gOpen state tracker was necessary in the fixing of this bug?
> 
> It looks like the correct thing to do but I'm curious what problem this causes related to this particular bug

It turns out that sending down those mousedown / mouseup events to the <select> was causing the `mozshowdropdown` event to be fired again, which caused the dropdown to be re-opened. This is a guard against re-entry.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test case

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(In reply to Neil Deakin from comment #24)
> > let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor)
> >                        .getInterface(Ci.nsIDOMWindowUtils);
> >           let rect = this.element.getBoundingClientRect();
> >           dwu.sendMouseEvent("mousedown", rect.left, rect.top, 0, 1, 0, true);
> >           dwu.sendMouseEvent("mouseup", rect.left, rect.top, 0, 1, 0, true);
> 
> Wouldn't this send a mousedown/mouseup event as well? What if you didn't
> select the option with the mouse?

On second thought, given that Blink seems to send mouse events even if the user used the keyboard to select the <option>, I _think_ we're probably fine with this.

This whole part of the HTML spec is a real tragedy. There's absolutely no clarity about what is expected, event-wise.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

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

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

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

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80fd85454e2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bd7cfc6d8c

Comment 34

[:Felipe Gomes (needinfo for replies!)]

Yeah. Eventually we should use the same mechanism in EventStateManager that we use to forward input events in the <browser> element, extended to also do this for <option>s in the menupopups. The hard part is maintaining a mapping of the list in the parent to the corresponding <option>s in the child, and keeping that dynamically updated.

This would in theory map to the non-e10s behavior and arguably be the correct behavior that should be spec'ed. However, it's questionable how much is worth investing in making it perfect given that other browsers already don't fully support it.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80fd85454e2e
https://hg.mozilla.org/mozilla-central/rev/d2bd7cfc6d8c

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

Approval Request Comment
[Feature/regressing bug #]:

e10s

[User impact if declined]:

Potentially slightly less web compatibility for websites adding event listeners to <select> elements.

[Describe test coverage new/current, TreeHerder]:

An automated test is in the other patch waiting for approval.

[Risks and why]: 

This is very low risk, and only effects users with e10s enabled.

[String/UUID change made/needed]:

None.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

See above.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

Canceling uplift request until I can investigate a possible regression.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

See comment 36

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

See comment 36

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8752854 [details]
MozReview Request: Bug 1266575 - Dispatch click event on <select> after item is selected when in e10s mode. r?felipe

Improve the compatibility, taking it

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c547e1686c8
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e4d77c786f9