shinokada / svelte-radix

310+ SVG Radix icons components for Svelte
https://svelte-radix.codewithshin.com
MIT License
28 stars 3 forks source link

[BUG]: Forwarding on:focus and on:blur events on the icons causes unpredictable focus behavior #1

Closed dslatkin closed 6 months ago

dslatkin commented 6 months ago

Description of the bug

SVG icons are focusable by default in the library, contrary to their default behavior in HTML, which causes especially confusing behavior when using them inside other focusable elements such as <a> or <button> tags when using keyboard navigation or assistive technology.

This only happens in Chrome but not on Firefox or Safari (at least in my testing on Mac).

I originally reported this in huntabyte/shadcn-svelte#867.

Thank you! 🙏

Steps To Reproduce

On the library's website:

  1. Go to the icon search page
  2. Type in "circle" into the search bar
  3. Tab multiple times to see icons are focusable despite being non-interactive

A bit more confusing, in shadcn-svelte you can see the issue created when the icon is used inside a button:

  1. Go to a component using an icon like the calendar
  2. Click inside the box to bring focus close to the component
  3. Tab through the interactive elements, notice focus needs to move through the previous/next month buttons twice, once for the <button> and once for the <svg>
  4. Push Space or Enter to see that focus on the icon won't do anything

Additional Information

Why this is happening (I think):

In Svelte, if you add on:focus or on:blur to an HTML element to forward events to parent components, it sets up an event listener even if no component listens for the event. Unfortunately, the SVG spec allows focus-related listeners on SVGs to make the element focusable, but it's only an optional detail for rendering engines to implement. Blink (Chrome) implements it but Safari and Firefox do not.

Because of this, a situation like

<button>
    <Calendar />
</button>

is especially confusing for a user who is using keyboard navigation or assistive technology. They will have to tab/navigate twice for each icon.

I think a couple solutions would be:

  1. remove the on:blur and on:focus event forwarding (a button or link containing the button should be used anyways for accessible interactivity)
  2. use a preprocessor library like svelte-preprocess-delegate-events to forward all events without Svelte setting up listeners

I tested the second option and it seems to work without causing the same issue. I could open a PR but I think the first option would be simpler and encourage much better accessibility practices in general. 🙂 Svelte 5's new implementation will fix this in the future.

shinokada commented 6 months ago

Will this work? https://svelte-radix.codewithshin.com/#Unfocusable_icon

You can create a global custom icon. https://svelte-radix.codewithshin.com/#Setting_Global_Icon_using_setContext Or create a default icon, https://svelte-radix.codewithshin.com/#Creating_a_Default_Icon_Setting

dslatkin commented 6 months ago

That would "work" in the sense it is a workaround, but it doesn't feel good and I don't think it really addresses the root cause of having an unused event listener inadvertently make something focusable. Some of what's nice about using a library for icons is that one doesn't have to concern themselves with this kind of detail and wouldn't run into "gotchas", especially for things to do with focus handling. 🙁

huntabyte commented 6 months ago

While the tabindex="1" solves the issue of being focusable via tab, I'm still experiencing other behavior due to these defaults:

https://github.com/shinokada/svelte-radix/assets/64506580/bf20b635-f037-4d4f-9c04-67556d552637

Of course this is possible to address by adding "outline: none" to every single icon, or I suppose defining the global icon.

Would it be possible to add a withEvents prop to the icon that would take care of conditionally rendering it with events forwarded?

<script>
    export let withEvents = false
    // ... rest
</script>

{#if withEvents}
    <svg on:focus on:blur on:whatever />
{:else}
    <svg />
{/if}

I'd really like to continue using this project, so if you're open to the idea please let me know. If you're strapped for time and need a PR, a few of us are willing to assist!

If this isn't something you see yourself supporting, please let me know so I can determine how to proceed!

shinokada commented 6 months ago

No problem. I can update it.

dslatkin commented 6 months ago

Hi @shinokada, I had a working prototype using the svelte-preprocess-delegate-events library, so I polished it off and opened a PR with it. Feel free to use it or go a different direction of course, just submitting in case to save you some work if that's something you'd want. 🙂

shinokada commented 6 months ago

Please try v1.1.0. I will update the docs later.

shinokada commented 6 months ago

I close the issue for now. Thank you for your contribution.