fix(focusZone): handle complex child reordering (#1225)

.changeset/yellow-dolphins-grin.md

@@ -0,0 +1,5 @@
+---
+"@primer/components": patch
+---
+
+handle complex child reordering within a focusZone

src/__tests__/behaviors/focusZone.tsx

@@ -7,6 +7,9 @@ async function nextTick() {
   return new Promise(resolve => setTimeout(resolve, 0))
 }
 
+const moveDown = () => userEvent.type(document.activeElement!, '{arrowdown}')
+const moveUp = () => userEvent.type(document.activeElement!, '{arrowup}')
+
 // Since we use strict `isTabbable` checks within focus trap, we need to mock these
 // properties that Jest does not populate.
 beforeAll(() => {
@@ -472,3 +475,49 @@ it('Should set aria-activedescendant correctly', () => {
 
   controller.abort()
 })
+
+it('Should handle elements being reordered', async () => {
+  const {container} = render(
+    <div>
+      <div id="focusZone">
+        <button tabIndex={0}>Apple</button>
+        <button tabIndex={0}>Banana</button>
+        <button tabIndex={0}>Cantaloupe</button>
+        <button tabIndex={0}>Durian</button>
+      </div>
+    </div>
+  )
+
+  const focusZoneContainer = container.querySelector<HTMLElement>('#focusZone')!
+  const [firstButton, secondButton, thirdButton, fourthButton] = focusZoneContainer.querySelectorAll('button')
+  const controller = focusZone(focusZoneContainer)
+
+  firstButton.focus()
+  expect(document.activeElement).toEqual(firstButton)
+
+  moveDown()
+  expect(document.activeElement).toEqual(secondButton)
+
+  moveUp()
+  expect(document.activeElement).toEqual(firstButton)
+
+  // move secondButton and thirdButton to the end of the zone, in reverse order
+  focusZoneContainer.appendChild(thirdButton)
+  focusZoneContainer.appendChild(secondButton)
+
+  // The mutation observer fires asynchronously
+  await nextTick()
+
+  expect(document.activeElement).toEqual(firstButton)
+
+  moveDown()
+  expect(document.activeElement).toEqual(fourthButton)
+
+  moveDown()
+  expect(document.activeElement).toEqual(thirdButton)
+
+  moveDown()
+  expect(document.activeElement).toEqual(secondButton)
+
+  controller.abort()
+})

src/behaviors/focusZone.ts

@@ -440,18 +440,21 @@ export function focusZone(container: HTMLElement, settings?: FocusZoneSettings):
   // If the DOM structure of the container changes, make sure we keep our state up-to-date
   // with respect to the focusable elements cache and its order
   const observer = new MutationObserver(mutations => {
+    // Perform all removals first, in case element order has simply changed
     for (const mutation of mutations) {
-      for (const addedNode of mutation.addedNodes) {
-        if (addedNode instanceof HTMLElement) {
-          beginFocusManagement(...iterateFocusableElements(addedNode))
-        }
-      }
       for (const removedNode of mutation.removedNodes) {
         if (removedNode instanceof HTMLElement) {
           endFocusManagement(...iterateFocusableElements(removedNode))
         }
       }
     }
+    for (const mutation of mutations) {
+      for (const addedNode of mutation.addedNodes) {
+        if (addedNode instanceof HTMLElement) {
+          beginFocusManagement(...iterateFocusableElements(addedNode))
+        }
+      }
+    }
   })
 
   observer.observe(container, {