diff --git a/packages/core/src/components/context-menu/contextMenu.tsx b/packages/core/src/components/context-menu/contextMenu.tsx index 0841ada8d4b..6ff15eb0659 100644 --- a/packages/core/src/components/context-menu/contextMenu.tsx +++ b/packages/core/src/components/context-menu/contextMenu.tsx @@ -19,23 +19,31 @@ 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 { public state: IContextMenuState = { + isDarkTheme: false, isOpen: false, + menu: null, + offset: null, }; public render() { @@ -43,10 +51,6 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { const content =
{this.state.menu}
; 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 =
; - // 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={
} transitionDuration={TRANSITION_DURATION} - > - {emptyTarget} - + />
); } @@ -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(, contextMenuElement) as ContextMenu; + contextMenu = ReactDOM.render(, 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; + } +} diff --git a/packages/core/src/components/context-menu/contextMenuTarget.tsx b/packages/core/src/components/context-menu/contextMenuTarget.tsx index 2358ceed1a0..65f1706ab1b 100644 --- a/packages/core/src/components/context-menu/contextMenuTarget.tsx +++ b/packages/core/src/components/context-menu/contextMenuTarget.tsx @@ -55,15 +55,7 @@ export function ContextMenuTarget>(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); } } diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index 5d67c5b2bbf..980759bb186 100644 --- a/packages/core/src/components/overlay/overlay.tsx +++ b/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 { ); const { transitionDuration, transitionName } = this.props; return ( - + {decoratedChild} ); @@ -356,12 +363,14 @@ export class Overlay extends React.PureComponent { 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 diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 5dd10fa2233..b99dec4b6b7 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/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 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} diff --git a/packages/core/test/context-menu/contextMenuTests.tsx b/packages/core/test/context-menu/contextMenuTests.tsx index 2fc1ce7232e..a0e721ac733 100644 --- a/packages/core/test/context-menu/contextMenuTests.tsx +++ b/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", () => { diff --git a/packages/core/test/overlay/overlayTests.tsx b/packages/core/test/overlay/overlayTests.tsx index 1cadd5a8dc6..c86fd80751c 100644 --- a/packages/core/test/overlay/overlayTests.tsx +++ b/packages/core/test/overlay/overlayTests.tsx @@ -112,6 +112,42 @@ describe("", () => { assert.isTrue(didOpen.calledOnce, "didOpen not invoked when overlay open"); }); + it("invokes didClose when Overlay is closed", done => { + const didClose = spy(); + mountWrapper( + + {createOverlayContents()} + , + ); + 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( + + {createOverlayContents()} + , + ); + 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({createOverlayContents()}); assert.lengthOf(wrapper.find(Portal), 0, "unexpected Portal"); diff --git a/packages/core/test/popover/popoverTests.tsx b/packages/core/test/popover/popoverTests.tsx index 1a6a76c745b..5595f2efdaa 100644 --- a/packages/core/test/popover/popoverTests.tsx +++ b/packages/core/test/popover/popoverTests.tsx @@ -173,7 +173,7 @@ describe("", () => { 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("", () => { 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", () => {