Skip to content

Commit 8bdb879

Browse files
rebeccaalpertrebeccaalpert
authored
fix(JumpLinks): Fix aria issues (#11950)
* fix(JumpLinks): Fix aria issues Remove aria-label from toggle if there is visible text. Add aria-labelled by if there is visible text. Added aria-labels to examples. * Address feedback * Address PR feedback * Add tests
1 parent f0bf2bc commit 8bdb879

File tree

9 files changed

+246
-23
lines changed

9 files changed

+246
-23
lines changed

‎packages/react-core/src/components/JumpLinks/JumpLinks.tsx‎

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import cssToggleDisplayVar from '@patternfly/react-tokens/dist/esm/c_jump_links_
77
import { Button } from '../Button';
88
import { JumpLinksItem, JumpLinksItemProps } from './JumpLinksItem';
99
import { JumpLinksList } from './JumpLinksList';
10-
import { canUseDOM, formatBreakpointMods } from '../../helpers/util';
10+
import { canUseDOM, formatBreakpointMods, getUniqueId } from '../../helpers/util';
1111

1212
export interface JumpLinksProps extends Omit<React.HTMLProps<HTMLElement>, 'label'> {
1313
/** Whether to center children. */
@@ -47,6 +47,8 @@ export interface JumpLinksProps extends Omit<React.HTMLProps<HTMLElement>, 'labe
4747
className?: string;
4848
/** Whether the current entry in the navigation history should be replaced when a JumpLinksItem is clicked. By default a new entry will be pushed to the navigation history. */
4949
shouldReplaceNavHistory?: boolean;
50+
/** Custom ID applied to label if alwaysShowLabel is applied, or expandable toggle. This is used for internal logic related to aria-label and aria-labelledby */
51+
labelId?: string;
5052
}
5153

5254
// Recursively find JumpLinkItems and return an array of all their scrollNodes
@@ -94,6 +96,7 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
9496
toggleAriaLabel = 'Toggle jump links',
9597
className,
9698
shouldReplaceNavHistory = false,
99+
labelId,
97100
...props
98101
}: JumpLinksProps) => {
99102
const hasScrollSpy = Boolean(scrollableRef || scrollableSelector);
@@ -106,6 +109,15 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
106109

107110
let scrollableElement: HTMLElement;
108111

112+
if (!label && !ariaLabel) {
113+
// eslint-disable-next-line no-console
114+
console.warn('JumpLinks: for accessibility reasons, an aria-label should be specified if no label is provided');
115+
}
116+
if (!label && !toggleAriaLabel && expandable) {
117+
// eslint-disable-next-line no-console
118+
console.warn('JumpLinks: for accessibility reasons, a toggleAriaLabel should be specified if no label is provided');
119+
}
120+
109121
const getScrollableElement = () => {
110122
if (scrollableRef) {
111123
if (scrollableRef instanceof HTMLElement) {
@@ -232,6 +244,11 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
232244
return child;
233245
});
234246

247+
const id = labelId ?? getUniqueId();
248+
const hasAriaLabelledBy = expandable || (label && alwaysShowLabel);
249+
const computedAriaLabel = hasAriaLabelledBy ? null : ariaLabel;
250+
const computedAriaLabelledBy = hasAriaLabelledBy ? id : null;
251+
235252
return (
236253
<nav
237254
className={css(
@@ -242,8 +259,9 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
242259
isExpanded && styles.modifiers.expanded,
243260
className
244261
)}
245-
aria-label={ariaLabel}
262+
aria-label={computedAriaLabel}
246263
ref={navRef}
264+
aria-labelledby={computedAriaLabelledBy}
247265
{...props}
248266
>
249267
<div className={styles.jumpLinksMain}>
@@ -253,21 +271,31 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
253271
<Button
254272
variant="plain"
255273
onClick={() => setIsExpanded(!isExpanded)}
256-
aria-label={toggleAriaLabel}
274+
aria-label={label ? null : toggleAriaLabel}
257275
aria-expanded={isExpanded}
258276
icon={
259277
<span className={styles.jumpLinksToggleIcon}>
260278
<AngleRightIcon />
261279
</span>
262280
}
281+
id={id}
263282
>
264283
{label && label}
265284
</Button>
266285
</div>
267286
)}
268-
{label && alwaysShowLabel && <div className={css(styles.jumpLinksLabel)}>{label}</div>}
287+
{label && alwaysShowLabel && !expandable && (
288+
<div className={css(styles.jumpLinksLabel)} id={id}>
289+
{label}
290+
</div>
291+
)}
269292
</div>
270-
<ul className={styles.jumpLinksList} role="list">
293+
<ul
294+
aria-label={computedAriaLabel}
295+
aria-labelledby={computedAriaLabelledBy}
296+
className={styles.jumpLinksList}
297+
role="list"
298+
>
271299
{cloneChildren(children)}
272300
</ul>
273301
</div>

‎packages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsx‎

Lines changed: 196 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
1-
import { render } from '@testing-library/react';
1+
import { render, screen } from '@testing-library/react';
22
import { JumpLinks } from '../JumpLinks';
33
import { JumpLinksItem } from '../JumpLinksItem';
44
import { JumpLinksList } from '../JumpLinksList';
5+
import * as utils from '../../../helpers/util';
6+
7+
jest.spyOn(utils, 'getUniqueId').mockReturnValue('unique_id_mock');
58

69
test('simple jumplinks', () => {
710
const { asFragment } = render(
@@ -75,3 +78,195 @@ test('expandable vertical with subsection', () => {
7578
);
7679
expect(asFragment()).toMatchSnapshot();
7780
});
81+
82+
// Revamped tests begin here
83+
84+
const jumpLinksItems = (
85+
<>
86+
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
87+
<JumpLinksItem href="#" isActive>
88+
Active section
89+
</JumpLinksItem>
90+
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
91+
</>
92+
);
93+
94+
const expandableJumpLinksItems = (
95+
<>
96+
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
97+
<JumpLinksItem href="#">
98+
Section with active subsection
99+
<JumpLinksList>
100+
<JumpLinksItem href="#" isActive>
101+
Active subsection
102+
</JumpLinksItem>
103+
<JumpLinksItem href="#">Inactive subsection</JumpLinksItem>
104+
<JumpLinksItem href="#">Inactive subsection</JumpLinksItem>
105+
</JumpLinksList>
106+
</JumpLinksItem>
107+
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
108+
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
109+
</>
110+
);
111+
112+
test('renders label be default', () => {
113+
render(<JumpLinks label="Jump to section">{jumpLinksItems}</JumpLinks>);
114+
expect(screen.getByText(/Jump to section/i)).toBeTruthy();
115+
});
116+
117+
test('does not render label when alwaysShowLabel is false', () => {
118+
render(
119+
<JumpLinks label="Jump to section" alwaysShowLabel={false}>
120+
{jumpLinksItems}
121+
</JumpLinks>
122+
);
123+
expect(screen.queryByText(/Jump to section/i)).toBeFalsy();
124+
});
125+
126+
test('uses aria-labelledby over aria-label when label and alwaysShowLabel are passed in', () => {
127+
render(<JumpLinks label="Jump to section">{expandableJumpLinksItems}</JumpLinks>);
128+
const navigation = screen.getByRole('navigation', { name: /Jump to section/i });
129+
expect(navigation).toHaveAttribute('aria-labelledby');
130+
expect(navigation).not.toHaveAttribute('aria-label');
131+
});
132+
133+
test('uses aria-labelledby over aria-label when expandable is passed in', () => {
134+
render(<JumpLinks expandable={{ default: 'expandable' }}>{expandableJumpLinksItems}</JumpLinks>);
135+
const navigation = screen.getByRole('navigation', { name: /Toggle jump links/i });
136+
expect(navigation).toHaveAttribute('aria-labelledby');
137+
expect(navigation).not.toHaveAttribute('aria-label');
138+
});
139+
140+
test('random id is used with aria-labelledby by default in expandable case', () => {
141+
render(<JumpLinks expandable={{ default: 'expandable' }}>{expandableJumpLinksItems}</JumpLinks>);
142+
const navigation = screen.getByRole('navigation', { name: /Toggle jump links/i });
143+
expect(navigation).toHaveAttribute('aria-labelledby', 'unique_id_mock');
144+
});
145+
146+
test('random id is used with aria-labelledby by default in label case', () => {
147+
render(
148+
<JumpLinks label="Jump to section" alwaysShowLabel>
149+
{expandableJumpLinksItems}
150+
</JumpLinks>
151+
);
152+
const navigation = screen.getByRole('navigation', { name: /Jump to section/i });
153+
expect(navigation).toHaveAttribute('aria-labelledby', 'unique_id_mock');
154+
});
155+
156+
test('custom labelId is used with aria-labelledby when it is passed in in expandable case', () => {
157+
render(
158+
<JumpLinks labelId="custom-id" expandable={{ default: 'expandable' }}>
159+
{expandableJumpLinksItems}
160+
</JumpLinks>
161+
);
162+
const navigation = screen.getByRole('navigation', { name: /Toggle jump links/i });
163+
expect(navigation).toHaveAttribute('aria-labelledby', 'custom-id');
164+
});
165+
166+
test('custom labelId is used with aria-labelledby when it is passed in in label case', () => {
167+
render(
168+
<JumpLinks label="Jump to section" labelId="custom-id" alwaysShowLabel>
169+
{expandableJumpLinksItems}
170+
</JumpLinks>
171+
);
172+
const navigation = screen.getByRole('navigation', { name: /Jump to section/i });
173+
expect(navigation).toHaveAttribute('aria-labelledby', 'custom-id');
174+
});
175+
176+
test('uses aria-label instead of aria-labelledby by default', () => {
177+
render(<JumpLinks aria-label="Custom aria label">{jumpLinksItems}</JumpLinks>);
178+
const navigation = screen.getByRole('navigation', { name: /Custom aria label/i });
179+
expect(navigation).toHaveAttribute('aria-label', 'Custom aria label');
180+
expect(navigation).not.toHaveAttribute('aria-labelledby');
181+
});
182+
183+
test('uses aria-label instead of aria-labelledby when label is provided but alwaysShowLabel is false', () => {
184+
render(
185+
<JumpLinks label="Jump to section" aria-label="Custom aria label" alwaysShowLabel={false}>
186+
{jumpLinksItems}
187+
</JumpLinks>
188+
);
189+
const navigation = screen.getByRole('navigation', { name: /Custom aria label/i });
190+
expect(navigation).toHaveAttribute('aria-label', 'Custom aria label');
191+
expect(navigation).not.toHaveAttribute('aria-labelledby');
192+
});
193+
194+
test('aria-labelledby is used with provided labelId even when aria-label is also provided in expandable case', () => {
195+
render(
196+
<JumpLinks labelId="custom-id" aria-label="Custom aria label" expandable={{ default: 'expandable' }}>
197+
{expandableJumpLinksItems}
198+
</JumpLinks>
199+
);
200+
const navigation = screen.getByRole('navigation');
201+
expect(navigation).toHaveAttribute('aria-labelledby', 'custom-id');
202+
expect(navigation).not.toHaveAttribute('aria-label');
203+
});
204+
205+
test('aria-labelledby is used with provided labelId even when aria-label is also provided in label case', () => {
206+
render(
207+
<JumpLinks labelId="custom-id" aria-label="Custom aria label" label="Jump to section">
208+
{jumpLinksItems}
209+
</JumpLinks>
210+
);
211+
const navigation = screen.getByRole('navigation');
212+
expect(navigation).toHaveAttribute('aria-labelledby', 'custom-id');
213+
expect(navigation).not.toHaveAttribute('aria-label');
214+
});
215+
216+
test('does not throw error when there is a label passed in"', () => {
217+
const warnMock = jest.fn() as any;
218+
global.console = { warn: warnMock } as any;
219+
render(
220+
<JumpLinks alwaysShowLabel label="Jump to section">
221+
{jumpLinksItems}
222+
</JumpLinks>
223+
);
224+
expect(warnMock).not.toHaveBeenCalled();
225+
});
226+
227+
test('does not throw error when there is an aria-label passed in"', () => {
228+
const warnMock = jest.fn() as any;
229+
global.console = { warn: warnMock } as any;
230+
render(
231+
<JumpLinks alwaysShowLabel aria-label="Jump to section">
232+
{jumpLinksItems}
233+
</JumpLinks>
234+
);
235+
expect(warnMock).not.toHaveBeenCalled();
236+
});
237+
238+
test('throws error when no label or ariaLabel are passed in', () => {
239+
const warnMock = jest.fn() as any;
240+
global.console = { warn: warnMock } as any;
241+
render(<JumpLinks alwaysShowLabel>{jumpLinksItems}</JumpLinks>);
242+
expect(warnMock).toHaveBeenCalled();
243+
});
244+
245+
test('does not throw error when there is a toggleAriaLabel and expandable prop passed in', () => {
246+
const warnMock = jest.fn() as any;
247+
global.console = { warn: warnMock } as any;
248+
render(
249+
<JumpLinks label="Jump to section" toggleAriaLabel="Jump to section" expandable={{ default: 'expandable' }}>
250+
{expandableJumpLinksItems}
251+
</JumpLinks>
252+
);
253+
expect(warnMock).not.toHaveBeenCalled();
254+
});
255+
256+
test('does not throw error when there is an aria-label and expandable prop passed in', () => {
257+
const warnMock = jest.fn() as any;
258+
global.console = { warn: warnMock } as any;
259+
render(
260+
<JumpLinks aria-label="Jump to section" expandable={{ default: 'expandable' }}>
261+
{expandableJumpLinksItems}
262+
</JumpLinks>
263+
);
264+
expect(warnMock).not.toHaveBeenCalled();
265+
});
266+
267+
test('throws error when there is no toggle aria-label or aria-label but expandable is passed in', () => {
268+
const warnMock = jest.fn() as any;
269+
global.console = { warn: warnMock } as any;
270+
render(<JumpLinks expandable={{ default: 'expandable' }}>{expandableJumpLinksItems}</JumpLinks>);
271+
expect(warnMock).toHaveBeenCalled();
272+
});

‎packages/react-core/src/components/JumpLinks/__tests__/__snapshots__/JumpLinks.test.tsx.snap‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
exports[`expandable vertical with subsection 1`] = `
44
<DocumentFragment>
55
<nav
6-
aria-label="Jump to section"
6+
aria-labelledby="unique_id_mock"
77
class="pf-v6-c-jump-links pf-m-vertical pf-m-expandable"
88
>
99
<div
@@ -17,11 +17,11 @@ exports[`expandable vertical with subsection 1`] = `
1717
>
1818
<button
1919
aria-expanded="false"
20-
aria-label="Toggle jump links"
2120
class="pf-v6-c-button pf-m-plain"
2221
data-ouia-component-id="OUIA-Generated-Button-plain-1"
2322
data-ouia-component-type="PF6/Button"
2423
data-ouia-safe="true"
24+
id="unique_id_mock"
2525
type="button"
2626
>
2727
<span
@@ -52,13 +52,9 @@ exports[`expandable vertical with subsection 1`] = `
5252
</span>
5353
</button>
5454
</div>
55-
<div
56-
class="pf-v6-c-jump-links__label"
57-
>
58-
Jump to section
59-
</div>
6055
</div>
6156
<ul
57+
aria-labelledby="unique_id_mock"
6258
class="pf-v6-c-jump-links__list"
6359
role="list"
6460
>
@@ -349,7 +345,7 @@ exports[`jumplinks centered 1`] = `
349345
exports[`jumplinks with label 1`] = `
350346
<DocumentFragment>
351347
<nav
352-
aria-label="Jump to section"
348+
aria-labelledby="unique_id_mock"
353349
class="pf-v6-c-jump-links pf-m-center"
354350
>
355351
<div
@@ -360,11 +356,13 @@ exports[`jumplinks with label 1`] = `
360356
>
361357
<div
362358
class="pf-v6-c-jump-links__label"
359+
id="unique_id_mock"
363360
>
364361
Jump to section
365362
</div>
366363
</div>
367364
<ul
365+
aria-labelledby="unique_id_mock"
368366
class="pf-v6-c-jump-links__list"
369367
role="list"
370368
>
@@ -550,7 +548,7 @@ exports[`simple jumplinks 1`] = `
550548
exports[`vertical with label 1`] = `
551549
<DocumentFragment>
552550
<nav
553-
aria-label="Jump to section"
551+
aria-labelledby="unique_id_mock"
554552
class="pf-v6-c-jump-links pf-m-vertical"
555553
>
556554
<div
@@ -561,11 +559,13 @@ exports[`vertical with label 1`] = `
561559
>
562560
<div
563561
class="pf-v6-c-jump-links__label"
562+
id="unique_id_mock"
564563
>
565564
Jump to section
566565
</div>
567566
</div>
568567
<ul
568+
aria-labelledby="unique_id_mock"
569569
class="pf-v6-c-jump-links__list"
570570
role="list"
571571
>

‎packages/react-core/src/components/JumpLinks/examples/JumpLinksBasic.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { JumpLinks, JumpLinksItem } from '@patternfly/react-core';
22

33
export const JumpLinksBasic: React.FunctionComponent = () => (
4-
<JumpLinks>
4+
<JumpLinks aria-label="Jump to basic example sections">
55
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
66
<JumpLinksItem href="#" isActive>
77
Active section

0 commit comments

Comments
 (0)