Bug 15816

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.

Confirmed.

don't think this is just an accessibility problem (if it still happens)

<rdar://problem/6100088>

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

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/ )?

good point. the initial comment which referenced VoiceOver threw me off

I can reproduce this issue using WebKitGtk from git master. I'd update the platform, but I don't seem to have that priv.

Reassigning to Forms instead of Accessibility, since there is no functionality to make accessible yet. It needs to work before that.

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!

Adding Chris Fleizach to CC

(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

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.

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!

(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?

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?

(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

Created attachment 73584 [details] New patch including tests

ping reviewer. :)

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.

(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+

Created attachment 76522 [details] Updated patch rebased to current git master Renamed isCtrlKey as keepSelection and more verbose changelogs.

Ping reviewers? ;-)

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.

(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?

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

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?

Created attachment 216818 [details] Updated Carlos patch to the trunk

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)

Anyone for review? Be kind this is my very first patch for WebCore

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).

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 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

(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.

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

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 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 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

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 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

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 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

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

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

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 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

Created attachment 220223 [details] Listbox multiple non contiguous selection Simply renamed the new layout test

Comment on attachment 220223 [details] Listbox multiple non contiguous selection Then with the flags should be better

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

> 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...

Created attachment 220732 [details] Listbox multiple non contiguous selection 2

Comment on attachment 220732 [details] Listbox multiple non contiguous selection 2 Clearing flags on attachment: 220732 Committed r161558: <http://trac.webkit.org/changeset/161558>

All reviewed patches have been landed. Closing bug.