Skip to content

fix: add accessible dialog semantics to Drawer #2434

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.

Sign up for GitHub

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

Closed
remove334 wants to merge 1 commit into from
Closed

fix: add accessible dialog semantics to Drawer #2434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion packages/react/src/components/Drawer/Drawer.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,63 @@
import React from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import Drawer from './';
import Drawer, { DrawerHeading } from './';
import axe from '../../axe';

afterEach(() => {
document.body.innerHTML = '';
jest.restoreAllMocks();
});

const renderHeading = () => <DrawerHeading>Drawer title</DrawerHeading>;

test('should render children', () => {
render(
<Drawer position="left" open data-testid="drawer">
{renderHeading()}
Hello World
</Drawer>
);
expect(screen.getByText('Hello World')).toBeInTheDocument();
});

test('should render as a dialog', () => {
render(
<Drawer position="left" open>
{renderHeading()}
Children
</Drawer>
);

expect(screen.getByRole('dialog', { name: 'Drawer title' })).toBeVisible();
});

test('should set aria-modal when modal', () => {
render(
<Drawer position="left" open>
{renderHeading()}
Children
</Drawer>
);

expect(screen.getByRole('dialog')).toHaveAttribute('aria-modal', 'true');
});

test('should not set aria-modal when non-modal', () => {
render(
<Drawer position="left" behavior="non-modal" open>
{renderHeading()}
Children
</Drawer>
);

expect(screen.getByRole('dialog')).not.toHaveAttribute('aria-modal');
});

test('should support className prop', () => {
render(
<Drawer position="left" className="bananas" open data-testid="drawer">
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -39,6 +76,7 @@ test('should support open prop', () => {
expect(drawer).not.toBeVisible();
rerender(
<Drawer position="left" data-testid="drawer" open>
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -58,6 +96,7 @@ test('should support position prop', () => {
};
const { rerender } = render(
<Drawer position="top" open data-testid="drawer">
{renderHeading()}
Children
</Drawer>
);
Expand Down Expand Up @@ -87,6 +126,7 @@ test('should call onClose prop on esc keypress', async () => {
const user = userEvent.setup();
render(
<Drawer position="left" data-testid="drawer" open onClose={onClose}>
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -101,6 +141,7 @@ test('should call onClose prop on click outside', async () => {
const user = userEvent.setup();
render(
<Drawer position="left" data-testid="drawer" open onClose={onClose}>
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -120,6 +161,7 @@ test('should set focus to drawer by default when opened', () => {
expect(screen.getByTestId('drawer')).not.toHaveFocus();
rerender(
<Drawer position="left" data-testid="drawer" open>
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -144,6 +186,7 @@ test('should set focus to focusable element when opened', () => {
initialFocus: button
}}
>
{renderHeading()}
<button>focus me</button>
</Drawer>
);
Expand Down Expand Up @@ -171,6 +214,7 @@ test('should set focus to custom element when opened', () => {
focusOptions={{ initialFocus: ref.current as HTMLElement }}
open
>
{renderHeading()}
<button>no focus me</button>
<button ref={ref}>focus me</button>
</Drawer>
Expand Down Expand Up @@ -199,6 +243,7 @@ test('should set focus to custom ref element', () => {
focusOptions={{ initialFocus: ref }}
open
>
{renderHeading()}
<button>no focus me</button>
<button ref={ref}>focus me</button>
</Drawer>
Expand All @@ -224,6 +269,7 @@ test('should return focus to triggering element when closed', () => {
<>
<button>trigger</button>
<Drawer position="left" data-testid="drawer" open>
{renderHeading()}
Children
</Drawer>
</>
Expand Down Expand Up @@ -260,6 +306,7 @@ test('should return focus to custom element when closed', () => {
open
focusOptions={{ returnFocus: button }}
>
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -280,6 +327,7 @@ test('should support ref prop', () => {
const ref = React.createRef<HTMLDivElement>();
render(
<Drawer position="left" open ref={ref} data-testid="drawer">
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -294,6 +342,7 @@ test('should not trap focus when behavior is non-modal', async () => {
<>
<button>outside</button>
<Drawer position="left" behavior="non-modal" open>
{renderHeading()}
<div>
<button>inside</button>
</div>
Expand All @@ -315,6 +364,7 @@ test('should not trap focus when behavior is non-modal', async () => {
test('should return no axe violations when open', async () => {
render(
<Drawer position="left" open data-testid="drawer">
{renderHeading()}
Children
</Drawer>
);
Expand All @@ -333,3 +383,15 @@ test('should return no axe violations when closed', async () => {
const results = await axe(await screen.findByTestId('drawer'));
expect(results).toHaveNoViolations();
});

test('should throw when opened without a DrawerHeading', () => {
expect(() =>
render(
<Drawer position="left" open>
Children
</Drawer>
)
).toThrow(
'Drawer: No heading provided. Include a DrawerHeading component for accessibility.'
);
});
20 changes: 20 additions & 0 deletions packages/react/src/components/Drawer/DrawerContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React, { createContext, useContext } from 'react';

interface DrawerContextValue {
headingId: string;
}

const DrawerContext = createContext<DrawerContextValue | null>(null);

function useDrawerContext(): DrawerContextValue {
const context = useContext(DrawerContext);
if (!context) {
throw new Error(
'Drawer compound components must be rendered within a Drawer'
);
}
return context;
}
Comment on lines +9 to +17

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to check that this guard throws if DrawerHeading is rendered outside a Drawer.


export { DrawerContext, useDrawerContext };
export type { DrawerContextValue };
60 changes: 59 additions & 1 deletion packages/react/src/components/Drawer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import React, {
useState,
useEffect,
useCallback,
useMemo,
useRef
} from 'react';
import { createPortal } from 'react-dom';
import classnames from 'classnames';
import { useId } from 'react-id-generator';
import Scrim from '../Scrim';
import ClickOutsideListener from '../ClickOutsideListener';
import useEscapeKey from '../../utils/useEscapeKey';
Expand All @@ -16,6 +18,11 @@ import useFocusTrap from '../../utils/useFocusTrap';
import resolveElement from '../../utils/resolveElement';
import AriaIsolate from '../../utils/aria-isolate';
import { isBrowser } from '../../utils/is-browser';
import {
DrawerContext,
useDrawerContext,
type DrawerContextValue
} from './DrawerContext';

export interface DrawerProps<
T extends HTMLElement = HTMLElement
Expand Down Expand Up @@ -50,6 +57,7 @@ const Drawer = forwardRef<HTMLDivElement, DrawerProps>(
) => {
const drawerRef = useSharedRef(ref);
const openRef = useRef(!!open);
const [headingId] = useId(1, 'drawer-title-');
const { initialFocus: focusInitial, returnFocus: focusReturn } =
focusOptions;
const [isTransitioning, setIsTransitioning] = useState(!!open);
Expand Down Expand Up @@ -115,6 +123,24 @@ const Drawer = forwardRef<HTMLDivElement, DrawerProps>(
returnFocusElement: focusReturn
});

useEffect(() => {
if (open && drawerRef.current) {
const hasHeading = drawerRef.current.querySelector('.Drawer__heading');
if (process.env.NODE_ENV !== 'production' && !hasHeading) {
throw Error(
'Drawer: No heading provided. Include a DrawerHeading component for accessibility.'
);
}
}
}, [open]);
Comment on lines +126 to +135

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue (blocking): This throws whenever there's no DrawerHeading. That breaks two cases:

  1. an existing drawer with no heading crashes on upgrade, and
  2. a drawer correctly named with aria-label gets flagged anyway. The drawer still renders, so this should warn, not crash.

Suggested change — accept any accessible name, and use console.error:

useEffect(() => {
  if (process.env.NODE_ENV === 'production' || !open || !drawerRef.current) return;
  const hasHeading = !!drawerRef.current.querySelector('.Drawer__heading');
  if (!hasHeading && !ariaLabel && !ariaLabelledby) {
    console.error(
      'Drawer: no accessible name. Add a DrawerHeading, a `heading` prop, or an `aria-label`.'
    );
  }
}, [open, ariaLabel, ariaLabelledby]);

ariaLabel/ariaLabelledby come from the destructure in the next comment. Keep throw only for the useDrawerContext case below, where nothing can render.


const contextValue: DrawerContextValue = useMemo(
() => ({
headingId
}),
[headingId]
);

const portalElement = resolveElement(portal);

return createPortal(
Expand All @@ -127,6 +153,7 @@ const Drawer = forwardRef<HTMLDivElement, DrawerProps>(
>
<div
ref={drawerRef}
role="dialog"
className={classnames(className, 'Drawer', {
'Drawer--open': !!open,
'Drawer--top': position === 'top',
Expand All @@ -135,14 +162,18 @@ const Drawer = forwardRef<HTMLDivElement, DrawerProps>(
'Drawer--right': position === 'right'
})}
aria-hidden={!open || undefined}
aria-modal={isModal ? true : undefined}
aria-labelledby={headingId}

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue (blocking): aria-labelledby is always set to headingId. If there's no DrawerHeading, it points at an id that doesn't exist. And if the caller passes aria-label, the drawer ends up with both a real label and a dangling reference.

Suggested change — read the label props and set it conditionally:

const { 'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, ...rest } = props;
const labelledBy = ariaLabelledby ?? (ariaLabel ? undefined : headingId);
// on the div:  aria-label={ariaLabel}  aria-labelledby={labelledBy}  {...rest}

Now a caller's aria-labelledby wins, an aria-label names the dialog cleanly, and the auto id is the fallback.

style={{
visibility: !open && !isTransitioning ? 'hidden' : undefined,
...style
}}
tabIndex={open ? -1 : undefined}
{...props}
>
{children}
<DrawerContext.Provider value={contextValue}>
{children}
</DrawerContext.Provider>
</div>
</ClickOutsideListener>
<Scrim show={!!open && !!isModal} />
Expand All @@ -156,4 +187,31 @@ const Drawer = forwardRef<HTMLDivElement, DrawerProps>(

Drawer.displayName = 'Drawer';

export interface DrawerHeadingProps extends React.HTMLAttributes<HTMLHeadingElement> {
children: React.ReactNode;
className?: string;
level?: number;
}
Comment on lines +190 to +194

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

level is typed as number, so level={7} would render an invalid <h7>. Narrowing it to 1 | 2 | 3 | 4 | 5 | 6 would catch that at compile time. DialogHeading has the same issue, but while were adding this, let's fix it here.


const DrawerHeading = ({
children,
className,
level = 2,
...other
}: DrawerHeadingProps) => {
const { headingId } = useDrawerContext();
const HeadingLevel = `h${level}` as 'h1';

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When level is typed as 1 | 2 | 3 | 4 | 5 | 6, please also remove as 'h1'; from here. We shouldn't need the arbitrary typecast anymore because it should infer a type of h1 | h2 | h3...

return (
<HeadingLevel
className={classnames('Drawer__heading', className)}
id={headingId}
{...other}
>
{children}
</HeadingLevel>
);
};
Comment on lines +196 to +213

@rheisler-deque rheisler-deque Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test asserts the heading's level. If the default or the h${level} logic regressed, the suite would still pass. Please add a test that checks the rendered level for both the default and a passed-in level.

DrawerHeading.displayName = 'DrawerHeading';

export default Drawer;
export { Drawer, DrawerHeading };
2 changes: 1 addition & 1 deletion packages/react/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export { default as Popover } from './components/Popover';
export { default as Timeline, TimelineItem } from './components/Timeline';
export { default as TextEllipsis } from './components/TextEllipsis';
export { default as CopyButton } from './components/CopyButton';
export { default as Drawer } from './components/Drawer';
export { default as Drawer, DrawerHeading } from './components/Drawer';
export { default as BottomSheet } from './components/BottomSheet';
export { default as AnchoredOverlay } from './components/AnchoredOverlay';
export { default as FieldGroup } from './components/FieldGroup';
Expand Down