-
Notifications
You must be signed in to change notification settings - Fork 72
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 15 commits
ce621ea
a76826e
58892b1
84dcd65
3192ddf
77eeca5
0f5fed4
0c27da5
91c57fe
6c1a865
a11fa22
5899e50
bd6d33e
2019a47
7ce2bd0
becc27b
df328af
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. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const handleAnimationEnd = () => { | ||||||||||||||||||||||
| const drawerElement = ref.current; | ||||||||||||||||||||||
| useIsomorphicLayoutEffect(() => { | ||||||||||||||||||||||
shaneeza marked this conversation as resolved.
Show resolved
Hide 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.
Show resolved
Hide resolved
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // 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'); | |
| } |
| 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.
queryFirstFocusableElementhas a set list of focusable selectors it considers and "the native focus behavior of the dialog element" is to look for children with theautofocusattribute. 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
initialFocusprop inModal. We don't useModalandDrawerin 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 asfocusModalChildElementfor the first part of this effectThere 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 like this pattern. However, I think if we go with this approach, then we might have to override the default dialog behavior since
initialFocusdoes not work by default.So the pattern would be in a native dialog and the embedded div:
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
autofocusto the 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.
What is the native dialog focus management you're concerned about losing? In
Modal,initialFocusis mostly additive and doesn't conflictautofocuscan only be applied to the typical focusable elements but can't be used for something like a code editor. Although the initial implementation ofModalused a css selector like"#my-code-editor", I added a ref option because it's more type-safe and robust than a stringThere 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.
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
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.
focusModalChildElementis the logic we use to determine how a child element should be focused. The consumer has full control using either theinitialFocusprop or theautoFocusattribute on a child element. IfinitialFocusis not specified and a child element does not haveautoFocusspecified, 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