Skip to content

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`
@RobinMalfait RobinMalfait merged commit 30a6d51 into main Apr 25, 2025
8 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-3660 branch April 25, 2025 12:52
Sekhmet added a commit to Sekhmet/headlessui that referenced this pull request Aug 25, 2025
Closes: tailwindlabs#3752
Reuses code from: tailwindlabs#3704

Right now `touchend` only checks if target is HTMLElement, but this
check will fail for some other elements (like SVG elements).

This change brings dom utils from tailwindlabs#3704 to Vue to properly resolve
target.
RobinMalfait added a commit that referenced this pull request Sep 1, 2025
Closes: #3752
Reuses code from: #3704

Right now `touchend` only checks if target is HTMLElement, but this
check will fail for some other elements (like SVG elements).

This change brings dom utils from #3704 to Vue to properly resolve
target.

Before:


https://github.com/user-attachments/assets/668b79df-b4d7-4287-acac-1c4a8777d6dd

After:


https://github.com/user-attachments/assets/92e328a8-0994-40df-b4ca-0ca610b81d72

---------

Co-authored-by: Robin Malfait <[email protected]>
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