Overlay: Events bubbling up to the page can conflict with global shortcuts

[siddharthkp]

via @dusave on slack: Is there a way to stop propagation of Escape when an anchored overlay is open? I just want the anchored overlay to close, but it's also bubbling up outside of the anchored overlay

Outdated repo (This feature was removed in memex, watch this video instead)

https://github.com/orgs/github/projects/4205/settings/fields/55540

1. Click on the dates of a given iteration, make the date picker show
2. Hit Escape
3. Notice it closes the date picker and settings. We only want it to close the date picker

---

Update:

Problem:

By default, pressing Escape in an Overlay bubbles up to the page outside the Overlay. Adding event.stopPropagation inside onEscape does not stop the events from bubbling up because of the way the handlers are wired.

const Page = () => {
  React.useEffect(() => {
    // global keydown listener on page.
    document.addEventListener('keydown', console.log('global handler:', event.key))
  }, [])

  return (
    <Overlay
      onEscape={event => {
        closeOverlay()
        // You'd expect this to prevent the event from bubbling 
        // to the global handler on the page, but it doesn't
        event.stopPropogation() 
      }}
    >
      ...
    </Overlay>
  )

Scope

This issue of course isn't just limited to "Escape". If an application has global shortcuts, events bubbling out of Overlay can fire global shortcuts.

Why does this happen?

When escape is pressed, you would expect it to be before because Overlay is a child of the Page, but this isn't the case. Overlay uses the useOnEscapePress hook which attaches an event handler on the document, this is great because no matter where the focus is, pressing Escape will close the top most Overlay.

However, because this event listener is on the document, we cannot predict the order in which it will be fired - will it be before the global handler on the page or after it? And that's why stopPropagation and preventDefault will not stop the global handler on the Page from catching this event. Here is a loom with a demo of this

There is a repro of this issue in our storybook setup and a failing test in our test suite that would help in finding a fix for this.

Prior work / failed attempts:

We made an attempt to fix this issue in https://github.com/primer/react/pull/1824 by moving the event listener to the Overlay instead of putting it on the document (with container.addEventListener) and adding a event.stopPropagation automatically.

This stopped events from bubbling up, but created a few other bugs:

1. The Overlay eagerly caught keydown events that were attached to children with onKeyDown inside the Overlay before they fired on the children first. (Story for regression testing)
2. The Overlay does not catch Escape presses when it is not in focus (which probably a bug anyway?)

Potential fix

After the changes in https://github.com/primer/react/pull/1861, we can attempt to move the event listener back to the Overlay container.

1. We will have to pair it with adding focus trap on the Overlay, so that focus does not move outside the Overlay and all Escape presses are caught by the Overlay.

2. Lastly, you should be able to call event.stopPropagation inside a keydown event on a child component inside Overlay (like a TextInput) to prevent the Overlay from closing if Escape is pressed. (Story for testing). If the Overlay is eagerly catching events meant for its children, we can move the event listener to a onKeyDown instead of container.addEventListener. Useful reference to event delegation in React

[rezrah]

Initial investigation: Had some difficulty with narrowing down an actual fix. Simply stopping propagation on the escape handler in AnchoredOverlay doesn’t work, as the escape handler doesn’t get called / is short-circuited in an ancestor event handler.

Things tried but didn’t work:

• Manually intercepting the keyDown handler inside AnchoredOverlay and DatePickerOverlay to stop propagation
• Short-circuiting the render function on keyboard events
• Tweaking the capture / focus target for the portal, to prevent it bubbling the synthetic events to ancestor keyboard handlers

May need some more context on how events are handled in the target project before triaging further.

Some related issues I stumbled upon, that indicate similar challenges (and some solutions) with event bubbling in Portals:

https://github.com/facebook/react/issues/11387 https://github.com/facebook/react/issues/19637

[siddharthkp]

This is a bug with Overlay abstraction (not AnchoredOverlay), Added repro with our storybook to the description ⬆️

[siddharthkp]

Hi! After trying a bunch of things out in primer and application land, finally settled with this: https://github.com/primer/react/pull/1824. Escape events on Overlays now simply don't bubble up the tree, which would fix the reported issue and avoid any future ones.

[rezrah]

Thanks @siddharthkp - just a heads up for anyone waiting on this fix - it will ship as part of https://github.com/primer/react/pull/1825

[siddharthkp]

This issue is back because after reverting https://github.com/primer/react/pull/1824.

Updated the original description with more information.

[github-actions[bot]]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

[siddharthkp]

This is still a limitation of the Overlay, keeping it open

[lesliecdubs]

No one is actively asking for this right now so we're going to close for the moment. Let's reopen if needed.