design-system-react

design-system-react copied to clipboard

The SLDS docs state:

When triggered, user focus should be placed on the first focusable element that isn't the close button. If the close button is the only focusable element, focus should be placed there

It doesn't seem like that's been implemented here?

Great callout. Yes, that's correct. Due to the Popover "parent" not knowing when children are rendered and present (DOM queries, etc). A Popover could even have a Spinner in some instances. The Popover is focusing on "the border." The library is open to passing a ref to a prop to override the default behavior and follow this best practice (or other proposals).

Do you have an implementation proposal?

I guess that makes sense. Would it work to tie into the onOpen callback? And possibly add an option to disable the auto-focus functionality for implementations that need to load content asynchronously? Or maybe I'm misunderstanding the complexity here... 🤔

Yes, I believe you are understanding correctly.

However, I'd be hesitant to disable the initial focus since at least it moves the focus to where the user needs to be (consider the close button, etc).

A Popover is a region toggle that we need to be in as soon as it's open. We're completely open to allowing the user to handle the initial focus.

As a user, I could write my own handler for onOpen that would focus on the first focusable element (falling back to the "close" btn, if there are no other focusable elements in the body). What I'm wondering is whether the Popover component itself should do this by default at the same time it calls the user's custom onOpen handler. And if there are some edge cases where users wouldn't want that default functionality, an option could be added for them to opt-out.

Basically I was starting to write my own custom wrapper around the Popover component that adds this default functionality, but then I realized it might make sense for this to be in the library for other users as well, since it's in the SLDS spec. What am I missing?

In my mind, this would be a firstElementRef (or similar name) prop that would override and disable the default behavior if present (let's err on the side of only allowing folks that know what they are doing to disable the container focus) ;-p

That prop would call focus on the element passed in.

Sounds good to me! If no one else picks this up before I (or someone from my team) gets to it, we'll submit a PR for consideration. 👍

This issue has been automatically marked as stale, because it has not had recent activity. It will be closed if no further activity occurs. Maintainers are responsible for tech debt and project health. This is most likely a new components or component feature request. Please submit a pull request for or request feedback on this feature. Thank you.