reactjs / react-modal

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

Warning: react-modal: App element is not defined. #576

Open StokeMasterJack opened 6 years ago

StokeMasterJack commented 6 years ago

I upgraded my yarn version and re-ran yarn install. And now I am getting this warning.

It's strange, because nothing in my code has changed. Something in node_modules is obviously different but who knows what. I can't do a diff on node_modules because that was never check in to git.

diasbruno commented 6 years ago

@StokeMasterJack we made some changes recently to api to define the app element. Can you get the log message you got?

diasbruno commented 6 years ago

Can you check if setting Modal.setAppElement the error goes away?

Ref: https://github.com/reactjs/react-modal#app-element

StokeMasterJack commented 6 years ago

Here is the exact log message:

Warning: react-modal: App element is not defined. Please use Modal.setAppElement(el) or set appElement={el}. This is needed so screen readers don't see main content when modal is opened. It is not recommended, but you can opt-out by setting ariaHideApp={false}.

diasbruno commented 6 years ago

Oh sorry, thought it was an error. This is related to this change #570. This is a reminder that not setting the app element will/can cause accessibility issues.

diasbruno commented 6 years ago

So, you need to explicit tell the modal that you don't want to use the app element with ariaHideApp={false} (if that's your case).

diasbruno commented 6 years ago

I'll keep this open. Documentation is missing for this behavior.

dlong500 commented 6 years ago

Can this feature be explained a bit more? When I try to use it I'm getting an error like so: ReferenceError: document is not defined coming from ariaAppHider.js

Also, is there a reason this configuration can't be encapsulated into the Modal props and must use Modal.setAppElement? How are you supposed to handle the feature given multiple Modals throughout a large app in various components? Is it just one global setting that affects all Modals?

dlong500 commented 6 years ago

Ah, I think the issue was my server rendering of the app. Once I wrapped the setAppElement call in a check to make sure it was in a client/browser environment then it appears to run OK.

I'm a still a bit unsure if things are working properly though especially because the docs state "If you are doing server-side rendering, you should use this property". That doesn't seem to make sense to me because the DOM is not even available at that point yet.

diasbruno commented 6 years ago

@dlong500 The documentation related to this is outdated. Also, we need to tests how the modal will work on a SSR environment. I did some tests to get it working, but very superficial - just to make it work.

For the multiple modal case, you can use appElement attribute.

mandazi commented 6 years ago

I am also getting this in the console log. No code changes in terms of my use of react-modal either. Not sure why I am getting it. Warning: react-modal: App element is not defined. Please useModal.setAppElement(el)or setappElement={el}. This is needed so screen readers don't see main content when modal is opened. It is not recommended, but you can opt-out by settingariaHideApp={false}.

mandazi commented 6 years ago

Nevermind I just read the thread again. Looks like I need the warning is correct. We need just need to specify it.

amangeot commented 6 years ago

Hello, I got the same warning, how do you guys make use of appElement?

Is it appElement={document.getElementById('root')} where 'root' would be where the app is mounted?

mockdeep commented 6 years ago

I was seeing this when the modal is unmounted without having been triggered. Adding this fixed the issue:

Modal.setAppElement('#app-base');
sksundram commented 6 years ago

@mockdeep Where did you add Modal.setAppElement('#app-base');

mockdeep commented 6 years ago

@sksundram right now I've got it in each of the files that I'm importing Modal for rendering. I suspect you could put it just about anywhere, though, like in your top-level app.js file, as long as it is called before the modal is rendered.

import Modal from 'react-modal';
import React from 'react';

function HelpModal({isOpen, closeModal}) {
  return (
    <Modal isOpen={isOpen} onRequestClose={closeModal}>
      'modal content'
    </Modal>
  );
}

Modal.setAppElement('#app-base');

export default HelpModal;
NathanielHill commented 6 years ago

For reference, since it was a pain for me, if you are doing SSR, use the following snippet:

  if (typeof(window) !== 'undefined') {
    ReactModal.setAppElement('body')
  }
lbelinsk commented 6 years ago

@diasbruno I wouldn't mind preparing the documentation for the ssr and appElement subjects. If that sounds ok, If there is any specific info you want me to elaborate on while I am at it, please let me know.

For the meantime, just for reference, for the projects which use react-modal, there is no need anymore to add the safety check that @NathanielHill suggested.

Instead of doing:

if (typeof(window) !== 'undefined') {
   ReactModal.setAppElement('body')
}

It is safe enough to do:

ReactModal.setAppElement('body')
diasbruno commented 6 years ago

@lbelinsk Sure, it would be really great.

catamphetamine commented 5 years ago

@NathanielHill @lbelinsk Doing ReactModal.setAppElement('body') is wrong because it sets aria-hidden property on body while the modal itself is also inside body. The point of appElement is the modal itself being outside the appElement. Otherwise you'd just make things weirder than without it. And it would not be a valid ARIA.

NathanielHill commented 5 years ago

@catamphetamine

Fair point. For me the issue was just setting it to begin with. If what your saying is correct, that does sound like an accessibility issue, so if your React app has a container <div> as a child of <body> then that would be a better target.

ReneCode commented 5 years ago

using the react-testing-library (https://testing-library.com/) I get rid of that warning with:

import Modal from "react-modal";

const { container } = render(<MyComponent />);
Modal.setAppElement(container);

....  // to the testing, use Modal

or, if you want to test the modal component directly:

const { container, rerender } render(<MyModalComponent isOpen={false} />);
Modal.setAppElement(container);
// now the appElement is set we can show the modal component
rerender(<MyModalComponent isOpen={false} />);

....  // to the testing
emmy29 commented 4 years ago

Adding ariaHideApp={false} to modal works for me. <Modal isOpen={this.state.modalOpen}ariaHideApp={false}> <LoginModal closeModal={() => this.openModal()}></LoginModal> </Modal>

raphab3 commented 2 years ago
Modal . setAppElement ( '#app-base' )

Solved it, in my case it was Modal.setAppElement("#root"); in the modal component

patricknazar commented 1 year ago

Got rid of the annoying logs in tests with Modal.setAppElement(document.querySelector('body')!)

diasbruno commented 1 year ago

To all...

This is the correct behavior for react-modal as already mention on this thread.

As @catamphetamine already mention and many others that have been working with accessibility, there is an old mistake with the default app element which uses the document.body to add the aria-hidden attribute. This cause a problem because every element that is going to be hidden is a direct descendant of document.body (the modal also becomes inaccessible).

To not break users code, we decided to make setAppElement required (or use appElement on each case where you use modal), but just warn about the problem to not break stuff...

This also applies to testing. You need to properly setup your tests to use react-modal when needed.

This is an example to do that (note that you might need to change a little bit depending on the testing framework you use):

let modalNode = null;

before(() => {
  modalNode = document.createElement('div');
  modalNode.id = "modals";
  document.appendChild(modalNode);
  ReactModal.setAppElement('#app');
}):

after(() => {
   document.removeChild(modalNode);
});
ADTC commented 4 months ago

In Next.js App Router, sometimes the client components could be pre-rendered in the server. I had been using appElement={document.querySelectorAll('.container')} but this would fail in the server-side pre-render with document is not defined. Adding ? after document will not work.

Using a try { ... } catch {/* ignore errors */} with a let variable will seem to work, but then I thought a little more deeply about this, and I realized that if the component is pre-rendered in server, then it will come to the client with appElement set to undefined. Then ReactModal will have an undefined appElement, possibly for a long time, until the next time the client component is rendered again in client.

So I think the below would be the correct way to do this. Even if the component is pre-rendered in server, it would hydrate the appElement with the correct element once the pre-render result is transmitted to the client. Is this correct?

import { useEffect, useState } from 'react'
import ReactModal from 'react-modal'

// in the component:
  const [appElement, setAppElement] = useState<NodeListOf<Element>>()

  useEffect(() => {
    // When Next.js pre-renders client components in server to generate initial HTML,
    // document is not defined. `?` will not work here, hence the effect & state variable.
    setAppElement(document.querySelectorAll('.container'))
  }, [])

  return (
    <ReactModal
      appElement={appElement}
      // ...
      >
    </ReactModal>
  )

I hope I'm not the first one to figure this out. 😆

romilsondev1 commented 4 months ago

In next js 13.5.4 I found the error.

I solved it as follows:

I added the following command to the pages/app.tsx directory

import Modal from 'react-modal';

and before defining my app() function

I added:

Modal.setAppElement('body');

import Modal from 'react-modal';

Modal.setAppElement('body');

export default function app(){