From 63dab0860f6e1ad7e180d0dfcd05de2741f9cce3 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Mon, 19 Dec 2022 16:32:34 -0800
Subject: [PATCH 01/15] tentative change to fix FocusScope strict mode

---
 packages/@react-aria/focus/src/FocusScope.tsx | 66 +++++++++++++++----
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index bd84a79c577..908add5f737 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -13,7 +13,7 @@
 import {FocusableElement} from '@react-types/shared';
 import {focusSafely} from './focusSafely';
 import {isElementVisible} from './isElementVisible';
-import React, {ReactNode, RefObject, useContext, useEffect, useMemo, useRef} from 'react';
+import React, {ReactNode, RefObject, useCallback, useContext, useEffect, useMemo, useRef} from 'react';
 import {useLayoutEffect} from '@react-aria/utils';
 
 
@@ -62,7 +62,8 @@ export interface FocusManager {
 type ScopeRef = RefObject<Element[]>;
 interface IFocusContext {
   scopeRef: ScopeRef,
-  focusManager: FocusManager
+  focusManager: FocusManager,
+  addParentToFocusScopeTree: () => ScopeRef
 }
 
 const FocusContext = React.createContext<IFocusContext>(null);
@@ -86,10 +87,57 @@ export function FocusScope(props: FocusScopeProps) {
   let scopeRef = useRef<Element[]>([]);
   let ctx = useContext(FocusContext);
 
+  let addParentToFocusScopeTree = useCallback(() => {
+    let parentScope = null;
+    if (ctx) {
+      parentScope = ctx.addParentToFocusScopeTree();
+    }
+
+    if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
+      focusScopeTree.addTreeNode(scopeRef, parentScope);
+    }
+
+    return scopeRef;
+  }, [ctx]);
+
+  // TODO: useCallback?
+  // let addParentToFocusScopeTree = () => {
+  //   let parentScope = null;
+  //   if (ctx) {
+  //     parentScope = ctx.addParentToFocusScopeTree();
+  //   }
+
+  //   if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
+  //     focusScopeTree.addTreeNode(scopeRef, parentScope);
+  //   }
+
+  //   return scopeRef;
+  // };
+
+  useLayoutEffect(() => {
+    let parentScope = null;
+    if (ctx) {
+      // Add parent to tree and every parent all the way up
+      parentScope = ctx.addParentToFocusScopeTree();
+    }
+
+    // Add self to tree
+    if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
+      focusScopeTree.addTreeNode(scopeRef, parentScope);
+    }
+  // TODO: double check this dep arrays
+  }, [ctx]);
+
+  useLayoutEffect(() => {
+    let node = focusScopeTree.getTreeNode(scopeRef);
+    node.contain = contain;
+  }, [contain]);
+
   // The parent scope is based on the JSX tree, using context.
   // However, if a new scope mounts outside the active scope (e.g. DialogContainer launched from a menu),
   // we want the parent scope to be the active scope instead.
   let ctxParent = ctx?.scopeRef ?? null;
+  // TODO: in render, move inside the useLayoutEffects? Maybe ok now since the focusScopeTree fast map is only updated in layoutEffect
   let parentScope = useMemo(() => activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : ctxParent, [ctxParent]);
 
   useLayoutEffect(() => {
@@ -104,15 +152,6 @@ export function FocusScope(props: FocusScopeProps) {
     scopeRef.current = nodes;
   }, [children, parentScope]);
 
-  // add to the focus scope tree in render order because useEffects/useLayoutEffects run children first whereas render runs parent first
-  // which matters when constructing a tree
-  if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
-    focusScopeTree.addTreeNode(scopeRef, parentScope);
-  }
-
-  let node = focusScopeTree.getTreeNode(scopeRef);
-  node.contain = contain;
-
   useActiveScopeTracker(scopeRef, restoreFocus, contain);
   useFocusContainment(scopeRef, contain);
   useRestoreFocus(scopeRef, restoreFocus, contain);
@@ -140,9 +179,8 @@ export function FocusScope(props: FocusScopeProps) {
   }, [scopeRef, parentScope]);
 
   let focusManager = createFocusManagerForScope(scopeRef);
-
   return (
-    <FocusContext.Provider value={{scopeRef, focusManager}}>
+    <FocusContext.Provider value={{scopeRef, focusManager, addParentToFocusScopeTree}}>
       <span data-focus-scope-start hidden ref={startRef} />
       {children}
       <span data-focus-scope-end hidden ref={endRef} />
@@ -608,6 +646,7 @@ function useRestoreFocus(scopeRef: RefObject<Element[]>, restoreFocus: boolean,
             // ancestor scope that is still in the tree.
             treeNode = clonedTree.getTreeNode(scopeRef);
             while (treeNode) {
+              // TODO: in raf
               if (treeNode.scopeRef && focusScopeTree.getTreeNode(treeNode.scopeRef)) {
                 focusFirstInScope(treeNode.scopeRef.current, true);
                 return;
@@ -805,6 +844,7 @@ class Tree {
     if (children.length > 0) {
       children.forEach(child => parentNode.addChild(child));
     }
+    // debugger
     this.fastMap.delete(node.scopeRef);
   }
 

From ca5c2f9aeb3bcf0b54858a0ddc5a4072a45d8d1f Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Mon, 19 Dec 2022 17:56:06 -0800
Subject: [PATCH 02/15] cleanup

---
 packages/@react-aria/focus/src/FocusScope.tsx | 39 +++++++------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 908add5f737..da932cd81ad 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -85,12 +85,16 @@ export function FocusScope(props: FocusScopeProps) {
   let startRef = useRef<HTMLSpanElement>();
   let endRef = useRef<HTMLSpanElement>();
   let scopeRef = useRef<Element[]>([]);
-  let ctx = useContext(FocusContext);
+  let {
+    addParentToFocusScopeTree: addParentToTreeMap,
+    scopeRef: parentScopeRef
+  } = useContext(FocusContext) || {};
 
+  // Called to add parent and every ancestor scopeRef if they haven't been added yet.
   let addParentToFocusScopeTree = useCallback(() => {
     let parentScope = null;
-    if (ctx) {
-      parentScope = ctx.addParentToFocusScopeTree();
+    if (addParentToTreeMap != null) {
+      parentScope = addParentToTreeMap();
     }
 
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
@@ -98,35 +102,20 @@ export function FocusScope(props: FocusScopeProps) {
     }
 
     return scopeRef;
-  }, [ctx]);
-
-  // TODO: useCallback?
-  // let addParentToFocusScopeTree = () => {
-  //   let parentScope = null;
-  //   if (ctx) {
-  //     parentScope = ctx.addParentToFocusScopeTree();
-  //   }
-
-  //   if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
-  //     focusScopeTree.addTreeNode(scopeRef, parentScope);
-  //   }
-
-  //   return scopeRef;
-  // };
+  }, [addParentToTreeMap]);
 
   useLayoutEffect(() => {
     let parentScope = null;
-    if (ctx) {
-      // Add parent to tree and every parent all the way up
-      parentScope = ctx.addParentToFocusScopeTree();
+    if (addParentToTreeMap != null) {
+      parentScope = addParentToTreeMap();
     }
 
-    // Add self to tree
+    // For outer most leaf in tree, need to add self to tree map
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
       focusScopeTree.addTreeNode(scopeRef, parentScope);
     }
-  // TODO: double check this dep arrays
-  }, [ctx]);
+  // TODO: double check this dep array
+  }, [addParentToTreeMap]);
 
   useLayoutEffect(() => {
     let node = focusScopeTree.getTreeNode(scopeRef);
@@ -136,7 +125,7 @@ export function FocusScope(props: FocusScopeProps) {
   // The parent scope is based on the JSX tree, using context.
   // However, if a new scope mounts outside the active scope (e.g. DialogContainer launched from a menu),
   // we want the parent scope to be the active scope instead.
-  let ctxParent = ctx?.scopeRef ?? null;
+  let ctxParent = parentScopeRef ?? null;
   // TODO: in render, move inside the useLayoutEffects? Maybe ok now since the focusScopeTree fast map is only updated in layoutEffect
   let parentScope = useMemo(() => activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : ctxParent, [ctxParent]);
 

From d6d99b28635307dce1e5cd7f73792a668a1d5e63 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Tue, 20 Dec 2022 11:25:38 -0800
Subject: [PATCH 03/15] step back some optimization until we fix all issues

---
 packages/@react-aria/focus/src/FocusScope.tsx | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index da932cd81ad..8c0734768af 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -90,8 +90,8 @@ export function FocusScope(props: FocusScopeProps) {
     scopeRef: parentScopeRef
   } = useContext(FocusContext) || {};
 
-  // Called to add parent and every ancestor scopeRef if they haven't been added yet.
-  let addParentToFocusScopeTree = useCallback(() => {
+  // Call to add current scope to Tree if it hasn't been added yet. Adds parent scope to Tree if it doesn't exist.
+  let addParentToFocusScopeTree = () => {
     let parentScope = null;
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
@@ -102,8 +102,9 @@ export function FocusScope(props: FocusScopeProps) {
     }
 
     return scopeRef;
-  }, [addParentToTreeMap]);
+  };
 
+  // Construct Tree on inital mount. Generate from top most parent to bottom most child by calling addParentToTreeMap from parent context.
   useLayoutEffect(() => {
     let parentScope = null;
     if (addParentToTreeMap != null) {
@@ -114,8 +115,8 @@ export function FocusScope(props: FocusScopeProps) {
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
       focusScopeTree.addTreeNode(scopeRef, parentScope);
     }
-  // TODO: double check this dep array
-  }, [addParentToTreeMap]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);
 
   useLayoutEffect(() => {
     let node = focusScopeTree.getTreeNode(scopeRef);

From ff22268d7d5e6499690c953131fe1b558038839f Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Wed, 21 Dec 2022 15:02:44 -0800
Subject: [PATCH 04/15] properly assign new top level scope parent

fix case where deleting a row via actionbar didnt restore focus to the new first row.
---
 packages/@react-aria/focus/src/FocusScope.tsx | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 8c0734768af..6dc07c032e1 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -95,6 +95,8 @@ export function FocusScope(props: FocusScopeProps) {
     let parentScope = null;
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
+    } else {
+      parentScope = activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : parentScope
     }
 
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {

From 0b321afda1fd651de39ee0f2ec21c2d65683b883 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Wed, 21 Dec 2022 16:17:40 -0800
Subject: [PATCH 05/15] fix table crud case

---
 packages/@react-aria/focus/src/FocusScope.tsx | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 6dc07c032e1..e29b98079b0 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -95,8 +95,12 @@ export function FocusScope(props: FocusScopeProps) {
     let parentScope = null;
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
-    } else {
-      parentScope = activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : parentScope
+    } else if (activeScope && focusScopeTree.getTreeNode(activeScope)) {
+      // TODO: I removed the ancestorScope check since the ctxParent is null in this case so isAncestorScope will never to true unles the activeScope is somehow null as well
+      // in which it is a moot point anyways (activeScope === ctxParent then)
+      // If addParentToTreeMap doesn't exist on context, then the current FocusScope is either standalone or could be a overlay (e.g. DialogContainer launched from a menu)
+      // and thus perhaps outside the active scope. For the latter, we'll want
+      parentScope = activeScope;
     }
 
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
@@ -111,13 +115,15 @@ export function FocusScope(props: FocusScopeProps) {
     let parentScope = null;
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
+    } else if (activeScope && focusScopeTree.getTreeNode(activeScope)) {
+      parentScope = activeScope;
     }
 
     // For outer most leaf in tree, need to add self to tree map
     if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
       focusScopeTree.addTreeNode(scopeRef, parentScope);
     }
-    // eslint-disable-next-line react-hooks/exhaustive-deps
+
   }, []);
 
   useLayoutEffect(() => {
@@ -131,7 +137,7 @@ export function FocusScope(props: FocusScopeProps) {
   let ctxParent = parentScopeRef ?? null;
   // TODO: in render, move inside the useLayoutEffects? Maybe ok now since the focusScopeTree fast map is only updated in layoutEffect
   let parentScope = useMemo(() => activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : ctxParent, [ctxParent]);
-
+  // TODO: remove this parentScope and its existanve in the other layouteffects
   useLayoutEffect(() => {
     // Find all rendered nodes between the sentinels and add them to the scope.
     let node = startRef.current.nextSibling;
@@ -638,7 +644,6 @@ function useRestoreFocus(scopeRef: RefObject<Element[]>, restoreFocus: boolean,
             // ancestor scope that is still in the tree.
             treeNode = clonedTree.getTreeNode(scopeRef);
             while (treeNode) {
-              // TODO: in raf
               if (treeNode.scopeRef && focusScopeTree.getTreeNode(treeNode.scopeRef)) {
                 focusFirstInScope(treeNode.scopeRef.current, true);
                 return;

From 8da00f3ad5816fb6423e06c3cb11938aeaff310a Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Wed, 21 Dec 2022 16:38:22 -0800
Subject: [PATCH 06/15] add optimizations to fit assumptions

---
 packages/@react-aria/focus/src/FocusScope.tsx | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index e29b98079b0..d164c4f1bdd 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -91,7 +91,7 @@ export function FocusScope(props: FocusScopeProps) {
   } = useContext(FocusContext) || {};
 
   // Call to add current scope to Tree if it hasn't been added yet. Adds parent scope to Tree if it doesn't exist.
-  let addParentToFocusScopeTree = () => {
+  let addParentToFocusScopeTree = useCallback(() => {
     let parentScope = null;
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
@@ -108,7 +108,7 @@ export function FocusScope(props: FocusScopeProps) {
     }
 
     return scopeRef;
-  };
+  }, [addParentToTreeMap]);
 
   // Construct Tree on inital mount. Generate from top most parent to bottom most child by calling addParentToTreeMap from parent context.
   useLayoutEffect(() => {
@@ -124,7 +124,7 @@ export function FocusScope(props: FocusScopeProps) {
       focusScopeTree.addTreeNode(scopeRef, parentScope);
     }
 
-  }, []);
+  }, [addParentToTreeMap]);
 
   useLayoutEffect(() => {
     let node = focusScopeTree.getTreeNode(scopeRef);
@@ -176,9 +176,15 @@ export function FocusScope(props: FocusScopeProps) {
     }
   }, [scopeRef, parentScope]);
 
-  let focusManager = createFocusManagerForScope(scopeRef);
+  let focusManager = useMemo(() => createFocusManagerForScope(scopeRef), []);
+  let value = useMemo(() => ({
+    scopeRef,
+    focusManager,
+    addParentToFocusScopeTree
+  }), [addParentToFocusScopeTree, focusManager]);
+
   return (
-    <FocusContext.Provider value={{scopeRef, focusManager, addParentToFocusScopeTree}}>
+    <FocusContext.Provider value={value}>
       <span data-focus-scope-start hidden ref={startRef} />
       {children}
       <span data-focus-scope-end hidden ref={endRef} />

From a4cc4ee75b7b3049f477d6a4062c7b4fb9ddeea2 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Wed, 21 Dec 2022 17:19:41 -0800
Subject: [PATCH 07/15] fix ButtonGroup strictmode test

---
 .../@react-spectrum/buttongroup/test/ButtonGroup.test.js | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js b/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
index 84bf97eec95..fc472de75ba 100644
--- a/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
+++ b/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
@@ -137,7 +137,14 @@ describe('ButtonGroup', function () {
       };
       let {getByTestId} = render(<ButtonGroupWithRefs setUp={setUp} />);
       let buttonGroup = getByTestId(buttonGroupId);
-      expect(buttonGroup).not.toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));
+
+      if (process.env.STRICT_MODE) {
+        // If strict mode, render cycle happens twice which means a 2nd useLayoutEffect is triggered -> mock widths are now already set since
+        // ButtonGroupWithRefs's useEffect has already run
+        expect(buttonGroup).toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));
+      } else {
+        expect(buttonGroup).not.toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));
+      }
 
       // ResizeObserver not actually implemented in jsdom, so rely on the fallback to window resize listener
       act(() => {window.dispatchEvent(new Event('resize'));});

From e7da44e3d457343b035208fb07bf129447c36b96 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Thu, 22 Dec 2022 11:55:38 -0800
Subject: [PATCH 08/15] fixing focuscope test for strict mode

---
 packages/@react-aria/focus/test/FocusScope.test.js | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js
index e7f6e0217dc..78f19c0160c 100644
--- a/packages/@react-aria/focus/test/FocusScope.test.js
+++ b/packages/@react-aria/focus/test/FocusScope.test.js
@@ -1521,6 +1521,12 @@ describe('FocusScope', function () {
 
       fireEvent.keyDown(document.activeElement, {key: 'Enter'});
       fireEvent.keyUp(document.activeElement, {key: 'Enter'});
+
+      // Needed for onBlur raf in useFocusContainment
+      act(() => {
+        jest.runAllTimers();
+      });
+      // Needed useRestoreFocus raf
       act(() => {
         jest.runAllTimers();
       });

From 2ca2c1e0014372649685d043aa06302d2fc3380c Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Thu, 22 Dec 2022 12:11:15 -0800
Subject: [PATCH 09/15] making DialogTrigger tests not fail due to strict mode

since strict mode unmounts and rerenders the dialog, we will always run into the "unmounted while open" error in the tests if defaultOpen or isOpen is true. Since I think we cant distingush a true unmount case vs strict modes unmount case, I opted to suppress the warning in the test instead
---
 packages/@react-spectrum/dialog/src/DialogTrigger.tsx  |  5 ++++-
 .../@react-spectrum/dialog/test/DialogTrigger.test.js  | 10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/packages/@react-spectrum/dialog/src/DialogTrigger.tsx b/packages/@react-spectrum/dialog/src/DialogTrigger.tsx
index ab913e81d28..2bc96da6fa7 100644
--- a/packages/@react-spectrum/dialog/src/DialogTrigger.tsx
+++ b/packages/@react-spectrum/dialog/src/DialogTrigger.tsx
@@ -50,7 +50,10 @@ function DialogTrigger(props: SpectrumDialogTriggerProps) {
 
   let state = useOverlayTriggerState(props);
   let wasOpen = useRef(false);
-  wasOpen.current = state.isOpen;
+  useEffect(() => {
+    wasOpen.current = state.isOpen;
+  }, [state.isOpen]);
+
   let isExiting = useRef(false);
   let onExiting = () => isExiting.current = true;
   let onExited = () => isExiting.current = false;
diff --git a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js
index d207dbed30e..e6ec54f21a5 100644
--- a/packages/@react-spectrum/dialog/test/DialogTrigger.test.js
+++ b/packages/@react-spectrum/dialog/test/DialogTrigger.test.js
@@ -27,6 +27,7 @@ import userEvent from '@testing-library/user-event';
 
 describe('DialogTrigger', function () {
   let matchMedia;
+  let warnMock;
   beforeAll(() => {
     jest.spyOn(window.screen, 'width', 'get').mockImplementation(() => 1024);
     jest.useFakeTimers('legacy');
@@ -40,6 +41,9 @@ describe('DialogTrigger', function () {
     matchMedia = new MatchMediaMock();
     // this needs to be a setTimeout so that the dialog can be removed from the dom before the callback is invoked
     jest.spyOn(window, 'requestAnimationFrame').mockImplementation(cb => setTimeout(() => cb(), 0));
+    if (process.env.STRICT_MODE) {
+      warnMock = jest.spyOn(global.console, 'warn').mockImplementation();
+    }
   });
 
   afterEach(() => {
@@ -55,6 +59,12 @@ describe('DialogTrigger', function () {
 
     matchMedia.clear();
     window.requestAnimationFrame.mockRestore();
+
+    if (process.env.STRICT_MODE && warnMock.mock.calls.length > 0) {
+      expect(warnMock).toHaveBeenCalledTimes(1);
+      expect(warnMock).toHaveBeenCalledWith('A DialogTrigger unmounted while open. This is likely due to being placed within a trigger that unmounts or inside a conditional. Consider using a DialogContainer instead.');
+      warnMock.mockRestore();
+    }
   });
 
   it('should trigger a modal by default', function () {

From c819d300ea486b5acf227dbee43e91cfc16fb130 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Tue, 3 Jan 2023 13:18:51 -0800
Subject: [PATCH 10/15] ensure activeScope is accurate

in strict mode, the layouteffect cleanup triggers even when the focusscope hasnt unmounted. This makes activeScope inaccurate and breaks the parentScope for future nested focusscopes (e.g. nested dialog trigger case)
---
 packages/@react-aria/focus/src/FocusScope.tsx | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index d164c4f1bdd..0e62b9197db 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -158,6 +158,24 @@ export function FocusScope(props: FocusScopeProps) {
   // this layout effect needs to run last so that focusScopeTree cleanup happens at the last moment possible
   useLayoutEffect(() => {
     if (scopeRef) {
+      let activeElement = document.activeElement;
+      let scope = null;
+      // In strict mode, active scope is incorrectly updated since cleanup will run even though scope hasn't unmounted.
+      // To fix this, we need to update the actual activeScope here
+      if (isElementInScope(activeElement, scopeRef.current)) {
+        // Since useLayoutEffect runs for children first, we need to traverse the focusScope tree and find the bottom most scope that
+        // contains the active element and set that as the activeScope
+        for (let node of focusScopeTree.traverse()) {
+          if (isElementInScope(activeElement, node.scopeRef.current)) {
+            scope = node;
+          }
+        }
+
+        if (scope === focusScopeTree.getTreeNode(scopeRef)) {
+          activeScope = scope.scopeRef;
+        }
+      }
+
       return () => {
         // Scope may have been re-parented.
         let parentScope = focusScopeTree.getTreeNode(scopeRef).parent.scopeRef;

From dfd0138a4b2d6cd70490258cab63c10611b66f4c Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Tue, 3 Jan 2023 13:42:26 -0800
Subject: [PATCH 11/15] fixing tabs tests

---
 .../@react-spectrum/tabs/test/Tabs.test.js    | 95 +++++++++----------
 packages/dev/test-utils/src/index.ts          |  1 +
 .../dev/test-utils/src/mockImplementation.ts  | 64 +++++++++++++
 3 files changed, 109 insertions(+), 51 deletions(-)
 create mode 100644 packages/dev/test-utils/src/mockImplementation.ts

diff --git a/packages/@react-spectrum/tabs/test/Tabs.test.js b/packages/@react-spectrum/tabs/test/Tabs.test.js
index 8c2a9996c5a..29bba78129a 100644
--- a/packages/@react-spectrum/tabs/test/Tabs.test.js
+++ b/packages/@react-spectrum/tabs/test/Tabs.test.js
@@ -10,7 +10,7 @@
  * governing permissions and limitations under the License.
  */
 
-import {act, fireEvent, render, triggerPress, waitFor, within} from '@react-spectrum/test-utils';
+import {act, fireEvent, mockImplementation, render, triggerPress, waitFor, within} from '@react-spectrum/test-utils';
 import {Item, TabList, TabPanels, Tabs} from '../src';
 import {Provider} from '@react-spectrum/provider';
 import React from 'react';
@@ -347,19 +347,21 @@ describe('Tabs', function () {
   });
 
   it('collapses when it can\'t render all the tabs horizontally', function () {
-    jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+    let target = [HTMLDivElement.prototype, 'getBoundingClientRect'];
+    let mockCalls = [
+      function () {
         return {
           right: 500
         };
-      }
-    }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+      },
+      function () {
         return {
           right: 700
         };
       }
-    });
+    ];
+
+    mockImplementation(target, mockCalls, false);
 
     let {getByRole, queryByRole} = renderComponent({
       'aria-label': 'Test Tabs',
@@ -394,18 +396,14 @@ describe('Tabs', function () {
   });
 
   it('doesn\'t collapse when it can render all the tabs horizontally', function () {
-    jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 500
-        };
-      }
+    jest.spyOn(HTMLDivElement.prototype, 'getBoundingClientRect').mockImplementationOnce(function () {
+      return {
+        right: 500
+      };
     }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 400
-        };
-      }
+      return {
+        right: 400
+      };
     });
 
     let {getByRole, queryByRole} = renderComponent();
@@ -415,20 +413,21 @@ describe('Tabs', function () {
   });
 
   it('dynamically collapses and expands on tab addition/subtraction', function () {
-
-    let spy = jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+    let target = [HTMLDivElement.prototype, 'getBoundingClientRect'];
+    let mockCalls = [
+      function () {
         return {
           right: 500
         };
-      }
-    }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+      },
+      function () {
         return {
           right: 400
         };
       }
-    });
+    ];
+
+    let spy = mockImplementation(target, mockCalls, false);
 
     let {getByRole, queryByRole, rerender} = render(
       <Provider theme={theme}>
@@ -453,17 +452,13 @@ describe('Tabs', function () {
     expect(queryByRole('button')).toBeNull();
 
     spy.mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 500
-        };
-      }
+      return {
+        right: 500
+      };
     }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 700
-        };
-      }
+      return {
+        right: 700
+      };
     });
 
     let newItems = [...defaultItems];
@@ -525,17 +520,13 @@ describe('Tabs', function () {
     expect(tabpanel).toHaveAttribute('aria-labelledby', defaultItems[0].id);
 
     spy.mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 500
-        };
-      }
+      return {
+        right: 500
+      };
     }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
-        return {
-          right: 400
-        };
-      }
+      return {
+        right: 400
+      };
     });
 
     newItems = [...defaultItems];
@@ -570,19 +561,21 @@ describe('Tabs', function () {
   });
 
   it('disabled tabs cannot be selected via collapse picker', function () {
-    jest.spyOn(HTMLElement.prototype, 'getBoundingClientRect').mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+    let target = [HTMLDivElement.prototype, 'getBoundingClientRect'];
+    let mockCalls = [
+      function () {
         return {
           right: 500
         };
-      }
-    }).mockImplementationOnce(function () {
-      if (this instanceof HTMLDivElement) {
+      },
+      function () {
         return {
           right: 700
         };
       }
-    });
+    ];
+
+    mockImplementation(target, mockCalls);
 
     let {getByRole, queryByRole} = renderComponent({
       'aria-label': 'Test Tabs',
diff --git a/packages/dev/test-utils/src/index.ts b/packages/dev/test-utils/src/index.ts
index e65e9d169d9..89ed02e73e3 100644
--- a/packages/dev/test-utils/src/index.ts
+++ b/packages/dev/test-utils/src/index.ts
@@ -16,3 +16,4 @@ export * from './events';
 export * from './testSSR';
 export * from './renderOverride';
 export * from './StrictModeWrapper';
+export * from './mockImplementation';
diff --git a/packages/dev/test-utils/src/mockImplementation.ts b/packages/dev/test-utils/src/mockImplementation.ts
new file mode 100644
index 00000000000..1b4ed951446
--- /dev/null
+++ b/packages/dev/test-utils/src/mockImplementation.ts
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2020 Adobe. All rights reserved.
+ * This file is licensed to you under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License. You may obtain a copy
+ * of the License at http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under
+ * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
+ * OF ANY KIND, either express or implied. See the License for the specific language
+ * governing permissions and limitations under the License.
+ */
+
+type Method = [any, string];
+type Access = [any, string, 'get' | 'set'];
+type SpyTarget = Method | Access
+type MockCalls = Array<() => any>
+
+// Util for mockImplementation that automatically handles extra calls needed for strict mode
+export function mockImplementation(spyTarget: SpyTarget, mockCalls: MockCalls, retainLastMock = true) {
+  if (mockCalls.length === 0) {
+    throw new Error('MockCalls must have at least one mock implementation provided');
+  }
+
+  // multiple override definition of spyOn doesn't work with our type
+  let spy = jest.spyOn(...(spyTarget as [any, string, 'get']));
+  mockCalls.forEach(mock => {
+    spy.mockImplementationOnce(mock);
+  });
+
+  if (process.env.STRICT_MODE) {
+    mockCalls.forEach(mock => {
+      spy.mockImplementationOnce(mock);
+    });
+  }
+
+  if (retainLastMock) {
+    spy.mockImplementation(mockCalls.at(-1));
+  }
+
+  return spy;
+}
+
+// Same as above but accepts an pre-existing spy instead of creating a new one
+export function mockImplementationAdditional(spy: jest.SpyInstance<any, []>, mockCalls: MockCalls, retainLastMock = true) {
+  if (mockCalls.length === 0) {
+    throw new Error('MockCalls must have at least one mock implementation provided');
+  }
+
+  mockCalls.forEach(mock => {
+    spy.mockImplementationOnce(mock);
+  });
+
+  if (process.env.STRICT_MODE) {
+    mockCalls.forEach(mock => {
+      spy.mockImplementationOnce(mock);
+    });
+  }
+
+  if (retainLastMock) {
+    spy.mockImplementation(mockCalls.at(-1));
+  }
+
+  return spy;
+}

From 9908d179d1b3ff8df83e44ab80db4fc482e0ab0a Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Tue, 3 Jan 2023 14:10:18 -0800
Subject: [PATCH 12/15] removing extraneous parentScope calculation

---
 packages/@react-aria/focus/src/FocusScope.tsx   | 17 ++++-------------
 .../@react-aria/focus/test/FocusScope.test.js   |  2 +-
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 0e62b9197db..4ad6e46d224 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -96,10 +96,8 @@ export function FocusScope(props: FocusScopeProps) {
     if (addParentToTreeMap != null) {
       parentScope = addParentToTreeMap();
     } else if (activeScope && focusScopeTree.getTreeNode(activeScope)) {
-      // TODO: I removed the ancestorScope check since the ctxParent is null in this case so isAncestorScope will never to true unles the activeScope is somehow null as well
-      // in which it is a moot point anyways (activeScope === ctxParent then)
       // If addParentToTreeMap doesn't exist on context, then the current FocusScope is either standalone or could be a overlay (e.g. DialogContainer launched from a menu)
-      // and thus perhaps outside the active scope. For the latter, we'll want
+      // and thus perhaps outside the active scope. For the latter, we'll want to set the new scope's parent as the active scope so we can restore focus when the scope unmounts
       parentScope = activeScope;
     }
 
@@ -131,13 +129,6 @@ export function FocusScope(props: FocusScopeProps) {
     node.contain = contain;
   }, [contain]);
 
-  // The parent scope is based on the JSX tree, using context.
-  // However, if a new scope mounts outside the active scope (e.g. DialogContainer launched from a menu),
-  // we want the parent scope to be the active scope instead.
-  let ctxParent = parentScopeRef ?? null;
-  // TODO: in render, move inside the useLayoutEffects? Maybe ok now since the focusScopeTree fast map is only updated in layoutEffect
-  let parentScope = useMemo(() => activeScope && focusScopeTree.getTreeNode(activeScope) && !isAncestorScope(activeScope, ctxParent) ? activeScope : ctxParent, [ctxParent]);
-  // TODO: remove this parentScope and its existanve in the other layouteffects
   useLayoutEffect(() => {
     // Find all rendered nodes between the sentinels and add them to the scope.
     let node = startRef.current.nextSibling;
@@ -148,7 +139,7 @@ export function FocusScope(props: FocusScopeProps) {
     }
 
     scopeRef.current = nodes;
-  }, [children, parentScope]);
+  }, [children]);
 
   useActiveScopeTracker(scopeRef, restoreFocus, contain);
   useFocusContainment(scopeRef, contain);
@@ -192,7 +183,7 @@ export function FocusScope(props: FocusScopeProps) {
         focusScopeTree.removeTreeNode(scopeRef);
       };
     }
-  }, [scopeRef, parentScope]);
+  }, [scopeRef]);
 
   let focusManager = useMemo(() => createFocusManagerForScope(scopeRef), []);
   let value = useMemo(() => ({
@@ -865,7 +856,7 @@ class Tree {
     if (children.length > 0) {
       children.forEach(child => parentNode.addChild(child));
     }
-    // debugger
+
     this.fastMap.delete(node.scopeRef);
   }
 
diff --git a/packages/@react-aria/focus/test/FocusScope.test.js b/packages/@react-aria/focus/test/FocusScope.test.js
index 78f19c0160c..30122fe59f7 100644
--- a/packages/@react-aria/focus/test/FocusScope.test.js
+++ b/packages/@react-aria/focus/test/FocusScope.test.js
@@ -1526,7 +1526,7 @@ describe('FocusScope', function () {
       act(() => {
         jest.runAllTimers();
       });
-      // Needed useRestoreFocus raf
+      // Needed for useRestoreFocus layout cleanup raf
       act(() => {
         jest.runAllTimers();
       });

From 982c9668d79ede8dcfb9c7a2edfef0d9193cd69a Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Tue, 3 Jan 2023 14:25:21 -0800
Subject: [PATCH 13/15] more cleanup

---
 packages/@react-aria/focus/src/FocusScope.tsx | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 4ad6e46d224..6555e77e986 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -61,7 +61,6 @@ export interface FocusManager {
 
 type ScopeRef = RefObject<Element[]>;
 interface IFocusContext {
-  scopeRef: ScopeRef,
   focusManager: FocusManager,
   addParentToFocusScopeTree: () => ScopeRef
 }
@@ -85,10 +84,7 @@ export function FocusScope(props: FocusScopeProps) {
   let startRef = useRef<HTMLSpanElement>();
   let endRef = useRef<HTMLSpanElement>();
   let scopeRef = useRef<Element[]>([]);
-  let {
-    addParentToFocusScopeTree: addParentToTreeMap,
-    scopeRef: parentScopeRef
-  } = useContext(FocusContext) || {};
+  let {addParentToFocusScopeTree: addParentToTreeMap} = useContext(FocusContext) || {};
 
   // Call to add current scope to Tree if it hasn't been added yet. Adds parent scope to Tree if it doesn't exist.
   let addParentToFocusScopeTree = useCallback(() => {
@@ -187,7 +183,6 @@ export function FocusScope(props: FocusScopeProps) {
 
   let focusManager = useMemo(() => createFocusManagerForScope(scopeRef), []);
   let value = useMemo(() => ({
-    scopeRef,
     focusManager,
     addParentToFocusScopeTree
   }), [addParentToFocusScopeTree, focusManager]);

From 0c72b09853212d0b59b508c2588274be45031514 Mon Sep 17 00:00:00 2001
From: Daniel Lu <dl1644@gmail.com>
Date: Wed, 4 Jan 2023 16:52:41 -0800
Subject: [PATCH 14/15] updating test for strict mode and non strict mode
 consistency

the previous expect was relying on the fact that ButtonGroup first measurement happened before we mock the widths and positions of the buttons + buttongroups. That doesnt work well in strict mode and didnt feel reflective of browser behavior so I removed that expect
---
 .../@react-spectrum/buttongroup/test/ButtonGroup.test.js | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js b/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
index fc472de75ba..c6c2734ea8e 100644
--- a/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
+++ b/packages/@react-spectrum/buttongroup/test/ButtonGroup.test.js
@@ -125,7 +125,6 @@ describe('ButtonGroup', function () {
   describe('resizing', () => {
     it('goes vertical if there is not enough room after buttongroup gets a new size', () => {
       let setUp = ({buttonGroup, button1, button2, button3}) => {
-        // can't do anything about first render, so this starts with the resize
         jest.spyOn(buttonGroup, 'offsetWidth', 'get').mockImplementationOnce(() => 88).mockImplementation(() => 90);
         jest.spyOn(button1, 'offsetLeft', 'get').mockImplementation(() => 0);
         jest.spyOn(button1, 'offsetWidth', 'get').mockImplementation(() => 30);
@@ -138,14 +137,6 @@ describe('ButtonGroup', function () {
       let {getByTestId} = render(<ButtonGroupWithRefs setUp={setUp} />);
       let buttonGroup = getByTestId(buttonGroupId);
 
-      if (process.env.STRICT_MODE) {
-        // If strict mode, render cycle happens twice which means a 2nd useLayoutEffect is triggered -> mock widths are now already set since
-        // ButtonGroupWithRefs's useEffect has already run
-        expect(buttonGroup).toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));
-      } else {
-        expect(buttonGroup).not.toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));
-      }
-
       // ResizeObserver not actually implemented in jsdom, so rely on the fallback to window resize listener
       act(() => {window.dispatchEvent(new Event('resize'));});
       expect(buttonGroup).toHaveAttribute('class', expect.stringContaining('spectrum-ButtonGroup--vertical'));

From cb5eab2671658a05f8f13c9517a49273ffa7902a Mon Sep 17 00:00:00 2001
From: Devon Govett <devongovett@gmail.com>
Date: Wed, 11 Jan 2023 09:10:09 -0800
Subject: [PATCH 15/15] Create FocusScope nodes top down and add them bottom up
 (#3898)

---
 packages/@react-aria/focus/src/FocusScope.tsx | 73 ++++++++-----------
 1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/packages/@react-aria/focus/src/FocusScope.tsx b/packages/@react-aria/focus/src/FocusScope.tsx
index 6555e77e986..e3766f5a8aa 100644
--- a/packages/@react-aria/focus/src/FocusScope.tsx
+++ b/packages/@react-aria/focus/src/FocusScope.tsx
@@ -13,7 +13,7 @@
 import {FocusableElement} from '@react-types/shared';
 import {focusSafely} from './focusSafely';
 import {isElementVisible} from './isElementVisible';
-import React, {ReactNode, RefObject, useCallback, useContext, useEffect, useMemo, useRef} from 'react';
+import React, {ReactNode, RefObject, useContext, useEffect, useMemo, useRef} from 'react';
 import {useLayoutEffect} from '@react-aria/utils';
 
 
@@ -62,7 +62,7 @@ export interface FocusManager {
 type ScopeRef = RefObject<Element[]>;
 interface IFocusContext {
   focusManager: FocusManager,
-  addParentToFocusScopeTree: () => ScopeRef
+  parentNode: TreeNode | null
 }
 
 const FocusContext = React.createContext<IFocusContext>(null);
@@ -84,41 +84,28 @@ export function FocusScope(props: FocusScopeProps) {
   let startRef = useRef<HTMLSpanElement>();
   let endRef = useRef<HTMLSpanElement>();
   let scopeRef = useRef<Element[]>([]);
-  let {addParentToFocusScopeTree: addParentToTreeMap} = useContext(FocusContext) || {};
-
-  // Call to add current scope to Tree if it hasn't been added yet. Adds parent scope to Tree if it doesn't exist.
-  let addParentToFocusScopeTree = useCallback(() => {
-    let parentScope = null;
-    if (addParentToTreeMap != null) {
-      parentScope = addParentToTreeMap();
-    } else if (activeScope && focusScopeTree.getTreeNode(activeScope)) {
-      // If addParentToTreeMap doesn't exist on context, then the current FocusScope is either standalone or could be a overlay (e.g. DialogContainer launched from a menu)
-      // and thus perhaps outside the active scope. For the latter, we'll want to set the new scope's parent as the active scope so we can restore focus when the scope unmounts
-      parentScope = activeScope;
-    }
-
-    if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
-      focusScopeTree.addTreeNode(scopeRef, parentScope);
-    }
+  let {parentNode} = useContext(FocusContext) || {};
 
-    return scopeRef;
-  }, [addParentToTreeMap]);
+  // Create a tree node here so we can add children to it even before it is added to the tree.
+  let node = useMemo(() => new TreeNode({scopeRef}), [scopeRef]);
 
-  // Construct Tree on inital mount. Generate from top most parent to bottom most child by calling addParentToTreeMap from parent context.
   useLayoutEffect(() => {
-    let parentScope = null;
-    if (addParentToTreeMap != null) {
-      parentScope = addParentToTreeMap();
-    } else if (activeScope && focusScopeTree.getTreeNode(activeScope)) {
-      parentScope = activeScope;
-    }
-
-    // For outer most leaf in tree, need to add self to tree map
-    if (focusScopeTree.getTreeNode(parentScope) && !focusScopeTree.getTreeNode(scopeRef)) {
-      focusScopeTree.addTreeNode(scopeRef, parentScope);
+    // If a new scope mounts outside the active scope, (e.g. DialogContainer launched from a menu),
+    // use the active scope as the parent instead of the parent from context. Layout effects run bottom
+    // up, so if the parent is not yet added to the tree, don't do this. Only the outer-most FocusScope
+    // that is being added should get the activeScope as its parent.
+    let parent = parentNode || focusScopeTree.root;
+    if (focusScopeTree.getTreeNode(parent.scopeRef) && activeScope && !isAncestorScope(activeScope, parent.scopeRef)) {
+      let activeNode = focusScopeTree.getTreeNode(activeScope);
+      if (activeNode) {
+        parent = activeNode;
+      }
     }
 
-  }, [addParentToTreeMap]);
+    // Add the node to the parent, and to the tree.
+    parent.addChild(node);
+    focusScopeTree.addNode(node);
+  }, [node, parentNode]);
 
   useLayoutEffect(() => {
     let node = focusScopeTree.getTreeNode(scopeRef);
@@ -143,7 +130,7 @@ export function FocusScope(props: FocusScopeProps) {
   useAutoFocus(scopeRef, autoFocus);
 
   // this layout effect needs to run last so that focusScopeTree cleanup happens at the last moment possible
-  useLayoutEffect(() => {
+  useEffect(() => {
     if (scopeRef) {
       let activeElement = document.activeElement;
       let scope = null;
@@ -184,8 +171,8 @@ export function FocusScope(props: FocusScopeProps) {
   let focusManager = useMemo(() => createFocusManagerForScope(scopeRef), []);
   let value = useMemo(() => ({
     focusManager,
-    addParentToFocusScopeTree
-  }), [addParentToFocusScopeTree, focusManager]);
+    parentNode: node
+  }), [node, focusManager]);
 
   return (
     <FocusContext.Provider value={value}>
@@ -799,7 +786,7 @@ function last(walker: TreeWalker) {
 
 
 class Tree {
-  private root: TreeNode;
+  root: TreeNode;
   private fastMap = new Map<ScopeRef, TreeNode>();
 
   constructor() {
@@ -826,6 +813,10 @@ class Tree {
     }
   }
 
+  addNode(node: TreeNode) {
+    this.fastMap.set(node.scopeRef, node);
+  }
+
   removeTreeNode(scopeRef: ScopeRef) {
     // never remove the root
     if (scopeRef === null) {
@@ -848,7 +839,7 @@ class Tree {
     }
     let children = node.children;
     parentNode.removeChild(node);
-    if (children.length > 0) {
+    if (children.size > 0) {
       children.forEach(child => parentNode.addChild(child));
     }
 
@@ -860,7 +851,7 @@ class Tree {
     if (node.scopeRef != null) {
       yield node;
     }
-    if (node.children.length > 0) {
+    if (node.children.size > 0) {
       for (let child of node.children) {
         yield* this.traverse(child);
       }
@@ -880,18 +871,18 @@ class TreeNode {
   public scopeRef: ScopeRef;
   public nodeToRestore: FocusableElement;
   public parent: TreeNode;
-  public children: TreeNode[] = [];
+  public children: Set<TreeNode> = new Set();
   public contain = false;
 
   constructor(props: {scopeRef: ScopeRef}) {
     this.scopeRef = props.scopeRef;
   }
   addChild(node: TreeNode) {
-    this.children.push(node);
+    this.children.add(node);
     node.parent = this;
   }
   removeChild(node: TreeNode) {
-    this.children.splice(this.children.indexOf(node), 1);
+    this.children.delete(node);
     node.parent = undefined;
   }
 }