Portal stopPropagationEvents is a no-op on React 17+

[gluxon]

Environment

• Package version(s): @blueprintjs/[email protected]
• Operating System: macOS
• Browser name and version: Chrome 119.0.6045.199

Code Sandbox

Repository: https://github.com/gluxon/blueprint-portal-stop-propagation-test/issues

React 16 repro: https://stackblitz.com/edit/stackblitz-starters-59vq3z?file=src%2FApp.tsx
React 17 repro: https://stackblitz.com/edit/stackblitz-starters-ahwxzs?file=src%2FApp.tsx
React 18 repro: https://stackblitz.com/edit/stackblitz-starters-rj3w1c?file=src%2FApp.tsx

Steps to reproduce

Create a <Portal> with stopPropagationEvents={["click"]}. The container's click handler will fire on React 17+.

function Test() {
  <div onClick={() => alert("click")}>
    <Portal stopPropagationEvents={["click"]}>
      {/* ... */}
    </Poral>
  </div>
}

React 16

https://stackblitz.com/edit/stackblitz-starters-59vq3z?file=src%2FApp.tsx

react16.mov

React 17

https://stackblitz.com/edit/stackblitz-starters-ahwxzs?file=src%2FApp.tsx

react17.mov

React 18

https://stackblitz.com/edit/stackblitz-starters-rj3w1c?file=src%2FApp.tsx

react18.mov

Actual behavior

The <Portal> click event propagation is only stopped on React 16. It is not stopped on 17 or 18.

Expected behavior

Configuring stopPropagationEvents stops events on React 16, 17, and 18.

Possible solution

Unsure so far, but this is likely due to React 17's event handling changes.

One thing I'm noticing is that Blueprint registers the stop propagation handlers on the exact same DOM node that's eventually passed to React.createPortal.

blueprint/packages/core/src/components/portal/portal.tsx

Line 93 in c3c1a47

addStopPropagationListeners(newPortalElement, stopPropagationEvents);

I would expect this to not work. The event.stopPropagation probably needs to happen on a React synthetic event rather than a real DOM event for React to stop propagating its synthetic events through a portal. The Portal component simply renders its children though, so I'm not sure if a fix is straightforward.

[gluxon]

In real world usage, we're configuring portalStopPropagationEvents on a <Popover /> method. In our case, this seems to be very minimal. I think another reasonable path forward would be to drop support for this prop in a future Blueprint major release since stopping event propagation is a bit messy in the first place.

[ab-pm]

Please consider undeprecating this feature, not necessarily on <Portal> itself but at least for the <Overlay> component.

We're using a <Popover content={<Menu … />}><button … /></Popover> in a location where (click) events from the button or the menu should not bubble up the component tree, but still should show and hide the popover. There's currently no way to achieve this nicely.

Some workarounds:

• make the popover controlled, and intercept the the event via onInteraction/onClose. This is unnecessary and requires a custom state.
• move the <Popover> higher in the component tree, where bubbling no longer matters - i.e. wrap it around the element that is handling the clicks, so that the portal is mounted besides not inside. Use renderTarget to still pass the target props to the button (or a div right around it), not to the top-level popover child. This is awkward (distancing the popover from the target) and no longer works when there's multiple buttons.
• ignore click events in the handler of the outer element that stem from inside the button or from inside the overlay. This increases coupling, requiring the element to know about its children and hardcoding dom selectors.

Suggested solution(s):

• expose onClick (and onMouseEnter, onMouseLeave, etc for the other interaction kinds) props for the <Popover> component and call them from the event handlers you attach to the target overlay container (or forward them directly, depending on the interaction kind)
• add stopPropagationEvents (not portalStopPropagationEvents) to <Popover> (and <Overlay>) components, stop propagation on the root element that is being rendered in the portal (not on the portal element itself!).
• call e.preventDefault() when your're handling a click event by opening/closing the overlay. This allows the outer components to simply ignore events that are already .defaultPrevented.

It might be nice to also have these affect the target (or have separate onOverlayClick/onClick resp. separate overlayStopPropagationEvents/stopPropagationEvents props), though that can already easily be implemented by using renderTarget and decorating the respective event handler prop.

[gluxon]

@ab-pm Great summary! I think you're right — we should consider this for the <Overlay>/<Popover> components.

Perhaps this is implementation minutiae, but I do think this would be a new feature rather than an un-deprecation. The original feature was on <Portal> (as you've noticed): Regardless of any deprecation decisions we make on the Blueprint team, this does not work today on React 17+.

I think what we need is a new feature that does support React 17+ with a different implementation or API. I like this idea:

add stopPropagationEvents (not portalStopPropagationEvents) to <Popover> (and <Overlay>) components, stop propagation on the root element that is being rendered in the portal (not on the portal element itself!).

Do you have a leaning for any of your suggested solutions? Otherwise I like that one the best.

[ab-pm]

Yeah, it wouldn't be an un-deprecation, rather I had in mind an easy update/migration path from (deprecatd) portalStopPropagationEvents to (new) overlayStopPropagationEvents.

I think the most consistent solution would be to have overlayStopPropagationEvents in PopoverProps (I suppose PopoverSharedProps actually? I'm not that proficient in blueprint yet) and stopPropagationEvents in OverlayProps (Overlay2Props?).
Having stopPropagationEvents (that would act on the target) in Popover would be a great addition on top. (Even nicer, just pass true instead of an array of event names to stop propagation of the interactionKind events?)

I think the preventDefault() invocation should also be added in any case, as it improves composability.