-
Notifications
You must be signed in to change notification settings - Fork 73
feat(drawer): add focus management to embedded drawers #3139
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
base: main
Are you sure you want to change the base?
Changes from 17 commits
ce621ea
a76826e
58892b1
84dcd65
3192ddf
77eeca5
0f5fed4
0c27da5
91c57fe
6c1a865
a11fa22
5899e50
bd6d33e
2019a47
7ce2bd0
becc27b
df328af
82c84b9
377e665
ce01ff2
8e7471d
cec886d
121aa91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@leafygreen-ui/drawer': minor | ||
| --- | ||
|
|
||
| - Adds focus management to embedded drawers. Embedded drawers will now automatically focus the first focusable element when opened and restore focus to the previously focused element when closed. Overlay drawers use the native focus behavior of the dialog element. | ||
| - Adds visually hidden element to announce drawer state changes to screen readers. | ||
| - Removes CSS visibility check from the `isOpen` test utility since `opacity, `visibility` and `display` properties do not change when the drawer is opened or closed. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { forwardRef, useEffect, useRef, useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { useInView } from 'react-intersection-observer'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { VisuallyHidden } from '@leafygreen-ui/a11y'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useIdAllocator, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useIsomorphicLayoutEffect, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -11,6 +12,7 @@ import IconButton from '@leafygreen-ui/icon-button'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import LeafyGreenProvider, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useDarkMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@leafygreen-ui/leafygreen-provider'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { queryFirstFocusableElement } from '@leafygreen-ui/lib'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { usePolymorphic } from '@leafygreen-ui/polymorphic'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { Position, useResizable } from '@leafygreen-ui/resizable'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { BaseFontSize } from '@leafygreen-ui/tokens'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -134,27 +136,58 @@ export const Drawer = forwardRef<HTMLDivElement, DrawerProps>( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, [id, open, registerDrawer, unregisterDrawer]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const previouslyFocusedRef = useRef<HTMLElement | null>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const hasHandledFocusRef = useRef<boolean>(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Focuses the first focusable element in the drawer when the animation ends. We have to manually handle this because we are hiding the drawer with visibility: hidden, which breaks the default focus behavior of dialog element. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const autoFocusElement = modalElement.querySelector( | |
| '[autofocus]', | |
| ) as HTMLElement; | |
| if (autoFocusElement) { | |
| // Don't need to explicitly call `focus` because the browser handles it | |
| return autoFocusElement; | |
| } | |
| // Otherwise, focus first focusable element | |
| const firstFocusableElement = queryFirstFocusableElement(modalElement); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually would love to huddle on this, I'll Slack you
shaneeza marked this conversation as resolved.
Show resolved
Hide resolved
shaneeza marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
| 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'); | |
| } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,6 +274,83 @@ const playClosesDrawerWhenActiveItemIsRemovedFromToolbarData = async ({ | |
| }); | ||
| }; | ||
|
|
||
| // Reusable play function for testing focus management with toolbar buttons | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 playToolbarFocusManagement = async ({ | ||
| canvasElement, | ||
| }: { | ||
| canvasElement: HTMLElement; | ||
| }) => { | ||
| const canvas = within(canvasElement); | ||
| const { getToolbarTestUtils, isOpen, getDrawer } = getTestUtils(); | ||
| const { getToolbarIconButtonByLabel } = getToolbarTestUtils(); | ||
| const codeButton = getToolbarIconButtonByLabel('Code')?.getElement(); | ||
| // Wait for the component to be fully rendered and find the button by test ID | ||
| const openCodeButton = await canvas.findByTestId('open-code-drawer-button'); | ||
|
|
||
| // Verify initial state | ||
| expect(isOpen()).toBe(false); | ||
| userEvent.click(openCodeButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(isOpen()).toBe(true); | ||
| expect(canvas.getByText('Code Title')).toBeVisible(); | ||
| expect(getDrawer()).toContain(document.activeElement); | ||
| }); | ||
|
|
||
| // Toggle the drawer close | ||
| userEvent.click(codeButton!); | ||
|
|
||
| await waitFor(() => { | ||
| expect(isOpen()).toBe(false); | ||
| }); | ||
|
|
||
| // Focus should remain on the toolbar button | ||
| await waitFor(() => { | ||
| expect(document.activeElement).toBe(codeButton); | ||
| }); | ||
| }; | ||
|
|
||
| // Reusable play function for testing focus management with main content button | ||
| const playMainContentButtonFocusManagement = async ({ | ||
| canvasElement, | ||
| }: { | ||
| canvasElement: HTMLElement; | ||
| }) => { | ||
| const canvas = within(canvasElement); | ||
| const { getCloseButtonUtils, isOpen, getDrawer } = getTestUtils(); | ||
|
|
||
| // Wait for the component to be fully rendered and find the button by test ID | ||
| const openCodeButton = await canvas.findByTestId('open-code-drawer-button'); | ||
|
|
||
| // Verify initial state | ||
| expect(isOpen()).toBe(false); | ||
| expect(openCodeButton).toBeInTheDocument(); | ||
|
|
||
| userEvent.click(openCodeButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(isOpen()).toBe(true); | ||
| expect(canvas.getByText('Code Title')).toBeVisible(); | ||
| expect(getDrawer()).toContain(document.activeElement); | ||
| }); | ||
|
|
||
| // Get the close button from the drawer | ||
| const closeButton = getCloseButtonUtils().getButton(); | ||
| expect(closeButton).toBeInTheDocument(); | ||
|
|
||
| // Click the close button to close the drawer | ||
| userEvent.click(closeButton!); | ||
|
|
||
| await waitFor(() => { | ||
| expect(isOpen()).toBe(false); | ||
| }); | ||
|
|
||
| // Focus should return to the original "Open Code Drawer" button | ||
| await waitFor(() => { | ||
| expect(document.activeElement).toBe(openCodeButton); | ||
| }); | ||
| }; | ||
|
|
||
| // For testing purposes. displayMode is read from the context, so we need to | ||
| // pass it down to the DrawerToolbarLayoutProps. | ||
| type DrawerToolbarLayoutPropsWithDisplayMode = DrawerToolbarLayoutProps & { | ||
|
|
@@ -326,7 +403,12 @@ const Template: StoryFn<DrawerToolbarLayoutPropsWithDisplayMode> = ({ | |
| padding: ${spacing[400]}px; | ||
| `} | ||
| > | ||
| <Button onClick={() => openDrawer('Code')}>Open Code Drawer</Button> | ||
| <Button | ||
| onClick={() => openDrawer('Code')} | ||
| data-testid="open-code-drawer-button" | ||
| > | ||
| Open Code Drawer | ||
| </Button> | ||
| <LongContent /> | ||
| <LongContent /> | ||
| </main> | ||
|
|
@@ -476,6 +558,24 @@ export const OverlayClosesDrawerWhenActiveItemIsRemovedFromToolbarData: StoryObj | |
| play: playClosesDrawerWhenActiveItemIsRemovedFromToolbarData, | ||
| }; | ||
|
|
||
| export const OverlayToolbarIsFocusedOnClose: StoryObj<DrawerToolbarLayoutPropsWithDisplayMode> = | ||
| { | ||
| render: (args: DrawerToolbarLayoutProps) => <Template {...args} />, | ||
| args: { | ||
| displayMode: DisplayMode.Overlay, | ||
| }, | ||
| play: playToolbarFocusManagement, | ||
| }; | ||
|
|
||
| export const OverlayButtonIsFocusedOnClose: StoryObj<DrawerToolbarLayoutPropsWithDisplayMode> = | ||
| { | ||
| render: (args: DrawerToolbarLayoutProps) => <Template {...args} />, | ||
| args: { | ||
| displayMode: DisplayMode.Overlay, | ||
| }, | ||
| play: playMainContentButtonFocusManagement, | ||
| }; | ||
|
|
||
| export const EmbeddedOpensFirstToolbarItem: StoryObj<DrawerToolbarLayoutPropsWithDisplayMode> = | ||
| { | ||
| render: (args: DrawerToolbarLayoutPropsWithDisplayMode) => ( | ||
|
|
@@ -542,6 +642,28 @@ export const EmbeddedClosesDrawerWhenActiveItemIsRemovedFromToolbarData: StoryOb | |
| play: playClosesDrawerWhenActiveItemIsRemovedFromToolbarData, | ||
| }; | ||
|
|
||
| export const EmbeddedToolbarIsFocusedOnClose: StoryObj<DrawerToolbarLayoutPropsWithDisplayMode> = | ||
| { | ||
| render: (args: DrawerToolbarLayoutPropsWithDisplayMode) => ( | ||
| <Template {...args} /> | ||
| ), | ||
| args: { | ||
| displayMode: DisplayMode.Embedded, | ||
| }, | ||
| play: playToolbarFocusManagement, | ||
| }; | ||
|
|
||
| export const EmbeddedButtonIsFocusedOnClose: StoryObj<DrawerToolbarLayoutPropsWithDisplayMode> = | ||
| { | ||
| render: (args: DrawerToolbarLayoutPropsWithDisplayMode) => ( | ||
| <Template {...args} /> | ||
| ), | ||
| args: { | ||
| displayMode: DisplayMode.Embedded, | ||
| }, | ||
| play: playMainContentButtonFocusManagement, | ||
| }; | ||
|
|
||
| interface MainContentProps { | ||
| dashboardButtonRef: React.RefObject<HTMLButtonElement>; | ||
| guideCueOpen: boolean; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removal of
@leafygreen-ui/tabsdependency 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.