From a43fda98d98a22c1908afc0089262b0a0fbd3542 Mon Sep 17 00:00:00 2001 From: rstachof Date: Fri, 6 Sep 2024 21:53:32 -0400 Subject: [PATCH] fix(Popover): add some logic to Popover to manage focus on blur --- src/components/Popover/LazyTippy.tsx | 1 - src/components/Popover/Popover.stories.tsx | 67 +++++++++++++++---- src/components/Popover/Popover.tsx | 21 +++--- src/components/Popover/Popover.types.ts | 6 +- src/components/Popover/Popover.unit.test.tsx | 60 ++++++++++++++++- .../tippy-plugins/hideOnBlurPlugin.test.ts | 8 ++- .../Popover/tippy-plugins/hideOnBlurPlugin.ts | 10 +-- .../Popover/tippy-plugins/hideOnEscPlugin.ts | 1 - 8 files changed, 139 insertions(+), 35 deletions(-) diff --git a/src/components/Popover/LazyTippy.tsx b/src/components/Popover/LazyTippy.tsx index e0028f44e1..1f8de095c4 100644 --- a/src/components/Popover/LazyTippy.tsx +++ b/src/components/Popover/LazyTippy.tsx @@ -6,7 +6,6 @@ import Tippy, { TippyProps } from '@tippyjs/react'; export type LazyTippyProps = TippyProps & { setInstance?: (instance?: TippyInstance) => void; continuePropagationOnTrigger?: boolean; - hasRelatedTarget?: boolean; }; /** diff --git a/src/components/Popover/Popover.stories.tsx b/src/components/Popover/Popover.stories.tsx index 71a5a20f77..b7e2a2eba7 100644 --- a/src/components/Popover/Popover.stories.tsx +++ b/src/components/Popover/Popover.stories.tsx @@ -490,15 +490,65 @@ Common.parameters = { }; const WithMeetingListItemWithAvatarWithPopover = Template((args) => { + return ( + + {}} + initials="AB" + > + Hover or click me! + + } + > +
+ test 1 + test 2 + test 3 +
+ + test + + } + trigger="click" + interactive + > +
+ test 4 + test 5 + test 6 +
+ + ); +}).bind({}); + +WithMeetingListItemWithAvatarWithPopover.argTypes = { ...argTypes }; + +WithMeetingListItemWithAvatarWithPopover.args = { + trigger: 'mouseenter', + placement: PLACEMENTS.TOP, + showArrow: true, + interactive: true, + appendTo: () => document.querySelector('#theme-provider'), +}; + +const WithHideOnBlur = Template((args) => { return ( <> -
+
test 1 } trigger="click" + focusBackOnTrigger interactive hideOnBlur disableFocusLock @@ -510,13 +560,14 @@ const WithMeetingListItemWithAvatarWithPopover = Template((args) =
-
+
test 2 } trigger="click" + focusBackOnTrigger interactive hideOnBlur disableFocusLock @@ -533,15 +584,6 @@ const WithMeetingListItemWithAvatarWithPopover = Template((args) = }).bind({}); -WithMeetingListItemWithAvatarWithPopover.argTypes = { ...argTypes }; - -WithMeetingListItemWithAvatarWithPopover.args = { - trigger: 'mouseenter', - placement: PLACEMENTS.TOP, - showArrow: true, - interactive: true, -}; - export { Example, InteractiveContent, @@ -558,4 +600,5 @@ export { WithSearchInput, WithMeetingListItemWithButtonsWithPopoverInList, WithMeetingListItemWithAvatarWithPopover, -}; + WithHideOnBlur +}; \ No newline at end of file diff --git a/src/components/Popover/Popover.tsx b/src/components/Popover/Popover.tsx index 86a6b25641..1316245a5b 100644 --- a/src/components/Popover/Popover.tsx +++ b/src/components/Popover/Popover.tsx @@ -16,8 +16,6 @@ import { addTippyPlugins } from './Popover.utils'; import { v4 as uuidV4 } from 'uuid'; import { PopoverInstance } from '.'; -type CustomInstance = PopoverInstance & { props: Props } & {hasRelatedTarget?: boolean;}; - /** * The Popover component allows adding a Popover to whatever provided * `triggerComponent`. It will show the Popover after a specific event, which is @@ -86,7 +84,7 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { ? DEFAULTS.FOCUS_BACK_ON_TRIGGER_COMPONENT_INTERACTIVE : DEFAULTS.FOCUS_BACK_ON_TRIGGER_COMPONENT_NON_INTERACTIVE); - const popoverInstance = React.useRef(undefined); + const popoverInstance = React.useRef(undefined); const generatedTriggerIdRef = useRef(uuidV4()); const generatedTriggerId = generatedTriggerIdRef.current; @@ -107,11 +105,16 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { const arrowId = React.useMemo(() => `${ARROW_ID}${uuidV4()}`, []); const popoverSetInstance = useCallback( - (instance?: CustomInstance) => { + (instance?: PopoverInstance) => { + if (instance) { + // initialize to whatever prop value is + // from now on, will be controlled by hideOnBlur plugin + instance.shouldFocusTrigger = focusBackOnTrigger; + } popoverInstance.current = instance; setInstance?.(instance); }, - [setInstance] + [setInstance, focusBackOnTrigger] ); const handleOnCloseButtonClick = useCallback(() => { @@ -121,12 +124,15 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { // needs special handling since FocusScope doesn't work with the Popover from Tippy // needs to focus back to the reference item when the popover is completely hidden const handleOnPopoverHidden = useCallback(() => { - if (focusBackOnTrigger && !popoverInstance?.current?.hasRelatedTarget) { + // When the popover hides, the focus goes to the next focusable element by default. Except if focusBackOnTrigger popover prop is true AND shouldFocusTrigger (determined by hideOnBlurPlugin) is true. + // shouldFocusTrigger is true when the focusout event has no relatedTarget, that is, focusOut was triggered by Esc or Click. + + if (focusBackOnTrigger && popoverInstance?.current?.shouldFocusTrigger) { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore popoverInstance.current?.reference?.focus(); } - }, [focusBackOnTrigger, popoverInstance?.current?.hasRelatedTarget]); + }, [focusBackOnTrigger, popoverInstance?.current?.shouldFocusTrigger]); useEffect(() => { firstFocusElement?.focus(); @@ -251,7 +257,6 @@ const Popover = forwardRef((props: Props, ref: ForwardedRef) => { }} setInstance={popoverSetInstance} zIndex={zIndex} - hasRelatedTarget={false} > {clonedTriggerComponent} diff --git a/src/components/Popover/Popover.types.ts b/src/components/Popover/Popover.types.ts index b6f4373d48..951b4acd8c 100644 --- a/src/components/Popover/Popover.types.ts +++ b/src/components/Popover/Popover.types.ts @@ -14,10 +14,12 @@ export type TriggerType = TippyProps['trigger']; export type PositioningStrategy = TippyProps['popperOptions']['strategy']; export type AppendToType = TippyProps['appendTo']; +export type CustomTippyPopoverProperties = { shouldFocusTrigger?: boolean }; + /** * Popover instance interface abstracted from Tippy.js */ -export type PopoverInstance = Instance; +export type PopoverInstance = Instance & CustomTippyPopoverProperties; export type PopoverCommonStyleProps = { /** @@ -242,6 +244,4 @@ export interface Props extends PopoverCommonStyleProps, Partial * @default `false` */ disableFocusLock?: boolean; - - hasRelatedTarget?: boolean; } diff --git a/src/components/Popover/Popover.unit.test.tsx b/src/components/Popover/Popover.unit.test.tsx index c913823cd3..9625a3f65a 100644 --- a/src/components/Popover/Popover.unit.test.tsx +++ b/src/components/Popover/Popover.unit.test.tsx @@ -2,7 +2,6 @@ import React, { FC, useCallback, useState } from 'react'; import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import '@testing-library/jest-dom'; - import ButtonSimple from '../ButtonSimple'; import Popover from './'; @@ -1194,6 +1193,65 @@ describe('', () => { }); }); + it('should not focus back on the trigger element when popover is hidden and shouldFocusTrigger is false', async () => { + const user = userEvent.setup(); + + render( + <> +
+ Click Me!} + interactive + hideOnBlur + disableFocusLock + focusBackOnTrigger + setInstance={(instance: PopoverInstance) => { + if (instance) { + instance.shouldFocusTrigger = false; + } + }} + > + + +
+
+ +
+ + ); + + checkRef(screen.getByRole('button', { name: 'Click Me!' })); + + // // assert no popover on screen + const contentBeforeClick = screen.queryByText('Content'); + expect(contentBeforeClick).not.toBeInTheDocument(); + + // // after click, popover should be shown + await openPopoverByClickingOnTriggerAndCheckContent(user); + + + // goes to listitem + await user.keyboard('{Tab}'); + + // tabs out of popover + await user.keyboard('{Tab}'); + + // Wait for the popover to be hidden + await waitFor(() => { + expect(screen.queryByText('Content')).not.toBeInTheDocument(); + }); + + // Check if the focus is not back on the trigger element + const triggerElement = screen.getByRole('button', { name: /click me!/i }); + expect(document.activeElement).not.toEqual(triggerElement); + + // Check if the focus is on the next focusable element + const anotherButton = screen.getByRole('button', { name: /another button/i }); + expect(document.activeElement).toEqual(anotherButton); + }); + + it('it should focus on the trigger component when focusBackOnTrigger= = true and popover gets closed', async () => { expect.assertions(6); const user = userEvent.setup(); diff --git a/src/components/Popover/tippy-plugins/hideOnBlurPlugin.test.ts b/src/components/Popover/tippy-plugins/hideOnBlurPlugin.test.ts index 82c002b5e6..780e84352f 100644 --- a/src/components/Popover/tippy-plugins/hideOnBlurPlugin.test.ts +++ b/src/components/Popover/tippy-plugins/hideOnBlurPlugin.test.ts @@ -54,6 +54,7 @@ describe('hideOnBlurPlugin', () => { }); popoverInstance.popper.dispatchEvent(focusOutEvent); + expect(popoverInstance.shouldFocusTrigger).toEqual(false); expect(popoverInstance.hide).toHaveBeenCalled(); }); @@ -70,6 +71,7 @@ describe('hideOnBlurPlugin', () => { }); popoverInstance.popper.dispatchEvent(focusOutEvent); + expect(popoverInstance.shouldFocusTrigger).toEqual(false); expect(popoverInstance.hide).not.toHaveBeenCalled(); }); @@ -88,11 +90,12 @@ describe('hideOnBlurPlugin', () => { }); popoverInstance.popper.dispatchEvent(focusOutEvent); + expect(popoverInstance.shouldFocusTrigger).toEqual(false); expect(popoverInstance.hide).not.toHaveBeenCalled(); }); - it('should not hide popover if relatedTarget is null', async () => { + it('should hide popover if relatedTarget is null', async () => { const { hideOnBlurPlugin } = await import('./hideOnBlurPlugin'); const popoverInstance = createPopoverInstance(); const plugin = hideOnBlurPlugin.fn(popoverInstance); @@ -103,7 +106,8 @@ describe('hideOnBlurPlugin', () => { }); popoverInstance.popper.dispatchEvent(focusOutEvent); + expect(popoverInstance.shouldFocusTrigger).toEqual(true); - expect(popoverInstance.hide).not.toHaveBeenCalled(); + expect(popoverInstance.hide).toHaveBeenCalled(); }); }); diff --git a/src/components/Popover/tippy-plugins/hideOnBlurPlugin.ts b/src/components/Popover/tippy-plugins/hideOnBlurPlugin.ts index 2ee1a71ce1..d667be1b5a 100644 --- a/src/components/Popover/tippy-plugins/hideOnBlurPlugin.ts +++ b/src/components/Popover/tippy-plugins/hideOnBlurPlugin.ts @@ -6,20 +6,16 @@ export interface PopperBlurPluginProps extends Props { isChildPopoverOpen?: boolean; } -type CustomInstance = PopoverInstance & { props: PopperBlurPluginProps } & { - hasRelatedTarget?: boolean; -}; - export const hideOnBlurPlugin: Plugin = { name: 'isChildPopoverOpen', defaultValue: false, - fn(instance: CustomInstance) { + fn(instance: PopoverInstance & { props: PopperBlurPluginProps }) { const focusOutHandler = (event) => { - instance.hasRelatedTarget = !!event.relatedTarget; + // if it doesn't have a related target (ie: Esc, or click, should focus back on trigger) + instance.shouldFocusTrigger = !event.relatedTarget; if ( !instance.props.isChildPopoverOpen && - event.relatedTarget && !instance.popper.contains(event.relatedTarget as Element) ) { instance.hide(); diff --git a/src/components/Popover/tippy-plugins/hideOnEscPlugin.ts b/src/components/Popover/tippy-plugins/hideOnEscPlugin.ts index dd108cb168..0041165d4b 100644 --- a/src/components/Popover/tippy-plugins/hideOnEscPlugin.ts +++ b/src/components/Popover/tippy-plugins/hideOnEscPlugin.ts @@ -7,7 +7,6 @@ const openedTippyInstances: TippyInstance[] = []; // hide the last opened popover when Escape key is pressed function onKeyDown(event: KeyboardEvent) { if (event.key === 'Escape' && openedTippyInstances.length !== 0) { - // RS_TODO: i think the better thing to do would be set custom attri on instance here const lastIdx = openedTippyInstances.length - 1; openedTippyInstances[lastIdx].hide(); }