Escape on nested Overlays (#1861)

* Add repro for draft issue overlay

* types amirite

* add tests for repro

* clean code and all that

* add test for Overlay event leaking out

* Use registry pattern like useClickOutside

* clean up tests

* remove lint warning

* remove incorrect comments

* consistent comment style

Author: siddharthkp

src/__tests__/Overlay.test.tsx

@@ -3,9 +3,11 @@ import {Overlay, Box, Text, ButtonDanger, Button} from '..'
 import {render, cleanup, waitFor, fireEvent, act} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import {axe, toHaveNoViolations} from 'jest-axe'
+import '@testing-library/jest-dom'
 import theme from '../theme'
 import BaseStyles from '../BaseStyles'
 import {ThemeProvider} from '../ThemeProvider'
+import {NestedOverlays, MemexNestedOverlays, MemexIssueOverlay} from '../stories/Overlay.stories'
 
 expect.extend(toHaveNoViolations)
 
@@ -100,4 +102,115 @@ describe('Overlay', () => {
     const cancelButtons = queryAllByText('Cancel')
     expect(cancelButtons).toHaveLength(0)
   })
+
+  it('should close the top most overlay on escape', () => {
+    const container = render(
+      <ThemeProvider>
+        <NestedOverlays />
+      </ThemeProvider>
+    )
+
+    // open first menu
+    userEvent.click(container.getByLabelText('Add this repository to a list'))
+    expect(container.getByText('Add to list')).toBeInTheDocument()
+
+    // open second menu
+    userEvent.click(container.getByText('Create list'))
+    expect(container.getByPlaceholderText('Name this list')).toBeInTheDocument()
+
+    // hitting escape on input should close the second menu but not the first
+    fireEvent.keyDown(container.getByPlaceholderText('Name this list'), {key: 'Escape', code: 'Escape'})
+    expect(container.queryByPlaceholderText('Name this list')).not.toBeInTheDocument()
+    // this breaks:
+    expect(container.getByText('Add to list')).toBeInTheDocument()
+
+    // hitting escape again in first overlay should close it
+    fireEvent.keyDown(container.getByText('Add to list'), {key: 'Escape', code: 'Escape'})
+    expect(container.queryByText('Add to list')).not.toBeInTheDocument()
+  })
+
+  it('memex repro: should only close the dropdown when escape is pressed', () => {
+    const container = render(
+      <ThemeProvider>
+        <MemexNestedOverlays />
+      </ThemeProvider>
+    )
+
+    // open first menu
+    userEvent.click(container.getByLabelText('Add custom iteration'))
+    expect(container.getByLabelText('Change duration unit')).toBeInTheDocument()
+
+    // open dropdown menu
+    userEvent.click(container.getByLabelText('Change duration unit'))
+    expect(container.getByRole('menu')).toBeInTheDocument()
+
+    // hitting escape on menu item should close the dropdown menu but not the overlay
+    fireEvent.keyDown(container.getByRole('menu'), {key: 'Escape', code: 'Escape'})
+    expect(container.queryByRole('menu')).not.toBeInTheDocument()
+    // this breaks:
+    expect(container.getByLabelText('Change duration unit')).toBeInTheDocument()
+  })
+
+  it('memex repro: should not close overlay when input has event.preventDefault', () => {
+    const container = render(
+      <ThemeProvider>
+        <MemexIssueOverlay />
+      </ThemeProvider>
+    )
+
+    // clicking the title opens overlay
+    userEvent.click(container.getByText('Implement draft issue editor'))
+    expect(container.getByLabelText('Change issue title')).toBeInTheDocument()
+
+    // clicking the button changes to input
+    userEvent.click(container.getByLabelText('Change issue title'))
+    expect(container.getByDisplayValue('Implement draft issue editor')).toBeInTheDocument()
+
+    // pressing Escape inside input brings back the button but does not close the overlay
+    fireEvent.keyDown(container.getByDisplayValue('Implement draft issue editor'), {key: 'Escape', code: 'Escape'})
+    expect(container.getByLabelText('Change issue title')).toBeInTheDocument()
+
+    // hitting Escape again should close the Overlay
+    fireEvent.keyDown(container.getByLabelText('Change issue title'), {key: 'Escape', code: 'Escape'})
+    expect(container.queryByLabelText('Change issue title')).not.toBeInTheDocument()
+  })
+
+  // https://github.com/primer/react/issues/1802
+  // eslint-disable-next-line jest/no-disabled-tests
+  it.skip('memex repro: should not leak overlay events to the document', () => {
+    const mockHandler = jest.fn()
+    const BugRepro1802 = () => {
+      const [isOpen, setIsOpen] = useState(false)
+      const closeOverlay = () => setIsOpen(false)
+      const buttonRef = useRef<HTMLButtonElement>(null)
+
+      React.useEffect(() => {
+        document.addEventListener('keydown', mockHandler)
+        return () => document.removeEventListener('keydown', mockHandler)
+      }, [])
+
+      return (
+        <ThemeProvider theme={theme}>
+          <BaseStyles>
+            <Button ref={buttonRef} onClick={() => setIsOpen(true)}>
+              open overlay
+            </Button>
+            {isOpen ? (
+              <Overlay returnFocusRef={buttonRef} onEscape={closeOverlay} onClickOutside={closeOverlay}>
+                <Text>Text inside Overlay</Text>
+              </Overlay>
+            ) : null}
+          </BaseStyles>
+        </ThemeProvider>
+      )
+    }
+
+    const container = render(<BugRepro1802 />)
+
+    userEvent.click(container.getByText('open overlay'))
+    fireEvent.keyDown(container.getByText('Text inside Overlay'), {key: 'Escape', code: 'Escape'})
+
+    // if stopPropagation worked, mockHandler would not have been called
+    expect(mockHandler).toHaveBeenCalledTimes(0)
+  })
 })

src/hooks/useOnEscapePress.ts

@@ -1,23 +1,34 @@
-import {useEffect, useCallback} from 'react'
-
-const handlers: ((e: KeyboardEvent) => void)[] = []
+import {useEffect, useCallback, useMemo} from 'react'
 
 /**
  * Calls all handlers in reverse order
  * @param event The KeyboardEvent generated by the Escape keydown.
  */
 function handleEscape(event: KeyboardEvent) {
-  if (event.key === 'Escape' && !event.defaultPrevented) {
-    for (let i = handlers.length - 1; i >= 0; --i) {
-      handlers[i](event)
+  if (!event.defaultPrevented) {
+    for (const handler of Object.values(registry).reverse()) {
+      handler(event)
       // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
-      if (event.defaultPrevented) {
-        break
-      }
+      if (event.defaultPrevented) break
     }
   }
 }
 
+type KeyboardEventCallback = (event: KeyboardEvent) => void
+
+const registry: {[id: number]: KeyboardEventCallback} = {}
+
+function register(id: number, handler: KeyboardEventCallback): void {
+  registry[id] = handler
+}
+
+function deregister(id: number) {
+  delete registry[id]
+}
+
+// For auto-incrementing unique identifiers for registered handlers.
+let handlerId = 0
+
 /**
  * Sets up a `keydown` listener on `window.document`. If
  * 1) The pressed key is "Escape", and
@@ -45,19 +56,26 @@ export const useOnEscapePress = (
 ): void => {
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const escapeCallback = useCallback(onEscape, callbackDependencies)
+
+  const handler = useCallback<KeyboardEventCallback>(
+    event => {
+      if (event.key === 'Escape') escapeCallback(event)
+    },
+    [escapeCallback]
+  )
+
+  const id = useMemo(() => handlerId++, [])
   useEffect(() => {
-    if (handlers.length === 0) {
+    if (Object.keys(registry).length === 0) {
       document.addEventListener('keydown', handleEscape)
     }
-    handlers.push(escapeCallback)
+    register(id, handler)
+
     return () => {
-      handlers.splice(
-        handlers.findIndex(h => h === escapeCallback),
-        1
-      )
-      if (handlers.length === 0) {
+      deregister(id)
+      if (Object.keys(registry).length === 0) {
         document.removeEventListener('keydown', handleEscape)
       }
     }
-  }, [escapeCallback])
+  }, [id, handler])
 }

src/hooks/useOverlay.tsx

@@ -29,6 +29,12 @@ export const useOverlay = ({
   const overlayRef = useProvidedRefOrCreate<HTMLDivElement>(_overlayRef)
   useOpenAndCloseFocus({containerRef: overlayRef, returnFocusRef, initialFocusRef, preventFocusOnOpen})
   useOnOutsideClick({containerRef: overlayRef, ignoreClickRefs, onClickOutside})
-  useOnEscapePress(onEscape)
+
+  // We only want one overlay to close at a time
+  const preventedDefaultOnEscape: UseOverlaySettings['onEscape'] = event => {
+    onEscape(event)
+    event.preventDefault()
+  }
+  useOnEscapePress(preventedDefaultOnEscape)
   return {ref: overlayRef}
 }