Remove erorr bypass from a11y-check and adjust formatting (#3744)

Shopify · Dec 21, 2020 · ee1f920 · ee1f920
1 parent 01b9f54
commit ee1f920
Show file tree

Hide file tree

Showing 9 changed files with 115 additions and 189 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -79,6 +79,7 @@ The format is based on [these versioning and changelog guidelines](https://git.i
 - Generalized Tooltip's `content` prop's type to not only accept string, but any `React.Node`. ([#3559](https://github.com/Shopify/polaris-react/pull/3559))
 - Updated `TopBar` to show the logo when there is no navigation or search fields ([#3523](https://github.com/Shopify/polaris-react/pull/3523))
 - Updated critical `Banner` icon to be a diamond ([#3567](https://github.com/Shopify/polaris-react/pull/3567))
+- Added `tabIndex` to `Scrollable` for keyboard focus ([#3744](https://github.com/Shopify/polaris-react/pull/3744))
 
 ### Bug fixes
 

diff --git a/UNRELEASED.md b/UNRELEASED.md
@@ -50,6 +50,7 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
 
 ### Code quality
 
+- Removed skipped accessibility tests and fixes component accessibility issues ([#3721](https://github.com/Shopify/polaris-react/pull/3721))
 - Extracted `TagsWrapper` from `Filters` for testability ([#3688](https://github.com/Shopify/polaris-react/pull/3688))
 
 ### Deprecations

diff --git a/scripts/accessibility-check.js b/scripts/accessibility-check.js
@@ -3,185 +3,106 @@ const os = require('os');
 
 const puppeteer = require('puppeteer');
 const pMap = require('p-map');
+const chalk = require('chalk');
 
 // eslint-disable-next-line node/no-path-concat
 const iframePath = `file://${__dirname}/../build/storybook/static/iframe.html`;
-
 const concurrentCount = os.cpus().length;
-console.log(`Running ${concurrentCount} concurrent pages at a time`);
+const skippedStoryIds = ['playground-playground', 'all-examples'];
+
+const getUrls = async (browser) => {
+  // Get the URLS from storybook
+  const page = await browser.newPage();
+  await page.goto(iframePath);
+  const stories = await page.evaluate(() =>
+    window.__STORYBOOK_CLIENT_API__.raw(),
+  );
+  await page.close();
+
+  const urls = stories.reduce((memo, story) => {
+    // If it is a story id that is not in excludedStoryIds
+    if (skippedStoryIds.every((id) => !story.id.includes(id))) {
+      const url = `${iframePath}?id=${story.id}`;
+      memo.push(
+        url,
+        `${url}&contexts=New%20Design%20Language%3DEnabled%20-%20Light%20Mode`,
+        // Dark mode has lots of errors. It is still very WIP so ignore for now
+        // `${url}&contexts=New%20Design%20Language%3DEnabled%20-%20Dark%20Mode`,
+      );
+    }
 
-(async function run() {
+    return memo;
+  }, []);
+
+  return urls;
+};
+
+const formatFailure = (fail) => {
+  return ` - ${fail.failureSummary.split('\n  ')[1]}\n   ${fail.html}`;
+};
+
+const formatMessage = (id, violations) => {
+  const url = chalk.underline.blue(
+    `http://localhost:6006/iframe.html?id=all-components-${id}`,
+  );
+  return violations
+    .map((fail) => {
+      const message = fail.nodes
+        .map((error) => formatFailure(error))
+        .join('\n');
+      return `${chalk.red(fail.impact)} => ${url}\n${message}`;
+    })
+    .join('\n');
+};
+
+(async () => {
   try {
+    //  Open a browser
+    console.log(chalk.bold(`🌐 Opening ${concurrentCount} tabs in chromium`));
     const browser = await puppeteer.launch();
-    const initialPage = await browser.newPage();
-
-    await initialPage.goto(iframePath);
-    const stories = await initialPage.evaluate(() => {
-      return window.__STORYBOOK_CLIENT_API__.raw();
-    });
-
-    await initialPage.close();
 
-    const storyUrls = stories.reduce((memo, story) => {
-      // There is no need to test the Playground, or the "All Examples" stories
-      const isSkippedStory =
-        story.kind === 'Playground/Playground' || story.name === 'All Examples';
+    // Get the test ids from storybook
+    const testIds = await getUrls(browser);
 
-      if (!isSkippedStory) {
-        const idParam = `id=${encodeURIComponent(story.id)}`;
-        memo.push(
-          idParam,
-          `${idParam}&contexts=Global%20Theming=Enabled%20-%20Light%20Mode`,
-          // Dark mode has lots of errors. It is still very WIP so ignore for now
-          // `${idParam}&contexts=Global%20Theming=Enabled%20-%20Dark%20Mode`,
-        );
-      }
-      return memo;
-    }, []);
+    console.log(chalk.bold(`🧪 Testing ${testIds.length} urls with axe`));
 
-    /**
-     * @param {string} url
-     */
+    // Test the pages with axe
     const testPage = async (url) => {
+      const id = url.replace(`${iframePath}?id=all-components-`, '');
+      console.log(` - ${id}`);
+
       try {
-        console.log(`Testing: ${url}`);
         const page = await browser.newPage();
-        await page.goto(`${iframePath}?${url}`);
-
-        const result = await page.evaluate(() => {
-          return window.axe.run(document.getElementById('root'), {});
-        });
-
+        await page.goto(url);
+        const result = await page.evaluate(() =>
+          window.axe.run(document.getElementById('root'), {}),
+        );
         await page.close();
 
-        if (result.violations.length === 0) {
-          return Promise.resolve({type: 'PASS', url, errorCount: 0});
+        if (result.violations.length) {
+          return formatMessage(id, result.violations);
         }
-
-        return Promise.resolve({
-          type: 'FAIL',
-          url,
-          errorCount: result.violations.length,
-          error: JSON.stringify(result.violations, null, 2),
-        });
       } catch (error) {
-        return Promise.resolve({
-          type: 'ERROR',
-          url,
-          error: JSON.stringify(error, null, 2),
-        });
+        return `please retry => ${id}:\n - ${error.message}`;
       }
     };
 
-    const results = await pMap(storyUrls, testPage, {
+    const results = await pMap(testIds, testPage, {
       concurrency: concurrentCount,
     });
-
     await browser.close();
 
-    if (results.length === 0) {
-      throw new Error('Component URLs could not be crawled');
-    }
-
-    // A list of urls with a count of known, expected failures
-    // Ideally this shouldn't exist for long as we fix issues
-    const expectedIssues = {
-      'id=all-components-modal--modal-with-scroll-listener': 1,
-      'id=all-components-modal--modal-with-scroll-listener&contexts=Global%20Theming=Enabled%20-%20Light%20Mode': 1,
-      'id=all-components-scrollable--default-scrollable-container': 1,
-      'id=all-components-scrollable--default-scrollable-container&contexts=Global%20Theming=Enabled%20-%20Light%20Mode': 1,
-    };
-
-    const {
-      resultsWithErrors,
-      resultsWithUnexpectedViolations,
-      resultsWithExpectedViolations,
-    } = results.reduce(
-      (memo, resultItem) => {
-        if (resultItem.type === 'ERROR') {
-          memo.resultsWithErrors.push(resultItem);
-        } else if (resultItem.type === 'FAIL') {
-          if (resultItem.errorCount === expectedIssues[resultItem.url]) {
-            memo.resultsWithExpectedViolations.push(resultItem);
-            // Delete items once we fine them, so we know what items haven't
-            // been triggered, so we can tell people they should be removed from
-            // the list
-            delete expectedIssues[resultItem.url];
-          } else {
-            memo.resultsWithUnexpectedViolations.push(resultItem);
-          }
-        }
-
-        return memo;
-      },
-      {
-        resultsWithErrors: [],
-        resultsWithUnexpectedViolations: [],
-        resultsWithExpectedViolations: [],
-      },
-    );
-
-    const errorCount = resultsWithErrors.length;
-
-    const unexpectedViolationsCount = resultsWithUnexpectedViolations.length;
-    const expectedViolationsCount = resultsWithExpectedViolations.length;
-    const totalViolationsCount =
-      unexpectedViolationsCount + expectedViolationsCount;
-
-    const untriggeredExpectedIssues = Object.entries(expectedIssues);
-    const untriggeredExpectedViolationsCount = untriggeredExpectedIssues.length;
-
-    console.log(
-      `There were ${totalViolationsCount} Issues reported! ${expectedViolationsCount} Issues were expected. ${untriggeredExpectedViolationsCount} Expected Issues were absent. There were ${errorCount} errors`,
-    );
-
-    if (
-      unexpectedViolationsCount === 0 &&
-      untriggeredExpectedViolationsCount === 0 &&
-      errorCount === 0
-    ) {
-      return;
-    }
-
-    if (unexpectedViolationsCount) {
-      console.log('---\n\nUnexpected Issues:');
-      resultsWithUnexpectedViolations.forEach((result) => {
-        console.log(
-          `${result.type} ${result.url} (${result.errorCount}): \n${result.error}`,
-        );
-      });
-    }
-
-    if (untriggeredExpectedViolationsCount) {
-      console.log('---\n\nExpected Issues that were not triggerd:');
-      untriggeredExpectedIssues.forEach(([url, expectedViolationCount]) => {
-        const result = results.find((resultItem) => resultItem.url === url);
-
-        if (!result) {
-          console.log(
-            `${url}: No matching story, remove this url from the expectedIssues array`,
-          );
-        } else if (result.type !== 'ERROR') {
-          console.log(
-            `${url}: Expected ${expectedViolationCount} issues, got ${result.errorCount}.`,
-          );
-        }
-      });
-    }
-
-    if (errorCount) {
-      console.log(
-        '---\n\nPages triggered an ERROR when trying to identify violations (you should rerun too see if this goes away):',
-      );
-
-      resultsWithErrors.forEach((result) => {
-        console.log(`${result.type} ${result.url}: \n${result.error}`);
-      });
+    // Format the results and log them out
+    const messages = results.filter((x) => x);
+    if (messages.length) {
+      console.error(chalk.bold(`‼️  Test failures found`));
+      console.log(messages.join('\n'));
+      process.exit(1);
+    } else {
+      console.log(chalk.bold('🧚‍♀️ Accessibility is all g'));
     }
-
-    process.exit(1);
-  } catch (err) {
-    console.log(err);
+  } catch (error) {
+    console.error(error.message);
     process.exit(1);
   }
 })();
diff --git a/src/components/Navigation/Navigation.scss b/src/components/Navigation/Navigation.scss
@@ -104,7 +104,7 @@ $disabled-fade: 0.6;
       left: -1 * spacing(tight);
       height: 100%;
       width: $active-indicator-width;
-      background-color: var(--p-action-primary);
+      background-color: var(--p-text-primary);
       border-top-right-radius: var(--p-border-radius-base);
       border-bottom-right-radius: var(--p-border-radius-base);
     }

diff --git a/src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx b/src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx
@@ -393,20 +393,21 @@ describe('<PopoverOverlay />', () => {
     });
 
     it('focuses the first focusbale node when autofocusTarget is set to FirstNode', () => {
-      const popoverOverlay = mountWithApp(
+      mountWithApp(
         <PopoverOverlay
           active
           id="PopoverOverlay-1"
           activator={activator}
           onClose={noop}
           autofocusTarget="first-node"
         >
-          <input type="text" />
+          <p>Hello world</p>
         </PopoverOverlay>,
       );
 
-      const focusTarget = popoverOverlay.find('input', {})!.domNode;
-      expect(document.activeElement).toBe(focusTarget);
+      expect(document.activeElement?.className).toBe(
+        'Pane Scrollable vertical',
+      );
     });
 
     it('does not focus when autofocusTarget is set to None', () => {

diff --git a/src/components/ResourceList/ResourceList.tsx b/src/components/ResourceList/ResourceList.tsx
@@ -674,12 +674,12 @@ export const ResourceList: ResourceListType = function ResourceList<ItemType>({
   const spinnerSize = items.length < 2 ? 'small' : 'large';
 
   const loadingOverlay = loading ? (
-    <li>
-      <div className={styles.SpinnerContainer} style={spinnerStyle}>
+    <>
+      <li className={styles.SpinnerContainer} style={spinnerStyle}>
         <Spinner size={spinnerSize} accessibilityLabel="Items are loading" />
-      </div>
-      <div className={styles.LoadingOverlay} />
-    </li>
+      </li>
+      <li className={styles.LoadingOverlay} />
+    </>
   ) : null;
 
   const className = classNames(

diff --git a/src/components/Scrollable/Scrollable.tsx b/src/components/Scrollable/Scrollable.tsx
@@ -130,6 +130,8 @@ export class Scrollable extends Component<ScrollableProps, State> {
             {...scrollable.props}
             {...rest}
             ref={this.setScrollArea}
+            // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex
+            tabIndex={0}
           >
             {children}
           </div>