Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

acmertz
Copy link
Contributor

@acmertz acmertz commented Dec 1, 2022

Reverts #7819, restoring the original change made in #7760 to fix #7728 - with a small tweak* to avoid a downstream accessibility issue caused by the original fix.

Please see the original PR (#7819) for more details on the issue being fixed.

*Investigation notes: The first version of this change simply added `:not([tabindex="-1"])` to the affected CSS selector string in JS. It was updated to include `:not([aria-disabled])` based on [this review comment](https://github.com//pull/7760#discussion_r1026916700), but this ultimately caused some Storybook accessibility failures in CI in web and prompted the original change to be reverted.

After creating a few snapshots with variations on this change, I narrowed down the issue to being caused by the addition of :not([aria-disabled]) to the FOCUSABLE_SELECTOR constant in focus.ts:

https://github.com/Shopify/polaris/blob/35ab232b5619b691fcfe4686d504b9b470836b2f/polaris-react/src/utilities/focus.ts#L8-L9

This caused failures like this to occur in web upon upgrading to the newest version of Polaris:

Error: Elements must only use allowed ARIA attributes (aria-allowed-attr)
Affected node(s):
    <div tabindex="0" aria-describedby="PolarisTooltipContent1" data-polaris-tooltip-activator="true" aria-controls="Polarispopover1" aria-owns="Polarispopover1" aria-expanded="false">
    <div tabindex="0" aria-describedby="PolarisTooltipContent5" data-polaris-tooltip-activator="true" aria-controls="Polarispopover2" aria-owns="Polarispopover2" aria-expanded="false">
    <div tabindex="0" aria-describedby="PolarisTooltipContent6" data-polaris-tooltip-activator="true" aria-controls="Polarispopover3" aria-owns="Polarispopover3" aria-expanded="false">
    <div tabindex="0" aria-describedby="PolarisTooltipContent9" data-polaris-tooltip-activator="true" aria-controls="Polarispopover4" aria-owns="Polarispopover4" aria-expanded="false">
  For help resolving this see: https://dequeuniversity.com/rules/axe/4.4/aria-allowed-attr?application=axe-puppeteer

It appears that the test wasn't happy with some of the ARIA attributes being added to the affected <div> elements. The aria-controls, aria-owns, and aria-expanded attributes are applied in set-activator-attributes.ts by the Popover component, which calls the focusNextFocusableNode() helper in focus.ts.

I wasn't able to find the exact reason why this caused the CI failures above, but my hypothesis right now is that the original change in #7819 caused a scenario where setActivatorAttributes() could be called for an element which lacks a role that allows it to use aria-controls, aria-owns, and aria-expanded - but the way in which the <Popover> manages its activator and wrapper elements with multiple refs, useEffect() calls, and state variables makes this tricky to confirm (coupled with the fact that the CI failure occurred in another repository, of course).

Update: The issue occurred because the selector in the original PR, :not([aria-disabled]), incorrectly excluded elements that had aria-disabled="false". Changing the selector to :not([aria-disabled="true"]) fixes the Storybook accessibility failure that prompted the initial revert.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React, {useState} from 'react';

import {Button, Modal, Page, Stack, TextField} from '../src';

export function Playground() {
  const [isModalOpen, setIsModalOpen] = useState(false);

  function handleClose() {
    setIsModalOpen(false);
  }

  function noop() {}

  return (
    <Page title="Playground">
      <Button onClick={() => setIsModalOpen(true)}>Open modal</Button>
      <Button disabled>Disabled button</Button>

      <Modal
        open={isModalOpen}
        onClose={handleClose}
        title="Example modal"
        primaryAction={{
          content: 'Primary action',
          onAction: handleClose,
          disabled: true,
        }}
      >
        <Modal.Section>
          <Stack vertical>
            <p>Some content</p>
            <TextField
              onChange={noop}
              autoComplete=""
              label="Text field 1"
              value=""
            />
            <TextField
              onChange={noop}
              autoComplete=""
              label="Text field 2"
              value=""
            />
          </Stack>
        </Modal.Section>
      </Modal>
    </Page>
  );
}
  1. Copy the above code into Playground.tsx and run yarn dev
  2. Confirm that the changes in this PR fix [Modal] Focus isn't trapped in the modal when the primary action is disabled #7728:
    1. Click "Open modal"
    2. Use the Tab key to cycle through focusable elements within the modal
    3. Confirm that focus wraps around to the beginning of the modal despite the primary action button being disabled

🎩 checklist

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

size-limit report 📦

Path Size
polaris-react-cjs 211.11 KB (+0.01% 🔺)
polaris-react-esm 136.33 KB (+0.01% 🔺)
polaris-react-esnext 192.18 KB (+0.01% 🔺)
polaris-react-css 42.12 KB (0%)

@acmertz
Copy link
Contributor Author

acmertz commented Dec 1, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2022

🫰✨ Thanks @acmertz! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221201223726
yarn add @shopify/polaris@0.0.0-snapshot-release-20221201223726

@acmertz
Copy link
Contributor Author

acmertz commented Dec 5, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

🫰✨ Thanks @acmertz! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221205180056
yarn add @shopify/polaris@0.0.0-snapshot-release-20221205180056

@acmertz
Copy link
Contributor Author

acmertz commented Dec 5, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

🫰✨ Thanks @acmertz! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221205194321
yarn add @shopify/polaris@0.0.0-snapshot-release-20221205194321

@acmertz
Copy link
Contributor Author

acmertz commented Dec 5, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

🫰✨ Thanks @acmertz! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221205223818
yarn add @shopify/polaris@0.0.0-snapshot-release-20221205223818

@acmertz
Copy link
Contributor Author

acmertz commented Dec 7, 2022

/snapit

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022

🫰✨ Thanks @acmertz! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221207154429
yarn add @shopify/polaris@0.0.0-snapshot-release-20221207154429

acmertz and others added 5 commits December 8, 2022 19:37
…index="-1"` in focus helpers Fixes #7728""

This reverts commit 839a26d.
Allows us to select buttons that would be excluded by just using `:not([aria-disabled])`
@acmertz acmertz force-pushed the revert-7819-revert-7760-acmertz/fix-focus-helpers-disabled-buttons branch from f6ca5ed to 733ef44 Compare December 8, 2022 19:41
@acmertz acmertz marked this pull request as ready for review December 8, 2022 19:42
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌🏽 :shipit:

@acmertz acmertz merged commit 51fcf51 into main Jan 5, 2023
@acmertz acmertz deleted the revert-7819-revert-7760-acmertz/fix-focus-helpers-disabled-buttons branch January 5, 2023 22:20
kyledurand pushed a commit that referenced this pull request Jan 6, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@10.18.0

### Minor Changes

- [#7992](#7992)
[`e8f74f4cd`](e8f74f4)
Thanks [@aveline](https://github.com/aveline)! - Added support for
`outline` to `Box`

### Patch Changes

- [#7988](#7988)
[`382784f4e`](382784f)
Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing
on ChoiceList children


- [#7899](#7899)
[`930f077eb`](930f077)
Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where
Tooltips nested in Scrollable containers sometimes don't update their
positions correctly


- [#7831](#7831)
[`47487ee0c`](47487ee)
Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus
helper functions to no longer treat buttons with `aria-disabled="true"`
and `tabindex="-1" (but no`disabled\` attribute) as focusable.

## @shopify/plugin-polaris@0.0.25



## polaris.shopify.com@0.28.2

### Patch Changes

- Updated dependencies
\[[`382784f4e`](382784f),
[`930f077eb`](930f077),
[`47487ee0c`](47487ee),
[`e8f74f4cd`](e8f74f4)]:
    -   @shopify/polaris@10.18.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@acmertz acmertz mentioned this pull request Apr 20, 2023
5 tasks
juzser pushed a commit to juzser/polaris that referenced this pull request Jul 27, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@10.18.0

### Minor Changes

- [Shopify#7992](Shopify#7992)
[`e8f74f4cd`](Shopify@e8f74f4)
Thanks [@aveline](https://github.com/aveline)! - Added support for
`outline` to `Box`

### Patch Changes

- [Shopify#7988](Shopify#7988)
[`382784f4e`](Shopify@382784f)
Thanks [@kyledurand](https://github.com/kyledurand)! - Reduced spacing
on ChoiceList children


- [Shopify#7899](Shopify#7899)
[`930f077eb`](Shopify@930f077)
Thanks [@jeradg](https://github.com/jeradg)! - Fixed a bug where
Tooltips nested in Scrollable containers sometimes don't update their
positions correctly


- [Shopify#7831](Shopify#7831)
[`47487ee0c`](Shopify@47487ee)
Thanks [@acmertz](https://github.com/acmertz)! - Updated the focus
helper functions to no longer treat buttons with `aria-disabled="true"`
and `tabindex="-1" (but no`disabled\` attribute) as focusable.

## @shopify/plugin-polaris@0.0.25



## polaris.shopify.com@0.28.2

### Patch Changes

- Updated dependencies
\[[`382784f4e`](Shopify@382784f),
[`930f077eb`](Shopify@930f077),
[`47487ee0c`](Shopify@47487ee),
[`e8f74f4cd`](Shopify@e8f74f4)]:
    -   @shopify/polaris@10.18.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Modal] Focus isn't trapped in the modal when the primary action is disabled
3 participants