-
Notifications
You must be signed in to change notification settings - Fork 48.2k
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
Comments
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 caseFor 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
The Case where it's super helpfulAssume the below, 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. |
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 https://codesandbox.io/s/collocate-concerns-with-portals-yrjkc Clicking outside the button exits the button editing mode but interacting with the property panel does not as it is inside the 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. |
Just popping the https://codesandbox.io/embed/usefocusenter-forked-x6i0n 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? |
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) This issue came up when using the 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 A workaround you are currently using, if any One workaround is to change the containment of components, effectively moving the <>
<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 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 I believe |
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. |
@jjenzz yes it did. We actually already had that in place, but another issue was preventing our code from reaching For others reading this, the @gaearon that said, I think my use case can be sufficiently worked around. Still interesting, nonetheless! Keep up the good work. |
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 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. |
Another example where this is super useful: CleanShot.2021-05-28.at.14.15.27.mp4Specifically, because we are using roving focus:
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),
};
} |
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 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> |
Hey all, here's a usecase where I think Portals should not bubble events according to the React tree: Namereverse-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." |
How relevant is this case still, with broad browser support for the |
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:
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!
The text was updated successfully, but these errors were encountered: