Element-bound events with useOnEscapePress are triggering all escape events globally

[iansan5653]

A change introduced in 34.4.0 was to add support for useOnEscapePress to bind events to specific elements instead of just the document root (and to implement that new functionality in the useOverlay hook):

#1824 4eab65e5 Thanks @siddharthkp! - Overlay: Attach escape handler to overlay container instead of document to fix stopPropagation

This makes sense, because the events need to be bound to the overlay container so that the overlay can stopPropogation and keep its events contained.

However, this hook is using a global function with a global array of event handlers, even with the element-bound events. So when an escape event is triggered on a top-level overlay, it still calls every escape event handler ever registered:

react/src/hooks/useOnEscapePress.ts

Lines 3 to 19 in 96473f3

	const handlers: ((e: KeyboardEvent) => void)[] = []
	/**
	* 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) {
	if (typeof handlers[i] === 'function') handlers[i](event)
	// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
	if (event.defaultPrevented) {
	break
	}
	}
	}
	}

An example of the undesirable behavior that this bug causes is visible in the Nested Overlays story:

• Click the dropdown arrow next to "Star"
• Click "+ Create List"
• Press Escape
  • Only the top overlay should close, but both do instead
• Click the dropdown arrow next to "Star" again
  • Both overlays open in incorrect order because the top overlay was never cleaned up properly

I see two possible ways to fix this:

1. For element-bound useOnEscapePress events, bind/unbind the passed handler function directly. This seems simple but I'm not sure why the behavior is the way it is in the first place - there may be a reason for it. Example of this change:

    if (element) element.addEventListener('keydown', escapeCallback)
    else document.addEventListener('keydown', escapeCallback)
    return () => {
      if (element) element.removeEventListener('keydown', escapeCallback)
      else document.removeEventListener('keydown', escapeCallback)
    }

2. Or, only use useOnEscapePress for global (root-bound) events, and just bind the escape event using react's native onKeyPress property instead.

cc @siddharthkp @mattcosta7

[siddharthkp]

@iansan5653 #1861 Should take care of this! I've added a story if you want to see the new behavior: https://primer-react-jhga7sawh-primer.vercel.app/react/storybook?path=/story/internal-components-overlay--memex-nested-overlays