Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected focusing behavior using keyboard navigation #867

Closed
dslatkin opened this issue Mar 7, 2024 · 11 comments
Closed

Unexpected focusing behavior using keyboard navigation #867

dslatkin opened this issue Mar 7, 2024 · 11 comments
Labels
status: blocked Another issue or external requirement is preventing implementation

Comments

@dslatkin
Copy link

dslatkin commented Mar 7, 2024

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
Copy link
Contributor

Hi;

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

Screen.Recording.2024-03-07.at.11.08.49.mov

@shyakadavis
Copy link
Contributor

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

@shyakadavis
Copy link
Contributor

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
Copy link
Author

dslatkin commented Mar 7, 2024

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
Copy link
Contributor

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
Copy link
Owner

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

@shyakadavis
Copy link
Contributor

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
Copy link
Owner

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 huntabyte closed this as not planned Won't fix, can't repro, duplicate, stale Mar 9, 2024
@huntabyte huntabyte reopened this Mar 9, 2024
@huntabyte
Copy link
Owner

Didn't mean to close!

@dslatkin
Copy link
Author

dslatkin commented Mar 9, 2024

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked Another issue or external requirement is preventing implementation
Projects
None yet
Development

No branches or pull requests

3 participants