Improve accessibility, fixes #359

[sheerun]

• adds aria-modal="true" to modal portal
• doesn't make body a default appElement
• warns if appElement is not set in any way

Fixes #359

Acceptance Checklist:

• All commits have been squashed to one.
  Unnecesary rule: You can squash with button when merging
• The commit message follows the guidelines in CONTRIBUTING.md.
  Unnecessary rule: You can edit commit message as well :)
• Documentation (README.md) and examples have been updated as needed.
• If this is a code change, a spec testing the functionality has been added.
• If the commit message has [changed] or [removed], there is an upgrade path above.

[sheerun]

I've updated the readme as well

[sheerun]

Also fixed prettier issues

[sheerun]

also satisfied eslint

[coveralls]

Coverage increased (+0.08%) to 86.341% when pulling 7c36750 on sheerun:accessibility into 700eb4e on reactjs:master.

[diasbruno]

Fantastic! I have to finish the review later, but I'll make sure we release this ASAP.

[diasbruno]

Running some tests, everything looks great. The only question I have is making setAppElement required.

react-modal has become more flexible in terms of accessibility, like a user may want to disable some a11y feature on a edge case...so, one option is to not set aria-hidden if appElement is not defined.

We could write a log message, on dev mode, explaining why not defining appElement it is bad.

Any thoughts?

[sheerun]

@diasbruno I didn't make setAppElement required. I just show a warning if it's not set.

I also updates PR so it doesn't show warning in production, tells how to opt-out with ariaHideApp and tells why setting this is necessary for accessibility. The message says now:

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

[diasbruno]

Oh, ok...what about the second part (don't set aria-hidden if appElement is not defined)? Having ariaHideApp={false} is great, but it will require users to fix their code (breaking change). Any ideas?

[sheerun]

@diasbruno As I said it's not a breaking change as it only shows a warning, and only in development.

[diasbruno]

Ah, ok. It will be noop in this case.

[coveralls]

Coverage increased (+0.2%) to 86.441% when pulling 123b5f1 on sheerun:accessibility into 700eb4e on reactjs:master.

[diasbruno]

Sorry, I wasn't paying attention to this case. I was think on something more explicit like appElement != null && ariaHideApp, but this way looks better.

[sheerun]

There is:

    if (ariaHideApp) {
      ariaAppHider.hide(appElement);
    }

but feel free to change it (this PR is editable by you). It seems artificial to me to pass ariaHideApp variable to show/hide, just to do simple && ariaHideApp

[diasbruno]

Thank you so much @sheerun!

[diasbruno]

Release v3.1.6.