Clicks on `Dialog v2`/`ConfirmationDialog` are registered as clicks outside any underlying `Overlay`

[iansan5653]

Normally Primer handles stacked Overlays pretty well - there is a global registry so that clicks on higher overlays don't register as clicks outside lower overlays.

However, the draft Dialog v2 does not use Overlay under the hood, so it does not participate in this global registry. This means that if a dialog is open over something else, clicks in the dialog may register as clicks outside the other thing, causing that lower item to try to close incorrectly.

An example of this bug was encountered in Projects, where a confirmation dialog is shown when attempting to close the issue preview side panel. The side panel is an Overlay with an onClickOutside handler that attempts to close the overlay. When a close is attempted, useConfirm is used under the hood to show a confirmation dialog. Any click in that confirmation dialog will cause another close attempt, opening a new confirmation dialog and preventing the underlying promise from ever resolving.

This can be seen in this video:

https://user-images.githubusercontent.com/2294248/223213871-919d5a6e-1c51-4d5e-936e-91e35f4497ac.mov

The solution for this is to make dialog call useOnClickOutside, even if the handler for that event is a no-op. This will cause it to be registered in the global registry. Alternatively, an Overlay could power the dialog component instead of a plain Portal.

[lesliecdubs]

👋🏻 @iansan5653 thanks for the detailed bug report. Is this behavior actively being used in production as far as you know? And can you share whether you've found a workaround?

[iansan5653]

Is this behavior actively being used in production as far as you know? And can you share whether you've found a workaround?

Yes, we've had to work around this in two instances.

Projects Single-Select Editing

In Projects, when the user creates a new field from the board view, a form appears in an Overlay. When the user adds a single-select option, they can then edit that open, opening a Dialog to manage that option. This is a Dialog over an Overlay where we want to keep the underlying Overlay open. Here's a video of this:

https://user-images.githubusercontent.com/2294248/225062741-ba51ed5f-f6cb-4ddc-9f5f-6e59bec41735.mov

Our workaround for this was to call a no-op useOnClickOutside hook in the Dialog implementation to register it onto the global outside-click handlers registry:

  useOnOutsideClick({containerRef, onClickOutside: e => e.preventDefault()})

Projects Side Panel

Also in Projects: when a user is editing an item in the side panel (an Overlay implementation). If they try to close the panel with unsaved changes, a ConfirmationDialog appears. Here again we want to keep the underlying Overlay open while the Dialog is interacted with on top of it:

https://user-images.githubusercontent.com/2294248/225064562-cbd93880-52ff-47a7-82f0-68ac79c245f4.mov

In this case, we worked around it at the Overlay callsite by setting the Overlay's onClickOutside handler to a no-op:

  <Overlay
    {...otherProps}
    onClickOutside={() => {})
  />

This only worked in this case because we render a backdrop under the side panel so we can just detect clicks on that instead.

[green6erry]

Hi all, I'm currently unable to reproduce the issue. When I edit an issue in a project and try to close by clicking the backdrop, then the "Discard changes?" confirmation dialog only appears once (as it's supposed to). This dialog remains on the screen until I click the 'x' or one of the confirmation dialog buttons. @iansan5653, can you clarify if this issue is still happening in production?

[iansan5653]

Hi @green6erry, this is because we've worked around the bug by setting the onClickOutside handler to a no-op in the side panel overlay so it isn't listening for clicks outside. Then we had to instead add a click listener to the backdrop to still be able to close on outside clicks.

I think we still need to fix this in PRC to avoid hacky workarounds like that

[green6erry]

Okay cool, so just to confirm, I'm re-creating this issue locally and then fixing it, but I won't be able to verify this is working in production later?

[iansan5653]

Correct - it's a bug in PRC but it's not an active bug in production anywhere right now because of our workarounds.

[green6erry]

After working on this and exploring options, we decided to create an epic as the changes required are substantial. I will link it shortly.

[lesliecdubs]

Thanks all. Unassigning @green6erry from this issue but leaving open in the backlog. Will prioritize via https://github.com/github/primer/issues/2266.

[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.

[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.