[popover2] feat(ContextMenu2): better disabled behavior

packages/docs-app/src/examples/core-examples/contextMenu2Example.tsx

@@ -36,11 +36,15 @@ export const ContextMenu2Example: React.FC<IExampleProps> = props => {
                     <MenuItem icon="layout-circle" text="Circle" />
                     <MenuItem icon="layout-grid" text="Grid" />
                 </MenuItem>
-                <MenuDivider />
-                <MenuItem
-                    disabled={true}
-                    text={`Clicked at (${Math.round(targetOffset.left)}, ${Math.round(targetOffset.top)})`}
-                />
+                {targetOffset === undefined ? undefined : (
+                    <>
+                        <MenuDivider />
+                        <MenuItem
+                            disabled={true}
+                            text={`Clicked at (${Math.round(targetOffset.left)}, ${Math.round(targetOffset.top)})`}
+                        />
+                    </>
+                )}
             </Menu>
         ),
         [],

packages/popover2/src/contextMenu2.tsx

@@ -38,11 +38,14 @@ type Offset = {
  * Render props relevant to the _content_ of a context menu (rendered as the underlying Popover's content).
  */
 export interface ContextMenu2ContentProps {
-    /** Whether the context menu is currently open */
+    /** Whether the context menu is currently open. */
     isOpen: boolean;
 
-    /** The computed target offset (x, y) coordinates for the context menu click event */
-    targetOffset: Offset;
+    /**
+     * The computed target offset (x, y) coordinates for the context menu click event.
+     * On first render, before any context menu click event has occurred, this will be undefined.
+     */
+    targetOffset: Offset | undefined;
 
     /** The context menu click event. If isOpen is false, this will be undefined. */
     mouseEvent: React.MouseEvent<HTMLElement> | undefined;
@@ -79,7 +82,6 @@ export interface ContextMenu2Props
     /**
      * Menu content. This will usually be a Blueprint `<Menu>` component.
      * This optionally functions as a render prop so you can use component state to render content.
-     * This should only be `undefined` if `disabled` is also set to `true`.
      */
     content: JSX.Element | ((props: ContextMenu2ContentProps) => JSX.Element) | undefined;
 
@@ -128,7 +130,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = ({
     tagName = "div",
     ...restProps
 }) => {
-    const [targetOffset, setTargetOffset] = React.useState<Offset>({ left: 0, top: 0 });
+    const [targetOffset, setTargetOffset] = React.useState<Offset | undefined>(undefined);
     const [mouseEvent, setMouseEvent] = React.useState<React.MouseEvent<HTMLElement>>();
     const [isOpen, setIsOpen] = React.useState<boolean>(false);
     const containerRef = React.useRef<HTMLDivElement>(null);
@@ -177,9 +179,9 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = ({
                     <div onContextMenu={cancelContextMenu}>{menu}</div>
                 }
                 enforceFocus={false}
-                // Generate key based on offset so a new Popover instance is created
+                // Generate key based on offset so that a new Popover instance is created
                 // when offset changes, to force recomputing position.
-                key={`${targetOffset.left}x${targetOffset.top}`}
+                key={getPopoverKey(targetOffset)}
                 hasBackdrop={true}
                 isOpen={isOpen}
                 minimal={true}
@@ -200,7 +202,12 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = ({
                 return;
             }
 
-            if (!disabled) {
+            // If disabled, we should avoid this extra work. Otherwise: if using the child function API,
+            // we need to make sure contentProps is up to date for correctness, so we handle the event regardless
+            // of whether the consumer returned an undefined menu.
+            const shouldHandleEvent = !disabled && (CoreUtils.isFunction(children) || maybePopover !== undefined);
+
+            if (shouldHandleEvent) {
                 e.preventDefault();
                 e.persist();
                 setMouseEvent(e);
@@ -252,3 +259,7 @@ function getContainingBlockOffset(targetElement: HTMLElement | null | undefined)
     }
     return { left: 0, top: 0 };
 }
+
+function getPopoverKey(targetOffset: Offset | undefined) {
+    return targetOffset === undefined ? "default" : `${targetOffset.left}x${targetOffset.top}`;
+}

packages/popover2/test/contextMenu2Tests.tsx

@@ -21,7 +21,7 @@ import * as React from "react";
 
 import { Menu, MenuItem } from "@blueprintjs/core";
 
-import { Classes, ContextMenu2, ContextMenu2Props, Popover2 } from "../src";
+import { Classes, ContextMenu2, ContextMenu2ContentProps, ContextMenu2Props, Popover2 } from "../src";
 
 const MENU_ITEMS = [
     <MenuItem key="left" icon="align-left" text="Align Left" />,
@@ -59,7 +59,7 @@ describe("ContextMenu2", () => {
         }
     });
 
-    describe("advanced usage", () => {
+    describe("advanced usage (child render function API)", () => {
         it("renders children and Popover", () => {
             const ctxMenu = mountTestMenu();
             assert.isTrue(ctxMenu.find(`.${TARGET_CLASSNAME}`).exists());
@@ -72,16 +72,37 @@ describe("ContextMenu2", () => {
             assert.isTrue(ctxMenu.find(Popover2).prop("isOpen"));
         });
 
-        function mountTestMenu() {
+        it("handles context menu event, even if content is undefined", () => {
+            const ctxMenu = mountTestMenu({ content: undefined });
+            let clickedInfo = ctxMenu.find("[data-testid='content-clicked-info']");
+            assert.strictEqual(clickedInfo.text().trim(), renderClickedInfo(undefined));
+            openCtxMenu(ctxMenu);
+            clickedInfo = ctxMenu.find("[data-testid='content-clicked-info']");
+            assert.strictEqual(clickedInfo.text().trim(), renderClickedInfo({ left: 10, top: 10 }));
+        });
+
+        it("does not handle context menu event when disabled={true}", () => {
+            const ctxMenu = mountTestMenu({ disabled: true });
+            let clickedInfo = ctxMenu.find("[data-testid='content-clicked-info']");
+            assert.strictEqual(clickedInfo.text().trim(), renderClickedInfo(undefined));
+            openCtxMenu(ctxMenu);
+            clickedInfo = ctxMenu.find("[data-testid='content-clicked-info']");
+            assert.strictEqual(clickedInfo.text().trim(), renderClickedInfo(undefined));
+        });
+
+        function mountTestMenu(props?: Partial<ContextMenu2Props>) {
             return mount(
-                <ContextMenu2 content={MENU} transitionDuration={0}>
-                    {props => (
+                <ContextMenu2 content={MENU} transitionDuration={0} {...props}>
+                    {ctxMenuProps => (
                         <div
-                            className={classNames(props.className, TARGET_CLASSNAME)}
-                            onContextMenu={props.onContextMenu}
-                            ref={props.ref}
+                            className={classNames(ctxMenuProps.className, TARGET_CLASSNAME)}
+                            onContextMenu={ctxMenuProps.onContextMenu}
+                            ref={ctxMenuProps.ref}
                         >
-                            {props.popover}
+                            {ctxMenuProps.popover}
+                            <span data-testid="content-clicked-info">
+                                {renderClickedInfo(ctxMenuProps.contentProps.targetOffset)}
+                            </span>
                         </div>
                     )}
                 </ContextMenu2>,
@@ -95,4 +116,8 @@ describe("ContextMenu2", () => {
             .simulate("contextmenu", { defaultPrevented: false, clientX: 10, clientY: 10 })
             .update();
     }
+
+    function renderClickedInfo(targetOffset: ContextMenu2ContentProps["targetOffset"]) {
+        return targetOffset === undefined ? "" : `Clicked at (${targetOffset.left}, ${targetOffset.top})`;
+    }
 });