Fix Overlay2 storing stale onClose callback #6874

evansjohnson · 2024-06-26T14:44:02Z

Fixes #6748

Checklist

Includes tests
[-] Update documentation

Changes proposed in this pull request:

There are 3 changes in this PR, will split to separate PRs on request but figured they are closely related:

Fix cleaning up event listeners - previously we'd always be trying to remove the current version of the event listener that will have never been set yet.
Ensure we re-attach latest version of event listeners if for example onClose updates. We handle onClose updating for esc key and backdrop click because the latest callback versions are always passed to the element those are attached to, but did not do a similar update to the document listeners. A test case has been added for this behavior.
Ensure we don't call a stale version of the overlayWillClose callback on component unmount: evanj/overlay-fix?expand=1#diff-50ceef9007

Reviewers should focus on:

Any possible breaking changes
Any possible mistakes when translating logic from overlayWillOpen callback directly into useEffect hooks

Screenshot

n/a

changelog-app · 2024-06-26T14:44:07Z

Generate changelog in `packages/core/changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

Fix Overlay2 storing stale onClose callback

Check the box to generate changelog(s)

Generate changelog entry

evansjohnson

it may be easier to first review this diff: https://github.com/palantir/blueprint/pull/6874/files/f49bee2d29a5a01eecfe831de5cf5121f1e4cb1a and then see the last commit that just moves the hooks up top lower

evansjohnson · 2024-06-26T14:47:42Z

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

@@ -325,14 +306,41 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
        }
    }, [isOpen, overlayWillOpen, overlayWillClose, prevIsOpen]);

-    // run once on unmount
+    // Important: clean up old document-level event listeners if their memoized values change (this is rare, but


I'm not sure if this order matters - I moved these below the useEffect that calls overlayWillOpen because these event listeners were attached towards the end of that method and I believe useEffect hooks are called in order. But now I'm seeing these were also attached before setRef(lastActiveElementBeforeOpened, getActiveElement(getRef(containerElement))); and now will be after.

I don't expect that to be an issue, but also don't see any issues if these get attached before the other code in overlayWillOpen runs.

evansjohnson · 2024-06-26T14:51:24Z

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

        return () => {
-            // we need to run cleanup code to remove some event handlers from the overlay element
-            overlayWillClose();


I wasn't able to find a test case that can assert this because we only call onClose from user actions, but I believe this is a function that has a stale closure around old versions of the document event listeners that may have already been removed.

Now we rely on the useEffect that adds the event listeners to clean up themselves - but continue calling this for the other overlay cleanup code. I didn't see any warnings about calling removeEventListener twice/for an already removed handler, and also didn't see any issues with this from my testing, but please flag if this seems bad.

I believe this is a function that has a stale closure around old versions of the document event listeners that may have already been removed.

Agree, overlayWillClose was not a dependency of this effect but itself had dependencies.

I didn't see any warnings about calling removeEventListener twice

Quoting the page you linked:

Calling removeEventListener() with arguments that do not identify any currently registered event listener on the EventTarget has no effect.

This should be fine.

svc-palantir-github · 2024-06-26T14:54:26Z

this should keep order closer to what it was before

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson · 2024-06-26T14:54:26Z

packages/core/test/overlay2/overlay2Tests.tsx

            dispatchMouseEvent(document.documentElement, "mousedown");
-            assert.isTrue(onClose.calledOnce);
+            assert.isTrue(onClose.notCalled);
+            assert.isTrue(onClose2.calledOnce);


this test case was previously failing - the other methods of closing handled updating onClose properly

svc-palantir-github · 2024-06-26T15:21:34Z

format

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson · 2024-06-26T20:07:43Z

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

+        // run cleanup code once on unmount, ensuring we call the most recent overlayWillClose callback
+        // by storing in a ref and keeping up to date
+        return () => {
+            mostRecetOverlayWillClose.current();


similar to #6753 (review) this diff brings this closer to the original behavior, as overlayWillClose was previously a class method and so could never be a stale reference

evansjohnson · 2024-06-26T20:10:44Z

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

-    // 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]);
-


linking diff that added: https://github.com/palantir/blueprint/pull/6681/files#diff-50ceef90072c82bdf126a28d4f90f718d8a384f60d4ac90e7b1a9d2c4f4e57d4R227-R232

I think this had the right idea but I don't think it does what the comment suggests

I don't think it does what the comment suggests

Do you mean because it's doing this directly in the effect vs. in a cleanup function returned from the effect? Or did you find something else?

yea, since it's directly in the effect it's reference the version of the handler that was just defined and never had a chance to be attached (or worse was just intentionally attached, but I didn't find a case of this happening)

invliD · 2024-06-26T20:15:33Z

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

        return () => {
-            // we need to run cleanup code to remove some event handlers from the overlay element
-            overlayWillClose();


I believe this is a function that has a stale closure around old versions of the document event listeners that may have already been removed.

Agree, overlayWillClose was not a dependency of this effect but itself had dependencies.

I didn't see any warnings about calling removeEventListener twice

Quoting the page you linked:

Calling removeEventListener() with arguments that do not identify any currently registered event listener on the EventTarget has no effect.

This should be fine.

invliD · 2024-06-26T20:18:56Z

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

-    // 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]);
-


I don't think it does what the comment suggests

Do you mean because it's doing this directly in the effect vs. in a cleanup function returned from the effect? Or did you find something else?

invliD · 2024-06-26T20:28:26Z

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

@@ -307,6 +276,8 @@ export const Overlay2 = React.forwardRef<OverlayInstance, Overlay2Props>((props,
            }
        }
    }, [closeOverlay, getLastOpened, handleDocumentFocus, handleDocumentMousedown, id]);
+    const mostRecetOverlayWillClose = React.useRef(overlayWillClose);


typo

Suggested change

const mostRecetOverlayWillClose = React.useRef(overlayWillClose);

const mostRecentOverlayWillClose = React.useRef(overlayWillClose);

ah thanks! I decided "mostRecent" was weird anyways and went with overlayWillCloseRef and also co-located it with the useEffect so it's a bit more clear why we have this ref

Invalidated by push of e7e56d3

svc-palantir-github · 2024-06-26T20:45:58Z

better name and co-locate

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

evansjohnson added 3 commits June 26, 2024 10:20

add test

f07eacf

fix and test all close conditions

f49bee2

this should keep order closer to what it was before

2f8be21

evansjohnson commented Jun 26, 2024

View reviewed changes

evansjohnson added 2 commits June 26, 2024 11:06

remove extra deps

0228043

format

e08e9e5

evansjohnson assigned ericjeney Jun 26, 2024

ericjeney assigned invliD Jun 26, 2024

evansjohnson commented Jun 26, 2024

View reviewed changes

invliD previously approved these changes Jun 26, 2024

View reviewed changes

better name and co-locate

faf9128

evansjohnson force-pushed the evanj/overlay-fix branch from e7e56d3 to faf9128 Compare June 26, 2024 20:33

evansjohnson unassigned ericjeney Jun 26, 2024

evansjohnson requested a review from invliD June 26, 2024 20:34

gluxon self-assigned this Jun 28, 2024

invliD approved these changes Jul 9, 2024

View reviewed changes

evansjohnson merged commit d623cb5 into develop Jul 9, 2024
12 of 14 checks passed

evansjohnson deleted the evanj/overlay-fix branch July 9, 2024 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Overlay2 storing stale onClose callback #6874

Fix Overlay2 storing stale onClose callback #6874

evansjohnson commented Jun 26, 2024

changelog-app bot commented Jun 26, 2024

evansjohnson left a comment

evansjohnson Jun 26, 2024

evansjohnson Jun 26, 2024

invliD Jun 26, 2024

svc-palantir-github commented Jun 26, 2024

evansjohnson Jun 26, 2024

svc-palantir-github commented Jun 26, 2024

evansjohnson Jun 26, 2024

evansjohnson Jun 26, 2024

invliD Jun 26, 2024

evansjohnson Jun 26, 2024

invliD Jun 26, 2024

invliD Jun 26, 2024

invliD Jun 26, 2024

evansjohnson Jun 26, 2024

svc-palantir-github commented Jun 26, 2024

	const mostRecetOverlayWillClose = React.useRef(overlayWillClose);
	const mostRecentOverlayWillClose = React.useRef(overlayWillClose);

Fix Overlay2 storing stale onClose callback #6874

Fix Overlay2 storing stale onClose callback #6874

Conversation

evansjohnson commented Jun 26, 2024

Fixes #6748

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

changelog-app bot commented Jun 26, 2024

Generate changelog in packages/core/changelog/@unreleased

evansjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jun 26, 2024

this should keep order closer to what it was before

Choose a reason for hiding this comment

svc-palantir-github commented Jun 26, 2024

format

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-palantir-github commented Jun 26, 2024

better name and co-locate

Generate changelog in `packages/core/changelog/@unreleased`