1296264 - e10s / non-e10s different behavior when the previously selected option of a <select> element is chosen again

Status: VERIFIED FIXED

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

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Note]:
- This is a follow up issue filed for Bug 1291078.

[Affected versions]:
- 51.0a1 (2016-08-17)
- 50.0a2 (2016-08-18)
- 49.0b4 build1 (20160814184416)
- 48.0.1 build3 (20160817112116)

[Affected platforms]:
- Windows 10 x64
- Ubuntu 14.04 x86 
- Mac OS X 10.10.4

[Steps to reproduce]:
1. Launch Firefox.
2. Enable e10s (if it isn't already).
2. Open https://bug1291078.bmoattachments.org/attachment.cgi?id=8776788.
3. Click the right menu button in order to open the menu.
4. Click an option from the menu in order to select it.
5. Open again the menu.
6. Click the same option (already selected in step 4).
- inspect the last fired events

[Expected result]:
- e10s and non-e10s behaviors correspond:  
 On step 6
 * mousedown
 * mouseup
 * click 
 are displayed (like in non-e10s case).

[Actual result]:
- Nothing is displayed.

[Regression range]:
- I will investigate this as soon as possible.

Comment 1

[Tracy Walker [:tracy]]

The option (in step 4 and step 6) remains selected. There is nothing user facing that is unexpected here.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

I don't think this is worth taking into a dot release. Wontfix for 48

Comment 3

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> I don't think this is worth taking into a dot release. Wontfix for 48

I agree, this is minor. This is another one of those things that the other browsers are all inconsistent on as well.

Comment 4

[Andrew Overholt [:overholt]]

Are we going to try to fix this for >= 49, Mike?

Comment 5

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

Yes. jaws and I are mentoring some MSU students who are going to try to combine the e10s and non-e10s select dropdown implementations. I'll see if I can make this part of the stack of work there.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Mike, according to all the context, looks this is something being planned to do. I set priority P2. Please change it as you see fit. (Also for bug 1096270)

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> Mike, according to all the context, looks this is something being planned to
> do. I set priority P2. Please change it as you see fit. 

(Also for bug 1096270)
              ^^^^^^^ typo here, should be bug 1296270 ^^"

Comment 8

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

P2 sounds fine to me, thanks hsinyi.

Comment 9

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Latest Nightly 53.0a1 (2016-12-28) build is also affected.
Analyzed the regression aspect. The issue is reproducible all the way back to 36.0a1 (2014-11-07) (when e10s was first enabled by default on a Nightly build) and previously. Therefore, this is not a recent regression.

Comment 10

[nadya]

Any word on when it can be expected? 
There is no workaround except for disabling e10s in order to get a click event. Other browsers appropriately trigger click event as there was indeed a click, even if it did not result in a change.

Comment 11

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 12

[Tommy Kuo (away forever...)]

Chrome:

When the user opens the menu, it triggers the following events by the order "mousedown," "mouseup" and "click" directly. If the user chooses a different option, "change" event would be fired after "click" event.

Safari:

When the user opens the menu, it triggers "mousedown" event. If the user chooses a different option, "change" event would be fired. If choosing the same option, nothing happens. After the menu closed, it triggers the following events by the order "mouseup" and "click."

Firefox:

When the user opens the menu, it triggers "mousedown" event. And if the user chooses the same option, nothing happens. Even the menu closed, there is no event fired. If the user chooses a different option, it triggers the following events "mouseup," "change" and "click."

Comment 13

[Ben Tian (inactive)]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #12)
> Firefox:

<e10s>

> When the user opens the menu, it triggers "mousedown" event. And if the user
> chooses the same option, nothing happens. Even the menu closed, there is no
> event fired. If the user chooses a different option, it triggers the
> following events "mouseup," "change" and "click."

<non-e10s>

When the user opens the menu, it triggers the following events by the order "mousedown," "mouseup" and "click" directly.
- If the user chooses a different option, it triggers the following events "mousedown," "mouseup," "change" and "click."
- If the user chooses the same option by clicking on <select>, it triggers the following events "mousedown," "mouseup," and "click." However if the user closes the menu by clicking outside <select>, no mouse event is fired.


=====
We may revise [1] to fire correspond mouse events but need to tell whether menu is closed by clicking on <select> or not.

[1] http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/toolkit/modules/SelectContentHelper.jsm#275-304

Comment 14

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Fire mouse events on <select> element no matter selected option changes or not

The patch revises event flow to fire mouse event (mousedown, mouseup, and click) no matter selected option changes or not.

Comment 15

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Do not fire mouse events when user closes <select> popup by clicking outside, r=mconley

The patch makes <select> fire no event when user closes popup by clicking outside.

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8910172 [details] [diff] [review]
Patch 1 (v1): Fire mouse events on <select> element no matter selected option changes or not

Olli, can you review my patches?

Two patches in total:
- The first patch fires mouse events (mousedown, mouseup, and click) on <select> when popup closes, no matter selected option changes or not.
- The second patch makes <select> fire no event if user closes popup by clicking outside it.


Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b5bb633ea3f5477ce73b69a8c033385b0784f0d

Comment 17

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

Comment on attachment 8910172 [details] [diff] [review]
Patch 1 (v1): Fire mouse events on <select> element no matter selected option changes or not

mconley knows a lot more about our <select> popup handling in e10s.

Comment 18

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

Comment on attachment 8910172 [details] [diff] [review]
Patch 1 (v1): Fire mouse events on <select> element no matter selected option changes or not

Review of attachment 8910172 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Does this pass the browser_selectpopup.js test?

::: toolkit/modules/SelectContentHelper.jsm
@@ +274,4 @@
>          break;
>  
>        case "Forms:DismissedDropDown":
> +        {

Nit: Thanks for wrapping this in a scope! Mind putting this opening brace up a line above, like:

case "Forms:DismissedDropDown": {
  // ...
}

That seems to be more in line with the style throughout the rest of the file.

Comment 19

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

Comment on attachment 8910173 [details] [diff] [review]
[final] Patch 2: Do not fire mouse events when user closes <select> popup by clicking outside, r=mconley

Review of attachment 8910173 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Just a naming suggestion.

::: toolkit/modules/SelectContentHelper.jsm
@@ +42,4 @@
>    this.element = aElement;
>    this.initialSelection = aElement[aElement.selectedIndex] || null;
>    this.global = aGlobal;
> +  this.closedWithClickOn = false;

This might be clearer as "this.closedWithMouse".

Comment 20

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

Comment on attachment 8910173 [details] [diff] [review]
[final] Patch 2: Do not fire mouse events when user closes <select> popup by clicking outside, r=mconley

Review of attachment 8910173 [details] [diff] [review]:
-----------------------------------------------------------------

Also, a new test for this case in browser_selectpopup.js would be truly excellent.

Comment 21

[Ben Tian (inactive)]

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #19)
> ::: toolkit/modules/SelectContentHelper.jsm
> @@ +42,4 @@
> >    this.element = aElement;
> >    this.initialSelection = aElement[aElement.selectedIndex] || null;
> >    this.global = aGlobal;
> > +  this.closedWithClickOn = false;
> 
> This might be clearer as "this.closedWithMouse".

|closedWithMouse| may refer to 'mouse click on dropdown' OR 'mouse click outside dropdown', no? Only closing by 'mouse click on dropdown' sets the flag and fires mouse events.

Let me know if I missed something.

> Thanks! Does this pass the browser_selectpopup.js test?

Yes it does.

(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from comment #20)
> Also, a new test for this case in browser_selectpopup.js would be truly
> excellent.

Sure. Will submit another patch for it.

Comment 22

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

(In reply to Ben Tian [:btian] from comment #21)
> 
> |closedWithMouse| may refer to 'mouse click on dropdown' OR 'mouse click
> outside dropdown', no? Only closing by 'mouse click on dropdown' sets the
> flag and fires mouse events.
> 

That's a fair point, okay.

Comment 23

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Fire mouse events on <select> element no matter selected option changes or not, r=mconley

Revise per reviewer's comment.

Comment 24

[Ben Tian (inactive)]

> (In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews from
> comment #20)
> > Also, a new test for this case in browser_selectpopup.js would be truly
> > excellent.
> 
> Sure. Will submit another patch for it.

browser_selectpopup.js has similar test that closes popup via ESC key [1]. In both tests (ESC key and click outside popup) SelectContentHelper receives message Forms:DismissedDropDown without prior Forms:SelectDropDownItem.

[1] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/base/content/test/forms/browser_selectpopup.js#195-200

Still checking how to synthesize mouse outside the popup, without mis-click on other elements.

Comment 25

[Ben Tian (inactive)]

Attachment: [final] Patch 2: Do not fire mouse events when user closes <select> popup by clicking outside, r=mconley

(In reply to Ben Tian [:btian] from comment #24)
> browser_selectpopup.js has similar test that closes popup via ESC key [1].
> In both tests (ESC key and click outside popup) SelectContentHelper receives
> message Forms:DismissedDropDown without prior Forms:SelectDropDownItem.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/browser/base/content/test/forms/
> browser_selectpopup.js#195-200

Decide to use the ESC key test as it covers click outside case. Also revise comment in patch 2 to add ESC key pressing case.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c856683ba3
Part 1: Fire mouse events on <select> element no matter selected option changes or not. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6002d9dfd96
Part 2: Do not fire mouse events when user closes <select> popup by clicking outside. r=mconley

Comment 27

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a4c856683ba3
https://hg.mozilla.org/mozilla-central/rev/a6002d9dfd96

Comment 28

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The issue is verified fixed on 58.0b6 build1 (20171123161455) across platforms (Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64).