[core] fix(Overlay2): use mutable stack ref, fix document event listeners

packages/core/src/components/overlay2/overlay2.tsx

@@ -55,6 +55,9 @@ export interface OverlayInstance {
     /** Document "focus" event handler which needs to be attached & detached appropriately. */
     handleDocumentFocus: (e: FocusEvent) => void;
 
+    /** Document "mousedown" event handler which needs to be attached & detached appropriately. */
+    handleDocumentMousedown?: (e: MouseEvent) => void;
+
     /** Unique ID for this overlay which helps to identify it across prop changes. */
     id: string;
 
@@ -148,7 +151,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
             const container = getRef(containerElement);
             const activeElement = getActiveElement(container);
 
-            if (container == null || activeElement == null || !isOpen) {
+            if (container == null || activeElement == null) {
                 return;
             }
 
@@ -159,58 +162,37 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
                 setIsAutoFocusing(false);
             }
         });
-    }, [isOpen]);
+    }, []);
+
+    /** Unique ID for this overlay in the global stack */
+    const id = useOverlay2ID();
+
+    // N.B. use `null` here and not simply `undefined` because `useImperativeHandle` will set `null` on unmount,
+    // and we need the following code to be resilient to that value.
+    const instance = React.useRef<OverlayInstance>(null);
 
     /**
-     * When multiple Overlays are open, this event handler is only active for the most recently
-     * opened one to avoid Overlays competing with each other for focus.
+     * When multiple `enforceFocus` Overlays are open, this event handler is only active for the most
+     * recently opened one to avoid Overlays competing with each other for focus.
      */
     const handleDocumentFocus = React.useCallback(
         (e: FocusEvent) => {
             // get the actual target even in the Shadow DOM
             // see https://github.com/palantir/blueprint/issues/4220
             const eventTarget = e.composed ? e.composedPath()[0] : e.target;
             const container = getRef(containerElement);
-            if (
-                enforceFocus &&
-                container != null &&
-                eventTarget instanceof Node &&
-                !container.contains(eventTarget as HTMLElement)
-            ) {
+            if (container != null && eventTarget instanceof Node && !container.contains(eventTarget as HTMLElement)) {
                 // prevent default focus behavior (sometimes auto-scrolls the page)
                 e.preventDefault();
                 e.stopImmediatePropagation();
                 bringFocusInsideOverlay();
             }
         },
-        [bringFocusInsideOverlay, enforceFocus],
+        [bringFocusInsideOverlay],
     );
 
-    const id = useOverlay2ID();
-
-    // N.B. use `null` here and not simply `undefined` because `useImperativeHandle` will set `null` on unmount,
-    // and we need the following code to be resilient to that value.
-    const instance = React.useRef<OverlayInstance>(null);
-    // send this instance's imperative handle to the the forwarded ref as well as our local ref
-    const ref = React.useMemo(() => mergeRefs(forwardedRef, instance), [forwardedRef]);
-    React.useImperativeHandle(
-        ref,
-        () => ({
-            bringFocusInsideOverlay,
-            containerElement,
-            handleDocumentFocus,
-            id,
-            props: {
-                autoFocus,
-                enforceFocus,
-                hasBackdrop,
-                usePortal,
-            },
-        }),
-        [autoFocus, bringFocusInsideOverlay, enforceFocus, handleDocumentFocus, hasBackdrop, id, usePortal],
-    );
-
-    const handleDocumentClick = React.useCallback(
+    // N.B. this listener is only kept attached when `isOpen={true}` and `canOutsideClickClose={true}`
+    const handleDocumentMousedown = React.useCallback(
         (e: MouseEvent) => {
             if (instance.current == null) {
                 return;
@@ -230,12 +212,52 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
                 },
             );
 
-            if (isOpen && !isClickInThisOverlayOrDescendant && canOutsideClickClose) {
+            if (!isClickInThisOverlayOrDescendant) {
                 // casting to any because this is a native event
                 onClose?.(e as any);
             }
         },
-        [canOutsideClickClose, getThisOverlayAndDescendants, isOpen, onClose],
+        [getThisOverlayAndDescendants, onClose],
+    );
+
+    // Important: clean up old document-level event listeners if their memoized values change (this is rare, but
+    // may happen, for example, if a user forgets to use `React.useCallback` in the `props.onClose` value).
+    // Otherwise, we will lose the reference to those values and create a memory leak since we won't be able
+    // to successfully detach them inside overlayWillClose.
+    React.useEffect(() => {
+        document.removeEventListener("mousedown", handleDocumentMousedown);
+    }, [handleDocumentMousedown]);
+    React.useEffect(() => {
+        document.removeEventListener("focus", handleDocumentFocus, /* useCapture */ true);
+    }, [handleDocumentFocus]);
+
+    // send this instance's imperative handle to the the forwarded ref as well as our local ref
+    const ref = React.useMemo(() => mergeRefs(forwardedRef, instance), [forwardedRef]);
+    React.useImperativeHandle(
+        ref,
+        () => ({
+            bringFocusInsideOverlay,
+            containerElement,
+            handleDocumentFocus,
+            handleDocumentMousedown,
+            id,
+            props: {
+                autoFocus,
+                enforceFocus,
+                hasBackdrop,
+                usePortal,
+            },
+        }),
+        [
+            autoFocus,
+            bringFocusInsideOverlay,
+            enforceFocus,
+            handleDocumentFocus,
+            handleDocumentMousedown,
+            hasBackdrop,
+            id,
+            usePortal,
+        ],
     );
 
     const handleContainerKeyDown = React.useCallback(
@@ -274,7 +296,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
         }
 
         if (canOutsideClickClose && !hasBackdrop) {
-            document.addEventListener("mousedown", handleDocumentClick);
+            document.addEventListener("mousedown", handleDocumentMousedown);
         }
 
         setRef(lastActiveElementBeforeOpened, getActiveElement(getRef(containerElement)));
@@ -284,7 +306,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
         canOutsideClickClose,
         enforceFocus,
         getLastOpened,
-        handleDocumentClick,
+        handleDocumentMousedown,
         handleDocumentFocus,
         hasBackdrop,
         openOverlay,
@@ -296,7 +318,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
         }
 
         document.removeEventListener("focus", handleDocumentFocus, /* useCapture */ true);
-        document.removeEventListener("mousedown", handleDocumentClick);
+        document.removeEventListener("mousedown", handleDocumentMousedown);
 
         closeOverlay(instance.current);
         const lastOpenedOverlay = getLastOpened();
@@ -309,7 +331,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
                 document.addEventListener("focus", lastOpenedOverlay.handleDocumentFocus, /* useCapture */ true);
             }
         }
-    }, [closeOverlay, getLastOpened, handleDocumentClick, handleDocumentFocus]);
+    }, [closeOverlay, getLastOpened, handleDocumentFocus, handleDocumentMousedown]);
 
     const prevIsOpen = usePrevious(isOpen) ?? false;
     React.useEffect(() => {
@@ -332,6 +354,7 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
     React.useEffect(() => {
         return () => {
             if (isOpen) {
+                // if the overlay is still open, we need to run cleanup code to remove some event handlers
                 overlayWillClose();
             }
         };

packages/core/src/components/popover/popover.md

@@ -426,7 +426,7 @@ performance. If your components are not updating in a synchronous fashion as exp
 `setTimeout` to wait for asynchronous Popover rendering to catch up:
 
 ```tsx
-import { Classes, Overlay, Popover } from "@blueprintjs/core";
+import { Classes, Overlay2, Popover } from "@blueprintjs/core";
 import { assert } from "chai";
 import { mount } from "enzyme";
 import { Target } from "react-popper";
@@ -445,7 +445,7 @@ wrapper.find(`.${Classes.POPOVER}`).hostNodes().simulate("mouseleave");
 
 setTimeout(() => {
     // Popover delays closing using setTimeout, so need to defer this check too.
-    const isOpen = wrapper.find(Overlay).prop("isOpen");
+    const isOpen = wrapper.find(Overlay2).prop("isOpen");
     assert.equal(isOpen, false);
 });
 ```

packages/core/src/components/popover/popover.tsx

@@ -206,7 +206,7 @@ export class Popover<
     public targetRef = React.createRef<HTMLElement>();
 
     /**
-     * Overlay transition container element ref.
+     * Overlay2 transition container element ref.
      */
     private transitionContainerElement = React.createRef<HTMLDivElement>();
 

packages/core/src/components/portal/portal.md

@@ -1,19 +1,19 @@
 @# Portal
 
-The __Portal__ component renders its children into a new DOM "subtree" outside of the current component
-hierarchy. It is an essential piece of the [Overlay](#core/components/overlay) component, responsible for
+The **Portal** component renders its children into a new DOM "subtree" outside of the current component
+hierarchy. It is an essential piece of the [Overlay2](#core/components/overlay2) component, responsible for
 ensuring that the overlay contents appear above the rest of the application. In most cases, you do not
 need to use a Portal directly; this documentation is provided only for reference.
 
 @## DOM Behavior
 
-__Portal__ component functions like a declarative `appendChild()`. The children of a __Portal__ are inserted into a *new child* of the target element. This target element is determined in the following order:
+**Portal** component functions like a declarative `appendChild()`. The children of a **Portal** are inserted into a _new child_ of the target element. This target element is determined in the following order:
+
 1. The `container` prop, if specified
 2. The `portalContainer` from the closest [PortalProvider](#core/context/portal-provider), if specified
 3. Otherwise `document.body`
 
-
-__Portal__ is used inside [Overlay](#core/components/overlay) to actually overlay the content on the
+**Portal** is used inside [Overlay2](#core/components/overlay2) to actually overlay the content on the
 application.
 
 <div class="@ns-callout @ns-intent-warning @ns-icon-move @ns-callout-has-body-content">
@@ -31,7 +31,7 @@ apply `position: absolute` to the `<body>` tag.
 
 @## React context options
 
-__Portal__ supports some customization through [React context](https://react.dev/learn/passing-data-deeply-with-context).
+**Portal** supports some customization through [React context](https://react.dev/learn/passing-data-deeply-with-context).
 Using this API can be helpful if you need to apply some custom styling or logic to _all_ Blueprint
 components which use portals (popovers, tooltips, dialogs, etc.). You can do so by rendering a
 [PortalProvider](#core/context/portal-provider) in your React tree
@@ -63,7 +63,7 @@ This feature uses React's legacy context API. Support for this API will be remov
 
 </div>
 
-__Portal__ supports the following options via the [React legacy context API](https://reactjs.org/docs/legacy-context.html).
+**Portal** supports the following options via the [React legacy context API](https://reactjs.org/docs/legacy-context.html).
 To use them, supply a child context to a subtree that contains the Portals you want to customize.
 
 @interface PortalLegacyContext

packages/core/src/components/toast/overlayToaster.tsx

@@ -235,7 +235,7 @@ export class OverlayToaster extends AbstractPureComponent<OverlayToasterProps, O
     }
 
     /**
-     * If provided `Toast` children, automaticaly upgrade them to `Toast2` elements so that `Overlay` can inject
+     * If provided `Toast` children, automaticaly upgrade them to `Toast2` elements so that `Overlay2` can inject
      * refs into them for use by `CSSTransition`. This is a bit hacky but ensures backwards compatibility for
      * `OverlayToaster`. It should be an uncommon code path in most applications, since we expect most usage to
      * occur via the imperative toaster APIs.

packages/core/src/components/toast/overlayToasterProps.ts

@@ -45,7 +45,7 @@ export interface OverlayToasterProps extends Props {
      */
     canEscapeKeyClear?: boolean;
 
-    /** Toasts to display inside the Overlay. */
+    /** Toasts to display inside the Overlay2. */
     children?: React.ReactNode;
 
     /**

packages/core/src/context/index.ts

@@ -23,7 +23,7 @@ export {
 export {
     OverlaysContext,
     OverlaysProvider,
-    type OverlaysContextInstance,
+    type OverlaysContextState,
     type OverlaysProviderProps,
 } from "./overlays/overlaysProvider";
 export { PortalContext, type PortalContextOptions, PortalProvider } from "./portal/portalProvider";

packages/core/src/context/overlays/overlaysProvider.tsx

@@ -18,7 +18,12 @@ import * as React from "react";
 
 import type { OverlayInstance } from "../../components/overlay2/overlay2";
 
-interface OverlaysContextState {
+// N.B. using a mutable ref for the stack is much easier to work with in the world of hooks and FCs.
+// This matches the mutable global behavior of the old Overlay implementation in Blueprint v5. An alternative
+// approach was considered with an immutable array data structure and a reducer, but that implementation
+// caused lots of unnecessary invalidation of `React.useCallback()` for document-level event handlers, which
+// led to memory leaks and bugs.
+export interface OverlaysContextState {
     /**
      * Whether the context instance is being used within a tree which has an `<OverlaysProvider>`.
      * `useOverlayStack()` will work if this is `false` in Blueprint v5, but this will be unsupported
@@ -32,18 +37,9 @@ interface OverlaysContextState {
     /**
      * The application-wide global overlay stack.
      */
-    stack: OverlayInstance[];
+    stack: React.MutableRefObject<OverlayInstance[]>;
 }
 
-export type OverlayStackAction =
-    | { type: "OPEN_OVERLAY" | "CLOSE_OVERLAY"; payload: OverlayInstance }
-    | { type: "RESET_STACK" };
-
-export type OverlaysContextInstance = readonly [OverlaysContextState, React.Dispatch<OverlayStackAction>];
-
-const initialOverlaysState: OverlaysContextState = { hasProvider: false, stack: [] };
-const noOpDispatch: React.Dispatch<OverlayStackAction> = () => null;
-
 /**
  * A React context used to interact with the overlay stack in an application.
  * Users should take care to make sure that only _one_ of these is instantiated and used within an
@@ -54,30 +50,10 @@ const noOpDispatch: React.Dispatch<OverlayStackAction> = () => null;
  *
  * For more information, see the [OverlaysProvider documentation](https://blueprintjs.com/docs/#core/context/overlays-provider).
  */
-export const OverlaysContext = React.createContext<OverlaysContextInstance>([initialOverlaysState, noOpDispatch]);
-
-/**
- * N.B. this is exported in order for `useOverlayStack()` to implement backwards-compatibility for overlays
- * outside of a `<OverlaysProvider>. It should stop being exported from this module in Blueprint v6.
- */
-export const overlaysReducer = (state: OverlaysContextState, action: OverlayStackAction): OverlaysContextState => {
-    switch (action.type) {
-        case "OPEN_OVERLAY":
-            return { ...state, stack: [...state.stack, action.payload] };
-        case "CLOSE_OVERLAY":
-            const index = state.stack.findIndex(o => o.id === action.payload.id);
-            if (index === -1) {
-                return state;
-            }
-            const newStack = state.stack.slice();
-            newStack.splice(index, 1);
-            return { ...state, stack: newStack };
-        case "RESET_STACK":
-            return initialOverlaysState;
-        default:
-            return state;
-    }
-};
+export const OverlaysContext = React.createContext<OverlaysContextState>({
+    hasProvider: false,
+    stack: { current: [] },
+});
 
 export interface OverlaysProviderProps {
     /** The component subtree which will have access to this overlay stack context. */
@@ -90,6 +66,7 @@ export interface OverlaysProviderProps {
  * @see https://blueprintjs.com/docs/#core/context/overlays-provider
  */
 export const OverlaysProvider = ({ children }: OverlaysProviderProps) => {
-    const contextValue = React.useReducer(overlaysReducer, { ...initialOverlaysState, hasProvider: true });
+    const stack = React.useRef<OverlayInstance[]>([]);
+    const contextValue = React.useMemo(() => ({ hasProvider: true, stack }), [stack]);
     return <OverlaysContext.Provider value={contextValue}>{children}</OverlaysContext.Provider>;
 };

packages/core/src/docs/variables.md

@@ -89,7 +89,7 @@ without writing much CSS. Here are some resources for learning flexbox:
 @## Layering
 
 Blueprint provides variables for three z-index layers. This should be enough for most use cases,
-especially if you make correct use of [stacking context][MDN]. [Overlay](#core/components/overlay)
+especially if you make correct use of [stacking context][MDN]. [Overlay2](#core/components/overlay2)
 components such as dialogs and popovers use these z-index values to configure their stacking
 contexts.