Closed Bug 1289528 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
e10s + ---
firefox51 --- fixed

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(3 files)

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.
Attached patch Part 3 - testSplinter Review
Attachment #8774823 - Flags: review?(mconley)
Attachment #8774823 - Attachment is patch: true
Attachment #8774823 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #8774821 - Flags: review?(mconley) → review+
Attachment #8774822 - Flags: review?(mconley) → review+
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.
Attachment #8774823 - Flags: review?(mconley) → review-
tracking-e10s: --- → +
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/82cf261d9fea
https://hg.mozilla.org/mozilla-central/rev/7e93e11713c4
https://hg.mozilla.org/mozilla-central/rev/8df572afbb5d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1194027
See Also: → 1327149
You need to log in before you can comment on or make changes to this bug.