From f65b31d34e8baca6f1d0733823f933d8b4f2c419 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 30 Apr 2020 00:26:21 +0200 Subject: [PATCH 1/3] [TrapFocus] Guard against dropped memo cache --- packages/material-ui/src/Modal/TrapFocus.js | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/material-ui/src/Modal/TrapFocus.js b/packages/material-ui/src/Modal/TrapFocus.js index 513072a488232d..0ee83181025a1e 100644 --- a/packages/material-ui/src/Modal/TrapFocus.js +++ b/packages/material-ui/src/Modal/TrapFocus.js @@ -31,15 +31,21 @@ function TrapFocus(props) { }, []); const handleRef = useForkRef(children.ref, handleOwnRef); - // ⚠️ You may rely on React.useMemo as a performance optimization, not as a semantic guarantee. - // https://reactjs.org/docs/hooks-reference.html#usememo - React.useMemo(() => { - if (!open || typeof window === 'undefined') { - return; - } - + const prevOpenRef = React.useRef(); + React.useEffect(() => { + prevOpenRef.current = true; + }, [open]); + if (!prevOpenRef.current && open && typeof window !== 'undefined') { + // WARNING: Potentially unsafe in concurrent mode. + // The way the read on `nodeToRestore` is setup could make this actually safe. + // Say render `open={false}` -> `open={true}` but never commit. + // We have now written a state that wasn't committed. But no committed effect + // will read this wrong value. We only read from `nodeToRestore` in effects + // that were committed on `open={true}` + // WARNING: Prevents the instance from being garbage collected. Should only + // hold a weak ref. nodeToRestore.current = getDoc().activeElement; - }, [open]); // eslint-disable-line react-hooks/exhaustive-deps + } React.useEffect(() => { if (!open) { From 444c8c2ae8f78129d8f90f436fe7d3b28a6a04a6 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 30 Apr 2020 12:06:15 +0200 Subject: [PATCH 2/3] Update packages/material-ui/src/Modal/TrapFocus.js Co-Authored-By: Josh Wooding <12938082+joshwooding@users.noreply.github.com> --- packages/material-ui/src/Modal/TrapFocus.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/material-ui/src/Modal/TrapFocus.js b/packages/material-ui/src/Modal/TrapFocus.js index 0ee83181025a1e..ac5782f9872d43 100644 --- a/packages/material-ui/src/Modal/TrapFocus.js +++ b/packages/material-ui/src/Modal/TrapFocus.js @@ -33,7 +33,7 @@ function TrapFocus(props) { const prevOpenRef = React.useRef(); React.useEffect(() => { - prevOpenRef.current = true; + prevOpenRef.current = open; }, [open]); if (!prevOpenRef.current && open && typeof window !== 'undefined') { // WARNING: Potentially unsafe in concurrent mode. From 5d0c2a502946ab1f6f3ec70383e1b59b45fcac03 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Sat, 2 May 2020 20:24:40 +0200 Subject: [PATCH 3/3] Update packages/material-ui/src/Modal/TrapFocus.js --- packages/material-ui/src/Modal/TrapFocus.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/material-ui/src/Modal/TrapFocus.js b/packages/material-ui/src/Modal/TrapFocus.js index ac5782f9872d43..0c6fbae6dfcbc0 100644 --- a/packages/material-ui/src/Modal/TrapFocus.js +++ b/packages/material-ui/src/Modal/TrapFocus.js @@ -38,7 +38,7 @@ function TrapFocus(props) { if (!prevOpenRef.current && open && typeof window !== 'undefined') { // WARNING: Potentially unsafe in concurrent mode. // The way the read on `nodeToRestore` is setup could make this actually safe. - // Say render `open={false}` -> `open={true}` but never commit. + // Say we render `open={false}` -> `open={true}` but never commit. // We have now written a state that wasn't committed. But no committed effect // will read this wrong value. We only read from `nodeToRestore` in effects // that were committed on `open={true}`