From 499a925f006a6fa065e04e836b46d16fd7e81483 Mon Sep 17 00:00:00 2001
From: Adi Dahiya <adahiya@palantir.com>
Date: Thu, 27 May 2021 17:04:41 +0000
Subject: [PATCH 1/2] [popover2] fix(ContextMenu2): simpler target positioning
 logic

---
 packages/core/src/common/classes.ts           |  5 ++-
 .../core/src/components/collapse/collapse.tsx |  5 +--
 packages/core/src/components/tree/_tree.scss  |  6 ---
 packages/core/src/components/tree/tree.tsx    |  2 +-
 packages/popover2/src/contextMenu2.tsx        | 38 +++++++------------
 packages/popover2/test/contextMenu2Tests.tsx  |  3 +-
 6 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/packages/core/src/common/classes.ts b/packages/core/src/common/classes.ts
index fd5cc9cea7..c63d74bae8 100644
--- a/packages/core/src/common/classes.ts
+++ b/packages/core/src/common/classes.ts
@@ -78,7 +78,10 @@ export const LIST_UNSTYLED = `${NS}-list-unstyled`;
 export const RTL = `${NS}-rtl`;
 
 // layout utilities
-/** @see https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block */
+/**
+ * @see https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block
+ * @deprecated this is no longer needed for ContextMenu2, will be removed in v4.0
+ */
 export const FIXED_POSITIONING_CONTAINING_BLOCK = `${NS}-fixed-positioning-containing-block`;
 
 // components
diff --git a/packages/core/src/components/collapse/collapse.tsx b/packages/core/src/components/collapse/collapse.tsx
index 107bdb72a9..74e770bdd1 100644
--- a/packages/core/src/components/collapse/collapse.tsx
+++ b/packages/core/src/components/collapse/collapse.tsx
@@ -185,9 +185,6 @@ export class Collapse extends AbstractPureComponent2<CollapseProps, ICollapseSta
             transition: isAutoHeight ? "none" : undefined,
         };
 
-        // in order to give hints to child elements which rely on CSS fixed positioning, we need to apply a class
-        // to the element which creates a new containing block with a non-empty `transform` property
-        // see https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block
         const contentsStyle = {
             // only use heightWhenOpen while closing
             transform: displayWithTransform ? "translateY(0)" : `translateY(-${this.state.heightWhenOpen}px)`,
@@ -202,7 +199,7 @@ export class Collapse extends AbstractPureComponent2<CollapseProps, ICollapseSta
                 style: containerStyle,
             },
             <div
-                className={classNames(Classes.COLLAPSE_BODY, Classes.FIXED_POSITIONING_CONTAINING_BLOCK)}
+                className={Classes.COLLAPSE_BODY}
                 ref={this.contentsRefHandler}
                 style={contentsStyle}
                 aria-hidden={!isContentVisible && this.props.keepChildrenMounted}
diff --git a/packages/core/src/components/tree/_tree.scss b/packages/core/src/components/tree/_tree.scss
index b78d128e1e..5c38585014 100644
--- a/packages/core/src/components/tree/_tree.scss
+++ b/packages/core/src/components/tree/_tree.scss
@@ -54,12 +54,6 @@ $tree-icon-spacing: ($tree-row-height - $pt-icon-size-standard) / 2 !default;
       }
     }
   }
-
-  &.#{$ns}-fixed-positioning-containing-block {
-    // use the same transform as the Collapse component, to mimic the behavior of creating a new
-    // containing block for children which are position: fixed (like ContextMenu2 targets)
-    transform: translateY(0);
-  }
 }
 
 .#{$ns}-tree-node-list {
diff --git a/packages/core/src/components/tree/tree.tsx b/packages/core/src/components/tree/tree.tsx
index 8492f4c87a..40dc863fea 100644
--- a/packages/core/src/components/tree/tree.tsx
+++ b/packages/core/src/components/tree/tree.tsx
@@ -97,7 +97,7 @@ export class Tree<T = {}> extends React.Component<TreeProps<T>> {
 
     public render() {
         return (
-            <div className={classNames(Classes.TREE, Classes.FIXED_POSITIONING_CONTAINING_BLOCK, this.props.className)}>
+            <div className={classNames(Classes.TREE, this.props.className)}>
                 {this.renderNodes(this.props.contents, [], Classes.TREE_ROOT)}
             </div>
         );
diff --git a/packages/popover2/src/contextMenu2.tsx b/packages/popover2/src/contextMenu2.tsx
index 202c6e5528..04acc72701 100644
--- a/packages/popover2/src/contextMenu2.tsx
+++ b/packages/popover2/src/contextMenu2.tsx
@@ -20,6 +20,7 @@ import * as React from "react";
 import {
     Classes as CoreClasses,
     IOverlayLifecycleProps,
+    Portal,
     Props,
     Utils as CoreUtils,
     mergeRefs,
@@ -69,9 +70,6 @@ export interface ContextMenu2ChildrenProps {
 
     /** Popover element rendered by ContextMenu, used to establish a click target to position the menu */
     popover: JSX.Element | undefined;
-
-    /** DOM ref for the context menu target, used to calculate menu position on the page */
-    ref: React.Ref<any>;
 }
 
 export interface ContextMenu2Props
@@ -130,10 +128,12 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
         tagName = "div",
         ...restProps
     } = props;
+
+    // click target offset relative to the page (e.pageX/pageY), since the target will be rendered in a Portal
     const [targetOffset, setTargetOffset] = React.useState<Offset | undefined>(undefined);
+    // hold a reference to the click mouse event to pass to content/child render functions
     const [mouseEvent, setMouseEvent] = React.useState<React.MouseEvent<HTMLElement>>();
     const [isOpen, setIsOpen] = React.useState<boolean>(false);
-    const containerRef = React.useRef<HTMLDivElement>(null);
 
     // If disabled prop is changed, we don't want our old context menu to stick around.
     // If it has just been enabled (disabled = false), then the menu ought to be opened by
@@ -155,11 +155,13 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
     const targetRef = React.useRef<HTMLDivElement>(null);
     const renderTarget = React.useCallback(
         ({ ref }: Popover2TargetProps) => (
-            <div
-                className={Classes.CONTEXT_MENU2_POPOVER2_TARGET}
-                style={targetOffset}
-                ref={mergeRefs(ref, targetRef)}
-            />
+            <Portal>
+                <div
+                    className={Classes.CONTEXT_MENU2_POPOVER2_TARGET}
+                    style={targetOffset}
+                    ref={mergeRefs(ref, targetRef)}
+                />
+            </Portal>
         ),
         [targetOffset],
     );
@@ -214,14 +216,13 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
                 e.preventDefault();
                 e.persist();
                 setMouseEvent(e);
-                const { left, top } = getContainingBlockOffset(containerRef.current);
-                setTargetOffset({ left: e.clientX - left, top: e.clientY - top });
+                setTargetOffset({ left: e.pageX, top: e.pageY });
                 setIsOpen(true);
             }
 
             onContextMenu?.(e);
         },
-        [containerRef.current, onContextMenu, disabled],
+        [onContextMenu, disabled],
     );
 
     const containerClassName = classNames(className, Classes.CONTEXT_MENU2);
@@ -232,7 +233,6 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
             contentProps,
             onContextMenu: handleContextMenu,
             popover: maybePopover,
-            ref: containerRef,
         });
     } else {
         return React.createElement(
@@ -240,7 +240,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
             {
                 className: containerClassName,
                 onContextMenu: handleContextMenu,
-                ref: mergeRefs(containerRef, userRef),
+                ref: userRef,
                 ...restProps,
             },
             maybePopover,
@@ -250,16 +250,6 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
 });
 ContextMenu2.displayName = "Blueprint.ContextMenu2";
 
-function getContainingBlockOffset(targetElement: HTMLElement | null | undefined): { left: number; top: number } {
-    if (targetElement != null) {
-        const containingBlock = targetElement.closest(`.${CoreClasses.FIXED_POSITIONING_CONTAINING_BLOCK}`);
-        if (containingBlock != null) {
-            return containingBlock.getBoundingClientRect();
-        }
-    }
-    return { left: 0, top: 0 };
-}
-
 function getPopoverKey(targetOffset: Offset | undefined) {
     return targetOffset === undefined ? "default" : `${targetOffset.left}x${targetOffset.top}`;
 }
diff --git a/packages/popover2/test/contextMenu2Tests.tsx b/packages/popover2/test/contextMenu2Tests.tsx
index 9ddf663592..3788134d46 100644
--- a/packages/popover2/test/contextMenu2Tests.tsx
+++ b/packages/popover2/test/contextMenu2Tests.tsx
@@ -104,7 +104,6 @@ describe("ContextMenu2", () => {
                         <div
                             className={classNames(ctxMenuProps.className, TARGET_CLASSNAME)}
                             onContextMenu={ctxMenuProps.onContextMenu}
-                            ref={ctxMenuProps.ref}
                         >
                             {ctxMenuProps.popover}
                             <span data-testid="content-clicked-info">
@@ -162,7 +161,7 @@ describe("ContextMenu2", () => {
     function openCtxMenu(ctxMenu: ReactWrapper) {
         ctxMenu
             .find(`.${TARGET_CLASSNAME}`)
-            .simulate("contextmenu", { defaultPrevented: false, clientX: 10, clientY: 10 })
+            .simulate("contextmenu", { defaultPrevented: false, pageX: 10, pageY: 10 })
             .update();
     }
 

From b1e9e80c658965a3b4e96b0c07230189a3c3eaba Mon Sep 17 00:00:00 2001
From: Adi Dahiya <adahiya@palantir.com>
Date: Thu, 27 May 2021 17:21:51 +0000
Subject: [PATCH 2/2] switch back to clientX/Y

---
 packages/popover2/src/contextMenu2.tsx       | 4 ++--
 packages/popover2/test/contextMenu2Tests.tsx | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/packages/popover2/src/contextMenu2.tsx b/packages/popover2/src/contextMenu2.tsx
index 04acc72701..0acab8dad7 100644
--- a/packages/popover2/src/contextMenu2.tsx
+++ b/packages/popover2/src/contextMenu2.tsx
@@ -129,7 +129,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
         ...restProps
     } = props;
 
-    // click target offset relative to the page (e.pageX/pageY), since the target will be rendered in a Portal
+    // click target offset relative to the viewport (e.clientX/clientY), since the target will be rendered in a Portal
     const [targetOffset, setTargetOffset] = React.useState<Offset | undefined>(undefined);
     // hold a reference to the click mouse event to pass to content/child render functions
     const [mouseEvent, setMouseEvent] = React.useState<React.MouseEvent<HTMLElement>>();
@@ -216,7 +216,7 @@ export const ContextMenu2: React.FC<ContextMenu2Props> = React.forwardRef<any, C
                 e.preventDefault();
                 e.persist();
                 setMouseEvent(e);
-                setTargetOffset({ left: e.pageX, top: e.pageY });
+                setTargetOffset({ left: e.clientX, top: e.clientY });
                 setIsOpen(true);
             }
 
diff --git a/packages/popover2/test/contextMenu2Tests.tsx b/packages/popover2/test/contextMenu2Tests.tsx
index 3788134d46..b6acdf04ba 100644
--- a/packages/popover2/test/contextMenu2Tests.tsx
+++ b/packages/popover2/test/contextMenu2Tests.tsx
@@ -161,7 +161,7 @@ describe("ContextMenu2", () => {
     function openCtxMenu(ctxMenu: ReactWrapper) {
         ctxMenu
             .find(`.${TARGET_CLASSNAME}`)
-            .simulate("contextmenu", { defaultPrevented: false, pageX: 10, pageY: 10 })
+            .simulate("contextmenu", { defaultPrevented: false, clientX: 10, clientY: 10 })
             .update();
     }