Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portal Event Bubbling Use Cases #19637

Open
gaearon opened this issue Aug 18, 2020 · 13 comments
Open

Portal Event Bubbling Use Cases #19637

gaearon opened this issue Aug 18, 2020 · 13 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2020

This is a spillover from #11387.

The goal of this issue is not to argue about whether the current React's behavior makes sense in all situations. Rather, it is collect a list of use cases, both when the current behavior works well, and when it doesn't, so that they can inform the next iteration of the related APIs. We can't commit to any concrete timeframe on this at the moment, but a list like this will definitely reduce the amount of time that we'd need to spend to get up to speed on the problem space when we're ready to approach it.

If you'd like to contribute a use case, please comment with:

  • A name for your pattern (come up with something unique so we can refer to it later)
  • A brief description of the UI (but a screenshot is worth a thousand words)
  • A small CodeSandbox demo, if you want to make a stronger case
  • How React event bubbling behavior breaks (or helps) your case
    • Include any information about other pitfalls you encountered, be very specific
  • A workaround you are currently using, if any
    • If you tried some workarounds but they cause issues, let us know which ones

Please keep this thread on topic and let's keep general discussion in #11387. This is not a good thread for "+1" or requests to solve this faster — it's a thread for gathering research.

Thank you!

@gaearon gaearon added Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Aug 18, 2020
@jquense
Copy link
Contributor

jquense commented Aug 18, 2020

Gonna toss the two typical cases we run into quickly here, and hopefully follow up with some actual code and more specific actual cases of workarounds or benefits as i think of places in our code bases that have em.

Canonical Modal case

For posterity I think this is probably the canonical "Why this is unhelpful" case, at least as far as we see in in issues via react-bootstrap.

function OpenModalButton() {
  const [show, setShow] = useState(false);
  return (
    <Button onClick={() => setShow((prev) => !prev)}>
      Open Me
      <Modal show={show} />
    </Button>
  );
}

This was more compelling before Fragment as it actively prevented encapsulating components naturally. This example does still cause pain when you need to rely on event bubbling further up. IME though that is usually easy to work around (editorializing: I think the workarounds are preferable patterns anyway). Cases where bubbling is depended on usually fall into two categories (as i've seen)

  • You care about the actual DOM hierarchy: workaround is using element.contains to check and filter out event (targets) that aren't relevant.
  • To avoiding prop drilling: workaround is pass a callback, use context

The Case where it's super helpful

Assume the below, CustomSelect renders a it's menu in a portal outside of the modal. As some backdrop, Modals should trap focus inside them, meaning as to tab through the modal body focus should cycle through the tabbable elements as if there was nothing else on the page below the modal. You don't want to accidentally focus an input underneath the modal.

That can be accomplished by ensuring focus/blur events are paired on the Modal. However, that relies on bubbling working Logically instead of "physically", e.g. moving focus to the Select menu actually moves focus outside the Modal element, but semantically focus should be considered inside the modal. With the React bubbling you get that for free, and without it you need to whitelist any possible outside elements, a task that is impossible to generalize for libraries and hard to maintain.

function MyModal({ show }) {
  return (
    <Modal show>
      <div>
        <CustomSelect>
          <Option>Click</Option>
        </CustomSelect>
      </div>
    </Modal>
  );
}

There is one Practical rub with this, which is in practice implementing focus trapping often involves adding event handlers manually to refs, bypassing the React event system. This usually means you don't get to benefit from the React portal bubbling even when you want too, ref: react-focus-lock, react-overlays. This is more of a problem with react missing another API, not the current behavior being bad.

@jjenzz
Copy link

jjenzz commented Oct 16, 2020

This is super helpful for suppressing outside click/focus in/focus out events from portalled content and can really help simplify project architecture.

Aside from the case mentioned like Tooltips, Selects etc, it can also be used to help avoid global state for Sidebar UIs because property panel code can be colocated with the thing they're configuring and outside click events can be prevented when interacting with these property panels:

https://codesandbox.io/s/collocate-concerns-with-portals-yrjkc

pr

Clicking outside the button exits the button editing mode but interacting with the property panel does not as it is inside the ButtonEditor React tree. I did not have to use any complex logic for this.

Imo, if people would like to bind events that respect the DOM tree only, then it would make sense for the code to explicitly bind to DOM events. It would more clearly communicate the intent.

@jjenzz
Copy link

jjenzz commented Oct 17, 2020

Just popping the Tooltip case here too where refocusing the menu button after the menu closes should not reactivate its tooltip (use keyboard, space to open/close - also desktop only, it's PoC code 😅).

https://codesandbox.io/embed/usefocusenter-forked-x6i0n

tooltip

I can continue posting more but the general pattern is the ability to prevent events from portalled content as it is considered inside. Is it worth me posting more use-cases for this?

@adamayres
Copy link

A name for your pattern (come up with something unique so we can refer to it later)

"unwanted-container-events-on-nested-portal"

A brief description of the UI (but a screenshot is worth a thousand words)

react-portal-event-bubbling-issue

This issue came up when using the Nav and Modal components in react-bootstrap. The Nav component applies some key press handlers to allow the arrow left and right keys to be used to navigate between links in the Nav. If one of those links is used to open the Modal component, which uses a portal to render, then the left and right arrow keys used inside of the modal will bubble up to the Nav which will move focus from the Modal and onto one of the links in the Nav.

A small CodeSandbox demo, if you want to make a stronger case

https://codesandbox.io/s/react-bootstrap-nav-arrow-key-bug-5eo1j

<Nav>
  <Nav.Link onClick={() => setShowModal(true)}>Show Modal</Nav.Link>
  <Modal show={showModal}>Some Modal</Modal>
</Nav>

How React event bubbling behavior breaks (or helps) your case

In this specific case the event bubble behavior through the portal breaks this specific use case. The keyboard events on the Nav are reasonable, however we do not want these keyboard events to be applied to the Modal that is triggered open by a link in the Nav.

A workaround you are currently using, if any

One workaround is to change the containment of components, effectively moving the Modal to not be a child of the Nav.

<>
  <Nav>
    <Nav.Link onClick={() => setShowModal(true)}>Show Modal</Nav.Link>
  </Nav>
  <Modal show={showModal}>Some Modal</Modal>
</>

If you tried some workarounds but they cause issues, let us know which ones

Changing the containment of components can be complicated by how components are organized for reuse. For example, in my specific use case the organization of my components is such that I have a reusable LoginLink component that I would like to contain the Modal. This allows the LoginLink component to be easily used in other components other than the Nav and for those other components to not have to worry about adding something like a LoginModal component along side the LoginLink. Example:

function LoginLink() {
  const [showModal, setShowModal] = useState(false);

  return (
    <>
      <Nav.Link onClick={() => setShowModal(true)}>Show Modal</Nav.Link>
      <Modal show={showModal}>Some Modal</Modal>
    </>
  );
}

function NavBar() {
  return (
    <Nav>
      <LoginLink />
    </Nav>
  );
}

@jmikrut
Copy link

jmikrut commented Mar 31, 2021

Hi all,

I've just run into this thread and have a use case that I think might provide some value.

Use Case:

Nested Forms

In Payload CMS, we have many cases where we use Portals to appropriately adhere to HTML spec regarding how forms cannot be nested within one another. To meet this requirement, we portal out "nested" forms into modal windows within a separate area of the DOM.

The Problem

As usual, our forms have a <button type="submit"> that is responsible for submitting the form if clicked. Looking at the DOM, all is well if we portal nested forms, because our "nested" form is actually not nested thanks to it being within a Portal. We are adhering to HTML spec and the main form itself exists in an entirely separate segment of the DOM.

The problem is that when we click the portal form's button to submit the portal form, its event bubbles up and submits the main form. This is unintended and caused us to do a lot of soul-searching to figure out what was happening.

Screenshot of main form with Rich Text editor:
payload-cms-main-form

Screenshot of modal form (to submit a relationship into the Rich Text editor):
payload-cms-relationship-form

The Workaround

Just need to ensure that event.stopPropagation() is called within the nested form onSubmit handler.

@jjenzz
Copy link

jjenzz commented Apr 1, 2021

We are currently evaluating workarounds but have not yet found a solution to the problem. Will update this comment with a workaround if we find one.

@jmikrut I believe event.stopPropagation would help? https://codesandbox.io/s/youthful-dream-4xfns?file=/src/App.js

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 1, 2021

Just wanted to say — thank you for submitting these, they're very helpful. The issue is still on our backlog to revisit, but it's going to make a huge difference to just have all these available as a reference. So please don't hesitate to post more.

@jmikrut
Copy link

jmikrut commented Apr 1, 2021

@jjenzz yes it did. We actually already had that in place, but another issue was preventing our code from reaching e.stopPropagation().

For others reading this, the stopPropagation call, in my case, needed to be placed in the form submit handler—NOT a submit button onClick or similar.

@gaearon that said, I think my use case can be sufficiently worked around. Still interesting, nonetheless! Keep up the good work.

@mormahr
Copy link

mormahr commented Apr 22, 2021

I'm tracking if the mouse is over a list. This is to prevent the user from clicking an element that was just inserted by a live update, when he really wanted to click on a different element.

The list rows allow a user to open a modal. When the modal is opened, the whole screen causes onMouseEnter/onMouseLeave to be fired for the list, as the modal backdrop covers the whole screen. This means that when the user clicks the backdrop to close and their cursor isn't inside the list the onMouseLeave never fires and the update block is held till they enter and leave the list area the next time.

Since the event handler is on the list container, and the modal is mounted several levels deep in the component tree, hoisting it outside requires an inversion of control that doesn't feel natural to react.

@jjenzz
Copy link

jjenzz commented May 28, 2021

Another example where this is super useful:

CleanShot.2021-05-28.at.14.15.27.mp4

Specifically, because we are using roving focus:

  • Focusing an item in the top level menu should close all submenus
  • Focusing a submenu trigger in the top level menu should close branches from the submenu

Made possible by this snippet:

function useFocusOutside(onFocusOutside?: (event: FocusEvent) => void) {
  const handleFocusOutside = useCallbackRef(onFocusOutside);
  const isFocusInsideReactTreeRef = React.useRef(false);
  React.useEffect(() => {
    const handleFocus = (event: FocusEvent) => {
      if (!isFocusInsideReactTreeRef.current) handleFocusOutside(event);
    };
    document.addEventListener('focusin', handleFocus);
    return () => document.removeEventListener('focusin', handleFocus);
  }, [handleFocusOutside]);
  return {
    onFocusCapture: () => (isFocusInsideReactTreeRef.current = true),
    onBlurCapture: () => (isFocusInsideReactTreeRef.current = false),
  };
}

@devongovett
Copy link
Contributor

We're now doing this check on all of our events in React Aria:

if (!e.currentTarget.contains(e.target)) {
  return;
}

See adobe/react-spectrum#1840.

This fixed issues we had with dialogs where something outside the dialog handled an event and called preventDefault on it, which would prevent the default for the event inside the dialog. For example, a text field inside a dialog that's mounted within a table row. Normally, the table row handles mouse down to select the row, and calls preventDefault. This was preventing focus from going to textfields within the dialog.

So far we haven't found a case where events bubbling through portals has been useful. We just keep finding more places where we need to add this check. It feels like a huge footgun in general, especially for components that can contain arbitrary children. If there might be a portal inside, then you have to remember to add these checks to every event you handle to ensure that things work as expected. The alternative is to add handlers for every possible event at the portal boundary and call stopPropagation.

The use cases for bubbling through portals are extremely narrow, and it seems like something you should at least have to explicitly opt into. Portals are used to move part of the react tree into a different location in the DOM. This is a choice made by the component for a good reason, and React should not try to hide that. Most cases for bubbling through portals could be solved in a more explicit way, by adding the necessary event handler inside the portal rather than outside (or in addition).

<div onFocusCapture={onFocusCapture}>
  <button>Outer</button>
  <Portal>
    <div onFocusCapture={onFocusCapture}>
      <button>Inner</button>
    </div>
  </Portal>
</div>

@adri1wald
Copy link

adri1wald commented May 7, 2022

Hey all, here's a usecase where I think Portals should not bubble events according to the React tree:

Name

reverse-portal

What's a reverse portal?

Reverse portals let you "pull a rendered element from elsewhere into a target location within your React tree. This allows you to reparent DOM nodes, so you can move React-rendered elements around your React tree and the DOM without re-rendering them."

httptoolkit/react-reverse-portal#13

@rjgotten
Copy link

rjgotten commented Apr 2, 2025

@jquense

The Case where it's super helpful

Assume the below, CustomSelect renders a it's menu in a portal outside of the modal. As some backdrop, Modals should trap focus inside them, meaning as to tab through the modal body focus should cycle through the tabbable elements as if there was nothing else on the page below the modal. You don't want to accidentally focus an input underneath the modal.

How relevant is this case still, with broad browser support for the inert attribute?
You put inert=true on the body. You put inert=false on your modal. You put inert=false on anything else that might need to be reachable but lives outside your modal for layering purposes. And done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants