From 6e2d563f7418df1c7f126ee9fe36574cc563eb50 Mon Sep 17 00:00:00 2001 From: Austin Sullivan Date: Tue, 1 Aug 2023 17:02:20 -0400 Subject: [PATCH] fix(Popper): fixed flicker when opening popper content (#9339) * fix(Popper): fixed flicker when opening popper content * fixed re-focus delay when closing popover * updated DatePicker test to have consistent value --- .../__snapshots__/DataList.test.tsx.snap | 2 +- .../DatePicker/__tests__/DatePicker.test.tsx | 2 +- .../__snapshots__/DatePicker.test.tsx.snap | 143 +++++++++--------- .../__tests__/__snapshots__/Nav.test.tsx.snap | 2 +- .../src/components/Popover/Popover.tsx | 43 ++---- .../__snapshots__/SearchInput.test.tsx.snap | 16 +- .../src/components/Tooltip/Tooltip.tsx | 61 +------- .../src/helpers/FocusTrap/FocusTrap.tsx | 6 +- .../react-core/src/helpers/Popper/Popper.tsx | 107 ++++++++++++- .../react-core/src/helpers/__mocks__/util.ts | 2 + packages/react-core/src/helpers/util.ts | 11 ++ 11 files changed, 230 insertions(+), 165 deletions(-) diff --git a/packages/react-core/src/components/DataList/__tests__/__snapshots__/DataList.test.tsx.snap b/packages/react-core/src/components/DataList/__tests__/__snapshots__/DataList.test.tsx.snap index 7b8b6990649..ee9d1720bb9 100644 --- a/packages/react-core/src/components/DataList/__tests__/__snapshots__/DataList.test.tsx.snap +++ b/packages/react-core/src/components/DataList/__tests__/__snapshots__/DataList.test.tsx.snap @@ -164,7 +164,7 @@ exports[`DataList DataListAction dropdown 1`] = ` data-ouia-component-id="OUIA-Generated-Dropdown-1" data-ouia-component-type="PF5/Dropdown" data-ouia-safe="true" - style="position: absolute; left: 0px; top: 0px; z-index: 9999;" + style="position: absolute; left: 0px; top: 0px; z-index: 9999; opacity: 0; transition: opacity 0ms cubic-bezier(.54, 1.5, .38, 1.11);" >
{ test('With popover opened', async () => { const user = userEvent.setup(); - const { asFragment } = render(); + const { asFragment } = render(); await user.click(screen.getByRole('button', { name: 'Toggle date picker' })); await screen.findByRole('button', { name: 'Previous month' }); diff --git a/packages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snap b/packages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snap index e41744bb558..ca761a7a7ef 100644 --- a/packages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snap +++ b/packages/react-core/src/components/DatePicker/__tests__/__snapshots__/DatePicker.test.tsx.snap @@ -26,7 +26,7 @@ exports[`With popover opened 1`] = ` data-ouia-safe="true" placeholder="YYYY-MM-DD" type="text" - value="" + value="2020-03-17" />
@@ -59,9 +59,12 @@ exports[`With popover opened 1`] = ` aria-describedby="popover-unique_id_mock-body" aria-label="" aria-modal="true" - class="pf-v5-c-popover pf-m-no-padding pf-m-width-auto pf-m-top" + class="pf-v5-c-popover pf-m-no-padding pf-m-width-auto pf-m-bottom" + data-popper-escaped="true" + data-popper-placement="bottom" + data-popper-reference-hidden="true" role="dialog" - style="opacity: 1; transition: opacity 300ms cubic-bezier(.54, 1.5, .38, 1.11); position: absolute; left: 0px; top: 0px; z-index: 9999;" + style="position: absolute; left: 0px; top: 0px; z-index: 9999; opacity: 1; transition: opacity 300ms cubic-bezier(.54, 1.5, .38, 1.11); min-width: auto; transform: translate(0px, 25px);" >
- December + March
@@ -324,23 +327,11 @@ exports[`With popover opened 1`] = ` - - - + diff --git a/packages/react-core/src/components/Nav/__tests__/__snapshots__/Nav.test.tsx.snap b/packages/react-core/src/components/Nav/__tests__/__snapshots__/Nav.test.tsx.snap index 3488517523b..f6fb87b033c 100644 --- a/packages/react-core/src/components/Nav/__tests__/__snapshots__/Nav.test.tsx.snap +++ b/packages/react-core/src/components/Nav/__tests__/__snapshots__/Nav.test.tsx.snap @@ -1096,7 +1096,7 @@ exports[`Nav Nav List with flyout 1`] = ` data-popper-escaped="true" data-popper-placement="right-start" data-popper-reference-hidden="true" - style="position: absolute; left: 0px; top: 0px; z-index: 9999; min-width: 0px; transform: translate(0px, 0px);" + style="position: absolute; left: 0px; top: 0px; z-index: 9999; opacity: 0; transition: opacity 0ms cubic-bezier(.54, 1.5, .38, 1.11); min-width: 0px; transform: translate(0px, 0px);" >
Flyout test diff --git a/packages/react-core/src/components/Popover/Popover.tsx b/packages/react-core/src/components/Popover/Popover.tsx index ec8ccabe9bd..515eafe25ba 100644 --- a/packages/react-core/src/components/Popover/Popover.tsx +++ b/packages/react-core/src/components/Popover/Popover.tsx @@ -14,7 +14,7 @@ import popoverMaxWidth from '@patternfly/react-tokens/dist/esm/c_popover_MaxWidt import popoverMinWidth from '@patternfly/react-tokens/dist/esm/c_popover_MinWidth'; import { ReactElement } from 'react'; import { FocusTrap } from '../../helpers'; -import { Popper, getOpacityTransition } from '../../helpers/Popper/Popper'; +import { Popper } from '../../helpers/Popper/Popper'; import { getUniqueId } from '../../helpers/util'; export enum PopoverPosition { @@ -273,11 +273,7 @@ export const Popover: React.FunctionComponent = ({ const uniqueId = id || getUniqueId(); const triggerManually = isVisible !== null; const [visible, setVisible] = React.useState(false); - const [opacity, setOpacity] = React.useState(0); const [focusTrapActive, setFocusTrapActive] = React.useState(Boolean(propWithFocusTrap)); - const transitionTimerRef = React.useRef(null); - const showTimerRef = React.useRef(null); - const hideTimerRef = React.useRef(null); const popoverRef = React.useRef(null); React.useEffect(() => { @@ -294,34 +290,15 @@ export const Popover: React.FunctionComponent = ({ }, [isVisible, triggerManually]); const show = (event?: MouseEvent | KeyboardEvent, withFocusTrap?: boolean) => { event && onShow(event); - - if (transitionTimerRef.current) { - clearTimeout(transitionTimerRef.current); - } - if (hideTimerRef.current) { - clearTimeout(hideTimerRef.current); - } - showTimerRef.current = setTimeout(() => { - setVisible(true); - setOpacity(1); - propWithFocusTrap !== false && withFocusTrap && setFocusTrapActive(true); - onShown(); - }, 0); + setVisible(true); + propWithFocusTrap !== false && withFocusTrap && setFocusTrapActive(true); }; + const hide = (event?: MouseEvent | KeyboardEvent) => { event && onHide(event); - if (showTimerRef.current) { - clearTimeout(showTimerRef.current); - } - hideTimerRef.current = setTimeout(() => { - setVisible(false); - setOpacity(0); - setFocusTrapActive(false); - transitionTimerRef.current = setTimeout(() => { - onHidden(); - }, animationDuration); - }, 0); + setVisible(false); }; + const positionModifiers = { top: styles.modifiers.top, bottom: styles.modifiers.bottom, @@ -426,9 +403,7 @@ export const Popover: React.FunctionComponent = ({ onMouseDown={onContentMouseDown} style={{ minWidth: hasCustomMinWidth ? minWidth : null, - maxWidth: hasCustomMaxWidth ? maxWidth : null, - opacity, - transition: getOpacityTransition(animationDuration) + maxWidth: hasCustomMaxWidth ? maxWidth : null }} {...rest} > @@ -477,6 +452,10 @@ export const Popover: React.FunctionComponent = ({ enableFlip={enableFlip} zIndex={zIndex} flipBehavior={flipBehavior} + animationDuration={animationDuration} + onHidden={onHidden} + onShown={onShown} + onHide={() => setFocusTrapActive(false)} /> ); diff --git a/packages/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snap b/packages/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snap index 71f630b8640..6fdbf2d2730 100644 --- a/packages/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snap +++ b/packages/react-core/src/components/SearchInput/__tests__/__snapshots__/SearchInput.test.tsx.snap @@ -130,6 +130,13 @@ exports[`SearchInput advanced search 1`] = `
+
`; @@ -270,7 +277,7 @@ exports[`SearchInput advanced search with custom attributes 1`] = ` data-popper-escaped="true" data-popper-placement="bottom-start" data-popper-reference-hidden="true" - style="position: absolute; top: 0px; left: 0px; transform: translate(0px, 0px); min-width: 0px; z-index: 9999;" + style="position: absolute; top: 0px; left: 0px; transform: translate(0px, 0px); min-width: 0px; z-index: 9999; opacity: 1; transition: opacity 0ms cubic-bezier(.54, 1.5, .38, 1.11);" >
+
`; diff --git a/packages/react-core/src/components/Tooltip/Tooltip.tsx b/packages/react-core/src/components/Tooltip/Tooltip.tsx index a41a1764286..472d428a8c5 100644 --- a/packages/react-core/src/components/Tooltip/Tooltip.tsx +++ b/packages/react-core/src/components/Tooltip/Tooltip.tsx @@ -7,7 +7,7 @@ import { TooltipArrow } from './TooltipArrow'; import { KeyTypes } from '../../helpers/constants'; import tooltipMaxWidth from '@patternfly/react-tokens/dist/esm/c_tooltip_MaxWidth'; import { ReactElement } from 'react'; -import { Popper, getOpacityTransition } from '../../helpers/Popper/Popper'; +import { Popper } from '../../helpers/Popper/Popper'; export enum TooltipPosition { auto = 'auto', @@ -174,30 +174,8 @@ export const Tooltip: React.FunctionComponent = ({ const triggerOnClick = trigger.includes('click'); const triggerManually = trigger === 'manual'; const [visible, setVisible] = React.useState(false); - const [opacity, setOpacity] = React.useState(0); - const transitionTimerRef = React.useRef(null); - const showTimerRef = React.useRef(null); - const hideTimerRef = React.useRef(null); const popperRef = React.createRef(); - const prevExitDelayRef = React.useRef(); - - const clearTimeouts = (timeoutRefs: React.RefObject[]) => { - timeoutRefs.forEach((ref) => { - if (ref.current) { - clearTimeout(ref.current); - } - }); - }; - - // Cancel all timers on unmount - React.useEffect( - () => () => { - clearTimeouts([transitionTimerRef, hideTimerRef, showTimerRef]); - }, - [] - ); - const onDocumentKeyDown = (event: KeyboardEvent) => { if (!triggerManually) { if (event.key === KeyTypes.Escape && visible) { @@ -222,36 +200,11 @@ export const Tooltip: React.FunctionComponent = ({ } }, [isVisible]); - React.useEffect(() => { - if (prevExitDelayRef.current < exitDelay) { - clearTimeouts([transitionTimerRef, hideTimerRef]); - hideTimerRef.current = setTimeout(() => { - setOpacity(0); - transitionTimerRef.current = setTimeout(() => { - setVisible(false); - onTooltipHidden(); - }, animationDuration); - }, exitDelay); - } - prevExitDelayRef.current = exitDelay; - }, [exitDelay]); - const show = () => { - clearTimeouts([transitionTimerRef, hideTimerRef]); - showTimerRef.current = setTimeout(() => { - setVisible(true); - setOpacity(1); - }, entryDelay); + setVisible(true); }; const hide = () => { - clearTimeouts([showTimerRef]); - hideTimerRef.current = setTimeout(() => { - setOpacity(0); - transitionTimerRef.current = setTimeout(() => { - setVisible(false); - onTooltipHidden(); - }, animationDuration); - }, exitDelay); + setVisible(false); }; const positionModifiers = { top: styles.modifiers.top, @@ -275,9 +228,7 @@ export const Tooltip: React.FunctionComponent = ({ role="tooltip" id={id} style={{ - maxWidth: hasCustomMaxWidth ? maxWidth : null, - opacity, - transition: getOpacityTransition(animationDuration) + maxWidth: hasCustomMaxWidth ? maxWidth : null }} ref={popperRef} {...rest} @@ -342,6 +293,10 @@ export const Tooltip: React.FunctionComponent = ({ enableFlip={enableFlip} zIndex={zIndex} flipBehavior={flipBehavior} + animationDuration={animationDuration} + entryDelay={entryDelay} + exitDelay={exitDelay} + onHidden={onTooltipHidden} /> ); }; diff --git a/packages/react-core/src/helpers/FocusTrap/FocusTrap.tsx b/packages/react-core/src/helpers/FocusTrap/FocusTrap.tsx index a2a96a7d597..17b333e0aa8 100644 --- a/packages/react-core/src/helpers/FocusTrap/FocusTrap.tsx +++ b/packages/react-core/src/helpers/FocusTrap/FocusTrap.tsx @@ -54,7 +54,7 @@ class FocusTrapBase extends React.Component { componentDidUpdate(prevProps: FocusTrapProps) { if (prevProps.active && !this.props.active) { - this.focusTrap.deactivate(); + this.deactivate(); } else if (!prevProps.active && this.props.active) { this.focusTrap.activate(); } @@ -67,6 +67,10 @@ class FocusTrapBase extends React.Component { } componentWillUnmount() { + this.deactivate(); + } + + deactivate() { this.focusTrap.deactivate(); if ( this.props.focusTrapOptions.returnFocusOnDeactivate !== false && diff --git a/packages/react-core/src/helpers/Popper/Popper.tsx b/packages/react-core/src/helpers/Popper/Popper.tsx index 75cfcec7687..403e232dfd6 100644 --- a/packages/react-core/src/helpers/Popper/Popper.tsx +++ b/packages/react-core/src/helpers/Popper/Popper.tsx @@ -2,6 +2,7 @@ import * as React from 'react'; import * as ReactDOM from 'react-dom'; import { usePopper } from './thirdparty/react-popper/usePopper'; import { Placement, Modifier } from './thirdparty/popper-core'; +import { clearTimeouts } from '../util'; import { css } from '@patternfly/react-styles'; import '@patternfly/react-styles/css/components/Popper/Popper.css'; @@ -146,6 +147,30 @@ export interface PopperProps { | 'right-start' | 'right-end' )[]; + /** The duration of the CSS fade transition animation. */ + animationDuration?: number; + /** Delay in ms before the popper appears */ + entryDelay?: number; + /** Delay in ms before the popper disappears */ + exitDelay?: number; + /** Callback when popper's hide transition has finished executing */ + onHidden?: () => void; + /** + * Lifecycle function invoked when the popper begins to transition out. + */ + onHide?: () => void; + /** + * Lifecycle function invoked when the popper has been mounted to the DOM. + */ + onMount?: () => void; + /** + * Lifecycle function invoked when the popper begins to transition in. + */ + onShow?: () => void; + /** + * Lifecycle function invoked when the popper has fully transitioned in. + */ + onShown?: () => void; } export const Popper: React.FunctionComponent = ({ @@ -176,22 +201,49 @@ export const Popper: React.FunctionComponent = ({ enableFlip = true, flipBehavior = 'flip', triggerRef, - popperRef + popperRef, + animationDuration = 0, + entryDelay = 0, + exitDelay = 0, + onHidden = () => {}, + onHide = () => {}, + onMount = () => {}, + onShow = () => {}, + onShown = () => {} }) => { const [triggerElement, setTriggerElement] = React.useState(null); const [refElement, setRefElement] = React.useState(null); const [popperElement, setPopperElement] = React.useState(null); const [popperContent, setPopperContent] = React.useState(null); const [ready, setReady] = React.useState(false); + const [opacity, setOpacity] = React.useState(0); + const [internalIsVisible, setInternalIsVisible] = React.useState(isVisible); + const transitionTimerRef = React.useRef(null); + const showTimerRef = React.useRef(null); + const hideTimerRef = React.useRef(null); + const prevExitDelayRef = React.useRef(); + const refOrTrigger = refElement || triggerElement; + const showPopper = isVisible || internalIsVisible; + const onDocumentClickCallback = React.useCallback( (event: MouseEvent) => onDocumentClick(event, refOrTrigger, popperElement), - [isVisible, triggerElement, refElement, popperElement, onDocumentClick] + [showPopper, triggerElement, refElement, popperElement, onDocumentClick] ); React.useEffect(() => { setReady(true); + onMount(); }, []); + + // Cancel all timers on unmount + React.useEffect( + () => () => { + clearTimeouts([transitionTimerRef, hideTimerRef, showTimerRef]); + }, + [] + ); + React.useEffect(() => { if (triggerRef) { if ((triggerRef as React.RefObject).current) { @@ -210,7 +262,7 @@ export const Popper: React.FunctionComponent = ({ setPopperElement(popperRef()); } } - }, [isVisible, popperRef]); + }, [showPopper, popperRef]); React.useEffect(() => { // Trigger a Popper update when content changes. const observer = new MutationObserver(() => { @@ -377,6 +429,48 @@ export const Popper: React.FunctionComponent = ({ } }, [popper]); + React.useEffect(() => { + if (prevExitDelayRef.current < exitDelay) { + clearTimeouts([transitionTimerRef, hideTimerRef]); + hideTimerRef.current = setTimeout(() => { + transitionTimerRef.current = setTimeout(() => { + setInternalIsVisible(false); + }, animationDuration); + }, exitDelay); + } + prevExitDelayRef.current = exitDelay; + }, [exitDelay]); + + const show = () => { + onShow(); + clearTimeouts([transitionTimerRef, hideTimerRef]); + showTimerRef.current = setTimeout(() => { + setInternalIsVisible(true); + setOpacity(1); + onShown(); + }, entryDelay); + }; + + const hide = () => { + onHide(); + clearTimeouts([showTimerRef]); + hideTimerRef.current = setTimeout(() => { + setOpacity(0); + transitionTimerRef.current = setTimeout(() => { + setInternalIsVisible(false); + onHidden(); + }, animationDuration); + }, exitDelay); + }; + + React.useEffect(() => { + if (isVisible) { + show(); + } else { + hide(); + } + }, [isVisible]); + // Returns the CSS modifier class in order to place the Popper's arrow properly // Depends on the position of the Popper relative to the reference element const modifierFromPopperPosition = () => { @@ -392,13 +486,16 @@ export const Popper: React.FunctionComponent = ({ style: { ...((popper.props && popper.props.style) || {}), ...popperStyles.popper, - zIndex + zIndex, + opacity, + transition: getOpacityTransition(animationDuration) }, ...attributes.popper }; const getMenuWithPopper = () => { const localPopper = React.cloneElement(popper, options); + return popperRef ? ( localPopper ) : ( @@ -425,7 +522,7 @@ export const Popper: React.FunctionComponent = ({ )} {triggerRef && trigger && React.isValidElement(trigger) && trigger} - {ready && isVisible && getPopper()} + {ready && showPopper && getPopper()} ); }; diff --git a/packages/react-core/src/helpers/__mocks__/util.ts b/packages/react-core/src/helpers/__mocks__/util.ts index 6b7efa971aa..e6b3036138a 100644 --- a/packages/react-core/src/helpers/__mocks__/util.ts +++ b/packages/react-core/src/helpers/__mocks__/util.ts @@ -1 +1,3 @@ export const getUniqueId = () => 'unique_id_mock'; + +export const clearTimeouts = () => {}; diff --git a/packages/react-core/src/helpers/util.ts b/packages/react-core/src/helpers/util.ts index c2b0ed5da27..7110e3277e3 100644 --- a/packages/react-core/src/helpers/util.ts +++ b/packages/react-core/src/helpers/util.ts @@ -519,3 +519,14 @@ export const preventedEvents = (events: string[]) => }), {} ); + +/** + * @param {React.RefObject[]} timeoutRefs - Timeout refs to clear + */ +export const clearTimeouts = (timeoutRefs: React.RefObject[]) => { + timeoutRefs.forEach((ref) => { + if (ref.current) { + clearTimeout(ref.current); + } + }); +};