From f6728a410a4dbd87e7c88a46e8aa003f1a8e4697 Mon Sep 17 00:00:00 2001 From: Andrew Musgrave Date: Thu, 11 Jul 2024 11:05:31 -0400 Subject: [PATCH] Exposed `close` function on `Popover` (#12341) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### WHY are these changes introduced? Fixes https://github.com/Shopify/polaris/issues/12336 Fixes https://github.com/Shopify/polaris/issues/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(null); const [popoverActive, setPopoverActive] = useState(true); const togglePopoverActive = () => setPopoverActive((popoverActive) => !popoverActive); const activator = ( ); return ( ); }, }; ``` ### 🎩 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 --- .changeset/nervous-plums-end.md | 5 ++ .../Combobox/tests/Combobox.test.tsx | 2 +- .../src/components/Popover/Popover.tsx | 70 +++++++++++-------- .../components/Popover/tests/Popover.test.tsx | 64 ++++++++++++++++- 4 files changed, 109 insertions(+), 32 deletions(-) create mode 100644 .changeset/nervous-plums-end.md diff --git a/.changeset/nervous-plums-end.md b/.changeset/nervous-plums-end.md new file mode 100644 index 00000000000..da226357525 --- /dev/null +++ b/.changeset/nervous-plums-end.md @@ -0,0 +1,5 @@ +--- +'@shopify/polaris': minor +--- + +Exposed a `close` function on popovers imperative handle diff --git a/polaris-react/src/components/Combobox/tests/Combobox.test.tsx b/polaris-react/src/components/Combobox/tests/Combobox.test.tsx index 7b43e55154a..f6884425667 100644 --- a/polaris-react/src/components/Combobox/tests/Combobox.test.tsx +++ b/polaris-react/src/components/Combobox/tests/Combobox.test.tsx @@ -189,7 +189,7 @@ describe('', () => { 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}) => { ref.current = { forceUpdatePosition: mockForceUpdatePosition, }; diff --git a/polaris-react/src/components/Popover/Popover.tsx b/polaris-react/src/components/Popover/Popover.tsx index b0eac4cf1f0..58aeb3c68d2 100644 --- a/polaris-react/src/components/Popover/Popover.tsx +++ b/polaris-react/src/components/Popover/Popover.tsx @@ -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 @@ -120,36 +122,6 @@ const PopoverComponent = forwardRef( 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) { @@ -181,6 +153,44 @@ const PopoverComponent = forwardRef( } }; + 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( diff --git a/polaris-react/src/components/Popover/tests/Popover.test.tsx b/polaris-react/src/components/Popover/tests/Popover.test.tsx index 941ebc689ed..8c0ed16402c 100644 --- a/polaris-react/src/components/Popover/tests/Popover.test.tsx +++ b/polaris-react/src/components/Popover/tests/Popover.test.tsx @@ -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'; @@ -396,7 +397,7 @@ describe('', () => { mountWithApp(); - expect(popoverRef).toStrictEqual({ + expect(popoverRef).toMatchObject({ current: { forceUpdatePosition: expect.anything(), }, @@ -404,6 +405,67 @@ describe('', () => { }); }); + describe('close', () => { + it('exposes a function that closes the popover & focuses the activator by default', () => { + const activatorId = 'focus-target'; + let popoverRef: React.RefObject | null = null; + + function Test() { + popoverRef = useRef(null); + + return ( + } + onClose={noop} + /> + ); + } + + const popover = mountWithApp(); + + 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 | null = null; + + function Test() { + popoverRef = useRef(null); + + return ( + <> + } + onClose={noop} + /> + ;