Skip to content

Fix focus not returned to SVG Element #3704

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

Merged
merged 13 commits into from
Apr 25, 2025
Merged

Fix focus not returned to SVG Element #3704

merged 13 commits into from
Apr 25, 2025

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Apr 24, 2025

This PR fixes an issue where the focus is not returned to an SVG element with a tabIndex correctly.

There are a few issues going on here:

  1. We assume that the element to focus (e.target) is an instanceof HTMLElement, but the SVGElement is not an instanceof HTMLElement.
  2. By using instanceof we are checking against concrete classes, so if this happen to cross certain contexts (Shadow DOM, Iframes, ...) then the instances would be different.

To solve this, we will now:

  1. Relax the types and only care about the actual attributes and methods we are interested in. In most cases this means changing internal types from HTMLElement to Element for example.
  2. We will check whether certain properties are available in the object to deduce the correct type from the object.

Fixes: #3660

Test plan

Added an SVG to open a Dialog component and made sure that pressing escape or clicking outside of the Dialog does restore the focus to the SVG itself.

<svg
  tabIndex={0}
  onKeyDown={(e) => {
    if (e.key === 'Enter' || e.key === ' ') {
      setIsOpen((v) => !v)
    }
  }}
  onClick={() => setIsOpen((v) => !v)}
  className="h-6 w-6 text-gray-500"
>
  <BookOpenIcon />
</svg>

Here is a video of that behavior:

Screen.Recording.2025-04-25.at.12.00.45.mov

Copy link

vercel bot commented Apr 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 11:53am
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 25, 2025 11:53am

This was also causing visual issues in my editor, so fixed it while I
was at it.
This also adds a new `isElement` which is more specific than a Node, but
less specific than an HTMLElement.

E.g.: an `SVG` is an `Element` but not an `HTMLElement`

- For `isNode` we just check for `nodeType`
- For `isElement` we check that it's a `Node`, and has a `tagName`
  (which is the same as the `nodeName` but only available in `Element`s)
- For `isHTMLElement` we check that it's an `Element`, and has an
  `accessKey` which is only available in `HTMLElement`s but not in
  `Element`.
Otherwise `SVG`s won't be focusable even if they are focusable because
they don't inherit from `HTMLElement`
`HTMLOrSVGElement` doesn't inherit from anything, so we union it with
`Element` to make future types happy.
For the `d.style(…)` function, we expect a node that has a `style`
attribute. We don't care what kind of node that it is.
We only require `.matches(…)` which is available on `Element` so no need
for `HTMLElement`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus not returned to SVGElement
1 participant