Improve keyboard handling and accessibility for Select.svelte and refactor

[grepgrok]

As noted in #2802, I noticed Select.svelte has really bad keyboard handling (for example, you could select an option, but the popover wouldn't close) and generally didn't follow a11y guidelines as a combobox widget. I fixed this to adhere to the ARIA APG combobox pattern. To avoid a spaghetti mess of stores trying to pass values around, I encompassed SelectOption within Select and adjusted all of where Select is used accordingly.

Known Issues:

• Select.svelte
  • The combobox should have some kind of label (preferably aria-labelledby, but aria-label works) but I'm not sure how to identify how that should be done for the many different ways Select is used.
  • The popup is supposed to catch Escape and close just the popup but the window catches it before and closes.
• DropdownItem.svelte
  • Since selected options are only supposed to take visual, not DOM, focus, I have a focus class to do that. I tried to get it to resemble hovering over the option, however, they are very subtly different in realization.
• I think hovering should probably change the selected option, the ARIA APG doesn't mention anything on this and the example they provide uses different styling for hover and keyboard selection. I don't think it is a serious accessibility or usability concern, but it may be something to decide on.

This PR includes commits currently in review in #2787 because... I'm not good at git, sorry. Suggestions on good branch structuring would be appreciated. (I suppose just 1 branch for each PR, but I didn't think about that for #2787 and that has come back to bite me in the butt, sorry again.)

[dae]

Thanks Ben! I'll give this a proper review in the next few days after 23.10.1 is out the door, and your previous PR has been merged in.

[dae]

I've given this a test. Some thoughts:

• I noticed this error in the console when opening a dropdown:

Uncaught TypeError: Cannot read properties of undefined (reading 'classList')

• When pressing space on a dropdown, the screen also scrolls down. We should probably be using preventDefault() as well?
• Likewise when using the arrow keys to scroll up/down the expanded dropdown

[dae]

I recommend reverting this change. We're not ready to introduce custom keyboard shortcuts yet and only have one caller, so this doesn't meet the rule of three, and this function can't handle things like modifier keys.

[grepgrok]

Alright, those should be fixed now. I also realized that the arrow keys weren't properly scrolling through the page and quickly fixed that

[dae]

Please inline the values from ids and roles in the usage sites, and remove these lines, as using constants for DOM properties is not a pattern that the rest of the codebase uses (and you've been inconsistent with usage here too, using role="combobox" below.

[dae]

One more issue I spotted: if you tab down to 'insertion order' in the deck options and hit enter or space, you then have to hit down twice before a highlight/focus appears.

[dae]

Another minor issue: if you tab down to a lower dropdown that requires screen scrolling, then after selecting it and using the arrow keys, the page scrolls up/down as you change the focused element. It would be nice if the scrolling could be easily prevented when the new element is already visible.

[grepgrok]

I'm not really sure how to replicate what you had in that last comment. It did bring my attention to some weird scrolling behavior going on that I fixed by only scrolling if the element is not visible which seems to work much better.

[dae]

Thanks Ben, this is looking pretty good at this point, and the scrolling issue I mentioned has been fixed. The one remaining issue I spotted is that after selecting a dropdown with the keyboard, the up/down arrows always start from the top element. If it's not too difficult to have the selection start from the currently-active element (and ideally, show the focus outline prior to the user pressing up/down), that would make it feel even more polished.

[grepgrok]

Yeah, I didn't really know how exactly to handle that, the APG are a little vague so I guess it is more stylistic. I decided to keep Enter/Space/Click as just opening the popup without focusing a selection and you can then hit down arrow to go to the first item or up arrow to go to the last item. If you would prefer that should actually be from the active element, I can do that pretty easily.

[alexarnaud]

Hello,

I'm Alex, a low-vision person and accessibility specialist. I'm new to Anki though. I hope I'm right in the thread.

If you're testing "Select-Only Combobox" from APG: if you select a value, close the list and open it again you go back to the previously selected value, which is the result you expect as a end-user. It's not very clear but APG says:

"Opens the listbox if it is not already displayed without moving focus or changing selection."

Going back to the top is a change in focus.

The document also says:

"NOTE: When visual focus is in the listbox, DOM focus remains on the combobox and the value of aria-activedescendant on the combobox is set to a value that refers to the listbox option that is visually indicated as focused. Where the following descriptions of keyboard commands mention focus, they are referring to the visual focus indicator. For more information about this focus management technique, see Managing Focus in Composites Using aria-activedescendant."

Kind regards,
Alex.

[dae]

Thanks Alex - specialist input is much appreciated! The example combobox on that page does behave like I would expect, with the focus being on the currently-selected element when opened.

Ben: if you're not sure how to implement that, I'd suggest creating a new issue that describes what is still to be done, and then we can merge this in as-is, since it's still an improvement over what we had before.

[grepgrok]

Alex, thanks for the feedback. I am only somewhat familiar with accessibility and generally better at telling if something is just frustrating to use. I believe I already have the aria-activedescendent stuff working, though I honestly have no idea how to properly test that. As for the focus, I think I was getting a little confused in the differences between focus, visual focus, selection, previously selected, and active in APG but your explanation definitely helped.

Dae, I was just a little busy the last few days so couldn't work on it. I think it should be better now though. If there are still issues, I think it would be best to make it an open issue and merge as-is. Also, I'm thinking it may be a good idea to open a general accessibility issue thread to collect any of the many little problems (or that's a label? or maybe individual issues? idk github too well). It would just be useful to easily identify anything that has accessibility or other UX issues.

[dae]

Updated behavior is much nicer - thanks for figuring that out!

I've added an a11y label that you can assign to new tickets. For tracking particular issues, separate tickets are better, as they can be individually closed. If you feel a general issue is also required to discuss general a11y topics rather than specific problems, you're welcome to create that too.