Skip to content
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

feat(Dialog): ensure that dialogs are labelled by their heading #1260

Merged
merged 9 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
"fontsource-roboto": "^3.0.3",
"html-webpack-plugin": "^5.5.0",
"husky": "^1.0.0-rc.14",
"jest-axe": "^5.0.1",
"jest-axe": "^8.0.0",
"lint-staged": "^7.2.2",
"log-symbols": "4",
"mdast-util-from-markdown": "^1.3.0",
Expand Down
13 changes: 13 additions & 0 deletions packages/react/__tests__/src/components/Dialog/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,19 @@ test('focuses heading when "show" prop is updated from falsey to truthy', (done)
});
});

test('associates dialog with heading', () => {
const dialog = mount(
<Dialog {...defaults} show={true}>
hello
</Dialog>
);
expect(dialog.find('[role="dialog"]').prop('aria-labelledby')).toBeTruthy();
expect(dialog.find('h2').prop('id')).toBeTruthy();
expect(dialog.find('[role="dialog"]').prop('aria-labelledby')).toEqual(
dialog.find('h2').prop('id')
);
});

test('calls onClose when clicked outside', () => {
const onClose = jest.fn();
const dialog = mount(
Expand Down
34 changes: 11 additions & 23 deletions packages/react/__tests__/src/components/Popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ afterEach(() => {
mountNode = null;
});

const update = async wrapper => {
const update = async (wrapper) => {
await act(async () => {
await new Promise(resolve => setImmediate(resolve));
await new Promise((resolve) => setImmediate(resolve));
wrapper.update();
});
};
Expand Down Expand Up @@ -78,6 +78,7 @@ const WrapperPrompt = ({ buttonProps = {}, tooltipProps = {} }) => {
target={ref}
show
onClose={onClose}
infoText="popover"
{...tooltipProps}
/>
</React.Fragment>
Expand All @@ -96,30 +97,21 @@ test('should auto-generate id', async () => {
const id = wrapper.find('.Popover').props().id;
expect(id).toBeTruthy();
expect(id).toEqual(
wrapper
.find('button')
.getDOMNode()
.getAttribute('aria-controls')
wrapper.find('button').getDOMNode().getAttribute('aria-controls')
);
});

test('should attach attribute aria-expanded correctly based on shown state', async () => {
const wrapper = mount(<Wrapper />);
await update(wrapper);
expect(
wrapper
.find('button')
.getDOMNode()
.getAttribute('aria-expanded')
wrapper.find('button').getDOMNode().getAttribute('aria-expanded')
).toBeTruthy();

const shownStateFalsy = mount(<Wrapper tooltipProps={{ show: false }} />);

expect(
shownStateFalsy
.find('button')
.getDOMNode()
.getAttribute('aria-expanded')
shownStateFalsy.find('button').getDOMNode().getAttribute('aria-expanded')
).toBeFalsy();
});

Expand All @@ -138,10 +130,7 @@ test('should not overwrite user provided id and aria-describedby', async () => {
await update(wrapper);
expect(wrapper.find('.Popover').props().id).toEqual('popoverid');
expect(
wrapper
.find('button')
.getDOMNode()
.getAttribute('aria-describedby')
wrapper.find('button').getDOMNode().getAttribute('aria-describedby')
).toEqual('foo popoverid');
});

Expand Down Expand Up @@ -267,7 +256,9 @@ test('variant="prompt" should return no axe violations', async () => {
});

test('should return no axe violations', async () => {
const wrapper = mount(<Wrapper />);
const wrapper = mount(
<Wrapper tooltipProps={{ variant: 'prompt', 'aria-label': 'popover' }} />
);
await update(wrapper);
expect(await axe(wrapper.html())).toHaveNoViolations();
});
Expand Down Expand Up @@ -341,9 +332,6 @@ test('aria-labelledby is set correctly for prompt variant', async () => {
const id = wrapper.find('.Popover').props().id;

expect(`${id}-label`).toEqual(
wrapper
.find('.Popover')
.getDOMNode()
.getAttribute('aria-labelledby')
wrapper.find('.Popover').getDOMNode().getAttribute('aria-labelledby')
);
});
2 changes: 1 addition & 1 deletion packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"enzyme": "^3.11.0",
"enzyme-adapter-react-16": "^1.15.2",
"jest": "^24.7.1",
"jest-axe": "^3.4.0",
"jest-axe": "^8.0.0",
"jsdom": "^16.2.2",
"jsdom-global": "^3.0.2",
"nyc": "^15.0.1",
Expand Down
4 changes: 4 additions & 0 deletions packages/react/src/components/Dialog/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Icon from '../Icon';
import ClickOutsideListener from '../ClickOutsideListener';
import AriaIsolate from '../../utils/aria-isolate';
import setRef from '../../utils/setRef';
import nextId from 'react-id-generator';
import { isBrowser } from '../../utils/is-browser';

export interface DialogProps extends React.HTMLAttributes<HTMLDivElement> {
Expand Down Expand Up @@ -58,6 +59,7 @@ export default class Dialog extends React.Component<DialogProps, DialogState> {

private element: HTMLDivElement | null;
private heading: HTMLHeadingElement | null;
private headingId: string = nextId('dialog-title-');

constructor(props: DialogProps) {
super(props);
Expand Down Expand Up @@ -148,6 +150,7 @@ export default class Dialog extends React.Component<DialogProps, DialogState> {
}
setRef(dialogRef, el);
}}
aria-labelledby={this.headingId}
{...other}
>
<div className="Dialog__inner">
Expand All @@ -156,6 +159,7 @@ export default class Dialog extends React.Component<DialogProps, DialogState> {
className="Dialog__heading"
ref={(el: HTMLHeadingElement) => (this.heading = el)}
tabIndex={-1}
id={this.headingId}
>
{typeof heading === 'object' && 'text' in heading
? heading.text
Expand Down
11 changes: 6 additions & 5 deletions packages/react/src/components/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ const Popover = forwardRef<HTMLDivElement, PopoverProps>(
initialPlacement;

const additionalProps =
variant === 'prompt' ? { 'aria-labelledby': `${id}-label` } : {};
variant === 'prompt' && !props['aria-label']
? { 'aria-labelledby': `${id}-label` }
: {};

// Keep targetElement in sync with target prop
useEffect(() => {
Expand Down Expand Up @@ -162,9 +164,8 @@ const Popover = forwardRef<HTMLDivElement, PopoverProps>(
useEffect(() => {
if (show && popoverRef.current) {
// Find the first focusable element inside the container
const firstFocusableElement = popoverRef.current.querySelector(
focusableSelector
);
const firstFocusableElement =
popoverRef.current.querySelector(focusableSelector);

if (firstFocusableElement instanceof HTMLElement) {
firstFocusableElement.focus();
Expand Down Expand Up @@ -253,8 +254,8 @@ const Popover = forwardRef<HTMLDivElement, PopoverProps>(
role="dialog"
style={styles.popper}
{...attributes.popper}
{...props}
{...additionalProps}
{...props}
scurker marked this conversation as resolved.
Show resolved Hide resolved
>
<div
className="Popover__popoverArrow"
Expand Down
Loading