ContextMenu fixes & Overlay didClose

packages/core/src/components/context-menu/contextMenu.tsx

@@ -19,34 +19,38 @@ export interface IOffset {
     top: number;
 }
 
+interface IContextMenuProps {
+    /** Callback invoked after the menu has finished transitioning to a closed state. */
+    didClose: () => void;
+}
+
 interface IContextMenuState {
-    isOpen?: boolean;
-    isDarkTheme?: boolean;
-    menu?: JSX.Element;
-    offset?: IOffset;
+    isOpen: boolean;
+    isDarkTheme: boolean;
+    menu: JSX.Element;
+    offset: IOffset;
     onClose?: () => void;
 }
 
 const POPPER_MODIFIERS: PopperModifiers = {
-    preventOverflow: { boundariesElement: "window" },
+    preventOverflow: { boundariesElement: "viewport" },
 };
 const TRANSITION_DURATION = 100;
 
 /* istanbul ignore next */
-class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> {
+class ContextMenu extends AbstractPureComponent<IContextMenuProps, IContextMenuState> {
     public state: IContextMenuState = {
+        isDarkTheme: false,
         isOpen: false,
+        menu: null,
+        offset: null,
     };
 
     public render() {
         // prevent right-clicking in a context menu
         const content = <div onContextMenu={this.cancelContextMenu}>{this.state.menu}</div>;
         const popoverClassName = classNames({ [Classes.DARK]: this.state.isDarkTheme });
 
-        // the target isn't relevant in this case. the context menu simply
-        // appears overlayed at the desired offset on the screen.
-        const emptyTarget = <div />;
-
         // wrap the popover in a positioned div to make sure it is properly
         // offset on the screen.
         return (
@@ -62,10 +66,10 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> {
                     onInteraction={this.handlePopoverInteraction}
                     position={Position.RIGHT_TOP}
                     popoverClassName={popoverClassName}
+                    popoverDidClose={this.props.didClose}
+                    target={<div />}
                     transitionDuration={TRANSITION_DURATION}
-                >
-                    {emptyTarget}
-                </Popover>
+                />
             </div>
         );
     }
@@ -105,6 +109,7 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> {
     };
 }
 
+let contextMenuElement: HTMLElement;
 let contextMenu: ContextMenu;
 
 /**
@@ -113,11 +118,11 @@ let contextMenu: ContextMenu;
  * room onscreen. The optional callback will be invoked when this menu closes.
  */
 export function show(menu: JSX.Element, offset: IOffset, onClose?: () => void, isDarkTheme?: boolean) {
-    if (contextMenu == null) {
-        const contextMenuElement = document.createElement("div");
+    if (contextMenuElement == null) {
+        contextMenuElement = document.createElement("div");
         contextMenuElement.classList.add(Classes.CONTEXT_MENU);
         document.body.appendChild(contextMenuElement);
-        contextMenu = ReactDOM.render(<ContextMenu />, contextMenuElement) as ContextMenu;
+        contextMenu = ReactDOM.render(<ContextMenu didClose={remove} />, contextMenuElement) as ContextMenu;
     }
 
     contextMenu.show(menu, offset, onClose, isDarkTheme);
@@ -134,3 +139,12 @@ export function hide() {
 export function isOpen() {
     return contextMenu != null && contextMenu.state.isOpen;
 }
+
+function remove() {
+    if (contextMenuElement != null) {
+        ReactDOM.unmountComponentAtNode(contextMenuElement);
+        contextMenuElement.remove();
+        contextMenuElement = null;
+        contextMenu = null;
+    }
+}

packages/core/src/components/context-menu/contextMenuTarget.tsx

@@ -55,15 +55,7 @@ export function ContextMenuTarget<T extends IConstructor<IContextMenuTarget>>(Wr
                         const htmlElement = ReactDOM.findDOMNode(this);
                         const darkTheme = htmlElement != null && isDarkTheme(htmlElement);
                         e.preventDefault();
-                        ContextMenu.show(
-                            menu,
-                            {
-                                left: e.clientX,
-                                top: e.clientY,
-                            },
-                            this.onContextMenuClose,
-                            darkTheme,
-                        );
+                        ContextMenu.show(menu, { left: e.clientX, top: e.clientY }, this.onContextMenuClose, darkTheme);
                     }
                 }
 

packages/core/src/components/overlay/overlay.tsx

@@ -104,6 +104,13 @@ export interface IOverlayProps extends IOverlayableProps, IBackdropProps, IProps
     /** Lifecycle callback invoked after the overlay opens and is mounted in the DOM. */
     didOpen?: () => any;
 
+    /**
+     * Lifecycle callback invoked after a child element finishes exiting the DOM.
+     * This will be invoked for each child of the `Overlay` except for the backdrop element.
+     * The argument is the underlying HTML element that left the DOM.
+     */
+    didClose?: (node: HTMLElement) => any;
+
     /**
      * Toggles the visibility of the overlay and its children.
      * This prop is required because the component is controlled.
@@ -261,7 +268,7 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
             );
         const { transitionDuration, transitionName } = this.props;
         return (
-            <CSSTransition classNames={transitionName} timeout={transitionDuration}>
+            <CSSTransition classNames={transitionName} onExited={this.props.didClose} timeout={transitionDuration}>
                 {decoratedChild}
             </CSSTransition>
         );
@@ -356,12 +363,14 @@ export class Overlay extends React.PureComponent<IOverlayProps, IOverlayState> {
         const { canOutsideClickClose, isOpen, onClose } = this.props;
         const eventTarget = e.target as HTMLElement;
 
-        const { openStack } = Overlay;
-        const stackIndex = openStack.indexOf(this);
-
-        const isClickInThisOverlayOrDescendant = openStack
+        const stackIndex = Overlay.openStack.indexOf(this);
+        const isClickInThisOverlayOrDescendant = Overlay.openStack
             .slice(stackIndex)
-            .some(({ containerElement }) => containerElement && containerElement.contains(eventTarget));
+            .some(({ containerElement: elem }) => {
+                // `elem` is the container of backdrop & content, so clicking on that container
+                // should not count as being "inside" the overlay.
+                return elem && elem.contains(eventTarget) && !elem.isSameNode(eventTarget);
+            });
 
         if (isOpen && canOutsideClickClose && !isClickInThisOverlayOrDescendant) {
             // casting to any because this is a native event

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

@@ -133,6 +133,11 @@ export interface IPopoverProps extends IOverlayableProps, IProps {
      */
     popoverClassName?: string;
 
+    /**
+     * Callback invoked after the popover closes and has been removed from the DOM.
+     */
+    popoverDidClose?: () => void;
+
     /**
      * Callback invoked when the popover opens after it is added to the DOM.
      */
@@ -322,6 +327,7 @@ export class Popover extends AbstractPureComponent<IPopoverProps, IPopoverState>
                     canEscapeKeyClose={this.props.canEscapeKeyClose}
                     canOutsideClickClose={this.props.interactionKind === PopoverInteractionKind.CLICK}
                     className={this.props.portalClassName}
+                    didClose={this.props.popoverDidClose}
                     didOpen={this.handleContentMount}
                     enforceFocus={this.props.enforceFocus}
                     hasBackdrop={hasBackdrop}

packages/core/test/context-menu/contextMenuTests.tsx

@@ -46,11 +46,18 @@ describe("ContextMenu", () => {
         assertContextMenuWasRendered();
     });
 
-    it("invokes onClose callback when menu closed", () => {
-        const onClose = spy();
-        ContextMenu.show(MENU, { left: 0, top: 0 }, onClose);
+    it("invokes onClose callback when menu closed", done => {
+        ContextMenu.show(MENU, { left: 0, top: 0 }, done);
+        ContextMenu.hide();
+    });
+
+    it("removes element from the DOM after closing", done => {
+        ContextMenu.show(MENU, { left: 0, top: 0 });
         ContextMenu.hide();
-        assert.isTrue(onClose.calledOnce, "onClose not called");
+        setTimeout(() => {
+            assert.isTrue(document.querySelector(`.${Classes.CONTEXT_MENU}`) == null);
+            done();
+        }, 110);
     });
 
     it("does not invoke previous onClose callback", () => {

packages/core/test/overlay/overlayTests.tsx

@@ -112,6 +112,42 @@ describe("<Overlay>", () => {
         assert.isTrue(didOpen.calledOnce, "didOpen not invoked when overlay open");
     });
 
+    it("invokes didClose when Overlay is closed", done => {
+        const didClose = spy();
+        mountWrapper(
+            <Overlay didClose={didClose} isOpen={true} transitionDuration={1}>
+                {createOverlayContents()}
+            </Overlay>,
+        );
+        assert.isTrue(didClose.notCalled, "didClose invoked when overlay open");
+
+        wrapper.setProps({ isOpen: false });
+        // didClose relies on transition onExited so we go async for a sec
+        setTimeout(() => {
+            wrapper.update();
+            assert.isTrue(didClose.calledOnce, "didClose not invoked when overlay closed");
+            assert.isFalse(wrapper.find("h1").exists(), "no content");
+            done();
+        });
+    });
+
+    it("invokes didClose when inline Overlay is closed", done => {
+        const didClose = spy();
+        mountWrapper(
+            <Overlay didClose={didClose} isOpen={true} usePortal={false} transitionDuration={1}>
+                {createOverlayContents()}
+            </Overlay>,
+        );
+        assert.isTrue(didClose.notCalled, "didClose invoked when overlay open");
+
+        wrapper.setProps({ isOpen: false });
+        // didClose relies on transition onExited so we go async for a sec
+        setTimeout(() => {
+            assert.isTrue(didClose.calledOnce, "didClose not invoked when overlay closed");
+            done();
+        });
+    });
+
     it("renders portal attached to body when not inline after first opened", () => {
         mountWrapper(<Overlay isOpen={false}>{createOverlayContents()}</Overlay>);
         assert.lengthOf(wrapper.find(Portal), 0, "unexpected Portal");

packages/core/test/popover/popoverTests.tsx

@@ -173,7 +173,7 @@ describe("<Popover>", () => {
         warnSpy.restore();
     });
 
-    it("lifecycle methods are called appropriately", () => {
+    it("lifecycle methods are called appropriately", done => {
         const popoverWillOpen = sinon.spy(() =>
             assert.lengthOf(testsContainerElement.getElementsByClassName(Classes.POPOVER), 0),
         );
@@ -183,21 +183,31 @@ describe("<Popover>", () => {
         const popoverWillClose = sinon.spy(() =>
             assert.lengthOf(testsContainerElement.getElementsByClassName(Classes.POPOVER), 1),
         );
+        // DOM is still present in didClose; element is removed shortly after
+        const popoverDidClose = sinon.spy();
 
         wrapper = renderPopover({
             interactionKind: PopoverInteractionKind.CLICK_TARGET_ONLY,
+            popoverDidClose,
             popoverDidOpen,
             popoverWillClose,
             popoverWillOpen,
+            transitionDuration: 1,
         }).simulateTarget("click");
         assert.isTrue(popoverWillOpen.calledOnce);
         assert.isTrue(popoverDidOpen.calledOnce);
         assert.isTrue(popoverWillClose.notCalled);
+        assert.isTrue(popoverDidClose.notCalled);
 
         wrapper.simulateTarget("click");
-        assert.isTrue(popoverDidOpen.calledOnce);
-        assert.isTrue(popoverWillOpen.calledOnce);
-        assert.isTrue(popoverWillClose.calledOnce);
+        setTimeout(() => {
+            assert.isTrue(popoverDidOpen.calledOnce);
+            assert.isTrue(popoverWillOpen.calledOnce);
+            assert.isTrue(popoverWillClose.calledOnce);
+            assert.isTrue(popoverDidClose.calledOnce);
+            assert.lengthOf(testsContainerElement.getElementsByClassName(Classes.POPOVER), 0);
+            done();
+        });
     });
 
     it("popoverDidOpen is called even if popoverWillOpen is not specified", () => {