Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions packages/react-aria/src/dialog/useDialog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,28 @@ export function useDialog(props: AriaDialogProps, ref: RefObject<FocusableElemen

useOverlayFocusContain();

// Warn in dev mode if the dialog has no accessible title.
// This catches a common mistake where useDialog and useOverlayTriggerState
// are used in the same component, causing the title element to not be
// in the DOM when useSlotId queries for it.
// Check the DOM element directly since aria-labelledby may be added by
// wrapper components (e.g. RAC Dialog uses trigger ID as a fallback).
let hasWarned = useRef(false);
useEffect(() => {
if (process.env.NODE_ENV !== 'production' && !hasWarned.current && ref.current) {
let el = ref.current;
let hasAriaLabel = el.hasAttribute('aria-label');
let hasAriaLabelledby = el.hasAttribute('aria-labelledby');
if (!hasAriaLabel && !hasAriaLabelledby) {
console.warn(
'A dialog must have a visible title for accessibility. ' +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'A dialog must have a visible title for accessibility. ' +
'A dialog must have a title for accessibility. ' +

'Either provide an aria-label or aria-labelledby prop, or render a heading element inside the dialog with the titleProps from useDialog.'
);
hasWarned.current = true;
}
}
});

// We do not use aria-modal due to a Safari bug which forces the first focusable element to be focused
// on mount when inside an iframe, no matter which element we programmatically focus.
// See https://bugs.webkit.org/show_bug.cgi?id=211934.
Expand Down
52 changes: 46 additions & 6 deletions packages/react-aria/test/dialog/useDialog.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,77 @@ import {useDialog} from '../../src/dialog/useDialog';

function Example(props) {
let ref = useRef();
let {dialogProps} = useDialog(props, ref);
return <div ref={ref} {...dialogProps} data-testid="test">{props.children}</div>;
let {dialogProps, titleProps} = useDialog(props, ref);
return (
<div ref={ref} {...dialogProps} data-testid="test">
{props.showTitle && <h2 {...titleProps}>Title</h2>}
{props.children}
</div>
);
}

describe('useDialog', function () {
it('should have role="dialog" by default', function () {
let res = render(<Example />);
let res = render(<Example aria-label="Test dialog" />);
let el = res.getByTestId('test');
expect(el).toHaveAttribute('role', 'dialog');
});

it('should accept role="alertdialog"', function () {
let res = render(<Example role="alertdialog" />);
let res = render(<Example role="alertdialog" aria-label="Test dialog" />);
let el = res.getByTestId('test');
expect(el).toHaveAttribute('role', 'alertdialog');
});

it('should focus the overlay on mount', function () {
let res = render(<Example />);
let res = render(<Example aria-label="Test dialog" />);
let el = res.getByTestId('test');
expect(el).toHaveAttribute('tabIndex', '-1');
expect(document.activeElement).toBe(el);
});

it('should not focus the overlay if something inside is auto focused', function () {
let res = render(
<Example>
<Example aria-label="Test dialog">
<input data-testid="input" autoFocus />
</Example>
);
let input = res.getByTestId('input');
expect(document.activeElement).toBe(input);
});

describe('dev warnings', function () {
let originalWarn;

beforeEach(function () {
originalWarn = console.warn;
console.warn = jest.fn();
});

afterEach(function () {
console.warn = originalWarn;
});

it('should warn when dialog has no accessible title', function () {
render(<Example />);
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining('A dialog must have a visible title for accessibility')
);
});

it('should not warn when aria-label is provided', function () {
render(<Example aria-label="My dialog" />);
expect(console.warn).not.toHaveBeenCalled();
});

it('should not warn when aria-labelledby is provided', function () {
render(<Example aria-labelledby="some-id" />);
expect(console.warn).not.toHaveBeenCalled();
});

it('should not warn when a title element is rendered', function () {
render(<Example showTitle />);
expect(console.warn).not.toHaveBeenCalled();
});
});
});
3 changes: 2 additions & 1 deletion scripts/setupTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ const ERROR_PATTERNS_WE_SHOULD_FIX_BUT_ALLOW = [
];

const WARNING_PATTERNS_WE_SHOULD_FIX_BUT_ALLOW = [
'Browserslist: caniuse-lite is outdated'
'Browserslist: caniuse-lite is outdated',
'A dialog must have a visible title for accessibility'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should not have this turned on. do you need it? I looks like you're already catching the warning with a jest spy/mock

];

function failTestOnConsoleError() {
Expand Down
Loading