From a795ae7b3b99397e9d26490b829ecc005a108e6d Mon Sep 17 00:00:00 2001 From: Andreas Bergmaier <53600133+ab-pm@users.noreply.github.com> Date: Wed, 15 May 2024 20:34:32 +0200 Subject: [PATCH] Remove `componentDidMount` and `componentDidUpdate` hooks from `` * avoid calling `setState` from `componentDidUpdate`, which leads to the component rendering twice in controlled mode * instead, just ignore local state when the component is in controlled mode * assert that `onInteraction` is always called with an event * also remove `hasDarkParent` and `isClosingViaEscapeKeypress` from react component state, put them in class properties instead and update them whenever the overlay is opening/closing --- .../core/src/components/popover/popover.tsx | 79 +++++++------------ .../components/popover/popoverSharedProps.ts | 4 +- .../components/multi-select/multiSelect.tsx | 2 +- .../select/src/components/select/select.tsx | 2 +- .../select/src/components/suggest/suggest.tsx | 2 +- 5 files changed, 34 insertions(+), 55 deletions(-) diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 9f8d65335be..72e45413113 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/packages/core/src/components/popover/popover.tsx @@ -132,9 +132,7 @@ export interface PopoverProps(); + // a flag that tells us to whether the target is in dark mode, so that also the overlay gets rendered in dark mode if inside a portal + private targetHasDarkParent: boolean | undefined = false; + /** * Overlay2 transition container element ref. */ private transitionContainerElement = React.createRef(); private cancelOpenTimeout?: () => void; + + // when an ESC keypress interaction closes the overlay, we want to force-enable `shouldReturnFocusOnClose` behavior + private isClosingViaEscapeKeypress: boolean = false; // a flag that lets us detect mouse movement between the target and popover, // now that mouseleave is triggered when you cross the gap between the two. @@ -221,8 +223,6 @@ export class Popover< // Reference to the Poppper.scheduleUpdate() function, this changes every time the popper is mounted private popperScheduleUpdate?: () => Promise | null>; - private isControlled = () => this.props.isOpen !== undefined; - // arrow is disabled if minimal, or if the arrow modifier was explicitly disabled private isArrowEnabled = () => !this.props.minimal && this.props.modifiers?.arrow?.enabled !== false; @@ -239,18 +239,16 @@ export class Popover< return this.popoverElement?.querySelector(`.${Classes.POPOVER}`); } - private getIsOpen(props: PopoverProps) { + // check whether the popover is currently open, taking either the controlled props value or the state value + private isOpen(): boolean { // disabled popovers should never be allowed to open. - if (props.disabled) { - return false; - } else { - return props.isOpen ?? props.defaultIsOpen!; - } + if (this.props.disabled) return false; + return this.props.isOpen ?? this.state.isOpen; } public render() { const { disabled, content, placement, position = "auto", positioningStrategy } = this.props; - const { isOpen } = this.state; + const isOpen = this.isOpen(); const isContentEmpty = content == null || (typeof content === "string" && content.trim() === ""); if (isContentEmpty) { @@ -280,27 +278,6 @@ export class Popover< ); } - public componentDidMount() { - this.updateDarkParent(); - } - - public componentDidUpdate(props: PopoverProps, state: PopoverState) { - super.componentDidUpdate(props, state); - this.updateDarkParent(); - - const nextIsOpen = this.getIsOpen(this.props); - - if (this.props.isOpen != null && nextIsOpen !== this.state.isOpen) { - this.setOpenState(nextIsOpen); - // tricky: setOpenState calls setState only if this.props.isOpen is - // not controlled, so we need to invoke setState manually here. - this.setState({ isOpen: nextIsOpen }); - } else if (this.props.disabled && this.state.isOpen && this.props.isOpen == null) { - // special case: close an uncontrolled popover when disabled is set to true - this.setOpenState(false); - } - } - protected validateProps(props: PopoverProps & { children?: React.ReactNode }) { if (props.isOpen == null && props.onInteraction != null) { console.warn(Errors.POPOVER_WARN_UNCONTROLLED_ONINTERACTION); @@ -345,8 +322,8 @@ export class Popover< private renderTarget = ({ ref: popperChildRef }: ReferenceChildrenProps) => { const { children, className, fill, openOnTargetFocus, renderTarget } = this.props; - const { isOpen } = this.state; - const isControlled = this.isControlled(); + const isOpen = this.isOpen(); + const isControlled = this.props.isOpen != null; const isHoverInteractionKind = this.isHoverInteractionKind(); let { targetTagName } = this.props; @@ -453,7 +430,7 @@ export class Popover< private renderPopover = (popperProps: PopperChildrenProps) => { const { autoFocus, enforceFocus, backdropProps, canEscapeKeyClose, hasBackdrop, interactionKind, usePortal } = this.props; - const { isClosingViaEscapeKeypress, isOpen } = this.state; + const isOpen = this.isOpen(); // compute an appropriate transform origin so the scale animation points towards target const transformOrigin = getTransformOrigin( @@ -482,7 +459,7 @@ export class Popover< const popoverClasses = classNames( Classes.POPOVER, { - [Classes.DARK]: this.props.inheritDarkTheme && this.state.hasDarkParent, + [Classes.DARK]: this.props.inheritDarkTheme && this.targetHasDarkParent, [Classes.MINIMAL]: this.props.minimal, [Classes.POPOVER_CAPTURING_DISMISS]: this.props.captureDismiss, [Classes.POPOVER_MATCH_TARGET_WIDTH]: this.props.matchTargetWidth, @@ -497,7 +474,7 @@ export class Popover< // if hover interaction, it doesn't make sense to take over focus control const shouldReturnFocusOnClose = this.isHoverInteractionKind() ? false - : isClosingViaEscapeKeypress + : this.isClosingViaEscapeKeypress ? true : this.props.shouldReturnFocusOnClose; @@ -735,7 +712,7 @@ export class Popover< private handleTargetClick = (e: React.MouseEvent | React.KeyboardEvent) => { // Target element(s) may fire simulated click event upon pressing ENTER/SPACE, which we should ignore // see: https://github.com/palantir/blueprint/issues/5775 - const shouldIgnoreClick = this.state.isOpen && this.isSimulatedButtonClick(e); + const shouldIgnoreClick = this.isOpen() && this.isSimulatedButtonClick(e); if (!shouldIgnoreClick) { // ensure click did not originate from within inline popover before closing if (!this.props.disabled && !this.isElementInPopover(e.target as HTMLElement)) { @@ -754,7 +731,7 @@ export class Popover< // a wrapper around setState({ isOpen }) that will call props.onInteraction instead when in controlled mode. // starts a timeout to delay changing the state if a non-zero duration is provided. - private setOpenState(isOpen: boolean, e?: React.SyntheticEvent, timeout?: number) { + private setOpenState(isOpen: boolean, e: React.SyntheticEvent, timeout?: number) { // cancel any existing timeout because we have new state this.cancelOpenTimeout?.(); if (timeout !== undefined && timeout > 0) { @@ -769,20 +746,22 @@ export class Popover< } else { this.props.onInteraction?.(isOpen, e); } - if (!isOpen) { + if (isOpen) { + this.targetHasDarkParent = this.checkDarkParent(); + } else { // non-null assertion because the only time `e` is undefined is when in controlled mode // or the rare special case in uncontrolled mode when the `disabled` flag is toggled true - this.props.onClose?.(e!); - this.setState({ isClosingViaEscapeKeypress: isEscapeKeypressEvent(e?.nativeEvent) }); + this.props.onClose?.(e); + this.isClosingViaEscapeKeypress = isEscapeKeypressEvent(e?.nativeEvent); } } } - private updateDarkParent() { - if (this.props.usePortal && this.state.isOpen) { - const hasDarkParent = this.targetRef.current?.closest(`.${Classes.DARK}`) != null; - this.setState({ hasDarkParent }); + private checkDarkParent() { + if (this.props.usePortal) { + return this.targetRef.current?.closest(`.${Classes.DARK}`) != null; } + return undefined; } private isElementInPopover(element: Element) { diff --git a/packages/core/src/components/popover/popoverSharedProps.ts b/packages/core/src/components/popover/popoverSharedProps.ts index 5c82ee1d2d8..431612492fa 100644 --- a/packages/core/src/components/popover/popoverSharedProps.ts +++ b/packages/core/src/components/popover/popoverSharedProps.ts @@ -204,9 +204,9 @@ export interface PopoverSharedProps) => void; + onInteraction?: (nextOpenState: boolean, e: React.SyntheticEvent) => void; /** * Whether the popover should open when its target is focused. If `true`, diff --git a/packages/select/src/components/multi-select/multiSelect.tsx b/packages/select/src/components/multi-select/multiSelect.tsx index bf4553719a9..82ebea06215 100644 --- a/packages/select/src/components/multi-select/multiSelect.tsx +++ b/packages/select/src/components/multi-select/multiSelect.tsx @@ -332,7 +332,7 @@ export class MultiSelect extends AbstractPureComponent, M // Popover interaction kind is CLICK, so this only handles click events. // Note that we defer to the next animation frame in order to get the latest activeElement - private handlePopoverInteraction = (nextOpenState: boolean, evt?: React.SyntheticEvent) => + private handlePopoverInteraction = (nextOpenState: boolean, evt: React.SyntheticEvent) => this.requestAnimationFrame(() => { const isInputFocused = this.input === Utils.getActiveElement(this.input); diff --git a/packages/select/src/components/select/select.tsx b/packages/select/src/components/select/select.tsx index a39b4330b87..bfb23e575a4 100644 --- a/packages/select/src/components/select/select.tsx +++ b/packages/select/src/components/select/select.tsx @@ -299,7 +299,7 @@ export class Select extends AbstractPureComponent, SelectState this.props.onItemSelect?.(item, event); }; - private handlePopoverInteraction = (isOpen: boolean, event?: React.SyntheticEvent) => { + private handlePopoverInteraction = (isOpen: boolean, event: React.SyntheticEvent) => { this.setState({ isOpen }); this.props.popoverProps?.onInteraction?.(isOpen, event); }; diff --git a/packages/select/src/components/suggest/suggest.tsx b/packages/select/src/components/suggest/suggest.tsx index 9cc3b871731..e76931a846f 100644 --- a/packages/select/src/components/suggest/suggest.tsx +++ b/packages/select/src/components/suggest/suggest.tsx @@ -333,7 +333,7 @@ export class Suggest extends AbstractPureComponent, SuggestSt // Popover interaction kind is CLICK, so this only handles click events. // Note that we defer to the next animation frame in order to get the latest activeElement - private handlePopoverInteraction = (nextOpenState: boolean, event?: React.SyntheticEvent) => + private handlePopoverInteraction = (nextOpenState: boolean, event: React.SyntheticEvent) => this.requestAnimationFrame(() => { const isInputFocused = this.inputElement === Utils.getActiveElement(this.inputElement); if (this.inputElement != null && !isInputFocused) {