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

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox51 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 3

3 years ago
Posted patch Part 3 - testSplinter Review
Attachment #8774823 - Flags: review?(mconley)
Assignee

Updated

3 years ago
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-

Updated

3 years ago
tracking-e10s: --- → +
Priority: -- → P1

Comment 7

3 years ago
bugherder
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee

Updated

3 years ago
Depends on: 1194027

Updated

2 years ago
See Also: → 1327149
You need to log in before you can comment on or make changes to this bug.