[popover2] fix(ContextMenu2): disable native menu correctly with cont…

…ent function API (#6227)

packages/popover2/src/contextMenu2.tsx

@@ -163,15 +163,30 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
     // if the menu was just opened, we should check for dark theme (but don't do this on every render)
     const isDarkTheme = React.useMemo(() => CoreUtils.isDarkTheme(childRef.current), [childRef, isOpen]);
 
+    const contentProps: ContextMenu2ContentProps = React.useMemo(
+        () => ({
+            isOpen,
+            mouseEvent,
+            targetOffset,
+        }),
+        [isOpen, mouseEvent, targetOffset],
+    );
+    // create a memoized function to render the menu so that we can call it if necessary in the "contextmenu" event
+    // handler which runs before this render function has a chance to re-run and update the `menu` variable
+    const renderMenu = React.useCallback(
+        (menuContentProps: ContextMenu2ContentProps) =>
+            disabled ? undefined : CoreUtils.isFunction(content) ? content(menuContentProps) : content,
+        [disabled, content],
+    );
+    const menuContent = React.useMemo(() => renderMenu(contentProps), [contentProps, renderMenu]);
+
     // only render the popover if there is content in the context menu;
     // this avoid doing unnecessary rendering & computation
-    const contentProps: ContextMenu2ContentProps = { isOpen, mouseEvent, targetOffset };
-    const menu = disabled ? undefined : CoreUtils.isFunction(content) ? content(contentProps) : content;
     const maybePopover =
-        menu === undefined ? undefined : (
+        menuContent === undefined ? undefined : (
             <ContextMenu2Popover
                 {...popoverProps}
-                content={menu}
+                content={menuContent}
                 isDarkTheme={isDarkTheme}
                 isOpen={isOpen}
                 targetOffset={targetOffset}
@@ -186,32 +201,34 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
                 return;
             }
 
-            const hasMenuContent = menu !== undefined;
-
-            // If disabled, we should avoid this extra work.
+            // If disabled, we should avoid the extra work in this event handler.
             // Otherwise: if using the child or content function APIs, we need to make sure contentProps gets updated,
             // so we handle the event regardless of whether the consumer returned an undefined menu.
             const shouldHandleEvent =
-                !disabled && (CoreUtils.isFunction(children) || CoreUtils.isFunction(content) || hasMenuContent);
-
-            // If there is no menu content, we shouldn't automatically swallow the contextmenu event, since the
-            // user probably wants to fall back to default browser behavior. If they still want to disable the
-            // native context menu in that case, they can do so with their own `onContextMenu` handler.
-            if (hasMenuContent) {
-                e.preventDefault();
-            }
+                !disabled && (CoreUtils.isFunction(children) || CoreUtils.isFunction(content) || content !== undefined);
 
             if (shouldHandleEvent) {
+                setIsOpen(true);
                 e.persist();
                 setMouseEvent(e);
-                setTargetOffset({ left: e.clientX, top: e.clientY });
-                setIsOpen(true);
+                const newTargetOffset = { left: e.clientX, top: e.clientY };
+                setTargetOffset(newTargetOffset);
                 tooltipCtxDispatch({ type: "FORCE_DISABLED_STATE" });
+
+                const newMenuContent = renderMenu({ isOpen: true, mouseEvent: e, targetOffset: newTargetOffset });
+
+                if (newMenuContent === undefined) {
+                    // If there is no menu content, we shouldn't automatically swallow the contextmenu event, since the
+                    // user probably wants to fall back to default browser behavior. If they still want to disable the
+                    // native context menu in that case, they can do so with their own `onContextMenu` handler.
+                } else {
+                    e.preventDefault();
+                }
             }
 
             onContextMenu?.(e);
         },
-        [children, content, disabled, onContextMenu, menu],
+        [children, onContextMenu, menuContent, renderMenu],
     );
 
     const containerClassName = classNames(className, Classes.CONTEXT_MENU2);

packages/popover2/test/contextMenu2Tests.tsx

@@ -193,6 +193,8 @@ describe("ContextMenu2", () => {
     });
 
     describe("advanced usage (content render function API)", () => {
+        const ALT_CONTENT_WRAPPER = "alternative-content-wrapper";
+
         it("renders children and menu content, prevents default context menu handler", done => {
             const onContextMenu = (e: React.MouseEvent) => {
                 assert.isTrue(e.defaultPrevented);
@@ -216,10 +218,37 @@ describe("ContextMenu2", () => {
             openCtxMenu(ctxMenu);
         });
 
-        function renderContent() {
+        it("updates menu if content prop value changes", () => {
+            const ctxMenu = mountTestMenu();
+            openCtxMenu(ctxMenu);
+            assert.isTrue(ctxMenu.find(`.${MENU_CLASSNAME}`).exists());
+            assert.isFalse(ctxMenu.find(`.${ALT_CONTENT_WRAPPER}`).exists());
+            ctxMenu.setProps({ content: renderAlternativeContent });
+            assert.isTrue(ctxMenu.find(`.${ALT_CONTENT_WRAPPER}`).exists());
+        });
+
+        it("updates menu if content render function return value changes", () => {
+            const testMenu = mount(<TestMenuWithChangingContent useAltContent={false} />, {
+                attachTo: containerElement,
+            });
+            openCtxMenu(testMenu);
+            assert.isTrue(testMenu.find(`.${MENU_CLASSNAME}`).exists());
+            assert.isFalse(testMenu.find(`.${ALT_CONTENT_WRAPPER}`).exists());
+            testMenu.setProps({ useAltContent: true });
+            assert.isTrue(testMenu.find(`.${ALT_CONTENT_WRAPPER}`).exists());
+        });
+
+        function renderContent({ mouseEvent, targetOffset }: ContextMenu2ContentProps) {
+            if (mouseEvent === undefined || targetOffset === undefined) {
+                return undefined;
+            }
             return MENU;
         }
 
+        function renderAlternativeContent() {
+            return <div className={ALT_CONTENT_WRAPPER}>{MENU}</div>;
+        }
+
         function mountTestMenu(props?: Partial<ContextMenu2Props>) {
             return mount(
                 <ContextMenu2 content={renderContent} popoverProps={{ transitionDuration: 0 }} {...props}>
@@ -228,6 +257,19 @@ describe("ContextMenu2", () => {
                 { attachTo: containerElement },
             );
         }
+
+        function TestMenuWithChangingContent({ useAltContent } = { useAltContent: false }) {
+            const content = React.useCallback(
+                (contentProps: ContextMenu2ContentProps) =>
+                    useAltContent ? renderAlternativeContent() : renderContent(contentProps),
+                [useAltContent],
+            );
+            return (
+                <ContextMenu2 content={content} popoverProps={{ transitionDuration: 0 }}>
+                    <div className={TARGET_CLASSNAME} />
+                </ContextMenu2>
+            );
+        }
     });
 
     describe("theming", () => {