reactjs / react-modal

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

Cannot register modal instance that's already open #808

Open Hainesy opened 4 years ago

Hainesy commented 4 years ago

When a modal is open, double-clicking on the background overlay results in the modal disappearing then quickly reappearing again with the following warning in the console:

react_devtools_backend.js:6 React-Modal: Cannot register modal instance that's already open 
    at ModalPortal (http://localhost:4500/static/js/0.chunk.js:60569:5)
    at Modal (http://localhost:4500/static/js/0.chunk.js:60212:5)

Additionally, the ReactModal__Body--open class remains on the body after closing the re-opened modal.

Hainesy commented 4 years ago

Managed to get around the issue by adding pointer-events: none to .ReactModal__Overlay--before-close.

diasbruno commented 4 years ago

@Hainesy

[edit] Thought we had a handler for double-click on the overlay. So, once we have that it should do only one action executed.

rahul-desai3 commented 3 years ago

I am getting same error even after setting pointer-events: none as mentioned above

tonhao-dev commented 3 years ago

When I click once outside the modal to close it I get: React-Modal: Cannot register modal instance that's already open

and when I click outside the modal again I get: portalOpenInstances.js:33 React-Modal: Unable to deregister [object Object] as it was never registered

and then the modal is closed

Hainesy commented 3 years ago

I agree this is still a problem

timothymalcham commented 2 years ago

I am now getting this error after upgrading to React 18. Prior to doing the upgrade, I did not see this error: React-Modal: Cannot register modal instance that's already open. Does anybody have any idea of what might have changed?

pioferen commented 2 years ago

This issue still occurs, I'm using version 3.5.1

diasbruno commented 2 years ago

Version 3.15.1 was released. Is this happening on this version?

pioferen commented 2 years ago

Version 3.15.1 was released. Is this happening on this version?

Yes, but I think it occurs only in development mode with react 18 and enabled StrictMode https://github.com/reactwg/react-18/discussions/19

diasbruno commented 2 years ago

Hmmm...this is interesting...could you please help creating a reproducible example?

pioferen commented 2 years ago

Hmmm...this is interesting...could you please help creating a reproducible example?

here: https://stackblitz.com/edit/nextjs-aznh2l?file=pages%2Findex.js

without modalIsOpen && error doesn't occur

diasbruno commented 2 years ago

Hmmmm...It's recommended to not use modal with conditional rendering. There is a problem with createPortal when using this way (need to check if it still happening. It starts happening on version +16.3 of react). Are you all using conditional rendering?

diasbruno commented 2 years ago

cc @Hainesy @timothymalcham @rahul-desai3 @luis-antonio-dev

Brhernann commented 2 years ago

Same warning

image

deep-cognite commented 2 years ago

Facing the same issue for one of my modals after upgrading to React 18. Any fixes to this? Funnily enough, the modal starts working "after a while" (I think when I leave the app and come back). Super strange

lassonico commented 2 years ago

¿En definitiva no hay solucion?

diasbruno commented 2 years ago

Please read this to help debugging this issue

It's recommended to not use modal with conditional rendering. There is a problem with createPortal when using this way (need to check if it still happening. It starts happening on version +16.3 of react). Are you all using conditional rendering?

A suggestion is: don't use conditional rendering with modals...there is no need to. There is no performance cost, there is no problem to maintain (it's only one state).

Sorry for this guys... :joy: But please let me know if you have a different case where this happens other than with conditional rendering.

EGurney commented 2 years ago

Please read this to help debugging this issue

It's recommended to not use modal with conditional rendering. There is a problem with createPortal when using this way (need to check if it still happening. It starts happening on version +16.3 of react). Are you all using conditional rendering?

A suggestion is: don't use conditional rendering with modals...there is no need to. There is no performance cost, there is no problem to maintain (it's only one state).

Sorry for this guys... 😂 But please let me know if you have a different case where this happens other than with conditional rendering.

This solved the issue for me.

I was conditionally rendering in my component, rather than letting the isOpen prop handle the rendering.

rodriguezmarting commented 1 year ago

The bug also happens when the Modal lives next to a component that conditionally renders. Plus it only happens when using base | afterOpen | beforeClose classNames.

Something like this won't work:

export const MainHeader2 = () => {
  const { isMobile } = useContext(ViewportContext);

  const [isOpen, setIsOpen] = useState(false);

  const toggleConfirmModalOrDrawer = () => {
    setIsOpen((prevState) => !prevState);
  };

  const doSomething = () => someAction();

  return (
    <header>
      <Logo handleClick={toggleConfirmModalOrDrawer} />
      <Modal
        className={{
          base: styles.modalContent,
          afterOpen: styles.modalContentAfterOpen,
          beforeClose: styles.modalContentBeforeClose,
        }}
        overlayClassName={styles.modalOverlay}
        isOpen={!isMobile && isOpen}
        onRequestClose={toggleConfirmModalOrDrawer}
      >
        <ConfirmLeaveModalContent
          onConfirm={doSomething}
          onDismiss={toggleConfirmModalOrDrawer}
        />
      </Modal>
      {isMobile && (
        <ConfirmLeaveDrawer
          onConfirm={doSomething}
          onDismiss={toggleConfirmModalOrDrawer}
        />
      )}
    </header>
  );
};
diasbruno commented 1 year ago

@rodriguezmarting Awesome. This is weird... Can you make a step to reproduce so we can see how are you triggering this behavior?

oroszi1mark commented 1 year ago

Using react-modal with nice-modal-react makes for a handy combination in my opinion, but unfortunately nice-modal-react also uses conditional rendering. The console warning can be triggered in this example.

Phanabani commented 1 year ago

I was having this error (react-modal@3.15.1, React@18.2.0), and after a lot of frustration found out that my problem was caused by using React.StrictMode in my root element, which apparently double-invokes some lifecycle methods in development mode (not production, though). As of now, this behavior is documented in the StrictMode docs -- search for "double-invoking the following functions".

I found this solution by accident, because I was investigating an effect being called twice and found this answer, which also referenced this answer which has more info about the double rendering of StrictMode.

It seems like this is harmless in my case because the only visible issue is the warning in console (modals are still displaying fine), but if anyone has a suggestion for suppressing the warning, I'd appreciate it! Removing the StrictMode tag stops this issue from happening, but I want to keep the benefits of using strict mode.

mleister97 commented 1 year ago

Please fix this issue!!!

diasbruno commented 1 year ago

Why don't you fix it, @mleister97?

honeymaro commented 1 year ago

I have the same issue.

no conditional rendering, no strict mode.

react-modal 3.16.1, react 18.2.0

diasbruno commented 1 year ago

This seems to be a mem leak on the src/helpers/portalOpenInstances.js, but it's most likely that this is caused by something that is holding the reference of a modal that should already be gone.

rodriguezmoradiego15 commented 1 year ago

We Can't use: {modalIsOpen && (......) The best way to use that is creating a component where we have all the code that we need, a componet seems something like this:

`<Modal isOpen={modal} style={customStyles}

and ModalProducto.jsx is something like this: export default function ModalProducto() { return (

ModalProducto

) } `

That way works for me

No podemos usar: {modalIsOpen && (......) Lo que se debe hacer es crear un componente con toda la lógica que necesitamos, algo parecido a lo siguiente: `<Modal isOpen={modal} style={customStyles}

`

y ModalProducto.jsx es algo como lo siguiente: `export default function ModalProducto() { return (

ModalProducto

) } ` Esa es la solución.

voldemote commented 1 year ago

This error is occurring because you haven't declared the CSS for the modal. If you declare the CSS in a global file such as index.css, the error will not happen.

.mymodal { position: fixed; top: 50%; left: 50%; transform: translate(-50%, -50%);

border: 1px solid #ccc; background: #fff; overflow: auto; border-radius: 4px; outline: none; padding: 20px; }

.myoverlay { position: fixed; top: 0; left: 0; right: 0; bottom: 0; background-color: rgba(0, 0, 0, 0.75); }

.ReactModal__Overlay { opacity: 0; transition: opacity 500ms ease-in-out; }

.ReactModal__Overlay--after-open { opacity: 1; }

.ReactModal__Overlay--before-close { opacity: 0; }

brianpeiris commented 1 year ago

If it helps, I was able to reproduce this issue with minimal changes to the simple_usage example in this repo.

The steps I took:

  1. Upgrade to React 18
  2. Add React.StrictMode to the example
  3. Use createRoot instead of ReactDOM.render
  4. Set isOpen to true by default

The warning then appears in the console when you open the example.

Here is the diff: https://github.com/reactjs/react-modal/compare/master...brianpeiris:react-modal:register-warning?w=1

I did not have to use conditional rendering to reproduce this issue, and more people will run into it as they adopt React 18. Notably, modern tooling, like Vite, enables StrictMode by default.

diasbruno commented 1 year ago

@brianpeiris Yep, this is not just a case of conditional rendering (but kinda related).

The problem seems to be a new feature of react 18 where it can keep some components alive to make some rendering optimizations. But this causes all sorts of problems because react-modal is a stateful component that needs to deal with subtrees.

diasbruno commented 1 year ago

I'm flagging this as hardcore. I did a little investigation, don't have time to fully jump into this, and, this is super annoying.

It's all about the new react version and this component src/components/Modal.js, specially when the component is unmounted.

If you want to be at the 1% top react dev, this is a great challenge.

You need to:

This is enough information to start digging in.

If anyone wants to fight with this, let me know. For any other questions, I'll be here to help.

Hebzebba commented 1 year ago

Version 3.15.1 was released. Is this happening on this version?

Yes, but I think it occurs only in development mode with react 18 and enabled StrictMode reactwg/react-18#19

Disabling StrictMode fix the issue for me

berryboylb commented 1 year ago

I don't

I'm flagging this as hardcore. I did a little investigation, don't have time to fully jump into this, and, this is super annoying.

It's all about the new react version and this component src/components/Modal.js, specially when the component is unmounted.

If you want to be at the 1% top react dev, this is a great challenge.

You need to:

  • Check the places where we (de)register the modal (mount and unmount)
  • Make sure react calls unmount when it is supposed to (this is actually a problem since react 16.3+)

This is enough information to start digging in.

If anyone wants to fight with this, let me know. For any other questions, I'll be here to help.

I don't want to be a top react dev, just fix the issue please.

diasbruno commented 1 year ago

@berryboylb :crying_cat_face:

shulcsm commented 1 year ago

Issue is here: https://github.com/reactjs/react-modal/blob/master/src/components/ModalPortal.js#L136

When strict mode unmounts isOpen is not yet initialized so it's not de-registering the modal correctly so it leaks instances and focus handlers.

This "works"

  if (this.state.isOpen || (typeof this.state.isOpen === 'undefined' && this.props.isOpen)) {
    this.afterClose();
  }

But honestly whole thing should be rewritten.

diasbruno commented 1 year ago

@shulcsm You can rewrite if you want. We will be really happy.

This library remains working on really dated versions of react to maintain backwards-compatibility. It's great, but also has some drawback...

Feel free to share some ideas regarding this.

doeg commented 4 months ago

Hey @diasbruno, want me to take this one? Feel free to assign it to me if so. 🙇

ADTC commented 2 months ago

This happened to me, not because of conditional rendering, but because I was forcing the modal to be already open upon page load:

  const [isOpen, setIsOpen] = useState(true)

The fix is to use an effect hook to open the modal after the component has loaded:

  const [isOpen, setIsOpen] = useState(false)

  // Fix for warning "Cannot register modal instance that's already open".
  // The modal should only be opened after the component has mounted.
  useEffect(() => setIsOpen(true), [])

This will help force the modal to be open as soon as the page loads, but avoids the warning "Cannot register modal instance that's already open".

zloegucci commented 3 weeks ago

Conditional rendering causes the problem, true. Change {isOpen && <Modal isOpen={isOpen}/>} to just <Modal isOpen={isOpen}/>

you can use dynamic (react or next)