Bug 15816

Summary: Cannot select multiple non-adjacent items in a multiple select control with the keyboard only
Product: WebKit Reporter: Benjamin Hawkes-Lewis <bhawkeslewis>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cfleizach, cgarcia, commit-queue, cshu, darin, dmazzoni, gustavo, jdiggs, kling, mario, mrobinson, p.jacquemart, rniwa, webkit, xan.lopez
Priority: P2 Keywords: InRadar
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
URL: http://benjaminhawkeslewis.com/www/test-cases/multiple-select.php
Bug Depends on:    
Bug Blocks: 25531    
Attachments:
Description Flags
Patch that implements multi-selection of non-adjacents items with keyboard
none
New patch including tests
none
Updated patch rebased to current git master
abarth: review-
Updated Carlos patch to the trunk
none
Rebase to master 2013-12-16
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Listbox multiple selection - Skipped on Mac
none
Listbox multiple non contiguous selection
none
Listbox multiple non contiguous selection 2 none

Description Benjamin Hawkes-Lewis 2007-11-03 06:45:13 PDT
You can select multiple non-adjacent items in a multiple select using Command-Click. It should be possible to do the same with the keyboard. In Firefox Mac you can press space to select an item, then press Control-Up or Control-Down to move through the options without losing your selection and press space to select additional items. Apple Documentation does mention a way to select multiple items with the VoiceOver cursor, but this doesn't seem to work in HTML select boxes.

http://docs.info.apple.com/article.html?path=VoiceOver/1.0/en/mh2083.html

You can use the simple multiple select test case above to confirm this.
Comment 1 Benjamin Hawkes-Lewis 2007-11-03 07:06:59 PDT
When I say "this doesn't seem to work" above, I mean you can select items but it seems to be impossible to reliably preserve that selection when you start moving around in the list.
Comment 2 Robert Blaut 2008-02-24 13:10:44 PST
Confirmed.
Comment 3 chris fleizach 2008-07-24 11:40:04 PDT
don't think this is just an accessibility problem (if it still happens)
Comment 4 chris fleizach 2008-07-24 11:49:44 PDT
<rdar://problem/6100088>
Comment 5 chris fleizach 2008-07-25 14:58:25 PDT
VoiceOver does handle this correctly. the "keyboard focus follows VO focus" just needs to be turned off, then the user can move up and down the list with VO and select what they want with VO+spacebar. You may need to wait until the next release of VoiceOver to verify, but this should work in Leopard as well
Comment 6 Benjamin Hawkes-Lewis 2008-07-26 06:10:07 PDT
Hang on a second, that sounds good for VO users, but what if you're not using VoiceOver and are just trying to use keyboard navigation (for example, if you have a physical disability that makes mouse use problematic: http://www.apple.com/accessibility/physical/ )?
Comment 7 chris fleizach 2008-07-27 10:31:59 PDT
good point. the initial comment which referenced VoiceOver threw me off
Comment 8 Joanmarie Diggs 2009-10-25 09:41:13 PDT
I can reproduce this issue using WebKitGtk from git master. I'd update the platform, but I don't seem to have that priv.
Comment 9 Gustavo Noronha (kov) 2009-10-26 09:13:24 PDT
Reassigning to Forms instead of Accessibility, since there is no functionality to make accessible yet. It needs to work before that.
Comment 10 Mario Sanchez Prada 2010-11-04 01:29:54 PDT
I might be wrong, but while working on bug 25679 I found out that items inside a selection listbox are not focusable, but just selectable so that makes impossible to navigate through the list of items with the keyboard without losing a previous selection unless you're using the Shift modifier, but that's only valid for adjacent items...

I don't have a strong opinion here (still have to look more and more code to be 100% sure about this), but I feel like allowing items to be focusable might be a requirement to fix this bug (and help with bug 25679 as well), because that way you could do something like this:

  1. Arrow to the first item to be selected, which will be 'selected' and 'focused' at that time.
  2. Press Ctrl+arrow to keep navigating through the list *without* selecting anything, and keeping the previous selection untouched. This way, the previously selected item would be in 'selected' state only, while the current item being navigated would be in 'focused' (in firefox this is indicated by a dotted rectangle around it).
  3. Press space to select another item, which will be 'focused' by that time and that would be added to the list of 'selected' items.
  4. Go to 2 or finish with the selection process

So, could somebody comment on this and confirm whether this makes any sense and/or that the items in a selection listbox are 'selectable' but not 'focusable' ?

Thanks!
Comment 11 Mario Sanchez Prada 2010-11-04 01:30:44 PDT
Adding Chris Fleizach to CC
Comment 12 chris fleizach 2010-11-04 10:33:35 PDT
(In reply to comment #10)
> I might be wrong, but while working on bug 25679 I found out that items inside a selection listbox are not focusable, but just selectable so that makes impossible to navigate through the list of items with the keyboard without losing a previous selection unless you're using the Shift modifier, but that's only valid for adjacent items...
> 

The listbox itself is focusable, the items inside the list are selectable. you don't want to press tab to move through every item in a list, which is why the list items are not focusable. This is standard across OSs.

> I don't have a strong opinion here (still have to look more and more code to be 100% sure about this), but I feel like allowing items to be focusable might be a requirement to fix this bug (and help with bug 25679 as well), because that way you could do something like this:

MacOS does not have a standard way to do non-contiguous selection of items in a list using a keyboard only (I'm sure it's a long-standing request however). For this to be made to work on the Mac, that would have to be resolved. If other platforms have a standard way to do this kind of selection, then it should be straightforward to implement that behavior
Comment 13 Carlos Garcia Campos 2010-11-08 01:22:42 PST
Created attachment 73223 [details]
Patch that implements multi-selection of non-adjacents items with keyboard

This patch does the same than firefox, when Control key is pressed you can move the focus over the list items and select/deselect with Space key. List items aren't focusable, focus is handled by the ListBox so that pressing TAB doesn't make the focus iterate over list items.
Comment 14 Chang Shu 2010-11-09 13:25:04 PST
Great patch! This is exactly what I am looking for as I am trying to make spatial navigation work on multiple select box. I just have one suggestion. Is it ok we remove the current behavior of up/down key and make your ctrl+up/down as the default up/down behavior? The reasons are as follows:
1. Use up/down key for single-select type of behavior of multiple-select does not make a lot sense;
2. Using less keys makes the code more portable. For example, on mobile phones, usually no ctrl key is available.
3. This is easy for a user to use the select box without reading some instructions.
Thanks!
Comment 15 Joanmarie Diggs 2010-11-10 12:53:14 PST
(In reply to comment #14)
> Great patch! 

Yay! Thanks for doing this implementation Carlos. :-)

> I just have one suggestion. Is it ok we remove the current behavior of up/down key and make your ctrl+up/down as the default up/down behavior?

I would really (really) encourage you not to do this.

> 1. Use up/down key for single-select type of behavior of multiple-select does not make a lot sense;

Perhaps not. HOWEVER, this is what users who use the keyboard instead of the mouse are accustomed to having: In Windows, in GNOME, and (I believe) in OS X. And you don't always know something is multiselect until you try to select multiple items. So maybe it doesn't make sense from a design perspective, but if it suddenly goes away, I fear we shall confuse a non-trivial number of users.

> 2. Using less keys makes the code more portable. For example, on mobile phones, usually no ctrl key is available.

Point very well taken. And I don't have any brilliant answers to this issue, other than could it be handled at the platform/port level?

> 3. This is easy for a user to use the select box without reading some instructions.

Exactly. See my response to number 1. ;-) ;-)

All joking aside, the removal of (unmodified) Up/Down in this case may mean less reading of instructions for one group, but it's going to mean more reading of instructions for another.

So... Ideas?
Comment 16 Chang Shu 2010-11-10 19:18:24 PST
From what I understand, your major concern is "regression". But I feel it's really frustrating to do both up/down and ctrl+up/down at the same time: you can easily erase all your selections by one accidental up or down. Anyway, for this kind of UI design issues, I don't know how WebKit finalizes this.
In terms of the patch, it is missing the layout tests so it can't land as it is.

> > 1. Use up/down key for single-select type of behavior of multiple-select does not make a lot sense;
> 
> Perhaps not. HOWEVER, this is what users who use the keyboard instead of the mouse are accustomed to having: In Windows, in GNOME, and (I believe) in OS X. And you don't always know something is multiselect until you try to select multiple items. So maybe it doesn't make sense from a design perspective, but if it suddenly goes away, I fear we shall confuse a non-trivial number of users.
> 
> > 2. Using less keys makes the code more portable. For example, on mobile phones, usually no ctrl key is available.
> 
> Point very well taken. And I don't have any brilliant answers to this issue, other than could it be handled at the platform/port level?
> 
> > 3. This is easy for a user to use the select box without reading some instructions.
> 
> Exactly. See my response to number 1. ;-) ;-)
> 
> All joking aside, the removal of (unmodified) Up/Down in this case may mean less reading of instructions for one group, but it's going to mean more reading of instructions for another.
> 
> So... Ideas?
Comment 17 Carlos Garcia Campos 2010-11-10 23:24:01 PST
(In reply to comment #16)
> From what I understand, your major concern is "regression". But I feel it's really frustrating to do both up/down and ctrl+up/down at the same time: you can easily erase all your selections by one accidental up or down. Anyway, for this kind of UI design issues, I don't know how WebKit finalizes this.
> In terms of the patch, it is missing the layout tests so it can't land as it is.
> 

Yes, I know, just wanted to make sure it made sense before spending more time. I'll write the tests and upload a new patch
Comment 18 Carlos Garcia Campos 2010-11-11 00:18:03 PST
Created attachment 73584 [details]
New patch including tests
Comment 19 Chang Shu 2010-11-22 07:29:49 PST
ping reviewer. :)
Comment 20 Martin Robinson 2010-11-30 07:20:57 PST
Comment on attachment 73584 [details]
New patch including tests

View in context: https://bugs.webkit.org/attachment.cgi?id=73584&action=review

> WebCore/dom/SelectElement.cpp:785
> +            bool isCtrlKey = data.multiple() && static_cast<KeyboardEvent*>(event)->metaKey();

This variable has a pretty confusing name, since it doesn't just track whether the control key is down or not.
Comment 21 Mario Sanchez Prada 2010-12-13 01:31:01 PST
(In reply to comment #20)
> (From update of attachment 73584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=73584&action=review
> 
> > WebCore/dom/SelectElement.cpp:785
> > +            bool isCtrlKey = data.multiple() && static_cast<KeyboardEvent*>(event)->metaKey();
> 
> This variable has a pretty confusing name, since it doesn't just track whether the control key is down or not.

My humble "casual review" :-):

What about naming it something like 'keepCurrentSelection' ? After reading the patch I think its purpose si to know precisely when we don't want to change the current selection despite of moving down or up through the list, so I guess that would be a good name.

Other than that, I think this patch lacks some comments in the ChangeLogs in order to qualify for the r+
Comment 22 Carlos Garcia Campos 2010-12-14 03:24:09 PST
Created attachment 76522 [details]
Updated patch rebased to current git master

Renamed isCtrlKey as keepSelection and more verbose changelogs.
Comment 23 Mario Sanchez Prada 2011-01-09 23:23:37 PST
Ping reviewers? ;-)
Comment 24 Adam Barth 2011-04-26 16:11:37 PDT
Comment on attachment 76522 [details]
Updated patch rebased to current git master

cfleizach says we shouldn't accept this patch until there is a standardized way of doing this on the Mac.  Also, the platform ifdefs in SelectElement probably aren't the right approach for introducing platform-specific behavior.
Comment 25 Mario Sanchez Prada 2011-04-26 16:45:45 PDT
(In reply to comment #24)
> (From update of attachment 76522 [details])
> cfleizach says we shouldn't accept this patch until there is a standardized way > of doing this on the Mac.  Also, the platform ifdefs in SelectElement probably 
> aren't the right approach for introducing platform-specific behavior.

Problem is that, as far as I know, Mac doesn't need this patch since they can get the same functionality through the VO cursor, which is something that we don't have in GTK (so that's why we need the ability to select multiple, non adjacent, options by using keyboard navigation only).

Opinions, thoughts?
Comment 26 chris fleizach 2011-04-26 16:51:51 PDT
yea i didn't mean that we shouldn't accept the patch ever, since other platforms support this mode. it should be integrated in a way that is cognizant of that fact
Comment 27 Dominic Mazzoni 2012-05-16 00:14:43 PDT
It'd be nice to have this in Chromium on Windows. What would be the correct way to modify this patch to support these keyboard shortcut changes on only some platforms?
Comment 28 Pascal Jacquemart 2013-11-13 10:07:25 PST
Created attachment 216818 [details]
Updated Carlos patch to the trunk
Comment 29 Pascal Jacquemart 2013-11-14 02:11:10 PST
Short update: Multiple selection can already be achieved with keyboard only by enabling the spatial navigation feature.
The latest patch follows this path while pressing CTRL modifier + Arrow (CMD modifier on Mac)
Comment 30 Pascal Jacquemart 2013-11-18 02:21:46 PST
Anyone for review? Be kind this is my very first patch for WebCore
Comment 31 Darin Adler 2013-11-19 08:58:38 PST
I think it’s clear that we do not want this behavior on Mac, so this patch can’t go in as is. We need to figure out what behavior we want on the various platforms before we can figure out exactly how to code this. I believe this is wanted on Windows or any platform where the UI is a copy of how Windows does things (most Unix platforms, I think).
Comment 32 Pascal Jacquemart 2013-11-20 03:18:14 PST
Thanks for feedback... 
  
This is just a general accessibility feature we would like on Gtk (active without spatial navigation)
I know that support for full keyboard navigation is a general requirement for OS X applications so I just had a look on OS X Accessibilty Keyboard Shortcuts -> https://developer.apple.com/library/mac/documentation/Accessibility/Conceptual/AccessibilityMacOSX/OSXAXKeyboardShortcuts/OSXAXKeyboardShortcuts.html

It seems that these shortcuts are defined for "full keyboard access" mode
- Control-arrow key: Moves focus to another value or cell within a control such as a table
- Space bar: Selects the highlighted control (equivalent to clicking the mouse button)

In the meantime Command-arrow up / down is a very useful shortcut within the finder so it could be confusing for user if the same is used in Safari
Let me know if you can commit on a shortcut for Safari or enable this for other platforms or give up?
Comment 33 chris fleizach 2013-11-30 16:01:42 PST
Comment on attachment 216818 [details]
Updated Carlos patch to the trunk

I think the name meta_selection could be better. maybe something like m_allowsNonContiguousSelection or something.

Does GTK already have a system wide paradigm for doing this. the Mac does not, so we're not going to add something for the Mac, but if GTK already has a platform idiom then it seems to be sense to support it for just that platform
Comment 34 Mario Sanchez Prada 2013-12-04 05:48:15 PST
(In reply to comment #33)
> (From update of attachment 216818 [details])
> I think the name meta_selection could be better. maybe something like 
> m_allowsNonContiguousSelection or something.

I agree with this too.

> Does GTK already have a system wide paradigm for doing this. the Mac does not,
> so we're not going to add something for the Mac, but if GTK already has a
> platform idiom then it seems to be sense to support it for just that platform

I'm not sure if I'm properly answering your question Chris, but in GTK this is is the expected behaviour when navigating with the keyboard through elements inside widgets supporting multiple selection.

So I would say that yes, it's an expected idiom in the GTK platform.
Comment 35 Pascal Jacquemart 2013-12-04 17:01:57 PST
Yes, I can tell Mario made the same comment offline ;-)

Sorry but I am afraid m_allowsNonContiguousSelection is worst
What about m_isMultipleSelectionWithKeyboard ?

Second point is to enable for all platforms but MAC. Are you ok with something like...

#if !PLATFORM(MAC)
            m_isMultipleSelectionWithKeyboard = m_multiple && static_cast<KeyboardEvent*>(event)->ctrlKey();
#endif

Being aware that m_isMultipleSelectionWithKeyboard would then defaults to false on MAC
Comment 36 Pascal Jacquemart 2013-12-17 06:54:47 PST
Created attachment 219416 [details]
Rebase to master 2013-12-16

Updated after latest comments from Mario and Chris
The code is simpler and ctrl+arrow shortcut is disabled for Mac
Comment 37 chris fleizach 2013-12-18 11:47:47 PST
Comment on attachment 219416 [details]
Rebase to master 2013-12-16

View in context: https://bugs.webkit.org/attachment.cgi?id=219416&action=review

> LayoutTests/fast/forms/listbox-selection-expected.txt:35
> +PASS selectionPattern("sl15") is "01010"

won't this fail on Mac?

> Source/WebCore/html/HTMLSelectElement.cpp:1429
> +            m_allowsNonContiguousSelection = m_multiple && (static_cast<KeyboardEvent*>(event)->ctrlKey() || isSpatialNavigationEnabled(document().frame()));

is this supported on Windows with the same key combo (sorry for my obtuseness)
Comment 38 Build Bot 2013-12-18 12:44:39 PST
Comment on attachment 219416 [details]
Rebase to master 2013-12-16

Attachment 219416 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48798060

New failing tests:
fast/forms/listbox-selection.html
Comment 39 Build Bot 2013-12-18 12:44:43 PST
Created attachment 219561 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 40 Build Bot 2013-12-18 18:03:10 PST
Comment on attachment 219416 [details]
Rebase to master 2013-12-16

Attachment 219416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/46878147

New failing tests:
fast/spatial-navigation/snav-multiple-select-optgroup.html
fast/forms/listbox-selection.html
fast/spatial-navigation/snav-multiple-select.html
Comment 41 Build Bot 2013-12-18 18:03:15 PST
Created attachment 219598 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 42 Build Bot 2013-12-18 20:01:23 PST
Comment on attachment 219416 [details]
Rebase to master 2013-12-16

Attachment 219416 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/50098438

New failing tests:
fast/spatial-navigation/snav-multiple-select-optgroup.html
fast/forms/listbox-selection.html
fast/spatial-navigation/snav-multiple-select.html
Comment 43 Build Bot 2013-12-18 20:01:33 PST
Created attachment 219607 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 44 Pascal Jacquemart 2013-12-19 09:47:26 PST
Created attachment 219655 [details]
Listbox multiple selection - Skipped on Mac

Slightly modified not to interfere with spatial navigation tests
Created a separate test in order to keep original listbox tests unchanged
- Actual listbox selection tests will still be running on Mac
- The new test for multiple non contiguous selection with keyboard is skipped on Mac
Comment 45 Pascal Jacquemart 2013-12-20 01:34:36 PST
Thanks Chris,

Indeed it was a mess on Mac, sorry about that

I am fine in enabling it for Windows because this CTRL + Arrow shortcut is already available in explorer to achieve multiple selection.

But I am afraid I am not able to compile on Windows. What happened to the win build bot?
Comment 46 chris fleizach 2013-12-20 07:44:21 PST
Comment on attachment 219655 [details]
Listbox multiple selection - Skipped on Mac

View in context: https://bugs.webkit.org/attachment.cgi?id=219655&action=review

> LayoutTests/ChangeLog:9
> +        * fast/forms/listbox-selection-3.html: Added.

is it possible to rename this test
listbox-non-contiguous-keyboard-selection.html

so that it's self describing? otherwise, i think this is ready to go
Comment 47 Pascal Jacquemart 2014-01-02 06:38:09 PST
Created attachment 220223 [details]
Listbox multiple non contiguous selection

Simply renamed the new layout test
Comment 48 Pascal Jacquemart 2014-01-08 03:42:59 PST
Comment on attachment 220223 [details]
Listbox multiple non contiguous selection

Then with the flags should be better
Comment 49 chris fleizach 2014-01-08 08:57:43 PST
Comment on attachment 220223 [details]
Listbox multiple non contiguous selection

View in context: https://bugs.webkit.org/attachment.cgi?id=220223&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:1464
> +#endif

I feel like this block of code should be

#if !PLATFORM(MAC)
    m_allowsNonContiguousSelection = m_multiple && (isSpatialNavigationEnabled || static_cast<KeyboardEvent*>(event)->ctrlKey())
#endif
Comment 50 Pascal Jacquemart 2014-01-09 05:15:08 PST
> I feel like this block of code should be
> 
> #if !PLATFORM(MAC)
>     m_allowsNonContiguousSelection = m_multiple && (isSpatialNavigationEnabled || static_cast<KeyboardEvent*>(event)->ctrlKey())
> #endif

Fine, indeed this more a C like coding style and I recognize mixing bitwise and logical operator might lead to unexpected issues

Uploading a new patch...
Comment 51 Pascal Jacquemart 2014-01-09 07:36:51 PST
Created attachment 220732 [details]
 Listbox multiple non contiguous selection 2
Comment 52 WebKit Commit Bot 2014-01-09 09:19:49 PST
Comment on attachment 220732 [details]
 Listbox multiple non contiguous selection 2

Clearing flags on attachment: 220732

Committed r161558: <http://trac.webkit.org/changeset/161558>
Comment 53 WebKit Commit Bot 2014-01-09 09:19:55 PST
All reviewed patches have been landed.  Closing bug.