Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TrapFocus] Guard against dropped memo cache #20848

Merged
merged 3 commits into from
May 3, 2020
Merged

[TrapFocus] Guard against dropped memo cache #20848

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f65b31d
[TrapFocus] Guard against dropped memo cache
eps1lon Apr 29, 2020
444c8c2
Update packages/material-ui/src/Modal/TrapFocus.js
eps1lon Apr 30, 2020
5d0c2a5
Update packages/material-ui/src/Modal/TrapFocus.js
eps1lon May 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions packages/material-ui/src/Modal/TrapFocus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
}, [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.
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
// 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) {
Expand Down