feat(drawer): add focus management to embedded drawers #3139
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ore focusing previously focused element
… for improved focus handling
🦋 Changeset detectedLatest commit: 8e7471d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive focus management to embedded drawers, enhancing accessibility and keyboard navigation. When an embedded drawer opens, focus automatically moves to the first focusable element within the drawer, and when closed, focus is restored to the previously focused element.
Key changes:
- Implements focus management logic in
PanelGridcomponent for embedded drawers - Removes redundant focus handling and ARIA attributes from existing components
- Adds interaction stories to test focus behavior for both embedded and overlay drawer modes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/drawer/src/LayoutComponent/PanelGrid/PanelGrid.tsx |
Adds focus management logic with useIsomorphicLayoutEffect to handle focus transitions |
packages/drawer/src/DrawerToolbarLayout/DrawerToolbarLayout/DrawerToolbarLayoutContent.tsx |
Removes redundant aria-live attributes that are now handled elsewhere |
packages/drawer/src/DrawerToolbarLayout/DrawerToolbarLayout/DrawerToolbarLayout.interactions.stories.tsx |
Adds comprehensive interaction stories to test focus management behavior |
packages/drawer/src/Drawer/Drawer.tsx |
Removes old focus handling logic and adds VisuallyHidden live region for screen reader announcements |
.changeset/beige-lights-kick.md |
Documents the changes for release notes |
Contributor
|
Size Change: +711 B (+0.04%) Total Size: 1.8 MB
ℹ️ View Unchanged
|
…sableElement for improved accessibility
…manage drawer toggle behavior and enhance focus management
…ing focus to previously focused element on close
…ractions and add resizer data attributes
| } else { | ||
| // If the previously focused element is no longer in the DOM, focus the body | ||
| // This mimics the behavior of the native HTML Dialog element | ||
| document.body.focus(); |
There was a problem hiding this comment.
[nitpick] Falling back to document.body.focus() may fail because body is not guaranteed to be focusable, leaving keyboard users without a clear focus target. Consider ensuring the fallback element is explicitly focusable (e.g., adding tabIndex='-1' to body or selecting another stable focusable ancestor) or skipping the fallback to avoid a failed focus call.
Suggested change
| document.body.focus(); | |
| const body = document.body; | |
| const hadTabIndex = body.hasAttribute('tabIndex'); | |
| if (!hadTabIndex) { | |
| body.setAttribute('tabIndex', '-1'); | |
| } | |
| body.focus(); | |
| if (!hadTabIndex) { | |
| body.removeAttribute('tabIndex'); | |
| } |
shaneeza
commented
Oct 16, 2025
| const isAriaVisible = element.getAttribute('aria-hidden') === 'false'; | ||
| const { display, opacity, visibility } = window.getComputedStyle(element); | ||
|
|
||
| const isCSSVisible = |
Collaborator
Author
There was a problem hiding this comment.
I'm removing this check because Drawer no longer updates these properties directly in the component, meaning the values were always the same regardless of whether the drawer was open or closed.
Collaborator
There was a problem hiding this comment.
noted, is there something else we should check in addition to the aria attribute?
stephl3
reviewed
Oct 17, 2025
| }); | ||
| }; | ||
|
|
||
| // Reusable play function for testing focus management with toolbar buttons |
Collaborator
There was a problem hiding this comment.
looks like there is a flaky snapshot because it's taking before animations/repositioning completes. can we add snapshot delays to ensure UI is stable before capturing? https://www.chromatic.com/docs/delay/
| const isAriaVisible = element.getAttribute('aria-hidden') === 'false'; | ||
| const { display, opacity, visibility } = window.getComputedStyle(element); | ||
|
|
||
| const isCSSVisible = |
Collaborator
There was a problem hiding this comment.
noted, is there something else we should check in addition to the aria attribute?
Comment on lines
143
to
146
| * Focuses the first focusable element in the drawer when the drawer is opened. | ||
| * Also handles restoring focus when the drawer is closed. | ||
| * | ||
| * This is only necessary for embedded drawers. Overlay drawers use the native focus behavior of the dialog element. |
Collaborator
There was a problem hiding this comment.
queryFirstFocusableElement has a set list of focusable selectors it considers and "the native focus behavior of the dialog element" is to look for children with the autofocus attribute. However, these don't actually cover all cases that consumers may want to focus, such as a code editor.
After receiving feedback about this use case, it led to this change reintroducing the initialFocus prop in Modal. We don't use Modal and Drawer in all the same ways, but seeing this change makes me think they should follow the same focus management pattern. We may be able to use a similar method as focusModalChildElement for the first part of this effect
Collaborator
Author
There was a problem hiding this comment.
I like this pattern. However, I think if we go with this approach, then we might have to override the default dialog behavior since initialFocus does not work by default.
So the pattern would be in a native dialog and the embedded div:
- Drawer opens
- We check the initialFocus prop
a) if auto, we find the autofocus attribute or the first focusable item
b) if provided, we focus that provided element
c) if null, we don't focus anything
We'll end up losing the native dialog focus management this way, but it does become more customizable.
However, I am curious about the modal focus management. Why do we need a ref option when you can add autofocus to the element?
Collaborator
There was a problem hiding this comment.
What is the native dialog focus management you're concerned about losing? In Modal, initialFocus is mostly additive and doesn't conflict
autofocus can only be applied to the typical focusable elements but can't be used for something like a code editor. Although the initial implementation of Modal used a css selector like "#my-code-editor", I added a ref option because it's more type-safe and robust than a string
Collaborator
Author
There was a problem hiding this comment.
What is the native dialog focus management you're concerned about losing?
Not necessarily losing, but having to re-implement it. I noticed here that if any child has autofocus, we let the browser focus it automatically, but we manually focus the first focusable element. So in this case, aren't we manually overriding the default behavior of finding the first focusable element and re-implementing it?
leafygreen-ui/packages/modal/src/utils/focusModalChildElement.ts
Lines 44 to 54 in f6058df
autofocus can only be applied to the typical focusable elements but can't be used for something like a code editor. Although the initial implementation of Modal used a css selector like "#my-code-editor", I added a ref option because it's more type-safe and robust than a string
Thanks for clarifying.
Collaborator
There was a problem hiding this comment.
So in this case, aren't we manually overriding the default behavior of finding the first focusable element and re-implementing it?
focusModalChildElement is the logic we use to determine how a child element should be focused. The consumer has full control using either the initialFocus prop or the autoFocus attribute on a child element. If initialFocus is not specified and a child element does not have autoFocus specified, then we query for the first focusable element. Happy to huddle on this if further clarification is needed!
Collaborator
Author
There was a problem hiding this comment.
Yes, actually would love to huddle on this, I'll Slack you
| return; | ||
| } | ||
|
|
||
| // Restore focus when closing (only if we had handled focus during this session) |
Collaborator
There was a problem hiding this comment.
This is starting to feel like a re-implementation of the <dialog> element's focus patterns. Is there an explicit ticket requesting this functionality? If so, I'm curious what do you think about using the <dialog> under the hood for displayMode="embedded"? Since we're using a non-modal <dialog> element, we already end up having to apply the overlay styles for displayMode="overlay". The benefit would be that we get focus management for free; trade-off is that we'd be incorrectly using a dialog element
Collaborator
Author
There was a problem hiding this comment.
There was no ticket requesting this. I just observed that the embedded drawer was not accessible when it was opened. And you're correct, I tried to mimic the native dialog behavior. I like that if we use a non-modal dialog, then we get this behavior for free, but I don't know if using dialog incorrectly is worth it. We're trying to be more intentional, and this seems semantically incorrect. It's not really a dialog, and screen readers would announce it as a dialog as well.
Also, If we end up adding the same focus management as the modal, then we probably won't even be using the free focus management from dialog.
Collaborator
Author
There was a problem hiding this comment.
Curious, why do you consider using a dialog incorrect in this case? Is it because a dialog is supposed to float on top of content?
Collaborator
There was a problem hiding this comment.
Yes, my understanding is that a dialog is intended for modal or non-modal overlays. What makes the embedded drawer not accessible? I'm curious what determines focus jumping to the embedded drawer as accessible vs not doing that?
Collaborator
Author
There was a problem hiding this comment.
My concern about the embedded drawer is that, when it is opened, the focus does not shift to the drawer. It's hard to find out what the correct accessibility behavior of the embedded drawer should be since this is a div and not a dialog. Since the overlay drawer is sitting on top of the content, it makes sense that the focus should automatically move to the drawer. For the embedded drawer, the drawer opens and shifts the content, but the focus remains on the trigger (if there is a trigger). The consumer would have to do a lot of tabbing to get to the drawer.
The research I've done on this hasn't been that helpful, but I still think I should do some more investigating before moving forward with this change.
Collaborator
There was a problem hiding this comment.
I see, this sounds like it may vary depending on product usage. Can we pull design in and maybe discuss usage with product engineers before applying to all cases?
Alternatively, we could put this behind a discriminated union prop and make this opt-in. i.e.
- if
overlay, rely onautoFocusbeing set (although this also may requireinitialFocus) - if
embedded, check prop to determine focus management. Could see this working in a few ways; a couple ideas are:- default focus first focusable (breaking change)
- default no focus (minor change) and allow users to specify
"auto"which would focus first focusable or specify an element ref
…nts before fallback
Comment on lines
+39
to
+40
| componentInitialFocus, | ||
| contextInitialFocus, |
There was a problem hiding this comment.
Add JSDoc comments for the new componentInitialFocus and contextInitialFocus parameters to explain their purpose and relationship to the resolved initialFocus value.
Comment on lines
188
to
190
| if (targetElement instanceof HTMLElement) { | ||
| targetElement.focus(); | ||
| return; |
There was a problem hiding this comment.
The instanceof HTMLElement check will always be true if targetElement is not null since it's retrieved via querySelector or a ref's current. Consider simplifying to check for null/undefined instead, or add a warning when the element isn't found.
Suggested change
| if (targetElement instanceof HTMLElement) { | |
| targetElement.focus(); | |
| return; | |
| if (targetElement) { | |
| targetElement.focus(); | |
| return; | |
| } else { | |
| // Optionally warn if the element was not found | |
| if (process.env.NODE_ENV !== 'production') { | |
| // eslint-disable-next-line no-console | |
| console.warn( | |
| '[LeafyGreenUI][Drawer] Unable to set focus: target element not found for initialFocus:', | |
| initialFocus, | |
| ); | |
| } |
Comment on lines
218
to
+254
| return ( | ||
| <LeafyGreenProvider darkMode={darkMode}> | ||
| {/* Live region for announcing drawer state changes to screen readers */} | ||
| {open && ( | ||
| <VisuallyHidden aria-live="polite" aria-atomic="true"> | ||
| {`${title} drawer`} | ||
| </VisuallyHidden> | ||
| )} |
There was a problem hiding this comment.
The live region announcement should specify whether the drawer is opening or closing for clearer screen reader feedback. Consider using 'opened' instead of just announcing the title, e.g., ${title} drawer opened.
Suggested change
| return ( | |
| <LeafyGreenProvider darkMode={darkMode}> | |
| {/* Live region for announcing drawer state changes to screen readers */} | |
| {open && ( | |
| <VisuallyHidden aria-live="polite" aria-atomic="true"> | |
| {`${title} drawer`} | |
| </VisuallyHidden> | |
| )} | |
| // State for live region announcement | |
| const [liveMessage, setLiveMessage] = useState<string>(''); | |
| const prevOpenRef = useRef<boolean>(open); | |
| useEffect(() => { | |
| if (open !== prevOpenRef.current) { | |
| setLiveMessage( | |
| `${title} drawer ${open ? 'opened' : 'closed'}` | |
| ); | |
| prevOpenRef.current = open; | |
| } | |
| }, [open, title]); | |
| return ( | |
| <LeafyGreenProvider darkMode={darkMode}> | |
| {/* Live region for announcing drawer state changes to screen readers */} | |
| <VisuallyHidden aria-live="polite" aria-atomic="true"> | |
| {liveMessage} | |
| </VisuallyHidden> |
| data-testid="secondary-button" | ||
| id="secondary-button" | ||
| autoFocus={true} | ||
| // react does not add the autofocus attribute to the button element, so it needs to be added manually for embedded mode |
There was a problem hiding this comment.
Add a comment explaining why the autofocus attribute must be manually added as a workaround for React not adding it automatically in embedded mode, referencing the existing comment on line 299.
Suggested change
| // react does not add the autofocus attribute to the button element, so it needs to be added manually for embedded mode | |
| // React does not add the `autofocus` attribute to the button element when using the `autoFocus` prop. | |
| // In embedded mode, the browser requires the `autofocus` attribute to be present on the element for it to receive focus automatically. | |
| // Therefore, we manually add the `autofocus` attribute as a workaround. See comment above for more details. |
| "@leafygreen-ui/lib": "workspace:^", | ||
| "@leafygreen-ui/palette": "workspace:^", | ||
| "@leafygreen-ui/polymorphic": "workspace:^", | ||
| "@leafygreen-ui/resizable": "workspace:^", |
There was a problem hiding this comment.
The removal of @leafygreen-ui/tabs dependency should be verified to ensure it's not used elsewhere in the package. If this is a cleanup unrelated to focus management, it should ideally be in a separate PR.
…ity and usability
Contributor
|
Coverage after merging s/drawer-focus into main will be
Coverage Report for Changed Files
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
This PR enhances the drawer component by adding comprehensive focus management for embedded drawers. The changes ensure that when an embedded drawer is opened, focus automatically moves to the first focusable element within the drawer, and when closed, focus is restored to the previously focused element (typically the trigger button). This improves keyboard navigation and accessibility compliance.
🎟️ Jira ticket: No Jira ticket found
✅ Checklist
pnpm changesetand documented my changes🧪 How to test changes
Test embedded drawer focus management:
Test overlay drawer behavior:
Test accessibility with screen readers:
Run the new interaction stories:
EmbeddedToolbarIsFocusedOnCloseandEmbeddedButtonIsFocusedOnCloseOverlayToolbarIsFocusedOnCloseandOverlayButtonIsFocusedOnClose