Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
23 changes: 18 additions & 5 deletions src/components/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -90,9 +92,20 @@ export const Popover: React.FunctionComponent<PopoverProps> & {
}

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, {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding an activatorDisabled prop, what do you think about deriving the value rather than explicitly setting it?

// HTMLElement doesn't contain `disabled` in its interface
// We could alternatively use instanceof `focusableActivator instnaceof HTMLButton && focusableActivator.disabled`
 const activatorDisabled = (focusableActivator as HTMLButtonElement).disabled;

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I ended up going the prop route is that the Popover can't expect/know what kind of element its activator is or whether it's even focusable. The above snippet would work, but we'd need to chain all other possibilities to that conditional (it could be a textfield, etc). Since disabling a Button, TextField etc is done explicitly, just telling the Popover that the activator, whatever it may be, is disabled explicitly is a lot easier to test.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't aware that activators can be non-focusable. Maybe this is something we should try and enforce through types. Seems like a huge accessibility issue to not be able to open a popover through keyboard interactions 😬 cc/ @dpersing since your assigned as well, what do you think?

Using HTMLButtonElement type was just a quick example. There's other ways we can achieve the same type safety that'll work on any disablable element e.g.

    const focusableActivator: HTMLElement & {disabled?: boolean} =
      firstFocusable || activatorContainer.current;
    const activatorDisabled =
      'disabled' in focusableActivator && Boolean(focusableActivator.disabled);

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately adding the typing alone doesn't help. The real source of the problem is the findFirstFocusableNode utility because it eliminates disabled elements as focusable selectors.

Modifying the FOCUSABLE_SELECTORs to remove the :not(disabled) in @shopify/javascript-utilities could cause unexpected breaking changes for other consumers of the utility, so I've updated the PR to use our own version.

id,
active,
ariaHaspopup,
activatorDisabled,
});
}, [id, active, ariaHaspopup]);

const handleClose = (source: PopoverCloseSource) => {
onClose(source);
Expand Down
9 changes: 7 additions & 2 deletions src/components/Popover/set-activator-attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,20 @@ export function setActivatorAttributes(
activator: HTMLElement,
{
id,
active,
active = false,
Copy link
Member Author

@chloerice chloerice Jan 27, 2020

Choose a reason for hiding this comment

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

@dpersing Adding a default value for the active prop prevents aria-expanded being defined as undefined on line 18/24.

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));
Expand Down
29 changes: 27 additions & 2 deletions src/components/Popover/tests/Popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,39 @@ describe('<Popover />', () => {
setActivatorAttributesSpy.mockRestore();
});

it('invokes setActivatorAttributes with active, ariaHasPopup and id', () => {
it('invokes setActivatorAttributes with active, ariaHasPopup, activatorDisabled, and id', () => {
mountWithApp(
<Popover active={false} activator={<div>Activator</div>} 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(
<Popover
active={false}
activator={<button disabled>Activator</button>}
onClose={spy}
/>,
);

expect(setActivatorAttributesSpy).toHaveBeenLastCalledWith(
expect.any(Object),
{
active: false,
ariaHaspopup: undefined,
id: 'Polarispopover1',
activatorDisabled: true,
},
);
});

Expand Down
64 changes: 59 additions & 5 deletions src/components/Popover/tests/set-activator-attributes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
14 changes: 14 additions & 0 deletions src/utilities/focus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down