diff --git a/CHANGELOG.md b/CHANGELOG.md index cf725cdab14..b25e7c36bac 100644 --- 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 index 4dd44688fa6..c30a70f9b35 100644 --- 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 index 647db293e32..e5955ce5366 100644 --- 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 index 271ccdcbf69..e172151f3d2 100644 --- 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 index cb823b84ddd..e88f17a5fe0 100644 --- a/src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx +++ b/src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx @@ -393,7 +393,7 @@ describe('', () => { }); it('focuses the first focusbale node when autofocusTarget is set to FirstNode', () => { - const popoverOverlay = mountWithApp( + mountWithApp( ', () => { onClose={noop} autofocusTarget="first-node" > - +

Hello world

, ); - 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 index f26a6890f38..e3a609fedd9 100644 --- a/src/components/ResourceList/ResourceList.tsx +++ b/src/components/ResourceList/ResourceList.tsx @@ -674,12 +674,12 @@ export const ResourceList: ResourceListType = function ResourceList({ const spinnerSize = items.length < 2 ? 'small' : 'large'; const loadingOverlay = loading ? ( -
  • -
    + <> +
  • - -
    -
    • + +
  • + ) : null; const className = classNames( diff --git a/src/components/Scrollable/Scrollable.tsx b/src/components/Scrollable/Scrollable.tsx index d55ffb88d3a..b89cb892b8f 100644 --- a/src/components/Scrollable/Scrollable.tsx +++ b/src/components/Scrollable/Scrollable.tsx @@ -130,6 +130,8 @@ export class Scrollable extends Component { {...scrollable.props} {...rest} ref={this.setScrollArea} + // eslint-disable-next-line jsx-a11y/no-noninteractive-tabindex + tabIndex={0} > {children} diff --git a/src/components/Tabs/README.md b/src/components/Tabs/README.md index 6c0960027db..4adcf3cd6b7 100644 --- a/src/components/Tabs/README.md +++ b/src/components/Tabs/README.md @@ -85,25 +85,25 @@ function TabsExample() { const tabs = [ { - id: 'all-customers', + id: 'all-customers-1', content: 'All', accessibilityLabel: 'All customers', - panelID: 'all-customers-content', + panelID: 'all-customers-content-1', }, { - id: 'accepts-marketing', + id: 'accepts-marketing-1', content: 'Accepts marketing', - panelID: 'accepts-marketing-content', + panelID: 'accepts-marketing-content-1', }, { - id: 'repeat-customers', + id: 'repeat-customers-1', content: 'Repeat customers', - panelID: 'repeat-customers-content', + panelID: 'repeat-customers-content-1', }, { - id: 'prospects', + id: 'prospects-1', content: 'Prospects', - panelID: 'prospects-content', + panelID: 'prospects-content-1', }, ]; @@ -146,15 +146,15 @@ function FittedTabsExample() { const tabs = [ { - id: 'all-customers-fitted', + id: 'all-customers-fitted-2', content: 'All', accessibilityLabel: 'All customers', - panelID: 'all-customers-fitted-content', + panelID: 'all-customers-fitted-content-2', }, { - id: 'accepts-marketing-fitted', + id: 'accepts-marketing-fitted-2', content: 'Accepts marketing', - panelID: 'accepts-marketing-fitted-Ccontent', + panelID: 'accepts-marketing-fitted-Ccontent-2', }, ]; @@ -199,23 +199,23 @@ function TabsWithBadgeExample() { const tabs = [ { - id: 'all-customers-fitted', + id: 'all-customers-fitted-3', content: ( All 10+ ), accessibilityLabel: 'All customers', - panelID: 'all-customers-fitted-content', + panelID: 'all-customers-fitted-content-3', }, { - id: 'accepts-marketing-fitted', + id: 'accepts-marketing-fitted-3', content: ( Accepts marketing 4 ), - panelID: 'accepts-marketing-fitted-Ccontent', + panelID: 'accepts-marketing-fitted-content-3', }, ]; @@ -246,25 +246,25 @@ function TabsWithCustomDisclosureExample() { const tabs = [ { - id: 'all-customers', + id: 'all-customers-4', content: 'All', accessibilityLabel: 'All customers', - panelID: 'all-customers-content', + panelID: 'all-customers-content-4', }, { - id: 'accepts-marketing', + id: 'accepts-marketing-4', content: 'Accepts marketing', - panelID: 'accepts-marketing-content', + panelID: 'accepts-marketing-content-4', }, { - id: 'repeat-customers', + id: 'repeat-customers-4', content: 'Repeat customers', - panelID: 'repeat-customers-content', + panelID: 'repeat-customers-content-4', }, { - id: 'prospects', + id: 'prospects-4', content: 'Prospects', - panelID: 'prospects-content', + panelID: 'prospects-content-4', }, ]; diff --git a/src/components/Tabs/components/Tab/Tab.tsx b/src/components/Tabs/components/Tab/Tab.tsx index e64587818c0..7d14c1e50de 100644 --- a/src/components/Tabs/components/Tab/Tab.tsx +++ b/src/components/Tabs/components/Tab/Tab.tsx @@ -127,7 +127,7 @@ export function Tab({ ); return ( -
  • +
  • {markup}
    • );