diff --git a/UNRELEASED.md b/UNRELEASED.md index 30d0005828b..99bf61b4aa6 100644 --- a/UNRELEASED.md +++ b/UNRELEASED.md @@ -10,6 +10,7 @@ - Fixed `DropZone` not supporting new file selection when `allowMultiple` is `false` ([#2737](https://github.com/Shopify/polaris-react/pull/2737)) ([#2737](https://github.com/Shopify/polaris-react/pull/2737)) - Fixed `DropZone` not supporting new file selection when `allowMultiple` is `false` ([#2737](https://github.com/Shopify/polaris-react/pull/2737)) - Fixed `Pagination` sizing on small screens with tooltips ([2747](https://github.com/Shopify/polaris-react/pull/2747)) +- Fixed `Popover` setting a `tabindex` and other accessibility attributes on the activator wrapper when the `activator` is disabled ([#2473](https://github.com/Shopify/polaris-react/pull/2473)) ### Documentation diff --git a/src/components/Popover/Popover.tsx b/src/components/Popover/Popover.tsx index 33c0a587e55..f1c5bd130f3 100644 --- a/src/components/Popover/Popover.tsx +++ b/src/components/Popover/Popover.tsx @@ -5,8 +5,10 @@ import React, { useState, AriaAttributes, } from 'react'; -import {findFirstFocusableNode} from '@shopify/javascript-utilities/focus'; -import {focusNextFocusableNode} from '../../utilities/focus'; +import { + findFirstFocusableNode, + focusNextFocusableNode, +} from '../../utilities/focus'; import {Portal} from '../Portal'; import {portal} from '../shared'; import {useUniqueId} from '../../utilities/unique-id'; @@ -90,9 +92,20 @@ export const Popover: React.FunctionComponent & { } const firstFocusable = findFirstFocusableNode(activatorContainer.current); - const focusableActivator = firstFocusable || activatorContainer.current; - setActivatorAttributes(focusableActivator, {id, active, ariaHaspopup}); - }, [active, ariaHaspopup, id]); + const focusableActivator: HTMLElement & { + disabled?: boolean; + } = firstFocusable || activatorContainer.current; + + const activatorDisabled = + 'disabled' in focusableActivator && Boolean(focusableActivator.disabled); + + setActivatorAttributes(focusableActivator, { + id, + active, + ariaHaspopup, + activatorDisabled, + }); + }, [id, active, ariaHaspopup]); const handleClose = (source: PopoverCloseSource) => { onClose(source); diff --git a/src/components/Popover/set-activator-attributes.ts b/src/components/Popover/set-activator-attributes.ts index 66af0cd288b..519cbe949ef 100644 --- a/src/components/Popover/set-activator-attributes.ts +++ b/src/components/Popover/set-activator-attributes.ts @@ -4,15 +4,20 @@ export function setActivatorAttributes( activator: HTMLElement, { id, - active, + active = false, ariaHaspopup, + activatorDisabled = false, }: { id: string; active: boolean; ariaHaspopup: AriaAttributes['aria-haspopup']; + activatorDisabled: boolean; }, ) { - activator.tabIndex = activator.tabIndex || 0; + if (!activatorDisabled) { + activator.tabIndex = activator.tabIndex || 0; + } + activator.setAttribute('aria-controls', id); activator.setAttribute('aria-owns', id); activator.setAttribute('aria-expanded', String(active)); diff --git a/src/components/Popover/tests/Popover.test.tsx b/src/components/Popover/tests/Popover.test.tsx index 8ea0d0e146e..8572e4886bf 100644 --- a/src/components/Popover/tests/Popover.test.tsx +++ b/src/components/Popover/tests/Popover.test.tsx @@ -21,14 +21,39 @@ describe('', () => { setActivatorAttributesSpy.mockRestore(); }); - it('invokes setActivatorAttributes with active, ariaHasPopup and id', () => { + it('invokes setActivatorAttributes with active, ariaHasPopup, activatorDisabled, and id', () => { mountWithApp( Activator} onClose={spy} />, ); expect(setActivatorAttributesSpy).toHaveBeenLastCalledWith( expect.any(Object), - {active: false, ariaHaspopup: undefined, id: 'Polarispopover1'}, + { + active: false, + ariaHaspopup: undefined, + id: 'Polarispopover1', + activatorDisabled: false, + }, + ); + }); + + it('invokes setActivatorAttributes activatorDisabled true when the activator is disabled', () => { + mountWithApp( + Activator} + onClose={spy} + />, + ); + + expect(setActivatorAttributesSpy).toHaveBeenLastCalledWith( + expect.any(Object), + { + active: false, + ariaHaspopup: undefined, + id: 'Polarispopover1', + activatorDisabled: true, + }, ); }); diff --git a/src/components/Popover/tests/set-activator-attributes.test.ts b/src/components/Popover/tests/set-activator-attributes.test.ts index 47f1a9fe042..515aa9e5894 100644 --- a/src/components/Popover/tests/set-activator-attributes.test.ts +++ b/src/components/Popover/tests/set-activator-attributes.test.ts @@ -4,31 +4,85 @@ describe('setActivatorAttributes', () => { const id = 'id'; it('applies aria-controls to the activator', () => { const div = document.createElement('div'); - setActivatorAttributes(div, {active: true, id, ariaHaspopup: true}); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: true, + activatorDisabled: false, + }); + expect(div.getAttribute('aria-controls')).toBe(id); }); it('applies aria-owns to the activator', () => { const div = document.createElement('div'); - setActivatorAttributes(div, {active: true, id, ariaHaspopup: true}); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: true, + activatorDisabled: false, + }); + expect(div.getAttribute('aria-owns')).toBe(id); }); it('applies aria-expanded to the activator', () => { const div = document.createElement('div'); - setActivatorAttributes(div, {active: true, id, ariaHaspopup: true}); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: true, + activatorDisabled: false, + }); + expect(div.getAttribute('aria-expanded')).toBe('true'); }); it('applies aria-haspopup to the activator', () => { const div = document.createElement('div'); - setActivatorAttributes(div, {active: true, id, ariaHaspopup: true}); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: true, + activatorDisabled: false, + }); + expect(div.getAttribute('aria-haspopup')).toBe('true'); }); it("does not apply aria-haspopover when it's undefined", () => { const div = document.createElement('div'); - setActivatorAttributes(div, {active: true, id, ariaHaspopup: undefined}); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: undefined, + activatorDisabled: false, + }); + expect(div.getAttribute('aria-haspopup')).toBeNull(); }); + + it('applies tabindex to the activator', () => { + const div = document.createElement('div'); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: true, + activatorDisabled: false, + }); + + expect(div.getAttribute('tabindex')).toBe('-1'); + }); + + it('does not apply tabindex when activatorDisabled is true', () => { + const div = document.createElement('div'); + setActivatorAttributes(div, { + active: true, + id, + ariaHaspopup: undefined, + activatorDisabled: true, + }); + + expect(div.getAttribute('tabindex')).toBeNull(); + }); }); diff --git a/src/utilities/focus.ts b/src/utilities/focus.ts index f01e446425d..3c482df31e8 100644 --- a/src/utilities/focus.ts +++ b/src/utilities/focus.ts @@ -34,6 +34,20 @@ export function nextFocusableNode( return null; } +// Popover needs to be able to find its activator even if it is disabled, which FOCUSABLE_SELECTOR doesn't support. + +export function findFirstFocusableNode( + element: HTMLElement, +): HTMLElement | null { + const focusableSelector = `a,button,frame,iframe,input:not([type=hidden]),select,textarea,*[tabindex]`; + + if (matches(element, focusableSelector)) { + return element; + } + + return element.querySelector(focusableSelector); +} + export function focusNextFocusableNode(node: HTMLElement, filter?: Filter) { const nextFocusable = nextFocusableNode(node, filter); if (nextFocusable && nextFocusable instanceof HTMLElement) {