[storybook-a11y-test] Set best practices for dealing with failing rules #2045

kaelig · 2021-09-22T06:25:18Z

Description

At the moment, folks have no other choice but to entirely disable accessibility tests in a story whenever it has a single violation. This is detrimental to accessibility testing as it could hide many issues.

The best practice is to either:

disable a rule at an element-level,
set a violation as reviewOnFail so that teams can get back to it later

This PR allows for very granular axe configuration settings that work both in Storybook and in this automated testing suite.

Before

MyStory.parameters = {
  a11y: {
    // Don't do this!
    disable: true,
  }
};

After

MyStory.parameters = {
  a11y: {
    // Instead, use one of these 👇
    // @see axe-core optionsParameter (https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#options-parameter)
    options: {
      rules: [
        {
          // Exclude some elements
          // <explain why here>
          id: 'autocomplete-valid',
          selector: '*:not([autocomplete="nope"])',
        },
        {
          // When there's a false positive, it's okay to disable a specific rule, for example:
          // Color contrast ratio doesn't need to meet 4.5:1, as elements are disabled
          // @link https://dequeuniversity.com/rules/axe/4.1/color-contrast?application=axeAPI
          id: 'color-contrast',
          enabled: false,
        },
        {
          // You may also want to
          // This needs to be fixed in the future
          // <explain why it was set to reviewOnFail>
          id: 'landmark-complementary-is-top-level',
          reviewOnFail: true,
        },
      ],
    },
  },
};

🎩 instructions

dev clone quilt
git fetch
git checkout allow-axe-rule-overrides
dev up
PKG_NAME=storybook-a11y-test yarn tophat

In your project (your mileage may vary regarding yarn scripts:

npx yalc link --no-pure @shopify/storybook-a11y-test
yarn run build-storybook
yarn run storybook:a11y

Type of change

Patch: Bug (non-breaking change which fixes an issue)
Minor: New feature (non-breaking change which adds functionality)
Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

Tested in both Shopify/web and Shopify/polaris-react.

Polaris didn't have any errors.

In Shopify/web, these errors popped up, probably due to the version of axe-core being used:

alex-page · 2021-09-22T15:03:02Z

I would love to learn more about why teams are not fixing:

whenever it has a single violation

Feels like we are solving the wrong problem and enabling teams to disable tests whenever they like.

dfmcphee · 2021-09-22T15:11:01Z

I would love to learn more about why teams are not fixing:
whenever it has a single violation
Feels like we are solving the wrong problem and enabling teams to disable tests whenever they like.

I am curious about this too. I have also seen one false positive or a flakey test cause many teams to disable these tests entirely though. Looking at web we have at least 90 different stories completely disabling accessibility tests, which is a major red flag. Still, I see this as a step forward. More similar to how we allow the disabling of a single eslint rule. Yes, this should always be an exception, but it is also necessary sometimes (and better than people opting out entirely).

I would really like to either have clear guidelines to make the comment explaining why required, or even better, a lint rule to enforce that.

BPScott · 2021-09-22T16:36:17Z

packages/storybook-a11y-test/src/index.ts

@@ -66,20 +68,21 @@ export const getCurrentStoryIds = async ({
  return stories.reduce(removeSkippedStories(skippedStoryIds), []);
 };

-const formatFailureNodes = (nodes) => {
+const formatFailureNodes = (nodes: axeCore.NodeResult[]) => {


I appreciate adding types. Any chance we could write these as regular function declarations rather than assigning to variables while you're in here?

Suggested change

const formatFailureNodes = (nodes: axeCore.NodeResult[]) => {

function formatFailureNodes(nodes: axeCore.NodeResult[]) {

etc

Fixed in 84cb409

This feels like something that should be linted against, as our styleguide doesn't clearly state that functions are the way to go https://github.com/Shopify/javascript#functions

packages/storybook-a11y-test/src/index.ts

BPScott · 2021-09-22T16:42:51Z

packages/storybook-a11y-test/src/index.ts

+        const axeElement: axeCore.ElementContext | undefined =
+          story.parameters.a11y && story.parameters.a11y.element;


These assignments might be a good excuse for optional chaining / nullish coalescing:

Suggested change

const axeElement: axeCore.ElementContext | undefined =

story.parameters.a11y && story.parameters.a11y.element;

const axeElement: axeCore.ElementContext | undefined = story.parameters.a11y?.element ?? {}

If you provide a type to the const story declaration above you wouldn't need to specify the types down here, something like:

interface Story { parameters: { a11y?: { element?: axeCore.ElementContext; config?: axeCore.spec; options?: axeCore.RunOptions; } } } const story: Story = ....

This part of the code is definitely a good one to refactor, thanks for the pointers.

What I ended up doing is aligning with what addon-a11y's runner does:

https://github.com/storybookjs/storybook/blob/78c580eac4c91231b5966116492e34de0a0df66f/addons/a11y/src/a11yRunner.ts

Let me know what you think!

You'll see this:

import type {A11yParameters} from '@storybook/addon-a11y/dist/ts3.9/params';

Ideally this type would be easier to import, so I opened a PR in Storybook, seeking a more elegant solution: storybookjs/storybook#16128

packages/storybook-a11y-test/src/index.ts

…nd running axe

dfmcphee · 2021-09-22T18:01:31Z

packages/storybook-a11y-test/README.md

  }
 })();
 ```

+### Ignoring specific violations in a Story


Could we add something here about only doing this if there are false positives, not to get around an existing accessibility problem being flagged?

Good nudge, yeah! And I think it's also useful during prototyping / early build GSD phases so I'll mention that.

I just pushed 82eae5b and went for a mix of guidelines + examples, feedback appreciated!

dfmcphee

This looks good to me now other than Ben's comment about converting those arrow functions / variables into regular functions.

Do we have a plan for getting teams to re-enable these in web after this change goes in?

BPScott

No ran but logic seems good to me

kaelig · 2021-09-23T18:59:10Z

Do we have a plan for getting teams to re-enable these in web after this change goes in?

The first step is to replace disable: true with a reviewOnFail: true on each failing rule (see example below):

PrototypeComponent.parameters = {
  a11y: {
    options: {
      rules: [
        {
          // Page-level semantics cause a violation and need to be reworked.
          // Currently discussing solutions with the accessibility team.
          //
          // @link https://github.com/Shopify/shopify/issues/123
          // @link https://dequeuniversity.com/rules/axe/4.3/landmark-complementary-is-top-level
          id: 'landmark-complementary-is-top-level',
          reviewOnFail: true,
        },
      ],
    },
  },
};

Once I'm done, we'll have a better idea of the scale of the problem and we can look into next steps (as in: how to nudge team toward fixing these issues).

kaelig requested a review from a team September 22, 2021 06:25

kaelig force-pushed the allow-axe-rule-overrides branch from c0268e3 to 08233a0 Compare September 22, 2021 07:24

kaelig requested review from dfmcphee and alex-page September 22, 2021 07:36

[storybook-a11y-test] Allow more granular a11y overrides

710d318

kaelig force-pushed the allow-axe-rule-overrides branch from 08233a0 to 710d318 Compare September 22, 2021 07:37

BPScott reviewed Sep 22, 2021

View reviewed changes

packages/storybook-a11y-test/src/index.ts Outdated Show resolved Hide resolved

BPScott reviewed Sep 22, 2021

View reviewed changes

packages/storybook-a11y-test/src/index.ts Outdated Show resolved Hide resolved

kaelig added 2 commits September 22, 2021 10:57

Improve typing, align with addon-a11y's way of getting story params a…

2a1198f

…nd running axe

Remove unnecessary promise chaining, don't catch

77342ed

dfmcphee reviewed Sep 22, 2021

View reviewed changes

kaelig added 2 commits September 22, 2021 17:58

Add examples and guidelines for ignoring rules

82eae5b

Revert timeout to 3000, remove mention to puppeteer's default

beffc0b

dfmcphee approved these changes Sep 23, 2021

View reviewed changes

kaelig requested a review from BPScott September 23, 2021 17:47

Convert constants to named functions

84cb409

BPScott approved these changes Sep 23, 2021

View reviewed changes

kaelig changed the title ~~[storybook-a11y-test] Allow more granular a11y overrides~~ [storybook-a11y-test] Set best practices for dealing with failing rules Sep 23, 2021

kaelig merged commit dfe2122 into main Sep 23, 2021

kaelig deleted the allow-axe-rule-overrides branch September 23, 2021 19:01

BPScott mentioned this pull request Jan 5, 2022

feat: Remove @shopify/polaris-tokens dependency Shopify/polaris#4868

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[storybook-a11y-test] Set best practices for dealing with failing rules #2045

[storybook-a11y-test] Set best practices for dealing with failing rules #2045

kaelig commented Sep 22, 2021 •

edited

alex-page commented Sep 22, 2021

dfmcphee commented Sep 22, 2021 •

edited

BPScott Sep 22, 2021

kaelig Sep 23, 2021

BPScott Sep 22, 2021

kaelig Sep 22, 2021

dfmcphee Sep 22, 2021

kaelig Sep 22, 2021

kaelig Sep 23, 2021

dfmcphee left a comment

BPScott left a comment

kaelig commented Sep 23, 2021

	const formatFailureNodes = (nodes: axeCore.NodeResult[]) => {
	function formatFailureNodes(nodes: axeCore.NodeResult[]) {

		const axeElement: axeCore.ElementContext \| undefined =
		story.parameters.a11y && story.parameters.a11y.element;

[storybook-a11y-test] Set best practices for dealing with failing rules #2045

[storybook-a11y-test] Set best practices for dealing with failing rules #2045

Conversation

kaelig commented Sep 22, 2021 • edited

Description

Before

After

🎩 instructions

Type of change

Checklist

alex-page commented Sep 22, 2021

dfmcphee commented Sep 22, 2021 • edited

BPScott Sep 22, 2021

Choose a reason for hiding this comment

kaelig Sep 23, 2021

Choose a reason for hiding this comment

BPScott Sep 22, 2021

Choose a reason for hiding this comment

kaelig Sep 22, 2021

Choose a reason for hiding this comment

dfmcphee Sep 22, 2021

Choose a reason for hiding this comment

kaelig Sep 22, 2021

Choose a reason for hiding this comment

kaelig Sep 23, 2021

Choose a reason for hiding this comment

dfmcphee left a comment

Choose a reason for hiding this comment

BPScott left a comment

Choose a reason for hiding this comment

kaelig commented Sep 23, 2021

kaelig commented Sep 22, 2021 •

edited

dfmcphee commented Sep 22, 2021 •

edited