huntabyte / shadcn-svelte

shadcn/ui, but for Svelte. ✨
https://shadcn-svelte.com
MIT License
5.14k stars 318 forks source link

Unexpected focusing behavior using keyboard navigation #867

Closed dslatkin closed 7 months ago

dslatkin commented 7 months ago

Describe the bug

If you focus on certain interactive elements using keyboard navigation, then move focus to the next focusable element, you'll find focus moves one level deeper into the same interactive element that is already focused.

This is confusing for those using keyboard navigation or assistive technologies as you would expect focus to move to the next interactive element.

It's made more confusing by the fact that you can't see the second focus ring in dark mode.

The issue only reproduces on Mac Chrome for me, but Firefox and Safari seem fine.

Reproduction

  1. Open the calendar component in the docs
  2. Click the month to put focus roughly within the component
  3. Push Tab until focus is on one of the buttons
  4. Push Tab again and notice focus is still within the button

This does not happen on the same component on the official shad-ui library.

Screenshots Before focus: image After focusing on button: image After moving to "next" focusable element: image

Logs

No response

System Info

Able to reproduce on Chrome, but not Safari or Firefox.

Severity

annoyance

shyakadavis commented 7 months ago

Hi;

Something to note; this only happens for the New York style.

https://github.com/huntabyte/shadcn-svelte/assets/87414827/ea0ce500-88c5-4103-b88e-043616fc1e6b

shyakadavis commented 7 months ago

Could it be that svelte-radix icons are applying their own tab index or something?

shyakadavis commented 7 months ago

Okay, had a look at a raw svelte-radix svg, an this is more or less what it looks like;

<script>
  import { getContext } from 'svelte';
  const ctx = getContext('iconCtx') ?? {};
  export let size = ctx.size || '24';
  export let role = ctx.role || 'img';
  export let color = ctx.color || 'currentColor';
  export let ariaLabel = 'accessibility';
</script>

<svg
  width={size}
  height={size}
  {...$$restProps}
  {role}
  aria-label={ariaLabel}
  on:click
  on:keydown
  on:keyup
  on:focus
  on:blur
  on:mouseenter
  on:mouseleave
  on:mouseover
  on:mouseout
  viewBox="0 0 15 15"
  fill={color}
  xmlns="http://www.w3.org/2000/svg"
>
...
</svg>

Thoughts:

  1. Since there is no tabindex attribute, I'm guessing the role attribute is making it tab-able? But is this the behaviour for elements with a role on them? I don't think so.
  2. Since it is actively listening for events such as on:focus & on:blur, I'm guessing this is what's making it focus-able and tab-able?

Will keep looking around, and if there is a way to skip/overwrite this at shadcn-svelte's level, or perhaps open an issue at svelte-radix.

dslatkin commented 7 months ago

Only have a bit of time right now, but if I open up dev tools on the calendar component and remove both the blur and focus events, the issue doesn't seem to happen.

I made a little demo on Codepen. If you add a focus event listener to an SVG, it becomes focusable. If you comment out the event listener, it's no longer focusable. Still only on Chrome, but not Firefox or Safari.

One short term fix in shadcn-svelte could be to capture the focus and blur events on the Radix icon and stop it propagating before it reaches the SVG.

shyakadavis commented 7 months ago

This tracks indeed.

One short term fix in shadcn-svelte could be to capture the focus and blur events on the Radix icon and stop it propagating before it reaches the SVG.

My main concern about this, is that wouldn't we be forced to apply that patch/fix to every instance of the New York style that is using a svelte-radix icon? IMO, this doesn't feel right.

Let me play around and look into possible solutions for this; hope to arrive to an ideal one. 🙂

huntabyte commented 7 months ago

Hmm, this will definitely be problematic/annoying/unideal.

shyakadavis commented 7 months ago

Hey, @huntabyte

There is a solution I found, but I'm not sure about the efficacy/accessibility/better implementation of it, and it would have to be implemented at svelte-radix level.

Basically, it's just applying a negative tabindex to all svg's since from mdn they say

A negative value (the exact negative value doesn't actually matter, usually tabindex="-1") means that the element is not reachable via sequential keyboard navigation.

I am yet to try this specifically for our use case, but it works and brings up 2 solutions:

  1. Mention somewhere in shadcn-svelte docs about this, encourage/ask to add a negative tabindex to overcome this
  2. Or, if it sounds reasonable, ask the author of svelte-radix to apply this. I'm not sure about this, though.

Wdyt? Whichever, lmt, and I'll try to see it through.

huntabyte commented 7 months ago

I think opening up an issue/PR with svelte-radix would be best tbh. I don't imagine anyone expects that to be the default behavior of an SVG to receive focus.

huntabyte commented 7 months ago

Didn't mean to close!

dslatkin commented 7 months ago

Digging a bit more, apparently this is actually part of the SVG spec as an optional implementation detail, which Blink/Chrome adopted and I guess the other browser engines didn't.

In particular, user agents may ... allow focus to reach elements which have been assigned listeners for mouse, pointer, or focus events.

The issue definitely comes from svelte-radix - specifically it seems that with event forwarding, like done here, Svelte always sets up an event listener, regardless if any consumers of the component actually listen to the event.

If svelte-radix adds a negative tabindex attribute by default, I think they should also remove the on:focus and on:blur events anyways, because those functions have effectively become inert by doing that. So in general, I would say they should either

  1. remove on:blur and on:focus or
  2. forward all events using a library like this

It looks like the core issue (Svelte setting essentially up inactive listeners) will be addressed in Svelte 5 anyways and that should be the preferred way moving forward to forward all listeners instead of guessing at the ones devs will care about. I can give a stab at addressing it in a PR with svelte-radix (or at least opening an issue) later tonight. 🙂

huntabyte commented 7 months ago

Closing as this is resolved when updating to the latest version of the icon pack.