From bb3c25832610eaecde3e636daaa4c32d736faf9a Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 17 Apr 2018 14:19:00 -0700 Subject: [PATCH 1/5] ContextMenu will not leave viewport, refactors --- .../components/context-menu/contextMenu.tsx | 22 +++++++++---------- .../context-menu/contextMenuTarget.tsx | 10 +-------- 2 files changed, 11 insertions(+), 21 deletions(-) diff --git a/packages/core/src/components/context-menu/contextMenu.tsx b/packages/core/src/components/context-menu/contextMenu.tsx index 0841ada8d4..a96176cc5f 100644 --- a/packages/core/src/components/context-menu/contextMenu.tsx +++ b/packages/core/src/components/context-menu/contextMenu.tsx @@ -20,22 +20,25 @@ export interface IOffset { } 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> { public state: IContextMenuState = { + isDarkTheme: false, isOpen: false, + menu: null, + offset: null, }; public render() { @@ -43,10 +46,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 +61,9 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { onInteraction={this.handlePopoverInteraction} position={Position.RIGHT_TOP} popoverClassName={popoverClassName} + target={
} transitionDuration={TRANSITION_DURATION} - > - {emptyTarget} - + />
); } diff --git a/packages/core/src/components/context-menu/contextMenuTarget.tsx b/packages/core/src/components/context-menu/contextMenuTarget.tsx index 2358ceed1a..65f1706ab1 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); } } From 0002959b493ce761b40a677a17187230126ca84d Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 17 Apr 2018 14:26:42 -0700 Subject: [PATCH 2/5] add Overlay didClose & Popover popoverDidClose lifecycle methods! --- .../core/src/components/overlay/overlay.tsx | 9 ++++- .../core/src/components/popover/popover.tsx | 6 ++++ packages/core/test/overlay/overlayTests.tsx | 36 +++++++++++++++++++ packages/core/test/popover/popoverTests.tsx | 18 +++++++--- 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index 5d67c5b2bb..8e07835e16 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} ); diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index 5dd10fa223..b99dec4b6b 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/overlay/overlayTests.tsx b/packages/core/test/overlay/overlayTests.tsx index 1cadd5a8dc..c86fd80751 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 1a6a76c745..5595f2efda 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", () => { From 313c66d49826f6ca39bb9f70d27e9929d244cc63 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 17 Apr 2018 14:54:46 -0700 Subject: [PATCH 3/5] remove ContextMenu element when the menu closes (using new popover lifecycle) --- .../components/context-menu/contextMenu.tsx | 19 +++++++++++++++---- .../test/context-menu/contextMenuTests.tsx | 15 +++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/packages/core/src/components/context-menu/contextMenu.tsx b/packages/core/src/components/context-menu/contextMenu.tsx index a96176cc5f..64a5cd297a 100644 --- a/packages/core/src/components/context-menu/contextMenu.tsx +++ b/packages/core/src/components/context-menu/contextMenu.tsx @@ -33,7 +33,7 @@ const POPPER_MODIFIERS: PopperModifiers = { const TRANSITION_DURATION = 100; /* istanbul ignore next */ -class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { +class ContextMenu extends AbstractPureComponent<{ didClose: () => void }, IContextMenuState> { public state: IContextMenuState = { isDarkTheme: false, isOpen: false, @@ -61,6 +61,7 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { onInteraction={this.handlePopoverInteraction} position={Position.RIGHT_TOP} popoverClassName={popoverClassName} + popoverDidClose={this.props.didClose} target={
} transitionDuration={TRANSITION_DURATION} /> @@ -103,6 +104,7 @@ class ContextMenu extends AbstractPureComponent<{}, IContextMenuState> { }; } +let contextMenuElement: HTMLElement; let contextMenu: ContextMenu; /** @@ -111,11 +113,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); @@ -132,3 +134,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/test/context-menu/contextMenuTests.tsx b/packages/core/test/context-menu/contextMenuTests.tsx index 2fc1ce7232..a0e721ac73 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", () => { From 5e24bcf930d647af10451ce4a507eab9de12c231 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 17 Apr 2018 14:54:55 -0700 Subject: [PATCH 4/5] fix Overlay outside click --- packages/core/src/components/overlay/overlay.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index 8e07835e16..2f8a1ac7e5 100644 --- a/packages/core/src/components/overlay/overlay.tsx +++ b/packages/core/src/components/overlay/overlay.tsx @@ -368,7 +368,7 @@ export class Overlay extends React.PureComponent { const isClickInThisOverlayOrDescendant = openStack .slice(stackIndex) - .some(({ containerElement }) => containerElement && containerElement.contains(eventTarget)); + .some(({ containerElement: elem }) => elem && elem.contains(eventTarget) && !elem.isSameNode(eventTarget)); if (isOpen && canOutsideClickClose && !isClickInThisOverlayOrDescendant) { // casting to any because this is a native event From f1db2186fd2867ae652087c03c31c787f2624c19 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 18 Apr 2018 11:27:10 -0700 Subject: [PATCH 5/5] ICMProps, comment --- .../core/src/components/context-menu/contextMenu.tsx | 7 ++++++- packages/core/src/components/overlay/overlay.tsx | 12 +++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/packages/core/src/components/context-menu/contextMenu.tsx b/packages/core/src/components/context-menu/contextMenu.tsx index 64a5cd297a..6ff15eb065 100644 --- a/packages/core/src/components/context-menu/contextMenu.tsx +++ b/packages/core/src/components/context-menu/contextMenu.tsx @@ -19,6 +19,11 @@ 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; @@ -33,7 +38,7 @@ const POPPER_MODIFIERS: PopperModifiers = { const TRANSITION_DURATION = 100; /* istanbul ignore next */ -class ContextMenu extends AbstractPureComponent<{ didClose: () => void }, IContextMenuState> { +class ContextMenu extends AbstractPureComponent { public state: IContextMenuState = { isDarkTheme: false, isOpen: false, diff --git a/packages/core/src/components/overlay/overlay.tsx b/packages/core/src/components/overlay/overlay.tsx index 2f8a1ac7e5..980759bb18 100644 --- a/packages/core/src/components/overlay/overlay.tsx +++ b/packages/core/src/components/overlay/overlay.tsx @@ -363,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: elem }) => elem && elem.contains(eventTarget) && !elem.isSameNode(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