fix(ui-drawer-layout,ui-a11y-utils): fix Tray closing immediately after opening and calling onDismiss

INSTUI-4828

Author: matyasf

packages/ui-a11y-utils/src/FocusRegion.ts

@@ -85,10 +85,12 @@ class FocusRegion {
     this._contextElement = element
     if (options) {
       this._options = options
-      this.addOrRemoveListeners(
-        options.shouldCloseOnDocumentClick,
-        options.shouldCloseOnEscape
-      )
+      if (this._active) {
+        this.addOrRemoveListeners(
+          options.shouldCloseOnDocumentClick,
+          options.shouldCloseOnEscape
+        )
+      }
     }
     if (this._keyboardFocusRegion) {
       this._keyboardFocusRegion.updateElement(element)
@@ -114,6 +116,8 @@ class FocusRegion {
   }
 
   handleDocumentClick = (event: React.PointerEvent) => {
+    // we used event.pointerType === 'mouse' here, but it is not supported in Safari
+    // https://developer.mozilla.org/en-US/docs/Web/API/PointerEvent/pointerType
     if (
       this._options.shouldCloseOnDocumentClick &&
       event.button === 0 &&

packages/ui-a11y-utils/src/__tests__/FocusRegion.test.tsx

@@ -68,7 +68,6 @@ describe('FocusRegion', () => {
       const outsideElement = document.createElement('div')
       document.body.appendChild(outsideElement)
 
-      fireEvent.mouseDown(outsideElement, { button: 0 })
       fireEvent.click(outsideElement, { button: 0, detail: 1 })
 
       await waitFor(() => {
@@ -91,7 +90,6 @@ describe('FocusRegion', () => {
       const outsideElement = document.createElement('div')
       document.body.appendChild(outsideElement)
 
-      fireEvent.mouseDown(outsideElement, { button: 0 })
       fireEvent.click(outsideElement, { button: 0, detail: 1 })
 
       // Wait a bit to ensure no dismiss is called
@@ -252,6 +250,42 @@ describe('FocusRegion', () => {
       // The internal state should be updated
       expect(focusRegion).toBeDefined()
     })
+
+    it('should not add event listeners when updateElement is called on inactive region', async () => {
+      const onDismiss = vi.fn()
+      focusRegion = new FocusRegion(container, {
+        shouldCloseOnDocumentClick: false,
+        shouldCloseOnEscape: false,
+        onDismiss
+      })
+
+      // Do NOT activate the region - it should remain inactive
+      expect(focusRegion.focused).toBe(false)
+
+      // Update element with options that would add listeners if region was active
+      const newContainer = document.createElement('div')
+      const newOptions = {
+        shouldCloseOnDocumentClick: true,
+        shouldCloseOnEscape: true,
+        onDismiss
+      }
+
+      focusRegion.updateElement(newContainer, newOptions)
+
+      // Region should still be inactive
+      expect(focusRegion.focused).toBe(false)
+
+      // Click outside the container - should NOT dismiss since no listeners were added
+      const outsideElement = document.createElement('div')
+      document.body.appendChild(outsideElement)
+
+      fireEvent.click(outsideElement, { button: 0, detail: 1 })
+
+      fireEvent.keyUp(document, { keyCode: 27 })
+      expect(onDismiss).not.toHaveBeenCalled()
+
+      document.body.removeChild(outsideElement)
+    })
   })
 
   describe('keyboardFocusable', () => {

packages/ui-drawer-layout/src/DrawerLayout/DrawerTray/props.ts

@@ -102,6 +102,15 @@ type PropsPassedToDialog = {
    */
   liveRegion?: LiveRegion
 
+  /**
+   * Event fired when the underlying FocusRegion is dismissed in overlay mode.
+   * This can happen if:
+   * - `shouldCloseOnDocumentClick` is `true` and the user
+   * clicks outside the `<DrawerLayout.Tray />`
+   * - If `shouldCloseOnEscape` is `true` and the user presses the ESC key.
+   *
+   * This should be used to close the `<DrawerLayout.Tray />` in these cases
+   */
   onDismiss?: (
     event: React.UIEvent | React.FocusEvent,
     documentClick?: boolean

packages/ui-drawer-layout/src/DrawerLayout/__tests__/DrawerLayout.test.tsx

@@ -22,10 +22,14 @@
  * SOFTWARE.
  */
 
-import { render, screen } from '@testing-library/react'
+import { render, screen, waitFor, fireEvent, act } from '@testing-library/react'
+import { vi } from 'vitest'
 import '@testing-library/jest-dom'
 
+import { DrawerLayout } from '../index'
 import DrawerLayoutFixture from '../__fixtures__/DrawerLayout.fixture'
+import { Button } from '@instructure/ui-buttons'
+import { View } from '@instructure/ui-view'
 
 describe('<DrawerLayout />', () => {
   it('should render', () => {
@@ -50,4 +54,48 @@ describe('<DrawerLayout />', () => {
     expect(contentWrapper).toHaveTextContent('Hello from content')
     expect(contentWrapper).toHaveAttribute('aria-label', 'Test DrawerContent')
   })
+
+  it('should close the tray when ESC is pressed in overlay mode', async () => {
+    let open = true
+    const msg = 'This is in the Tray'
+    const onDismiss = vi.fn(() => {
+      open = false
+    })
+    // Small layout width to trigger overlay mode
+    const TestComponent = () => (
+      <View height="25rem" as="div" position="relative">
+        <DrawerLayout minWidth="4444px">
+          <DrawerLayout.Tray
+            id="DrawerLayoutTrayExample1"
+            open={open}
+            label="Drawer Tray Start Example"
+            onDismiss={onDismiss}
+          >
+            <Button onClick={() => (open = false)}>Close Tray</Button>
+            {msg}
+          </DrawerLayout.Tray>
+          <DrawerLayout.Content label="Drawer content example">
+            <Button
+              onClick={() => {
+                open = true
+              }}
+            >
+              Expand tray
+            </Button>
+          </DrawerLayout.Content>
+        </DrawerLayout>
+      </View>
+    )
+    const { rerender } = render(<TestComponent />)
+    expect(screen.getByText(msg)).toBeInTheDocument()
+    await act(() => new Promise((resolve) => requestAnimationFrame(resolve)))
+    fireEvent.keyUp(document, { keyCode: 27 }) // ESC key
+    await waitFor(() => {
+      expect(onDismiss).toHaveBeenCalled()
+    })
+    rerender(<TestComponent />)
+    await waitFor(() => {
+      expect(screen.queryByText(msg)).not.toBeInTheDocument()
+    })
+  })
 })

packages/ui-tray/src/Tray/__tests__/Tray.test.tsx

@@ -22,22 +22,31 @@
  * SOFTWARE.
  */
 
-import { render, screen, waitFor } from '@testing-library/react'
+import { act, fireEvent, render, screen } from '@testing-library/react'
 import { vi } from 'vitest'
 import '@testing-library/jest-dom'
+import { useState } from 'react'
 
 import { Tray } from '../index'
 import type { TrayProps } from '../props'
 
 describe('<Tray />', async () => {
-  it('should render nothing and have a node with no parent when closed', async () => {
+  beforeAll(() => {
+    vi.useFakeTimers()
+  })
+
+  afterAll(() => {
+    vi.useRealTimers()
+  })
+
+  it('should render nothing and have a node with no parent when closed', () => {
     render(<Tray label="Tray Example">Hello Tray</Tray>)
 
     const trayContent = screen.queryByText('Hello Tray')
     expect(trayContent).not.toBeInTheDocument()
   })
 
-  it('should render children and have a node with a parent when open', async () => {
+  it('should render children and have a node with a parent when open', () => {
     render(
       <Tray label="Tray Example" open>
         Hello Tray
@@ -48,31 +57,30 @@ describe('<Tray />', async () => {
     expect(trayContent).toBeInTheDocument()
   })
 
-  it('should apply the a11y attributes', async () => {
+  it('should apply the a11y attributes', () => {
     render(
       <Tray label="Tray Example" open>
         Hello Tray
       </Tray>
     )
     const tray = screen.getByRole('dialog')
-
     expect(tray).toHaveAttribute('aria-label', 'Tray Example')
   })
 
-  it('should support onOpen prop', async () => {
+  it('should support onOpen prop', () => {
     const onOpen = vi.fn()
     render(
       <Tray label="Tray Example" open onOpen={onOpen}>
         Hello Tray
       </Tray>
     )
-
-    await waitFor(() => {
-      expect(onOpen).toHaveBeenCalled()
+    act(() => {
+      vi.runAllTimers()
     })
+    expect(onOpen).toHaveBeenCalled()
   })
 
-  it('should support onClose prop', async () => {
+  it('should support onClose prop', () => {
     const onClose = vi.fn()
 
     const { rerender } = render(
@@ -87,13 +95,13 @@ describe('<Tray />', async () => {
         Hello Tray
       </Tray>
     )
-
-    await waitFor(() => {
-      expect(onClose).toHaveBeenCalled()
+    act(() => {
+      vi.runAllTimers()
     })
+    expect(onClose).toHaveBeenCalled()
   })
 
-  it('should take a prop for finding default focus', async () => {
+  it('should take a prop for finding default focus', () => {
     render(
       <Tray
         label="Tray Example"
@@ -106,10 +114,10 @@ describe('<Tray />', async () => {
       </Tray>
     )
     const input = screen.getByLabelText('my-input-label')
-
-    await waitFor(() => {
-      expect(document.activeElement).toBe(input)
+    act(() => {
+      vi.runAllTimers()
     })
+    expect(document.activeElement).toBe(input)
   })
 
   describe('transition()', () => {
@@ -139,7 +147,6 @@ describe('<Tray />', async () => {
         }
       }
     }
-
     for (const dir in placements) {
       describe(`when text direction is '${dir}'`, () => {
         for (const placement in placements[dir].enteringPlacements) {
@@ -158,15 +165,16 @@ describe('<Tray />', async () => {
                 Hello
               </Tray>
             )
-            await waitFor(() => {
-              expect(onEntered).toHaveBeenCalled()
+            act(() => {
+              vi.runAllTimers()
             })
+            expect(onEntered).toHaveBeenCalled()
           })
         }
 
         for (const placement in placements[dir].exitingPlacements) {
           const val = placements[dir].exitingPlacements[placement]
-          it(`returns ${val} for ${placement} when exiting`, async () => {
+          it(`returns ${val} for ${placement} when exiting`, () => {
             const onExited = vi.fn()
             document.documentElement.setAttribute('dir', dir)
 
@@ -192,12 +200,79 @@ describe('<Tray />', async () => {
                 Hello
               </Tray>
             )
-            await waitFor(() => {
-              expect(onExited).toHaveBeenCalled()
+            act(() => {
+              vi.runAllTimers()
             })
+            expect(onExited).toHaveBeenCalled()
           })
         }
       })
     }
   })
+
+  it('should open, close via dismiss, and reopen with shouldCloseOnDocumentClick enabled', async () => {
+    const onDismiss = vi.fn()
+    const onEntered = vi.fn()
+    const onExited = vi.fn()
+
+    const TrayWithButton = () => {
+      const [isOpen, setIsOpen] = useState(false)
+
+      const handleDismiss = () => {
+        setIsOpen(false)
+        onDismiss()
+      }
+
+      return (
+        <div>
+          <div>Outside of Tray</div>
+          <button onClick={() => setIsOpen(!isOpen)}>Toggle Tray</button>
+          <Tray
+            label="Tray Example"
+            shouldCloseOnDocumentClick
+            open={isOpen}
+            onDismiss={handleDismiss}
+            onEntered={onEntered}
+            onExited={onExited}
+          >
+            <div>Tray Content</div>
+          </Tray>
+        </div>
+      )
+    }
+    render(<TrayWithButton />)
+    // 1. Open Tray
+    expect(screen.queryByText('Tray Content')).not.toBeInTheDocument()
+    const button = screen.getByText('Toggle Tray')
+    fireEvent.click(button, { button: 0, detail: 1 })
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(onEntered).toHaveBeenCalled()
+    expect(onDismiss).not.toHaveBeenCalled()
+    expect(screen.getByText('Tray Content')).toBeInTheDocument()
+    // 2. Close Tray be clicking outside it
+    // event.detail and button are needed because FocusRegion.ts/handleDocumentClick
+    fireEvent.click(document, { button: 0, detail: 1 })
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(onDismiss).toHaveBeenCalled()
+    expect(onExited).toHaveBeenCalled()
+    expect(screen.queryByText('Tray Content')).not.toBeInTheDocument()
+
+    onEntered.mockClear()
+    onDismiss.mockClear()
+    onExited.mockClear()
+
+    // 3. click Button again, Tray should reopen.
+    fireEvent.click(button, { button: 0, detail: 1 })
+    act(() => {
+      vi.runAllTimers()
+    })
+    expect(onEntered).toHaveBeenCalled()
+    expect(onDismiss).not.toHaveBeenCalled()
+    expect(onExited).not.toHaveBeenCalled()
+    expect(screen.getByText('Tray Content')).toBeInTheDocument()
+  })
 })