Skip to content

Commit

Permalink
Exposed close function on Popover (#12341)
Browse files Browse the repository at this point in the history
### WHY are these changes introduced?

Fixes #12336
Fixes #12249

### WHAT is this pull request doing?

This PR exposes a `close` function on popovers imperative handle to
handle accessibility when closing the popover from an outside source.

### WHY did you take this approach?

Originally I thought about using an effect to watch the active status,
and automatically manage focus when the popover is closed. However, we
can't guarantee that the user will always want popover to manage focus.
Perhaps, they'll self manage focus to another area of the page, or
navigate entirely.

Exposing a `close` function allows us to:
* Release this in a minor version, rather than a major one
* There's less work required to migrate as you won't need to audit all
instance of popover
* Allows for accessible focus, while allowing popover to contain the
business logic
* It allows for the flexibility of how you'll manage the user experience
when the popover is closed

### Giphy



https://github.com/Shopify/polaris/assets/24610840/7b87b030-11ee-43b2-9ea8-da61265e53f1



### How to 🎩

**Playground code**

```
import React, {useRef, useState, useEffect} from 'react';

import type {PopoverPublicAPI} from '../src';
import {Page, Button, Popover, ActionList} from '../src';

console.log('Logging active element every 15 seconds');

export const Playground = {
  tags: ['skip-tests'],
  render() {
    useEffect(() => {
      const interval = setInterval(() => {
        console.log(document.activeElement);
      }, 15000);

      return () => {
        clearInterval(interval);
      };
    }, []);

    const popoverRef = useRef<PopoverPublicAPI>(null);
    const [popoverActive, setPopoverActive] = useState(true);

    const togglePopoverActive = () =>
      setPopoverActive((popoverActive) => !popoverActive);

    const activator = (
      <Button onClick={togglePopoverActive} disclosure>
        More actions
      </Button>
    );

    return (
      <Page title="Playground">
        <Popover
          active={popoverActive}
          activator={activator}
          autofocusTarget="first-node"
          onClose={togglePopoverActive}
          ref={popoverRef}
        >
          <ActionList
            actionRole="menuitem"
            items={[
              {
                content: 'Focus activator',
                onAction() {
                  popoverRef.current?.close();
                },
              },
              {
                content: 'Focus next node',
                onAction() {
                  popoverRef.current?.close('next-node');
                },
              },
            ]}
          />
        </Popover>
        <Button>Next focusable node</Button>
      </Page>
    );
  },
};
```


### 🎩 checklist

- [ ] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
  • Loading branch information
AndrewMusgrave committed Jul 11, 2024
1 parent b0c2e12 commit f6728a4
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 32 deletions.
5 changes: 5 additions & 0 deletions .changeset/nervous-plums-end.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Exposed a `close` function on popovers imperative handle
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ describe('<Combobox />', () => {
it('calls Popover.forceUpdatePosition() when onOptionSelected is triggered and allowMultiple is true and there are children', () => {
const mockForceUpdatePosition = jest.fn();
mockUseImperativeHandle.mockImplementation(
(ref: {current: PopoverPublicAPI}) => {
(ref: {current: Partial<PopoverPublicAPI>}) => {
ref.current = {
forceUpdatePosition: mockForceUpdatePosition,
};
Expand Down
70 changes: 40 additions & 30 deletions polaris-react/src/components/Popover/Popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ export interface PopoverProps {
captureOverscroll?: boolean;
}

type CloseTarget = 'activator' | 'next-node';
export interface PopoverPublicAPI {
forceUpdatePosition(): void;
close(target?: CloseTarget): void;
}

// TypeScript can't generate types that correctly infer the typing of
Expand Down Expand Up @@ -120,36 +122,6 @@ const PopoverComponent = forwardRef<PopoverPublicAPI, PopoverProps>(
overlayRef.current?.forceUpdatePosition();
}

useImperativeHandle(ref, () => {
return {
forceUpdatePosition,
};
});

const setAccessibilityAttributes = useCallback(() => {
if (activatorContainer.current == null) {
return;
}

const firstFocusable = findFirstFocusableNodeIncludingDisabled(
activatorContainer.current,
);
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);
if (activatorContainer.current == null || preventFocusOnClose) {
Expand Down Expand Up @@ -181,6 +153,44 @@ const PopoverComponent = forwardRef<PopoverPublicAPI, PopoverProps>(
}
};

useImperativeHandle(ref, () => {
return {
forceUpdatePosition,
close: (target = 'activator') => {
const source =
target === 'activator'
? PopoverCloseSource.EscapeKeypress
: PopoverCloseSource.FocusOut;

handleClose(source);
},
};
});

const setAccessibilityAttributes = useCallback(() => {
if (activatorContainer.current == null) {
return;
}

const firstFocusable = findFirstFocusableNodeIncludingDisabled(
activatorContainer.current,
);
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]);

useEffect(() => {
if (!activatorNode && activatorContainer.current) {
setActivatorNode(
Expand Down
64 changes: 63 additions & 1 deletion polaris-react/src/components/Popover/tests/Popover.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React, {useCallback, useRef, useState} from 'react';
import {mountWithApp} from 'tests/utilities';
import {act} from 'react-dom/test-utils';

import {Portal} from '../../Portal';
import {PositionedOverlay} from '../../PositionedOverlay';
Expand Down Expand Up @@ -396,14 +397,75 @@ describe('<Popover />', () => {

mountWithApp(<Test />);

expect(popoverRef).toStrictEqual({
expect(popoverRef).toMatchObject({
current: {
forceUpdatePosition: expect.anything(),
},
});
});
});

describe('close', () => {
it('exposes a function that closes the popover & focuses the activator by default', () => {
const activatorId = 'focus-target';
let popoverRef: React.RefObject<PopoverPublicAPI> | null = null;

function Test() {
popoverRef = useRef(null);

return (
<Popover
ref={popoverRef}
active
activator={<button id={activatorId} />}
onClose={noop}
/>
);
}

const popover = mountWithApp(<Test />);

act(() => {
popoverRef?.current?.close();
});

const focusTarget = popover.find('button', {id: activatorId})!.domNode;

expect(document.activeElement).toBe(focusTarget);
});

it('exposes a function that closes the popover & focuses the next node when the next-node option is used', () => {
const nextFocusedId = 'focus-target2';
let popoverRef: React.RefObject<PopoverPublicAPI> | null = null;

function Test() {
popoverRef = useRef(null);

return (
<>
<Popover
ref={popoverRef}
active
activator={<button />}
onClose={noop}
/>
<button id={nextFocusedId} />
</>
);
}

const popover = mountWithApp(<Test />);

act(() => {
popoverRef?.current?.close('next-node');
});

const focusTarget = popover.find('button', {id: nextFocusedId})!.domNode;

expect(document.activeElement).toBe(focusTarget);
});
});

describe('captureOverscroll', () => {
const TestActivator = <button>Activator</button>;

Expand Down

0 comments on commit f6728a4

Please sign in to comment.