1289528 - [e10s] select fires click event when option is selected even when keyboard is used

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Neil Deakin]

Open a select element dropdown and use the keyboard to select an item (cursor and enter)

A mousedown, mouseup and click event fire even though the mouse wasn't used. We should be consistent with non-e10s select elements here and not fire them.

We probably need to fire key events instead but the patches here just disable the mouse events. Also, we may want to investigate touch events.

Comment 1

[Neil Deakin]

Attachment: Part 1 - the mouse events should fire on <option> not the <select>

Comment 2

[Neil Deakin]

Attachment: Part 2 - don't fire the mouse events when enter closes the menu

Comment 3

[Neil Deakin]

Attachment: Part 3 - test

Comment 4

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

Comment on attachment 8774821 [details] [diff] [review]
Part 1 - the mouse events should fire on <option> not the <select>

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

I'm really surprised how easy it is to dispatch events to <option> elements here... I remember having a hell of a time with it last year. I wonder what changed? Or maybe I missed something simple.

Comment 5

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

Comment on attachment 8774823 [details] [diff] [review]
Part 3 - test

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

Just some minor suggestions, but otherwise, pretty happy with this. Thanks Neil!

::: browser/base/content/test/general/browser_selectpopup.js
@@ +299,5 @@
>      gBrowser,
>      url: URL,
>    }, function*(browser) {
> +    for (let mode of ["enter", "click"]) {
> +      let menulist = document.getElementById("ContentSelectDropdown");

Can probably put this and the selectPopup definition outside of this loop.

@@ +347,5 @@
> +            Assert.ok(event.bubbles, "All of these events should bubble");
> +            Assert.equal(event.cancelable, expectation.cancelable,
> +                         "Cancellation property should match");
> +            Assert.equal(event.target.localName,
> +                         expectation.targetIsOption ? "option" : "select", 

Nit: Trailing whitespace

@@ +360,5 @@
> +            select.addEventListener(expectation.type, onEvent);
> +          }
> +
> +          if (mode == "enter") {
> +            expected.splice(2, 3);

I know that these events are unlikely to change, but splicing out the mouse events like this seems pretty non-obvious at first glance.

I think we can make this clearer by having two different arrays of expectations - one for the enter case, and one for the click.

Perhaps let's define an Object in this test like this:

let expectations = {
  "click": [{
    type: "input",
    cancelable: false,
    targetIsOption: false,
  }, {
    // ...
  }],

  "enter": [{
    type: "input",
    cancelable: false,
    targetIsOption: false,
  }, {
    // ...
  }],
};

Then we can iterate those keys in the outer loop, and pass in the mode and appropriate expectations to the ContentTask.

I think that'd make this test more obvious.

Comment 6

[Neil Deakin]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82cf261d9fea83f9d8fc2818c34f99b6ff2b3655
Bug 1289528, fire click event at option, not at select, r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e93e11713c4c46016c136601cbb26ac9439edb7
Bug 1289528, don't fire click event when option selected with keyboard, r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/8df572afbb5dd8c060eafefcb901d476b2862b69
Bug 1289528, add tests for keyboard and mouse selection of options, r=mconley

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82cf261d9fea
https://hg.mozilla.org/mozilla-central/rev/7e93e11713c4
https://hg.mozilla.org/mozilla-central/rev/8df572afbb5d