Closed Bug 1149745 Opened 5 years ago Closed 5 years ago

[e10s] Using the keyboard doesn't set the value in a <select> element

Categories

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

x86
All
defect
Not set
minor
Points:
3

Tracking

()

VERIFIED FIXED
mozilla41
Iteration:
41.2 - Jun 8
Tracking Status
e10s m7+ ---
firefox40 --- affected
firefox41 --- fixed
firefox44 --- verified

People

(Reporter: bokeefe, Assigned: enndeakin)

References

(Depends on 2 open bugs, Blocks 2 open bugs, )

Details

Attachments

(1 file, 3 obsolete files)

STR:
1. Open a page with a <select> element (this bug should work)
2. Click in the select element to open the dropdown (e.g. the OS list)
3. Type any key that would select an item in the list (e.g. 'M', for Mac OS X)

Expected results:
Corresponding element is highlighted, and the selection (above the list) is updated. (e.g. Mac OS X)

Actual results:
Corresponding element is highlighted, but the selection retains the previous value (e.g. Windows 7). Pressing 'tab' at this point closes the drop-down, but leaves the selection unchanged.

I can reproduce consistently with e10s enabled (with both my normal profile and a clean one); it works fine in a non-e10s window or in safe mode.


I added a smaller test case as a data uri in the URL field, as an option.
Hardware: x86_64 → x86
Confirmed on Mac, as well.
OS: Windows 7 → All
Assignee: nobody → gwright
tracking-e10s: --- → m7+
Duplicate of this bug: 1154097
Blocks: e10s-select
We probably want to make menulist on Windows select items as the keyboard navigates them. On Mac and Linux though the current behaviour for menulist is correct (which only selects once Enter is pressed).

If we want to instead emulate the non-e10s behaviour of <selects> here, we would need to add an attribute to menulist to control this behaviour which platform-specific defaults.

I should note though that Chrome uses the native behaviour for <select> on Mac which doesn't select on keypress (and presumambly on linux), and we may prefer to use that instead, which would mean this is a Windows-specific bug.
Can UX weigh in on the desired behaviour for selects here?
Flags: needinfo?(philipp)
Using the system behavior sounds like the right choice here.

I don't have a Mac available for testing at the moment, but the behavior on Windows should definitely be that moving the selection using the keyboard sets the selection instantly.
Flags: needinfo?(philipp)
Attached patch Work in progress patch (obsolete) — Splinter Review
This seems to work. One minor issue is:

1. Select an item by cursor down with the keyboard.
2. Press escape

The change event fires when the popup closes rather than when focus changes.
Assignee: gwright → enndeakin
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8
Points: --- → 3
Attached patch menulist-selectonkey (obsolete) — Splinter Review
I built this on top of bug 1047713.

Let me know if someone else should review some parts.
Attachment #8608909 - Attachment is obsolete: true
Attachment #8612439 - Flags: review?(felipc)
Comment on attachment 8612439 [details] [diff] [review]
menulist-selectonkey

Change to an accessibilty test, as items are now selected on Windows during cursor navigation, and Escape doesn't reset the value, which matches Windows behaviour of dropdowns.
Attachment #8612439 - Flags: review?(surkov.alexander)
Attachment #8612439 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8612439 [details] [diff] [review]
menulist-selectonkey

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

I don't usually review layout/xul patches, though it seems most of the work here is just passing down the new parameter introduced, except for the new code in nsMenuPopupFrame which looks simple enough.
Attachment #8612439 - Flags: review?(felipc) → review+
Comment on attachment 8612439 [details] [diff] [review]
menulist-selectonkey

I'll ask Neil to review the layout/xul changes anyway.
Attachment #8612439 - Flags: review?(neil)
Comment on attachment 8612439 [details] [diff] [review]
menulist-selectonkey

>+    // On Windows, a menulist should update its value whenever navigation was
>+    // done by the keyboard
So, I see the point of this, but I'm concerned that this could confuse existing menulist users, who might be expecting a command event when the value changes.
Neil, FWIW, Firefox hasn't had the behavior that just changing the value of the dropdown via the arrow keys would cause a new page to load. You always have to tab away from the combobox to entice a dynamic page load. This has been so since I joined Mozilla almost 8 years ago, and it is a very well-respected Firefox feature. ;)
Marco, are you referring to a specific menulist?
(In reply to neil@parkwaycc.co.uk from comment #11)
> So, I see the point of this, but I'm concerned that this could confuse
> existing menulist users, who might be expecting a command event when the
> value changes.

I'll make sure to fire the command event in this case.
(In reply to Neil Deakin from comment #13)
> Marco, are you referring to a specific menulist?

Yes, the "Language" combobox on https://mozilla.org. You select a new value with the arrow keys, and when you tab away or close the combobox (e. g. Alt+UpArrow on Windows), the page gets reloaded with the new language content without having to press a button to initiate the load.

In other browsers, changing the value with up and down arrows, without first expanding the combobox with alt+down arrow, the page would reload immediately upon arrow key press. In Firefox, it only reloads when either tabbing away or pressing alt+up arrow.
(In reply to Neil Deakin from comment #14)
> (In reply to neil@parkwaycc.co.uk from comment #11)
> > So, I see the point of this, but I'm concerned that this could confuse
> > existing menulist users, who might be expecting a command event when the
> > value changes.
> 
> I'll make sure to fire the command event in this case.

Actually, I don't think that's right. The command event should fire on Enter/Return or by clicking the mouse, and the select event should fire when the selection changes. I believe this is what happens with this patch as well.
Flags: needinfo?(neil)
(In reply to Marco Zehe from comment #15)
> (In reply to Neil Deakin from comment #13)
> > Marco, are you referring to a specific menulist?
> Yes, the "Language" combobox on https://mozilla.org.
Sorry, but I was talking about XUL menulists in the UI. Since the select is fired for programmatic value changes, I expect most consumers actually listen for command events; indeed, the only user of <menulist onselect> goes out of its way to avoid extraneous select events when they probably wanted to listen for command events. By comparison, there are are 46 examples in comm-central that put the oncommand on the same line as the menulist. If the arrow keys can change the selection without firing a command event, this could result in confusing or misleading UI.
Flags: needinfo?(neil)
Also updated the test to handle more keyboard navigation checks that would have failed without the IsOpen check I added to this version of the patch.
Attachment #8612439 - Attachment is obsolete: true
Attachment #8612439 - Flags: review?(neil)
Attachment #8622587 - Flags: review?(neil)
Comment on attachment 8622587 [details] [diff] [review]
Updated patch to send command events

Sorry for the delay, I've been really busy for the last fortnight.

>+  is(menulist.selectedIndex, isWindows ? 3 : 1, "Select item 2 selectedIndex");
Select item 3 selectedIndex?

>   // On Windows, one can navigate on disabled menuitems
>-  let expectedIndex = (navigator.platform.indexOf("Win") >= 0) ? 5 : 9;
>+  let expectedIndex = isWindows ? 5 : 9;
>      
>   is(menulist.menuBoxObject.activeChild, menulist.getItemAtIndex(expectedIndex),
>      "Skip optgroup header and disabled items select item 7");
>+  is(menulist.selectedIndex, isWindows ? 5 : 1, "Select or skip disabled item selectedIndex");
(I'm not sure how native menulists behave in the presence of disabled items, but IE8 or IE11's selects don't allow you to select disabled items at all, much like non-e10s selects.)

>+            menulist->SetSelectedItem(item);
>+
>+            // Fire a command event as well, but we don't want to close the menu,
>+            // blink it, or update any other state of the menuitem.
>+            nsContentUtils::DispatchXULCommand(aMenuItem->GetContent(),
>+                                               nsContentUtils::IsCallerChrome(),
>+                                               nullptr, PresContext()->PresShell(),
>+                                               false, false, false, false);
Doesn't menulist.xml's command event handler set the selected item anyway?

>diff --git a/toolkit/modules/SelectParentHelper.jsm b/toolkit/modules/SelectParentHelper.jsm
Now that you're firing command events are these changes necessary?
> >+  is(menulist.selectedIndex, isWindows ? 3 : 1, "Select item 2 selectedIndex");
> Select item 3 selectedIndex?

Will fix.

> (I'm not sure how native menulists behave in the presence of disabled items,
> but IE8 or IE11's selects don't allow you to select disabled items at all,
> much like non-e10s selects.)
> 

We could change this, but I think in a separate bug.


> >+            menulist->SetSelectedItem(item);
> >+
> >+            // Fire a command event as well, but we don't want to close the menu,
> >+            // blink it, or update any other state of the menuitem.
> >+            nsContentUtils::DispatchXULCommand(aMenuItem->GetContent(),
> >+                                               nsContentUtils::IsCallerChrome(),
> >+                                               nullptr, PresContext()->PresShell(),
> >+                                               false, false, false, false);
> Doesn't menulist.xml's command event handler set the selected item anyway?

I will change this.

> >diff --git a/toolkit/modules/SelectParentHelper.jsm b/toolkit/modules/SelectParentHelper.jsm
> Now that you're firing command events are these changes necessary?

I think only the change to not call hidePopup.
Attachment #8622587 - Attachment is obsolete: true
Attachment #8622587 - Flags: review?(neil)
Attachment #8626213 - Flags: review?(neil)
Comment on attachment 8626213 [details] [diff] [review]
menulist-selectonkey

>   receiveMessage: function(message) {
>     switch (message.name) {
>       case "Forms:SelectDropDownItem":
>-        if (this.element.selectedIndex != message.data.value) {
>-          this.element.selectedIndex = message.data.value;
>-
>+        this.element.selectedIndex = message.data.value;
>+        break;
>+      case "Forms:DismissedDropDown":
[IMHO this could do with a blank line after the break;]

>-  _registerListeners: function(popup) {
>-    popup.addEventListener("command", this);
>-    popup.addEventListener("popuphidden", this);
>+  _registerListeners: function(menupopup) {
>+    menupopup.addEventListener("command", this);
>+    menupopup.addEventListener("popuphidden", this);
>   },
> 
>-  _unregisterListeners: function(popup) {
>-    popup.removeEventListener("command", this);
>-    popup.removeEventListener("popuphidden", this);
>+  _unregisterListeners: function(menupopup) {
>+    menupopup.removeEventListener("command", this);
>+    menupopup.removeEventListener("popuphidden", this);
[Not sure it's worth renaming this as the callers always call it popup anyway.]
Attachment #8626213 - Flags: review?(neil) → review+
(In reply to Neil Deakin from comment #20)
> > (I'm not sure how native menulists behave in the presence of disabled items,
> > but IE8 or IE11's selects don't allow you to select disabled items at all,
> > much like non-e10s selects.)
> 
> We could change this, but I think in a separate bug.

Particularly since disabling options doesn't seem to work in e10s anyway.
https://hg.mozilla.org/mozilla-central/rev/11373e6e556f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1178373
Depends on: 1186398
QA Whiteboard: [good first verify][verify in Nightly only]
I have successfully reproduced the bug in firefox Nightly 40.0a1 (2015-03-31) with windows 10 PRO N (64 bit) 

Verified as fixed with 43.0a1 as comment 0

Build ID:  20150821030204
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0

[testday-20150821]
Reproduced this bug on Nightly 39.0a1 (2015-03-13) (Build ID: 20150313030231) by following comment 0 on Linux,64 bit

This Bug is now verified as fixed on Latest Firefox Nightly 44.0a1 (2015-10-03)  

Build ID: 20151003030225
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0

As it is also verified on Windows (Comment 26), Marking it as verified!
Status: RESOLVED → VERIFIED
Depends on: 1255513
Depends on: 1525896
You need to log in before you can comment on or make changes to this bug.