418692 - Cannot open HTML <select> using [space]

Status: NEW

(Firefox :: Keyboard Navigation, defect)

Description

[Richard J. Turner]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3

There seems to be no way to expand a <select> box to view the options in it using just the keyboard. In Safari, and in normal applications, [space] displays the options and then the cursor keys can be used to highlight one and [space] again to select it.

Hitting [space], or [enter] does nothing - although one can use the cursor keys to iterate through the options. Being able to only see one option at a time is not very user-friendly though!

Reproducible: Always

Steps to Reproduce:
1. Open a page with a form on it
2. Use [tab] to step through the form until a <select> is highlighted
3. Hit the space-bar on the keyboard
Actual Results:  
The <select> box does not expand to show more options

Expected Results:  
The <select> box 'opens' to display a menu of options to choose from. One should then be able to use cursor keys to highlight an option and use the space-bar again to choose one, whereupon the <select> box 'closes' again with one's chosen option selected.

I would tag the severity of this as 'minor', except that it does pose a bit of an accessibility issue - the only way to expand the <select> appears to be to click on it with a mouse, but some users don't have the luxury of being able to use a mouse and it's not unlikely that those same users would have difficulty using a <select> box for which they can only see a single option at a time.

Comment 1

[Robert Sesek]

Attachment: Patch

Comment 2

[Robert Sesek]

Attachment: Unit Test WIP

I need some help with the unit test. There doesn't appear to be an interface for checking if the select menu is open or closed, which would be the easiest way to test this. This WIP tries to use sendMouseEvent() but I can't get it to work. Help appreciated :-).

Comment 3

[Boris Zbarsky [:bzbarsky]]

> There seems to be no way to expand a <select> box to view the options in it

Option-down?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 355248 [details] [diff] [review]
Patch

If we do want to do this, you want to just change the condition for DropDownToggleKey being called in nsListControlFrame::KeyPress, no?  In particular, this patch looks wrong to me in terms of firing onchange events, etc.

The change to make space do this probably needs review from Aaron, in any case.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and please add -p and -U 8 to your diff options?  That makes the diffs much easier to review.

Comment 6

[Robert Sesek]

Attachment: Patch v2

(In reply to comment #4)
> (From update of attachment 355248 [details] [diff] [review])
> If we do want to do this, you want to just change the condition for
> DropDownToggleKey being called in nsListControlFrame::KeyPress, no?  In
> particular, this patch looks wrong to me in terms of firing onchange events,
> etc.
> 
> The change to make space do this probably needs review from Aaron, in any case.

You're right, that's the better place to be making this change. I've updated the patch. Thanks.

(In reply to comment #5)
> Oh, and please add -p and -U 8 to your diff options?  That makes the diffs much
> easier to review.

Sorry about that. Fixed in this patch.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So just to check, shift-space, cmd-space, option-space, ctrl-space should all drop down the select?  Or do those send different charcodes?

Comment 8

[Aaron Leventhal]

Comment on attachment 355317 [details] [diff] [review]
Patch v2

You'll need review from an official layout peer, but it looks good to me. Are there any other keys, such as unmodified down arrow, which should work?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I can do the layout review, but I'd like an answer to comment 7 first.

Comment 10

[Robert Sesek]

(In reply to comment #7)
> So just to check, shift-space, cmd-space, option-space, ctrl-space should all
> drop down the select?  Or do those send different charcodes?

Shift+Space: Works
Ctrl+Space: Doesn't work
Opt+Space: Doesn't work
Cmd+Space: Doesn't work (intercepted by Spotlight)

(In reply to comment #8)
> (From update of attachment 355317 [details] [diff] [review])
> You'll need review from an official layout peer, but it looks good to me. Are
> there any other keys, such as unmodified down arrow, which should work?

Straight arrow-down will only advance the current selection. This patch actually removes the Alt/Opt+Down/Up to open a menu. I'm now rethinking this decision, as it could confuse cross-platform users if they try that regular shortcut key. I'll attach a slightly modified version of the patch that changes the #else to a #endif. That way, Mac users just get one more key binding but still retain the Firefox defaults.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Cmd+Space: Doesn't work (intercepted by Spotlight)

Does it work if you change the spotlight shortcut to something else?  And are those values for Gecko with your patch, or for Safari?  Or both?

Comment 12

[Robert Sesek]

Attachment: Patch v3

Here's said slightly-modified patch.

Comment 13

[Robert Sesek]

(In reply to comment #11)
> > Cmd+Space: Doesn't work (intercepted by Spotlight)
> 
> Does it work if you change the spotlight shortcut to something else?  And are
> those values for Gecko with your patch, or for Safari?  Or both?

I changed my Spotlight preferences to disable Cmd+Space, and here are the results:

Space by itself generates charcode 32.
Opt+Space generates 160.
Ctrl+Space doesn't generate a charcode.
Cmd+Space generates 32 as well, which opens the menu.

This behavior is identical to Safari's.

Comment 14

[Aaron Leventhal]

What about XUL menulists?

Comment 15

[Robert Sesek]

(In reply to comment #14)
> What about XUL menulists?

Strangely, different behavior. Space can be used to open them, but not close. Up/down keys not only alter the current selection, but also pop up the menu to do so. Furthermore, menus wrap-around when using the arrow keys.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Robert, are you waiting on someone with the last patch?

Comment 17

[Robert Sesek]

Sorry, I forgot to mark Aaron as review?. Aaron also mentioned getting a layout peer to review this?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I can do that and sr once Aaron reviews...  Just toss it in my sr bucket?

Comment 19

[alexander :surkov (:asurkov)]

Comment on attachment 355444 [details] [diff] [review]
Patch v3

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff --git a/layout/forms/nsListControlFrame.cpp b/layout/forms/nsListControlFrame.cpp
>--- a/layout/forms/nsListControlFrame.cpp
>+++ b/layout/forms/nsListControlFrame.cpp
>@@ -2510,8 +2510,16 @@
>   keyEvent->GetCharCode(&charcode);
> 
>   PRBool isAlt = PR_FALSE;
>+  keyEvent->GetAltKey(&isAlt);

nit: it doesn't make sense to create/initialize isAlt before your mac part.

>+  // in Mac OS X, it's normal to be able to open a list box via the space bar
>+#if defined(XP_MACOSX)
>+  if (charcode == nsIDOMKeyEvent::DOM_VK_SPACE) {
>+    DropDownToggleKey(aKeyEvent);
>+    return NS_OK;
>+  }
>+#endif
> 
>-  keyEvent->GetAltKey(&isAlt);

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Created an attachment (id=355249) [details]
> Unit Test WIP
> 
> I need some help with the unit test. There doesn't appear to be an interface
> for checking if the select menu is open or closed, which would be the easiest
> way to test this. This WIP tries to use sendMouseEvent() but I can't get it to
> work. Help appreciated :-).

I guess technically you could listen popupshowing/popuphidding events.

Comment 21

[Neil Deakin]

(In reply to comment #20)
> I guess technically you could listen popupshowing/popuphidding events.

Except that <select> don't use xul popups, so don't fire them.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> (In reply to comment #20)
> > I guess technically you could listen popupshowing/popuphidding events.
> 
> Except that <select> don't use xul popups, so don't fire them.

We should http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.cpp#384

Comment 23

[Aaron Leventhal]

Comment on attachment 355444 [details] [diff] [review]
Patch v3

I don't need to be the one who reviews keyboard nav code changes in Gecko, at this point. But, r+ anyway.

Comment 24

[Neil Deakin]

(In reply to comment #22)
> We should
> http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.cpp#384

Hmmm. That means that html select elements are firing non-standard events. Maybe they should be investigated to see if any sites are using them and/or removed. On the other hand, the events have been there since 2001.

Comment 25

[alexander :surkov (:asurkov)]

iirc popuphiding is used in accessibility and additinally accessibility needs for popupshown event (possibly popupshowing can take the place of popupshown) from html:select.