Revert "Overlay: Attach escape handler to overlay container (#1824)" (#1856)

* Revert "Overlay: Attach escape handler to overlay container (#1824)"

This reverts commit 4eab65e.

* add changeset

Author: siddharthkp

.changeset/revert-1824.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Revert "Overlay: Attach escape handler to overlay container"

src/__tests__/Overlay.test.tsx

@@ -56,32 +56,6 @@ const TestComponent = ({initialFocus, callback}: TestComponentSettings) => {
   )
 }
 
-// https://github.com/primer/react/issues/1802
-const BugRepro1802 = ({mockHandler}: {mockHandler: (event: KeyboardEvent) => void}) => {
-  const [isOpen, setIsOpen] = useState(false)
-  const buttonRef = useRef<HTMLButtonElement>(null)
-
-  React.useEffect(() => {
-    document.addEventListener('keydown', mockHandler)
-    return () => document.removeEventListener('keydown', mockHandler)
-  }, [mockHandler])
-
-  return (
-    <ThemeProvider theme={theme}>
-      <BaseStyles>
-        <Button ref={buttonRef} onClick={() => setIsOpen(!isOpen)}>
-          open overlay
-        </Button>
-        {isOpen ? (
-          <Overlay returnFocusRef={buttonRef} onEscape={() => setIsOpen(false)} onClickOutside={() => setIsOpen(false)}>
-            <Text>Text inside Overlay</Text>
-          </Overlay>
-        ) : null}
-      </BaseStyles>
-    </ThemeProvider>
-  )
-}
-
 describe('Overlay', () => {
   it('should have no axe violations', async () => {
     const {container} = render(<TestComponent />)
@@ -126,14 +100,4 @@ describe('Overlay', () => {
     const cancelButtons = queryAllByText('Cancel')
     expect(cancelButtons).toHaveLength(0)
   })
-
-  it('should call stop propagation', () => {
-    const mockHandler = jest.fn()
-    const {getByText} = render(<BugRepro1802 mockHandler={mockHandler} />)
-    act(() => userEvent.click(getByText('open overlay')))
-    const domNode = getByText('Text inside Overlay')
-    fireEvent.keyDown(domNode, {key: 'Escape', code: 'Escape', keyCode: 27, charCode: 27})
-    // if stopPropagation worked, mockHandler would not have been called
-    expect(mockHandler).toHaveBeenCalledTimes(0)
-  })
 })

src/hooks/useOnEscapePress.ts

@@ -9,7 +9,7 @@ const handlers: ((e: KeyboardEvent) => void)[] = []
 function handleEscape(event: KeyboardEvent) {
   if (event.key === 'Escape' && !event.defaultPrevented) {
     for (let i = handlers.length - 1; i >= 0; --i) {
-      if (typeof handlers[i] === 'function') handlers[i](event)
+      handlers[i](event)
       // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
       if (event.defaultPrevented) {
         break
@@ -19,7 +19,7 @@ function handleEscape(event: KeyboardEvent) {
 }
 
 /**
- * Sets up a `keydown` listener on element passed | `window.document`. If
+ * Sets up a `keydown` listener on `window.document`. If
  * 1) The pressed key is "Escape", and
  * 2) The event has not had `.preventDefault()` called
  * The given callback will be executed.
@@ -38,24 +38,16 @@ function handleEscape(event: KeyboardEvent) {
  * @param callbackDependencies {React.DependencyList} The dependencies of the given
  * `onEscape` callback for memoization. Omit this param if the callback is already
  * memoized. See `React.useCallback` for more info on memoization.
- *
- * @param containerRef {React.RefObject<HTMLElement>} The overlay element to attach the
- * handlers on. If not provided, fallback to document.
  */
 export const useOnEscapePress = (
   onEscape: (e: KeyboardEvent) => void,
-  callbackDependencies: React.DependencyList = [onEscape],
-  containerRef?: React.RefObject<HTMLElement>
+  callbackDependencies: React.DependencyList = [onEscape]
 ): void => {
   // eslint-disable-next-line react-hooks/exhaustive-deps
   const escapeCallback = useCallback(onEscape, callbackDependencies)
-
   useEffect(() => {
-    const element = containerRef?.current
-
     if (handlers.length === 0) {
-      if (element) element.addEventListener('keydown', handleEscape)
-      else document.addEventListener('keydown', handleEscape)
+      document.addEventListener('keydown', handleEscape)
     }
     handlers.push(escapeCallback)
     return () => {
@@ -64,9 +56,8 @@ export const useOnEscapePress = (
         1
       )
       if (handlers.length === 0) {
-        if (element) element.removeEventListener('keydown', handleEscape)
-        else document.removeEventListener('keydown', handleEscape)
+        document.removeEventListener('keydown', handleEscape)
       }
     }
-  }, [escapeCallback, containerRef])
+  }, [escapeCallback])
 }

src/hooks/useOverlay.tsx

@@ -29,12 +29,6 @@ export const useOverlay = ({
   const overlayRef = useProvidedRefOrCreate<HTMLDivElement>(_overlayRef)
   useOpenAndCloseFocus({containerRef: overlayRef, returnFocusRef, initialFocusRef, preventFocusOnOpen})
   useOnOutsideClick({containerRef: overlayRef, ignoreClickRefs, onClickOutside})
-
-  /** Don't bubble Escape event up the tree  */
-  const onEscapeWithStoppedPropagation: UseOverlaySettings['onEscape'] = event => {
-    event.stopPropagation()
-    onEscape(event)
-  }
-  useOnEscapePress(onEscapeWithStoppedPropagation, undefined, overlayRef)
+  useOnEscapePress(onEscape)
   return {ref: overlayRef}
 }

src/stories/Overlay.stories.tsx

@@ -1,25 +1,7 @@
 import React, {useState, useRef, useCallback} from 'react'
-import ReactDOM from 'react-dom'
 import {Meta} from '@storybook/react'
 import styled from 'styled-components'
-import {TriangleDownIcon, PlusIcon} from '@primer/octicons-react'
-import {
-  BaseStyles,
-  Overlay,
-  Button,
-  ButtonInvisible,
-  ButtonPrimary,
-  ButtonGroup,
-  Text,
-  ButtonDanger,
-  ThemeProvider,
-  Box,
-  StyledOcticon,
-  Checkbox,
-  ChoiceInputField,
-  TextInput,
-  ActionList
-} from '..'
+import {BaseStyles, Overlay, Button, Text, ButtonDanger, ThemeProvider, Box} from '..'
 import type {AnchorSide} from '@primer/behaviors'
 import {DropdownMenu, DropdownButton} from '../DropdownMenu'
 import {ItemInput} from '../ActionList/List'
@@ -229,97 +211,3 @@ export const OverlayOnTopOfOverlay = ({anchorSide}: OverlayProps) => {
     </Box>
   )
 }
-
-export const NestedOverlays = () => {
-  const [listOverlayOpen, setListOverlayOpen] = React.useState(false)
-  const [createListOverlayOpen, setCreateListOverlayOpen] = React.useState(false)
-
-  const buttonRef = useRef<HTMLButtonElement>(null)
-  const secondaryButtonRef = useRef<HTMLButtonElement>(null)
-
-  React.useEffect(() => {
-    // eslint-disable-next-line no-console
-    const handler = (event: KeyboardEvent) => console.log('global handler:', event.key)
-    document.addEventListener('keydown', handler)
-    return () => document.removeEventListener('keydown', handler)
-  }, [])
-
-  const hostElement = document.createElement('div')
-  ReactDOM.render(<div>hello</div>, hostElement)
-
-  return (
-    <div>
-      <TextInput />
-      <ButtonGroup display="block" my={2}>
-        <Button>Star</Button>
-        <Button
-          aria-label="Add this repository to a list"
-          ref={buttonRef}
-          onClick={() => setListOverlayOpen(!listOverlayOpen)}
-          sx={{paddingX: 2}}
-        >
-          <TriangleDownIcon />
-        </Button>
-      </ButtonGroup>
-      {listOverlayOpen && (
-        <Overlay
-          width="medium"
-          onEscape={() => setListOverlayOpen(false)}
-          onClickOutside={() => setListOverlayOpen(false)}
-          returnFocusRef={buttonRef}
-          ignoreClickRefs={[buttonRef]}
-          top={60}
-          left={16}
-        >
-          <Box sx={{display: 'flex', flexDirection: 'column', py: 2}}>
-            <Box sx={{paddingX: 3}}>
-              <Text color="fg.muted" sx={{fontSize: 1}}>
-                Add to list
-              </Text>
-              <Box sx={{marginY: 1}}>
-                <ChoiceInputField>
-                  <ChoiceInputField.Label>My stack</ChoiceInputField.Label>
-                  <Checkbox />
-                </ChoiceInputField>
-                <ChoiceInputField>
-                  <ChoiceInputField.Label>Want to try</ChoiceInputField.Label>
-                  <Checkbox />
-                </ChoiceInputField>
-              </Box>
-            </Box>
-            <ActionList.Divider />
-            <ButtonInvisible
-              ref={secondaryButtonRef}
-              sx={{textAlign: 'left', px: 2, mx: 2}}
-              onClick={() => setCreateListOverlayOpen(!createListOverlayOpen)}
-            >
-              <StyledOcticon icon={PlusIcon} sx={{mr: 1}} />
-              Create list
-            </ButtonInvisible>
-          </Box>
-          {createListOverlayOpen && (
-            <Overlay
-              width="large"
-              onEscape={() => setCreateListOverlayOpen(false)}
-              onClickOutside={() => setCreateListOverlayOpen(false)}
-              returnFocusRef={secondaryButtonRef}
-              ignoreClickRefs={[secondaryButtonRef]}
-              top={120}
-              left={64}
-            >
-              <Box as="form" sx={{display: 'flex', flexDirection: 'column', p: 3}}>
-                <Text color="fg.muted" sx={{fontSize: 1, mb: 3}}>
-                  Create a list to organize your starred repositories.
-                </Text>
-                <TextInput placeholder="Name this list" sx={{mb: 2}} />
-                <TextInput as="textarea" placeholder="Write a description" rows="3" sx={{mb: 2, textarea: {p: 2}}} />
-
-                <ButtonPrimary onClick={() => setCreateListOverlayOpen(!createListOverlayOpen)}>Create</ButtonPrimary>
-              </Box>
-            </Overlay>
-          )}
-        </Overlay>
-      )}
-    </div>
-  )
-}