Upgrade axe and address accessibility issues #1276

robinmetral · 2021-11-17T20:02:05Z

Purpose

This is actually a tricky one so I'll try to document my thinking as much as possible here.

Background and context

We're one major behind with jext-axe (we're on 4.1.0, the current version is 5.0.1). Dependabot has been desperately trying to upgrade it (#1141), but it broke unit tests.

This PR upgrades the dependencies, and addresses the axe-related issues in Circuit components.

Approach and changes

Resolve to axe-core@^4.3.5 using yarn resolutions (details in comments below)
Add role="progressbar" to the time-based ProgressBar (details in comments below)
Hide the Carousel's progress indicator to screen readers using aria-hidden (details in comments below)

Definition of done

vercel · 2021-11-17T20:02:07Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sumup/oss-circuit-ui/DW6AruovpkRtD7se7QGN4yoiupJi
✅ Preview: https://oss-circuit-ui-git-upgrade-axe-sumup.vercel.app

changeset-bot · 2021-11-17T20:02:07Z

🦋 Changeset detected

Latest commit: 16fcbe8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sumup/circuit-ui	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2021-11-17T20:02:14Z

Codecov Report

Merging #1276 (16fcbe8) into main (698abd1) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1276   +/-   ##
=======================================
  Coverage   92.34%   92.34%           
=======================================
  Files         186      186           
  Lines        3669     3669           
  Branches     1182     1182           
=======================================
  Hits         3388     3388           
  Misses        263      263           
  Partials       18       18

Impacted Files	Coverage Δ
...ackages/circuit-ui/components/Carousel/Carousel.js	`100.00% <ø> (ø)`
.../circuit-ui/components/ProgressBar/ProgressBar.tsx	`95.65% <ø> (ø)`

robinmetral · 2021-11-17T20:37:24Z

package.json

@@ -76,5 +76,8 @@
    "storybook-addon-performance": "^0.16.1",
    "svgo": "^2.0.3",
    "typescript": "^4.4.3"
+  },
+  "resolutions": {
+    "axe-core": "^4.3.5"


This ensures that our underlying axe-core version (dependency of jest-axe et. al.) is above 4.3.3.

Why?

Starting in [email protected]), role="presentation" was flagged as invalid on <svg> elements, probably as a side-effect of dequelabs/axe-core#2989 (see our failing CI run).

This bug was reported in dequelabs/axe-core#3082, and solved in dequelabs/axe-core#3124 (patch released in [email protected]).

Resolving to a version above 4.3.3 is one way to address the issue—another would have been to pin axe-core to a version below 4.3.0.

Also note: there was another issue with axe-core affecting the SideNavigation specs, similar to dequelabs/axe-core#2674.

The problem was apparently a bug in an isVisible matcher that would not account for zero width in some cases. In our case, it affected the SideNavigation's mobile specs and ended up timing out (see our failing CI run)

It was fixed in dequelabs/axe-core#3172 and released in [email protected], so resolving to ^4.3.5 is fine for us.

robinmetral · 2021-11-17T20:47:26Z

packages/circuit-ui/components/ProgressBar/ProgressBar.tsx

@@ -260,6 +260,7 @@ export function ProgressBar({
        />
      ) : (
        <TimeProgress
+          role="progressbar"


The ProgressBar's time-based variant (story) started breaking in CI back when dependabot tried to upgrade jest-axe to v5 (see our failing CI run). As I wrote in a comment back then:

So the new jest-axe version is unhappy because the ProgressBar's time-based variant (when there is no given max or value props) doesn't have the proper aria-attributes.

Looking at the difference between it and the step-based variant, I think it might be because it's missing a progressbar role (?).

However, I'm not sure it should have this role. The only place I can see this time-based variant used is in the carousel, where it's mostly a visual indicator that the next image is about to be shown.

To answer my own questions:

I think it might be because it's missing a progressbar role (?).

Not quite: it was failing because aria-labelledby cannot be used on a <span> without a role.

However, I'm not sure it should have this role.

I now think it should. Leaving aside the Carousel case (that's for another comment), and looking at MDN for role="progressbar":

If the actual value of the progressbar can be determined, the developer has to indicate this progress using the aria-valuenow, aria-valuemin, and aria-valuemax attributes¹. If the progress value is indeterminate, the developer should omit the aria-valuenow attribute.

I think it's safe to add role="progressbar" here in this case. An alternative would be to remove aria-labelledby, meaning that the label would only be loosely associated with the progress bar.

We will also revisit accessibility for this component as part of the a11y audit fixes we'll apply this month.

Footnotes

That's what we're already doing for the ProgressBar's step-based variant ↩

robinmetral · 2021-11-17T20:54:33Z

packages/circuit-ui/components/Carousel/Carousel.js

@@ -139,6 +139,7 @@ const Carousel = ({
              <StyledStatus step={state.step} total={slidesTotal} />

              <StyledProgressBar
+                aria-hidden


This is the only place where we're using the ProgressBar's time-based variant inside Circuit.

It's also the reason I hesitated to add role="progressbar" to the ProgressBar—as I wrote back then:

The only place I can see this time-based variant used is in the carousel, where it's mostly a visual indicator that the next image is about to be shown.

Also: I added the role just to try it out, and ran into another error because the Carousel isn't exposing a label prop for its progress bar (so the aria-labelledby points to an empty span). Here again, I'm not sure what would be the expected label, if any. Something like "In progress" doesn't sound quite right.

On second thought, it simply seems to me like the progress indicator should only be visual here. It's not actually loading, it's only an indicator for when the next carousel slide will come up.

Therefore: aria-hidden.

⚠️ the Carousel component itself has many accessibility issues, e.g. it's not using live regions to announce new content, it's not handling focus properly, etc. See this guide for reference. However, that's a different topic altogether, and should probably be tackled when the Carousel gets redesigned (it's on the backlog).

connor-baer

Your reasoning makes sense to me 👍🏻

Robin Métral added 4 commits November 17, 2021 18:38

Upgrade jest-axe

1d66f08

Add resolution for axe-core to ^4.3.5

ccabb73

Visually hide the PregressBar in the Carousel component

3cbae64

Add role=progressbar to the time-based ProgressBar

35b013e

github-actions bot added the 🗂 circuit-ui label Nov 17, 2021

robinmetral commented Nov 17, 2021

View reviewed changes

sumup-oss deleted a comment from sumup-clark bot Nov 17, 2021

Add changesets

16fcbe8

vercel bot deployed to Preview November 17, 2021 21:15 View deployment

robinmetral marked this pull request as ready for review November 17, 2021 21:15

robinmetral requested a review from a team as a code owner November 17, 2021 21:15

robinmetral requested review from connor-baer and removed request for a team November 17, 2021 21:15

robinmetral changed the title ~~Upgrade axe~~ Upgrade axe and address accessibility issues Nov 17, 2021

connor-baer approved these changes Nov 17, 2021

View reviewed changes

connor-baer added the ♿ accessibility Improves usability label Nov 17, 2021

robinmetral merged commit 701a668 into main Nov 18, 2021

robinmetral deleted the upgrade-axe branch November 18, 2021 07:38

github-actions bot mentioned this pull request Nov 18, 2021

Version Packages #1277

Merged

robinmetral mentioned this pull request Dec 6, 2022

Unpin axe-core sub-dependency #1871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade axe and address accessibility issues #1276

Upgrade axe and address accessibility issues #1276

robinmetral commented Nov 17, 2021 •

edited

Loading

vercel bot commented Nov 17, 2021 •

edited

Loading

changeset-bot bot commented Nov 17, 2021 •

edited

Loading

codecov bot commented Nov 17, 2021 •

edited

Loading

robinmetral Nov 17, 2021

robinmetral Nov 17, 2021

robinmetral Nov 17, 2021

robinmetral Nov 17, 2021

connor-baer left a comment

Upgrade axe and address accessibility issues #1276

Upgrade axe and address accessibility issues #1276

Conversation

robinmetral commented Nov 17, 2021 • edited Loading

Purpose

Background and context

Approach and changes

Definition of done

vercel bot commented Nov 17, 2021 • edited Loading

changeset-bot bot commented Nov 17, 2021 • edited Loading

🦋 Changeset detected

codecov bot commented Nov 17, 2021 • edited Loading

Codecov Report

robinmetral Nov 17, 2021

Choose a reason for hiding this comment

robinmetral Nov 17, 2021

Choose a reason for hiding this comment

robinmetral Nov 17, 2021

Choose a reason for hiding this comment

Footnotes

robinmetral Nov 17, 2021

Choose a reason for hiding this comment

connor-baer left a comment

Choose a reason for hiding this comment

robinmetral commented Nov 17, 2021 •

edited

Loading

vercel bot commented Nov 17, 2021 •

edited

Loading

changeset-bot bot commented Nov 17, 2021 •

edited

Loading

codecov bot commented Nov 17, 2021 •

edited

Loading