🐛 <Listbox>: 4 bug fixes for scrolling & focus

[dmcnamara-eng]

✨ What Changed & Why

Fixes for the following 4 <Listbox> bugs related to scrolling & focus:

1. 🐛 Prevent arrow keys from scrolling the page 8f83502
2. 🐛 Scroll selected option into view ebccd58
3. 🐛 Scroll searched option into view 855f9e6
4. 🐛 it should be possible to open a listbox without submitting the form d14513a

[alexlafroscia]

(It's not sufficient to preventDefault on these events as we handle them, because page scrolling occurs during the capturing phase of event propagation.)

This is a good catch; I was wondering about this!

Is it possible for us to listen for the events during the capture phase as well to call .preventDefault rather than needing to add these additional event listeners?

[dmcnamara-eng]

Is it possible for us to listen for the events during the capture phase as well to call .preventDefault rather than needing to add these additional event listeners?

0eca5fa has a potentially cleaner solution to this. We can preventDefault during the keydown event, which is where the browser listens for it.

[dmcnamara-eng]

@alexlafroscia @GavinJoyce I reckon this is ready to merge. This fixes two important bugs when embedding the listbox in a (long) form.

[alexlafroscia]

Hey @dmcnamara-eng, sorry I can't push this through right now, I'm on my honeymoon and am not doing OSS while on vacation. I promise I'll check this out when I return next week! I've been a little neglectful of my OSS duties with the wedding but will be back with energy once I return home.

[dmcnamara-eng]

Welcome back and congratulations @alexlafroscia 🍯 🌚

I think that as long as the trigger button has role="button" on it, the form shouldn't be submitted... is that something you can confirm?

Yay! This is a cleaner fix of course. Actually this behaviour is determined by type=button, so that is fixed in f6687fb and 8976a1a reverts the event prevention. I've added a regression test for this is 8aad605

I think we have covered a lot here. I can squash the commits before merging?

Not sure why checks are failing? Checks are passing 🎉

[dmcnamara-eng]

I've cleaned this PR up and commit history for the following 4 <Listbox> bugs related to scrolling & focus:

1. 🐛 Prevent arrow keys from scrolling the page 8f83502
2. 🐛 Scroll selected option into view ebccd58
3. 🐛 Scroll searched option into view 855f9e6
4. 🐛 it should be possible to open a listbox without submitting the form d14513a

We are using this branch in production and these fixes feel solid 👍

Please 👀 @alexlafroscia 💥

[alexlafroscia]

Two tiny thoughts (neither of which are blockers here, by any means)

1. We could move this array up to the module scope to avoid creating it on every keydown event from the user.
2. We could make it a Set instead of an array, which might make the code read a little more easily

if (HANDLED_KEYDOWN_EVENTS.has(event.key)) {
  ...
}

[dmcnamara-eng]

👍 Agreed. Much cleaner and more performant. I've fixed that here: 6f34df6

[alexlafroscia]

Thank you so much for all of your patience with me @dmcnamara-eng -- this looks great! Thank you for this PR 🙏