Skip to content

Commit

Permalink
PhosphorIcon: default role to img when aria-label is provided (#2358)
Browse files Browse the repository at this point in the history
## Summary:
When an `aria-label` is provided, there should be a `role` applied to the element to give it meaning to browsers and screenreaders. This PR defaults the `role` to `"img"` in `PhosphorIcon` when an `aria-label` is provided.

I'm not sure what our definitions are for "major", "minor", and "patch" releases. _I_ would consider this a `minor` release since we are changing the behavior (even if it is slight), but it is backwards compatible so `patch` might be fine.

Issue: WB-1795

## Test plan:
- Tests pass
- Axe is happy

Author: SeanMcP

Reviewers: jeresig

Required Reviewers:

Approved By: jeresig

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ⏭️  dependabot

Pull Request URL: #2358
  • Loading branch information
SeanMcP authored Nov 13, 2024
1 parent e1c98df commit c111059
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 1 deletion.
5 changes: 5 additions & 0 deletions .changeset/dirty-dragons-fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/wonder-blocks-icon": minor
---

defaults the role of PhosphorIcon to "img" when an aria-label is provided
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,16 @@ describe("PhosphorIcon", () => {
);
});

it("applies role=img when aria-label is provided", async () => {
// Arrange

// Act
render(<PhosphorIcon icon={Plus} aria-label="something" />);

// Assert
expect(screen.getByRole("img")).toBeInTheDocument();
});

it("calls viewportPixelsForSize with size from props", async () => {
// Arrange
const viewPortPixelsForSizeSpy = jest.spyOn(
Expand Down
5 changes: 4 additions & 1 deletion packages/wonder-blocks-icon/src/components/phosphor-icon.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ type Props = Pick<AriaProps, "aria-hidden" | "aria-label" | "role"> & {
className?: string;

/**
* The role of the icon.
* The role of the icon. Will default to `img` if an `aria-label` is
* provided.
* @see https://www.w3.org/WAI/WCAG21/Techniques/aria/ARIA24
*/
role?: "img";
Expand Down Expand Up @@ -89,6 +90,7 @@ export const PhosphorIcon = React.forwardRef(function PhosphorIcon(
style,
testId,
className,
role,
...sharedProps
} = props;

Expand All @@ -113,6 +115,7 @@ export const PhosphorIcon = React.forwardRef(function PhosphorIcon(
]}
data-testid={testId}
ref={ref}
role={role ?? sharedProps["aria-label"] ? "img" : undefined}
/>
);
});
Expand Down

0 comments on commit c111059

Please sign in to comment.