Skip to content

Commit 0c203b0

Browse files
ParkreinerParkreiner
authored
fix: correct markup for Abbr component (#19317)
Fixes some accidental styling issues introduced in #19242 ## Changes made - Updated styles - Added support for `className` prop so that we can override the styles as needed - Removed the aria-label in favor of injecting the main text directly ## Notes - This feels like a case where the changes in the previous PR were actually *correct overall*, but something with our MUI+Tailwind setup created conflicting styles, and we accidentally introduced an underline style that shouldn't be there - Removed the Aria label because I've realized in the past year that Aria is really easy to misuse, and it's best just to do things with the base HTML features as much as possible. There's a risk that the old code had compliance issues with certain types of screen readers (even though it worked fine when I did manual testing back in 2023). These changes hopefully remove those risks completely
1 parent 1ffc5a0 commit 0c203b0

File tree

3 files changed

+50
-39
lines changed

3 files changed

+50
-39
lines changed

‎site/src/components/Abbr/Abbr.stories.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ const meta: Meta<typeof Abbr> = {
66
component: Abbr,
77
decorators: [
88
(Story) => (
9-
<>
9+
<div className="max-w-prose text-base">
1010
<p>Try the following text out in a screen reader!</p>
1111
<Story />
12-
</>
12+
</div>
1313
),
1414
],
1515
};
@@ -25,9 +25,9 @@ export const InlinedShorthand: Story = {
2525
},
2626
decorators: [
2727
(Story) => (
28-
<p className="max-w-2xl">
28+
<p>
2929
The physical pain of getting bonked on the head with a cartoon mallet
30-
lasts precisely 593{" "}
30+
lasts precisely 593
3131
<span className="underline decoration-dotted">
3232
<Story />
3333
</span>

‎site/src/components/Abbr/Abbr.test.tsx

Lines changed: 34 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,14 @@
11
import { render, screen } from "@testing-library/react";
2-
import { Abbr, type Pronunciation } from "./Abbr";
2+
import { Abbr } from "./Abbr";
33

44
type AbbreviationData = {
55
abbreviation: string;
66
title: string;
77
expectedLabel: string;
88
};
99

10-
type AssertionInput = AbbreviationData & {
11-
pronunciation: Pronunciation;
12-
};
13-
14-
function assertAccessibleLabel({
15-
abbreviation,
16-
title,
17-
expectedLabel,
18-
pronunciation,
19-
}: AssertionInput) {
20-
const { unmount } = render(
21-
<Abbr title={title} pronunciation={pronunciation}>
22-
{abbreviation}
23-
</Abbr>,
24-
);
25-
26-
screen.getByLabelText(expectedLabel, { selector: "abbr" });
27-
unmount();
28-
}
29-
3010
describe(Abbr.name, () => {
31-
it("Has an aria-label that equals the title if the abbreviation is shorthand", () => {
11+
it("Omits abbreviation from screen-reader output if it is shorthand", () => {
3212
const sampleShorthands: AbbreviationData[] = [
3313
{
3414
abbreviation: "ms",
@@ -43,11 +23,22 @@ describe(Abbr.name, () => {
4323
];
4424

4525
for (const shorthand of sampleShorthands) {
46-
assertAccessibleLabel({ ...shorthand, pronunciation: "shorthand" });
26+
const { unmount } = render(
27+
<Abbr title={shorthand.title} pronunciation="shorthand">
28+
{shorthand.abbreviation}
29+
</Abbr>,
30+
);
31+
32+
// The <abbr> element doesn't have any ARIA role semantics baked in,
33+
// so we have to get a little bit more creative with making sure the
34+
// expected content is on screen in an accessible way
35+
const element = screen.getByTitle(shorthand.title);
36+
expect(element).toHaveTextContent(shorthand.expectedLabel);
37+
unmount();
4738
}
4839
});
4940

50-
it("Has an aria label with title and 'flattened' pronunciation if abbreviation is acronym", () => {
41+
it("Adds title and 'flattened' pronunciation if abbreviation is acronym", () => {
5142
const sampleAcronyms: AbbreviationData[] = [
5243
{
5344
abbreviation: "NASA",
@@ -67,11 +58,19 @@ describe(Abbr.name, () => {
6758
];
6859

6960
for (const acronym of sampleAcronyms) {
70-
assertAccessibleLabel({ ...acronym, pronunciation: "acronym" });
61+
const { unmount } = render(
62+
<Abbr title={acronym.title} pronunciation="acronym">
63+
{acronym.abbreviation}
64+
</Abbr>,
65+
);
66+
67+
const element = screen.getByTitle(acronym.title);
68+
expect(element).toHaveTextContent(acronym.expectedLabel);
69+
unmount();
7170
}
7271
});
7372

74-
it("Has an aria label with title and initialized pronunciation if abbreviation is initialism", () => {
73+
it("Adds title and initialized pronunciation if abbreviation is initialism", () => {
7574
const sampleInitialisms: AbbreviationData[] = [
7675
{
7776
abbreviation: "FBI",
@@ -91,7 +90,15 @@ describe(Abbr.name, () => {
9190
];
9291

9392
for (const initialism of sampleInitialisms) {
94-
assertAccessibleLabel({ ...initialism, pronunciation: "initialism" });
93+
const { unmount } = render(
94+
<Abbr title={initialism.title} pronunciation="initialism">
95+
{initialism.abbreviation}
96+
</Abbr>,
97+
);
98+
99+
const element = screen.getByTitle(initialism.title);
100+
expect(element).toHaveTextContent(initialism.expectedLabel);
101+
unmount();
95102
}
96103
});
97104
});

‎site/src/components/Abbr/Abbr.tsx

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import type { FC, HTMLAttributes } from "react";
22
import { cn } from "utils/cn";
33

4-
export type Pronunciation = "shorthand" | "acronym" | "initialism";
4+
type Pronunciation = "shorthand" | "acronym" | "initialism";
55

66
type AbbrProps = HTMLAttributes<HTMLElement> & {
77
children: string;
88
title: string;
99
pronunciation?: Pronunciation;
10+
className?: string;
1011
};
1112

1213
/**
@@ -22,23 +23,26 @@ export const Abbr: FC<AbbrProps> = ({
2223
children,
2324
title,
2425
pronunciation = "shorthand",
26+
className,
2527
...delegatedProps
2628
}) => {
2729
return (
2830
<abbr
29-
// Title attributes usually aren't natively available to screen readers;
30-
// always have to supplement with aria-label
31+
// Adding title to make things a little easier for sighted users,
32+
// but titles aren't always exposed via screen readers. Still have
33+
// to inject the actual text content inside the abbr itself
3134
title={title}
32-
aria-label={getAccessibleLabel(children, title, pronunciation)}
3335
className={cn(
34-
"decoration-inherit",
35-
children === children.toUpperCase()
36-
? "tracking-wide"
37-
: "tracking-normal",
36+
"no-underline tracking-normal",
37+
children === children.toUpperCase() && "tracking-wide",
38+
className,
3839
)}
3940
{...delegatedProps}
4041
>
4142
<span aria-hidden>{children}</span>
43+
<span className="sr-only">
44+
{getAccessibleLabel(children, title, pronunciation)}
45+
</span>
4246
</abbr>
4347
);
4448
};

0 commit comments

Comments
 (0)