[core] fix(Popover): 'lazy' support; add tests for 'shouldReturnFocusOnClose'

packages/core/src/components/popover/popover.tsx

@@ -111,7 +111,7 @@ export interface PopoverProps<TProps extends DefaultPopoverTargetHTMLProps = Def
      * Whether the application should return focus to the last active element in the
      * document after this popover closes.
      *
-     * This is automatically set to `false` if this is a hover interaction popover.
+     * This is automatically set (overridden) to `false` for hover interaction popovers.
      *
      * If you are attaching a popover _and_ a tooltip to the same target, you must take
      * care to either disable this prop for the popover _or_ disable the tooltip's
@@ -439,7 +439,16 @@ export class Popover<
     };
 
     private renderPopover = (popperProps: PopperChildrenProps) => {
-        const { interactionKind, shouldReturnFocusOnClose, usePortal } = this.props;
+        const {
+            autoFocus,
+            enforceFocus,
+            backdropProps,
+            canEscapeKeyClose,
+            hasBackdrop,
+            interactionKind,
+            shouldReturnFocusOnClose,
+            usePortal,
+        } = this.props;
         const { isOpen } = this.state;
 
         // compute an appropriate transform origin so the scale animation points towards target
@@ -484,22 +493,23 @@ export class Popover<
 
         return (
             <Overlay
-                autoFocus={this.props.autoFocus ?? defaultAutoFocus}
+                autoFocus={autoFocus ?? defaultAutoFocus}
                 backdropClassName={Classes.POPOVER_BACKDROP}
-                backdropProps={this.props.backdropProps}
-                canEscapeKeyClose={this.props.canEscapeKeyClose}
-                canOutsideClickClose={this.props.interactionKind === PopoverInteractionKind.CLICK}
-                enforceFocus={this.props.enforceFocus}
-                hasBackdrop={this.props.hasBackdrop}
+                backdropProps={backdropProps}
+                canEscapeKeyClose={canEscapeKeyClose}
+                canOutsideClickClose={interactionKind === PopoverInteractionKind.CLICK}
+                enforceFocus={enforceFocus}
+                hasBackdrop={hasBackdrop}
                 isOpen={isOpen}
+                lazy={this.props.lazy}
                 onClose={this.handleOverlayClose}
                 onClosed={this.props.onClosed}
                 onClosing={this.props.onClosing}
                 onOpened={this.props.onOpened}
                 onOpening={this.props.onOpening}
                 transitionDuration={this.props.transitionDuration}
                 transitionName={Classes.POPOVER}
-                usePortal={this.props.usePortal}
+                usePortal={usePortal}
                 portalClassName={this.props.portalClassName}
                 portalContainer={this.props.portalContainer}
                 portalStopPropagationEvents={this.props.portalStopPropagationEvents}

packages/core/test/popover/popoverTests.tsx

@@ -239,6 +239,64 @@ describe("<Popover>", () => {
         });
     });
 
+    describe("focus management when shouldReturnFocusOnClose={true}", () => {
+        const targetClassName = "test-target";
+        const commonProps: Partial<PopoverProps> = {
+            className: targetClassName,
+            interactionKind: PopoverInteractionKind.CLICK,
+            openOnTargetFocus: false,
+            popoverClassName: Classes.POPOVER_CONTENT_SIZING,
+            shouldReturnFocusOnClose: true,
+            transitionDuration: 0,
+            usePortal: true,
+        };
+
+        it("moves focus to overlay when opened", done => {
+            function handleOpened() {
+                assert.notEqual(document.activeElement, document.body, "body element should not have focus");
+                assert.isTrue(
+                    document.activeElement?.closest(`.${Classes.OVERLAY}`) !== null,
+                    "focus should be inside overlay",
+                );
+                done();
+            }
+
+            wrapper = renderPopover({ ...commonProps, onOpened: handleOpened });
+            wrapper.focusTargetButton();
+            wrapper.simulateTarget("click");
+        });
+
+        it("returns focus to target element when closed", done => {
+            function handleClosed(wrapper2: PopoverWrapper) {
+                wrapper2.assertIsOpen(false);
+                assert.notEqual(document.activeElement, document.body, "body element should not have focus");
+                assert.isTrue(
+                    document.activeElement?.closest(`.${targetClassName}`) != null,
+                    "focus should be on target",
+                );
+            }
+
+            wrapper = renderPopover(commonProps);
+            wrapper.focusTargetButton();
+            assert.strictEqual(
+                document.activeElement,
+                wrapper.targetElement.querySelector("button"),
+                "button should have document focus",
+            );
+
+            wrapper.simulateTarget("click");
+            // wait for it to open, then click again to close
+            wrapper.then(
+                w => {
+                    w.assertIsOpen();
+                    w.simulateTarget("click");
+                },
+                // wait for it to close
+                () => wrapper?.then(handleClosed, done),
+            );
+        });
+    });
+
     describe("openOnTargetFocus", () => {
         describe("if true (default)", () => {
             it('adds tabindex="0" to target\'s child node when interactionKind is HOVER', () => {
@@ -297,10 +355,9 @@ describe("<Popover>", () => {
                     interactionKind: "hover",
                     usePortal: true,
                 });
-                const targetElement = wrapper.findClass(Classes.POPOVER_TARGET);
-                targetElement.simulate("focus");
-                targetElement.simulate("blur");
-                assert.isTrue(wrapper.state("isOpen"));
+                wrapper.simulateTarget("focus");
+                wrapper.simulateTarget("blur");
+                wrapper.assertIsOpen();
             });
         });
 
@@ -837,7 +894,9 @@ describe("<Popover>", () => {
         assertFindClass(className: string, expected?: boolean, msg?: string): this;
         assertIsOpen(isOpen?: boolean): this;
         assertOnInteractionCalled(called?: boolean): this;
+        focusTargetButton(): this;
         simulateContent(eventName: string, ...args: any[]): this;
+        /** Careful: simulating "focus" is unsupported by Enzyme, see https://stackoverflow.com/a/56892875/7406866 */
         simulateTarget(eventName: string, ...args: any[]): this;
         findClass(className: string): ReactWrapper<React.HTMLAttributes<HTMLElement>, any>;
         sendEscapeKey(): this;
@@ -872,11 +931,11 @@ describe("<Popover>", () => {
         };
         wrapper.assertIsOpen = (isOpen = true, index = 0) => {
             const overlay = wrapper!.find(Overlay).at(index);
-            assert.equal(overlay.prop("isOpen"), isOpen, "assertIsOpen");
+            assert.equal(overlay.prop("isOpen"), isOpen, "PopoverWrapper#assertIsOpen()");
             return wrapper!;
         };
         wrapper.assertOnInteractionCalled = (called = true) => {
-            assert.strictEqual(onInteractionSpy.called, called, "assertOnInteractionCalled");
+            assert.strictEqual(onInteractionSpy.called, called, "PopoverWrapper#assertOnInteractionCalled()");
             return wrapper!;
         };
         wrapper.findClass = (className: string) => wrapper!.find(`.${className}`).hostNodes();
@@ -888,6 +947,10 @@ describe("<Popover>", () => {
             wrapper!.findClass(Classes.POPOVER_TARGET).simulate(eventName, ...args);
             return wrapper!;
         };
+        wrapper.focusTargetButton = () => {
+            wrapper!.find(`[data-testid="target-button"]`).hostNodes().getDOMNode<HTMLElement>().focus();
+            return wrapper!;
+        };
         wrapper.sendEscapeKey = () => {
             wrapper!.findClass(Classes.OVERLAY_OPEN).simulate("keydown", {
                 key: "Escape",

packages/docs-app/src/examples/core-examples/popoverExample.tsx

@@ -21,6 +21,7 @@ import {
     Button,
     Classes,
     Code,
+    Divider,
     FormGroup,
     H5,
     HTMLSelect,
@@ -71,8 +72,10 @@ export interface PopoverExampleState {
     matchTargetWidth: boolean;
     minimal?: boolean;
     modifiers?: PopperModifierOverrides;
+    openOnTargetFocus: boolean;
     placement?: Placement;
     rangeSliderValue?: NumberRange;
+    shouldReturnFocusOnClose: boolean;
     sliderValue?: number;
     usePortal?: boolean;
 }
@@ -97,8 +100,10 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
             flip: { enabled: true },
             preventOverflow: { enabled: true },
         },
+        openOnTargetFocus: true,
         placement: "auto",
         rangeSliderValue: [0, 10],
+        shouldReturnFocusOnClose: false,
         sliderValue: 5,
         usePortal: true,
     };
@@ -139,6 +144,12 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
 
     private toggleMinimal = handleBooleanChange(minimal => this.setState({ minimal }));
 
+    private toggleOpenOnTargetFocus = handleBooleanChange(openOnTargetFocus => this.setState({ openOnTargetFocus }));
+
+    private toggleShouldReturnFocusOnClose = handleBooleanChange(shouldReturnFocusOnClose =>
+        this.setState({ openOnTargetFocus: shouldReturnFocusOnClose ? false : undefined, shouldReturnFocusOnClose }),
+    );
+
     private toggleUsePortal = handleBooleanChange(usePortal => {
         if (usePortal) {
             this.setState({ hasBackdrop: false, inheritDarkTheme: false });
@@ -168,8 +179,9 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
                 <div className="docs-popover-example-scroll" ref={this.centerScroll}>
                     <Popover
                         popoverClassName={exampleIndex <= 2 ? Classes.POPOVER_CONTENT_SIZING : ""}
-                        portalClassName="foo"
+                        portalClassName="docs-popover-example-portal"
                         {...popoverProps}
+                        content={this.getContents(exampleIndex)}
                         boundary={
                             boundary === "scrollParent"
                                 ? this.scrollParentElement ?? undefined
@@ -179,7 +191,6 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
                         }
                         enforceFocus={false}
                         isOpen={this.state.isControlled ? this.state.isOpen : undefined}
-                        content={this.getContents(exampleIndex)}
                     >
                         <Button intent={Intent.PRIMARY} text={buttonText} tabIndex={0} />
                     </Popover>
@@ -194,12 +205,14 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
     }
 
     private renderOptions() {
-        const { matchTargetWidth, modifiers, placement } = this.state;
+        const { interactionKind, matchTargetWidth, modifiers, placement } = this.state;
         const { arrow, flip, preventOverflow } = modifiers;
 
         // popper.js requires this modiifer for "auto" placement
         const forceFlipEnabled = placement.startsWith("auto");
 
+        const isHoverInteractionKind = interactionKind === "hover" || interactionKind === "hover-target";
+
         return (
             <>
                 <H5>Appearance</H5>
@@ -208,11 +221,7 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
                     label="Position when opened"
                     labelFor="position"
                 >
-                    <HTMLSelect
-                        value={this.state.placement}
-                        onChange={this.handlePlacementChange}
-                        options={PopperPlacements}
-                    />
+                    <HTMLSelect value={placement} onChange={this.handlePlacementChange} options={PopperPlacements} />
                 </FormGroup>
                 <FormGroup label="Example content">
                     <HTMLSelect value={this.state.exampleIndex} onChange={this.handleExampleIndexChange}>
@@ -241,15 +250,28 @@ export class PopoverExample extends React.PureComponent<ExampleProps, PopoverExa
                 <H5>Interactions</H5>
                 <RadioGroup
                     label="Interaction kind"
-                    selectedValue={this.state.interactionKind.toString()}
+                    selectedValue={interactionKind.toString()}
                     options={INTERACTION_KINDS}
                     onChange={this.handleInteractionChange}
                 />
+                <Divider />
                 <Switch
                     checked={this.state.canEscapeKeyClose}
                     label="Can escape key close"
                     onChange={this.toggleEscapeKey}
                 />
+                <Switch
+                    checked={this.state.openOnTargetFocus}
+                    disabled={!isHoverInteractionKind}
+                    label="Open on target focus"
+                    onChange={this.toggleOpenOnTargetFocus}
+                />
+                <Switch
+                    checked={isHoverInteractionKind ? false : this.state.shouldReturnFocusOnClose}
+                    disabled={isHoverInteractionKind}
+                    label="Should return focus on close"
+                    onChange={this.toggleShouldReturnFocusOnClose}
+                />
 
                 <H5>Modifiers</H5>
                 <Switch checked={arrow.enabled} label="Arrow" onChange={this.getModifierChangeHandler("arrow")} />