-
Notifications
You must be signed in to change notification settings - Fork 79
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
Changes from 15 commits
ce621ea
a76826e
58892b1
84dcd65
3192ddf
77eeca5
0f5fed4
0c27da5
91c57fe
6c1a865
a11fa22
5899e50
bd6d33e
2019a47
7ce2bd0
becc27b
df328af
82c84b9
377e665
ce01ff2
8e7471d
cec886d
121aa91
46dee5d
f8c7d11
d19f239
5560a82
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,5 @@ | ||
| --- | ||
| '@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. |
| 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
After receiving feedback about this use case, it led to this change reintroducing the
Collaborator
Author
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. I like this pattern. However, I think if we go with this approach, then we might have to override the default dialog behavior since So the pattern would be in a native dialog and the embedded div:
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
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. What is the native dialog focus management you're concerned about losing? In
Collaborator
Author
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.
Not necessarily losing, but having to re-implement it. I noticed here that if any child has leafygreen-ui/packages/modal/src/utils/focusModalChildElement.ts Lines 44 to 54 in f6058df
Thanks for clarifying.
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.
Collaborator
Author
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. Yes, actually would love to huddle on this, I'll Slack you |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleAnimationEnd = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const drawerElement = ref.current; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useIsomorphicLayoutEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
shaneeza marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isOverlay) return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if the drawerElement is null or is a div, which means it is not a dialog element. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!drawerElement || drawerElement instanceof HTMLDivElement) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (open && !hasHandledFocusRef.current) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Store the currently focused element when opening (only once per open session) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| previouslyFocusedRef.current = document.activeElement as HTMLElement; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hasHandledFocusRef.current = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (open) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const firstFocusable = drawerElement.querySelector( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (firstFocusable as HTMLElement)?.focus(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ref.current === null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Find and focus the first focusable element in the drawer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const firstFocusableElement = queryFirstFocusableElement(ref.current); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| firstFocusableElement?.focus(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if (!open && hasHandledFocusRef.current) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if the current focus is not in the drawer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This means the user has navigated away from the drawer, like the toolbar, and we should not restore focus. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!ref.current?.contains(document.activeElement)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hasHandledFocusRef.current = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| previouslyFocusedRef.current = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
shaneeza marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Restore focus when closing (only if we had handled focus during this session) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. This is starting to feel like a re-implementation of the
Collaborator
Author
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. 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 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. Choose a reason for hiding this commentThe 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?
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. 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. Choose a reason for hiding this commentThe 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.
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. 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 (previouslyFocusedRef.current) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if the previously focused element is still in the DOM | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (document.contains(previouslyFocusedRef.current)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| previouslyFocusedRef.current.focus(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } 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(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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,92 @@ | |
| }); | ||
| }; | ||
|
|
||
| // 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, getCloseButtonUtils, 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); | ||
| }); | ||
|
|
||
| // Click the close button | ||
| userEvent.click(codeButton!); | ||
|
|
||
| await waitFor(() => { | ||
| expect(isOpen()).toBe(false); | ||
| }); | ||
|
|
||
| // Focus should return to the original toolbar button that opened the drawer | ||
| 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(); | ||
|
|
||
| // Focus and click the "Open Code Drawer" button to open drawer | ||
| openCodeButton.focus(); | ||
|
|
||
| // Verify focus is on the button - wait for focus to be applied | ||
| await waitFor(() => { | ||
| expect(document.activeElement).toBe(openCodeButton); | ||
| }); | ||
|
|
||
| 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 +412,12 @@ | |
| 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 +567,24 @@ | |
| 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 +651,28 @@ | |
| 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.