search

cisco-sbg-ui / magna-react

magna-react.vercel.app
5 stars 0 forks source link

ASelects (sometimes) changes selection on keyboard up/down presses instead of just focusing the next/previous option #184

Closed mvelkCisco closed 1 year ago

mvelkCisco commented 1 year ago

Describe the bug

This is a 2 in 1 bug (because of the "sometimes" part).

When an item is selected in ASelect, opening the select and hitting the up/down keys changes the selected item immediately. I think this is the intended behavior, judging based on the code: https://github.com/cisco-sbg-ui/magna-react/blob/canary/framework/components/ASelect/ASelect.js#L262.

However I don't think that's how a native select behaves. The up/down keys should highlight/focus the option, then user should confirm selection with Enter/Spacebar. See https://www.w3schools.com/tags/tryit.asp?filename=tryhtml_select.

The most confusing part is that it behaves inconsistently for me over time, as I interact with it. This part I don't understand from the code. Try:

  1. fresh load https://magna-react.vercel.app/components/select and don't touch anything
  2. click the "Fruit" in the select in the first example
  3. hit up and then down keys -> observe selection changing right away
  4. click the page background to close the select
  5. click the same select again to open it
  6. hit up and then down keys -> selection not changing anymore, just highlight and focus (which I would expect in 3. as well)

Here's a video: https://user-images.githubusercontent.com/51229231/219678163-bcf3f29b-869a-428e-bc4a-36d8a5e094ae.mov

This also causes issues for my AsyncSelect mentioned in #183. The selected item might not be even loaded. By hitting the up/down keys, the selection is changed immediately, without any possibility for going back (originally selected item is not on the screen).

To sum up: The selection should never change just by hitting up/down keys. Just the focus. Enter/Spacebar confirmation should be required to confirm the selection. And it should behave like this consistently.

Environment (please complete the following information):

rwharrisjr commented 1 year ago

@mvelkCisco Good find on this one. Issue seemed to be caused by competing key listeners. There is a set on ASelect, for the actual input box - when focused the input can use up/down to select items without the menu being open. However, when the menu is open the set of key listeners on AMenu would get used - if the menu was in focus. The check to focus the menu missed the ref.current on the first open (since it was checking for the selected item, which wouldn't be rendered to the DOM before the first mount), so the logic fell to the listeners on ASelect.

After #184 the behavior should be