Closed Bug 1266575 Opened 8 years ago Closed 8 years ago

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

Categories

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

36 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s m9+ ---
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: smaug, Assigned: mconley)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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...
Flags: needinfo?(bugs)
That bug focus mostly on mousemoves and mouseovers and such, at least that is how I read it. click is way more important.
Flags: needinfo?(bugs)
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.
tracking-e10s: --- → ?
Assignee: nobody → jmathies
Yoink
Assignee: jmathies → 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?
Flags: needinfo?(bugs)
Roping in miketaylr who might have an opinion on this too.
Flags: needinfo?(miket)
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?
Flags: needinfo?(bugs)
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.
Attached file Test case (obsolete) —
Attachment #8752271 - Attachment mime type: text/plain → text/html
ni'ing myself to test my machine with Edge on it when I get back to the office.
Flags: needinfo?(mconley)
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.)
Flags: needinfo?(miket)
(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.
Flags: needinfo?(mconley) → needinfo?(miket)
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. ^_^
Flags: needinfo?(miket)
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?
Flags: needinfo?(miket)
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?
Flags: needinfo?(miket) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
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.
Attachment #8752854 - Flags: review?(felipc)
Attachment #8752855 - Flags: review?(felipc)
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/
Attachment #8752854 - Flags: review?(felipc)
Attachment #8752855 - Flags: review?(felipc)
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/
> 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?
(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 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
Attachment #8752854 - Flags: review?(felipc) → review+
Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

https://reviewboard.mozilla.org/r/52830/#review50080
Attachment #8752855 - Flags: review?(felipc) → review+
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.
Attached file Test case
Attachment #8752271 - Attachment is obsolete: true
Attachment #8753475 - Attachment mime type: text/plain → text/html
(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 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 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/
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.
https://hg.mozilla.org/mozilla-central/rev/80fd85454e2e
https://hg.mozilla.org/mozilla-central/rev/d2bd7cfc6d8c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8752854 - Flags: approval-mozilla-aurora?
Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

See above.
Attachment #8752855 - Flags: approval-mozilla-aurora?
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.
Attachment #8752854 - Flags: approval-mozilla-aurora?
Attachment #8752855 - Flags: approval-mozilla-aurora?
Depends on: 1275384
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
Attachment #8752854 - Flags: approval-mozilla-aurora?
Comment on attachment 8752855 [details]
MozReview Request: Bug 1266575 - Regression test. r?felipe

See comment 36
Attachment #8752855 - Flags: approval-mozilla-aurora?
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
Attachment #8752854 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8752855 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1291078
Depends on: 1291078
You need to log in before you can comment on or make changes to this bug.