Dialog V2 does not work when stacked over Overlays or other components using useOnClickOutside

[iansan5653]

Description

The new draft Dialog component does not use useOnClickOutside under the hood, because it has its own backdrop element that it could handle clicks on if necessary. This makes sense, but unfortunately breaks the dialog when stacked over Overlay, AnchoredOverlay, or similar.

This is because when a component under the dialog has registered a click-outside handler using useOnClickOutside, a click in the Dialog is seen as a click outside of the overlay.

useOnClickOutside automatically handles this for stacked overlays by maintaining a global registry, but because Dialog does not call useOnClickOutside, it does not get registered to that global registry.

The workaround here is non-obvious but simple: call useOnClickOutside inside Dialog, even though the handler will be a no-op. The containerRef passed to useOnClickOutside must include the backdrop element so that there are never any clicks outside registered. This will cause all clicks to be registered as inside the Dialog, stopping them from propagating.

const backdropRef = useRef(null)

useOnClickOutside({containerRef: backdropRef, onClickOutside: () => { /* no-op */ }})

Steps to reproduce

I couldn't get CodeSandbox working with the latest version of Primer, so here's a full demo as code instead:

import { Button, Overlay, OverlayProps } from "@primer/react";
import { Dialog } from "@primer/react/drafts";
import { useState, useRef } from "react";

const OverlayWithDialog = (props: OverlayProps) => {
  const [dialogVisible, setDialogVisible] = useState(false);

  return (
    <>
      <Overlay {...props}>
        <Button onClick={() => setDialogVisible(true)}>Open Dialog</Button>
      </Overlay>

      {dialogVisible && (
        <Dialog onClose={() => setDialogVisible(false)}>
          Clicking inside me should not close me or the overlay, but it does.
        </Dialog>
      )}
    </>
  );
};

export default function App() {
  const [overlayVisible, setOverlayVisible] = useState(false);
  const buttonRef = useRef<HTMLButtonElement>(null);

  return (
    <>
      <Button onClick={() => setOverlayVisible(true)} ref={buttonRef}>
        Open Overlay
      </Button>
      {overlayVisible && (
        <OverlayWithDialog
          returnFocusRef={buttonRef}
          onClickOutside={() => setOverlayVisible(false)}
          onEscape={() => setOverlayVisible(false)}
        />
      )}
    </>
  );
}

Version

v35.19.0

Browser

Chrome

[github-actions]

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.

[github-actions]

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.

[iansan5653]

🙃

[github-actions]

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.

[lesliecdubs]

@keithamus is there a chance https://github.com/github/primer/issues/2531 would solve this?

[keithamus]

Yes. We wouldn’t use onclickoutside but rather the native dialog functionality (which would need to be polyfilled but it’s something we can scope into that epic)

[lesliecdubs]

Thanks! Adding this as a piece of "Corresponding Work" to https://github.com/github/primer/issues/2531 for future consideration.