[popover2] fix(showContextMenu): unmount previous renders

packages/popover2/src/context-menu2-popover.md

@@ -42,4 +42,9 @@ export function hideContextMenu(): void;
 ```
 
 These are useful in some cases when working with imperative code that does not follow typical React paradigms.
-Note that these functions _rely on global state stored in Blueprint library code_, so they should be used with caution.
+Note that these functions come with come caveats, and thus they should be used with caution:
+
+-   they rely on global state stored in Blueprint library code.
+-   they create a new React DOM tree via `ReactDOM.render()`, which means they do not preserve any existing React
+    context from the calling code.
+-   they do _not_ automatically detect dark theme, so you must manualy set the `{ isDarkTheme: true }` property

packages/popover2/src/contextMenu2Singleton.tsx

@@ -46,7 +46,13 @@ export function showContextMenu(props: Omit<ContextMenu2PopoverProps, "isOpen">)
         contextMenuElement = document.createElement("div");
         contextMenuElement.classList.add(Classes.CONTEXT_MENU2);
         document.body.appendChild(contextMenuElement);
+    } else {
+        // N.B. It's important to unmount previous instances of the ContextMenu2Popover rendered by this function.
+        // Otherwise, React will detect no change in props sent to the already-mounted component, and therefore
+        // do nothing after the first call to this function, leading to bugs like https://github.com/palantir/blueprint/issues/5949
+        ReactDOM.unmountComponentAtNode(contextMenuElement);
     }
+
     ReactDOM.render(<UncontrolledContextMenu2Popover {...props} />, contextMenuElement);
 }
 

packages/popover2/test/contextMenu2SingletonTests.tsx

@@ -17,54 +17,92 @@
 import { assert } from "chai";
 import * as React from "react";
 
-import { Menu, MenuItem } from "@blueprintjs/core";
+import { Classes as CoreClasses, Menu, MenuItem } from "@blueprintjs/core";
+import { dispatchMouseEvent } from "@blueprintjs/test-commons";
 
-import { hideContextMenu, showContextMenu } from "../src";
+import { Classes, hideContextMenu, showContextMenu } from "../src";
 
 const TEST_MENU_CLASS_NAME = "test-menu";
 const MENU = (
     <Menu className={TEST_MENU_CLASS_NAME}>
-        <MenuItem icon="align-left" text="Align Left" />,
-        <MenuItem icon="align-center" text="Align Center" />,
-        <MenuItem icon="align-right" text="Align Right" />,
+        <MenuItem icon="align-left" text="Align Left" />
+        <MenuItem icon="align-center" text="Align Center" />
+        <MenuItem icon="align-right" text="Align Right" />
     </Menu>
 );
+const MENU_TARGET_OFFSET = {
+    left: 10,
+    top: 10,
+};
+
+function assertMenuState(isOpen = true) {
+    const ctxMenuElement = document.querySelectorAll<HTMLElement>(`.${TEST_MENU_CLASS_NAME}`);
+    if (isOpen) {
+        assert.isTrue(ctxMenuElement.length === 1, "Expected menu to be rendered on the page");
+        assert.isNotNull(ctxMenuElement[0].closest(`.${CoreClasses.OVERLAY_OPEN}`), "Expected overlay to be open");
+    } else {
+        if (ctxMenuElement.length > 0) {
+            assert.isNull(ctxMenuElement[0].closest(`.${CoreClasses.OVERLAY_OPEN}`), "Expected overlay to be closed");
+        }
+    }
+}
+
+function dismissContextMenu() {
+    const backdrop = document.querySelector<HTMLElement>(`.${Classes.CONTEXT_MENU2_BACKDROP}`);
+    if (backdrop != null) {
+        dispatchMouseEvent(backdrop, "mousedown");
+    }
+}
 
 describe("showContextMenu() + hideContextMenu()", () => {
-    afterEach(() => {
-        hideContextMenu();
+    let containerElement: HTMLElement | undefined;
+
+    before(() => {
+        // create an element on the page with non-zero dimensions so that we can trigger a context menu above it
+        containerElement = document.createElement("div");
+        containerElement.setAttribute("style", "width: 100px; height: 100px;");
+        document.body.appendChild(containerElement);
+    });
+
+    beforeEach(() => {
+        assertMenuState(false);
+    });
+
+    after(() => {
+        containerElement?.remove();
     });
 
     it("shows a menu with the imperative API", done => {
         showContextMenu({
             content: MENU,
             onOpened: () => {
-                const ctxMenuElement = document.querySelectorAll(`.${TEST_MENU_CLASS_NAME}`);
-                assert.isTrue(ctxMenuElement.length === 1);
-                done();
-            },
-            targetOffset: {
-                left: 10,
-                top: 10,
+                // defer assertions until the next animation frame; otherwise, this might throw an error
+                // inside the <TransitionGroup>, which may throw off test debugging
+                requestAnimationFrame(() => {
+                    assertMenuState(true);
+                    // important: close menu for the next test
+                    dismissContextMenu();
+                    done();
+                });
             },
+            targetOffset: MENU_TARGET_OFFSET,
+            transitionDuration: 0,
         });
     });
 
     it("hides a menu with the imperative API", done => {
         showContextMenu({
             content: MENU,
             onOpened: () => {
-                hideContextMenu();
-                setTimeout(() => {
-                    const ctxMenuElement = document.querySelectorAll(`.${TEST_MENU_CLASS_NAME}`);
-                    assert.isTrue(ctxMenuElement.length === 0);
+                // defer assertions until the next animation frame
+                requestAnimationFrame(() => {
+                    hideContextMenu();
+                    assertMenuState(false);
                     done();
                 });
             },
-            targetOffset: {
-                left: 10,
-                top: 10,
-            },
+            targetOffset: MENU_TARGET_OFFSET,
+            transitionDuration: 0,
         });
     });
 });

packages/popover2/test/contextMenu2Tests.tsx

@@ -18,6 +18,7 @@ import { assert } from "chai";
 import classNames from "classnames";
 import { mount, ReactWrapper } from "enzyme";
 import * as React from "react";
+import * as ReactDOM from "react-dom";
 import { spy } from "sinon";
 
 import { Classes as CoreClasses, Drawer, Menu, MenuItem } from "@blueprintjs/core";
@@ -35,9 +36,9 @@ import {
 
 const MENU = (
     <Menu>
-        <MenuItem icon="align-left" text="Align Left" />,
-        <MenuItem icon="align-center" text="Align Center" />,
-        <MenuItem icon="align-right" text="Align Right" />,
+        <MenuItem icon="align-left" text="Align Left" />
+        <MenuItem icon="align-center" text="Align Center" />
+        <MenuItem icon="align-right" text="Align Right" />
     </Menu>
 );
 const TARGET_CLASSNAME = "test-target";
@@ -56,7 +57,8 @@ describe("ContextMenu2", () => {
         document.body.appendChild(containerElement);
     });
     afterEach(() => {
-        containerElement?.remove();
+        ReactDOM.unmountComponentAtNode(containerElement!);
+        containerElement!.remove();
     });
 
     describe("basic usage", () => {
@@ -191,6 +193,7 @@ describe("ContextMenu2", () => {
                 ctxMenuPopover.hasClass(CoreClasses.DARK),
                 "ContextMenu2 popover should be open WITH dark theme applied",
             );
+            closeCtxMenu(wrapper);
         });
 
         it("detects theme change (dark -> light)", () => {
@@ -209,6 +212,7 @@ describe("ContextMenu2", () => {
                 ctxMenuPopover.hasClass(CoreClasses.DARK),
                 "ContextMenu2 popover should be open WITHOUT dark theme applied",
             );
+            closeCtxMenu(wrapper);
         });
     });
 
@@ -488,14 +492,6 @@ describe("ContextMenu2", () => {
             }
             target.hostNodes().closest(`.${Classes.POPOVER2_TARGET}`).simulate("mouseenter");
         }
-
-        function closeCtxMenu(wrapper: ReactWrapper) {
-            const backdrop = wrapper.find(`.${Classes.CONTEXT_MENU2_BACKDROP}`);
-            if (backdrop.exists()) {
-                backdrop.simulate("click");
-                wrapper.update();
-            }
-        }
     });
 
     function openCtxMenu(ctxMenu: ReactWrapper, targetClassName = TARGET_CLASSNAME) {
@@ -510,6 +506,14 @@ describe("ContextMenu2", () => {
             .update();
     }
 
+    function closeCtxMenu(wrapper: ReactWrapper) {
+        const backdrop = wrapper.find(`.${Classes.CONTEXT_MENU2_BACKDROP}`);
+        if (backdrop.exists()) {
+            backdrop.simulate("mousedown");
+            wrapper.update();
+        }
+    }
+
     function renderClickedInfo(targetOffset: ContextMenu2ContentProps["targetOffset"]) {
         return targetOffset === undefined ? "" : `Clicked at (${targetOffset.left}, ${targetOffset.top})`;
     }