sl-select is not looping sl-options on keydown

arunks5 commented 1 month ago

Describe the bug

sl-select component is not looping sl-options on keydown. It is focusing only the first sl-option. This behavior is implemented in w3.org https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/ .

To Reproduce

Steps to reproduce the behavior:

Go to https://codepen.io/Arun-Kumar-S/pen/LYKKgbm
Click on sl-select. Panel will be opened
Press S. Shoelace option will be focused.
Again press S. No changes.

On again pressing S, Summer option should have been focused.

Demo

Screenshots

If applicable, add screenshots to help explain the bug.

Browser / OS

OS: [e.g. Mac, Windows]
Browser: [e.g. Chrome, Firefox, Safari]
Browser version: [e.g. 22]

Additional information

Provide any additional information about the bug here.

CetinSert commented 1 month ago

For reference[^1], on RTCode.io, we have implemented this 'first letter focus' behavior for <sl-menu> and <sl-menu-item>s:

with :focus-visible, :focus-within, :has(), ::first-letter to Underline the keys of the active <sl-menu> <sl-menu-item>s:

[^1]: 👆🏻 Source code references.

open the RTCode.io playground
right click → View page source

search for

first-letter-focus for CSS _excerpt

      sl-menu:has(:focus-within:focus-visible):not(:has(sl-menu:focus-within)) {
        & > sl-menu-item:not([disabled]):not(:has(#ubt))::part(label)::first-letter {
          text-decoration: underline 1px solid color-mix(in oklch, currentColor, 75% transparent);
          text-underline-offset: 1.5px;
        }
      }

firstLetterFocus for JS _excerpt

      SlMenu.firstLetterFocus = (function() { // works with RTCode.io overrides
        let enabled = false;
        let lastKeyPressed = '';
        let lastKeyTime = 0;
        function toggle(enable) { enable = !!enable; document.querySelectorAll('sl-menu').forEach(menu => menu[`${enable ? 'add' : 'remove'}EventListener`]('keydown', menuKeyHandler)); enabled = enable; }
        function menuKeyHandler(e) { if (!enabled || e.modKey) return;
          const key = e.key.toLowerCase();
          const currentTime = Date.now();

          // Reset if different key is pressed or if too much time has passed since last key press
          if (key !== lastKeyPressed || currentTime - lastKeyTime > 1000) {
            lastKeyPressed = key;
            lastKeyTime = currentTime;
          }

          // Filter menu items based on first letter and focus accordingly
          const items = Array.from(document.activeElement.closest('sl-menu')?.querySelectorAll?.(':scope > sl-menu-item:not([disabled])') ?? [])?.filter?.(item =>
            item.textContent.trim().toLowerCase().startsWith(key) && getComputedStyle(item)?.display !== 'none'
          ); console.warn('firstLetterFocus', { items });

          if (items.length > 0) {
            // Find the next item to focus
            e.stopImmediatePropagation();
            const currentIndex = items.indexOf(document.activeElement);
            const nextIndex = (currentIndex + 1) % items.length; console.warn('firstLetterFocus', { nextIndex });
            items[nextIndex].focus();
          }
        }
        return {
          get enabled()  { return enabled;  },
          set enabled(v) {        toggle(v) }
        };
      })();

claviska commented 1 week ago

This component is designed to work like a standard <select>, where you can type an entire term and it will focus the correct one. The logic for this is located here:

https://github.com/shoelace-style/shoelace/blob/73c469ff45a56f359aaac11630fe185be4c5f178/src/components/select/select.component.ts#L375-L413

Our upcoming combobox will follow ARIA APG as closely as possible, but this component intends to behave like a <select> and that seems to be working.

KonnorRogers commented 1 week ago

@claviska Unfortunately <select> behaves as they describe.

If I have:

<select>
  <option>Adam</option>
  <option>Addison</option>
  <option>Amanda</option>
</select>

And I have the select focused + open and type "aaa" my focus will shift to "Amanda"

Now, this behavior is actually problematic.

Try this codepen in Chrome:

https://codepen.io/paramagicdev/pen/ZEgrPvv

My findings were it got stuck on the first two options and kept looping through them and never made it to the 3rd option. TBH, I never even knew this was the expected behavior until today, and the behavior is clearly broken in Safari and Chrome.

Its hard to tell from the video, but all I'm doing is opening the <select> and hitting the "a" key.

https://github.com/user-attachments/assets/9f833cb8-601d-49e8-b049-7e83fd7b6ce4

arunks5 commented 1 week ago

@claviska @KonnorRogers yes, the native select works with the same behavior. I missed to document at the beginning. As this is closed now, do we have any plan to implement this in <sl-select> ?

claviska commented 1 week ago

Unfortunately <select> behaves as they describe. If I have ... And I have the select focused + open and type "aaa" my focus will shift to "Amanda"

Depends on the browser…cycling through the same letter is a Firefox-only thing :)

KonnorRogers commented 1 week ago

@claviska Chrome cycles too...but only when the <select> is closed 🙃

claviska commented 1 week ago

Hmm. That makes a bit more sense.

shoelace-style / shoelace