search

reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.37k stars 808 forks source link

Either document or rethink Portal usage #699

Open reverie opened 6 years ago

reverie commented 6 years ago

Summary:

The usage of React's Portal feature causes surprising behavior. A modal is visually and conceptually a separate interface element from the element that opens the portal.

Example 1: If you click on a button to open a modal, should clicking on that modal bubble up to the button? No.

Example 2: If you open a modal from a form, for example to autocomplete an input to that form, should submitting the modal's form submit the outer form? No.

What makes this behavior worse than merely surprising is that it is not documented anywhere. Given that modals are separate from the elements that create them, I don't think creation of a Portal should be the default behavior of this library. If it will remain the default, it should be documented, and ideally an option to turn it off should be provided.

diasbruno commented 6 years ago

Hi @reverie, those are good points.

It's always possible that the documentation doesn't answer all the questions. Users always find different ways to use a libraries and/or sometimes features need clarification that is not there...

Look interesting this idea of change how the modal is mounted on the DOM. If you can elaborate some ideas.

reverie commented 6 years ago

Yes, I wonder whether it’s possible to avoid using Portal at all—it might not be. Some of my issues might be with Portal itself, like if it propagates events in ways that are surprising relative to the browser’s natural propagation rules.

diasbruno commented 6 years ago

@reverie yes, I agree it can be confuse. This should be the way how synthetic events propagates (if I'm remembering correctly). If you can make a small example, I'll try to have a look and we can discuss what can be clarified in the docs and/or if there are better ways to handle this case.

jcrben commented 5 years ago

I think this is the upstream issue for the confusing bubbling: https://github.com/facebook/react/issues/11387

Seems like a bit of a dealbreaker, altho there's a proposed workaround or two such as @sebmarkbage in https://github.com/facebook/react/issues/11387#issuecomment-366142349 involves pulling the modal up higher in the tree - which doesn't seem so great as the Portal might be way below the button and moving them around breaks cohesion.

bsansouci commented 5 years ago

After reading https://github.com/facebook/react/issues/11387 it seems to me like I cannot use react-modal since the event propagation will break my app. I don't want to register events handlers for every kind of event and call stopPropagation because it's clear that that will have side effects (for example). One way to mitigate this issue is to use react-modal-hook put the ModalProvider component as high as possible in the component tree, but that still uses portals which seem to have complex behavior that is potentially error prone.

I'd love to see react-modal fix this issue internally!

juanitogan commented 4 years ago

I just wasted maybe 4 hours on this. I'm upgrading an app from React 0.14.7 to 16. This brings a react-modal upgrade from 0.6.1 to 3.11.2. The old setup didn't propagate events up from the modal. That makes sense for a modal. Right? Isn't that pretty much the definition of a modal?

Now, all of a sudden, a form submit in the modal also submits the form outside the modal!?? How does that make sense? With a complete lack of documentation around this new counter-intuitive "feature," it took me a long time to work out what had changed and how to fix it in the most sensible way. Okay, stupid me, I don't work with React and JS much these days and I'm pretty sure I've never had to use stopPropagation() before. Thus, I spent a lot of time here rereading the docs and examples, and pulling up issue posts about onClick that were red herrings.

So much for the principle of least surprise. BTW, I took a very quick glance at some of the PRs. When fixing this, please modalize all events and not just onClick. Until then, this plugin should be renamed to looks-like-a-react-modal-but-not-really, and document why it needs to be called that in big, red letters at the top of the readme.