Make MultiSelect keep single selection unless a checkbox is clicked

[acelaya]

Depends on #1664
Part of #1658

Change MultiSelect behavior, so that clicking on an option only keeps that option selected, and you have to click on the checkbox specifically in order to select multiple options.

The way the component interacts with the keyboard is by allowing you to focus the checkbox by pressing ArrowRight while an option is focused. This will focus the checkbox itself, allowing it to be changed by pressing Space.

While the checkbox is focused, pressing ArrowLeft will return focus to its surrounding option.

Grabacion.de.pantalla.desde.2024-08-19.17-25-32.webm

Todo

• Implement proper accessible markup
• Implement keyboard logic and focusable checkboxes
  I implemented it like in Sentry. When the right arrow is pressed, the checkbox for the currently focused option is focused, and when the left arrow is pressed, the focus returns to the option.
• Close listbox after an item is selected, unless using the checkboxes
• Review how the component is announced by screen readers. Compare with Sentry's one.
• Add tests

Out of scope

The "All students" option is meant to reset selection, which means "all" are selected in the MultiSelect context. However, that option itself is never marked as selected.

This is a known issue that already exists in current MultiSelect and I plan to address separately.

[acelaya]

This should have always been onKeyDown, not onKeyPress, as the keypress event is deprecated.

[acelaya]

I first tried to simply do e.stopPropagation() in the checkbox event handler, but it is an onChange handler that triggers after this onClick, so it didn't work.

That's why I did this explicit check here instead.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (6c318ba) to head (6dcec19).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #1665   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           61        61           
  Lines         1044      1061   +17     
  Branches       406       413    +7     
=========================================
+ Hits          1044      1061   +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robertknight]

I implemented it like in Sentry. When the right arrow is pressed, the checkbox for the currently focused option is focused, and when the left arrow is pressed, the focus returns to the option.

For the benefit of future developers, this refers to this control in Sentry's issue browser:

[robertknight]

This looks great. One minor detail I flagged up is whether we should be using key (the logical key) rather than code (the physical key) here. I believe it is very rare that these would be different for the Space and Enter keys specifically but we should be in the habit of knowing which we are using.

[acelaya]

I think the logic to use right/left arrows to move focus between the checkbox and the option, could potentially be implemented using useArrowKeyNavigation with horizontal navigation.

However, I did a quick check and there are some limitations:

• The hook does not expect to be defined in a component and its children, making them conflict with each other for some reason.
• One of the focusable options is the container itself, which the hook does not currently allow. Changing the markup would have other implications.

Because of these, I will consider refactoring it later, and keep it as is for now.

[robertknight]

The value of KeyboardEvent.key is ' ' for the space key. Otherwise LGTM. I should add that I'm not completely certain whether key or code is the "correct" choice here, though key is definitely correct if you want to match printed keys. From a quick look at native select implementations, it seems they internally use the deprecated keyCode.

[acelaya]

I should add that I'm not completely certain whether key or code is the "correct" choice here, though key is definitely correct if you want to match printed keys

I asked ChatGPT, and this was the answer:

When checking if a key has been pressed, it is generally more correct to use KeyboardEvent.key for the following reasons:

1. Readability: KeyboardEvent.key returns the actual value of the key pressed, which is more readable and intuitive (e.g., "a", "Enter", "ArrowDown").
2. Localization: KeyboardEvent.key takes into account the user's keyboard layout and locale, providing the correct character or key name.
3. Consistency: KeyboardEvent.key is consistent across different browsers and platforms.

KeyboardEvent.code is useful when you need to detect the physical key on the keyboard, regardless of the keyboard layout (e.g., "KeyA", "Enter", "ArrowDown"). This can be useful for games or applications where the physical key position is important.

In most cases, KeyboardEvent.key is preferred for general key detection.

Additionally, I quickly checked other keyboard event handler and we seem to use key always (or almost), so I think it makes sense to use that.

[robertknight]

LGTM