Skip to content
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

[change] Class names being added to body during render of non-opened modals #436

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import ReactModal from 'react-modal';
*/
className="ReactModal__Content"
/*
String className to be applied to the document.body.
String className to be applied to the document.body (must be a constant string).
See the `Styles` section for more details.
*/
bodyOpenClassName="ReactModal__Body--open"
Expand Down
4 changes: 4 additions & 0 deletions docs/styles/classes.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,9 @@

Sometimes it may be preferable to use CSS classes rather than inline styles. You can use the `className` and `overlayClassName` props to specify a given CSS class for each of those.
You can override the default class that is added to `document.body` when the modal is open by defining a property `bodyOpenClassName`.

It's required that `bodyOpenClassName` must be `constant string`, otherwise we would end up with a complex system to manage which class name
should appear or be removed from `document.body` from which modal (if using multiple modals simultaneously).

Note: If you provide those props all default styles will not be applied, leaving all styles under control of the CSS class.
The `portalClassName` can also be used however there are no styles by default applied
19 changes: 19 additions & 0 deletions specs/Modal.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,25 @@ describe('State', () => {
unmountModal();
expect(!isBodyWithReactModalOpenClass()).toBeTruthy();
});

it('should not add classes to document.body for unopened modals', () => {
renderModal({ isOpen: true });
expect(isBodyWithReactModalOpenClass()).toBeTruthy();
renderModal({ isOpen: false, bodyOpenClassName: 'testBodyClass' });
expect(!isBodyWithReactModalOpenClass('testBodyClass')).toBeTruthy()
});

it('should not remove classes from document.body when rendering unopened modal', () => {
renderModal({ isOpen: true });
expect(isBodyWithReactModalOpenClass()).toBeTruthy();
renderModal({ isOpen: false, bodyOpenClassName: 'testBodyClass' });
renderModal({ isOpen: false });
expect(!isBodyWithReactModalOpenClass('testBodyClass')).toBeTruthy()
expect(isBodyWithReactModalOpenClass()).toBeTruthy();
renderModal({ isOpen: false });
renderModal({ isOpen: false });
expect(isBodyWithReactModalOpenClass()).toBeTruthy();
});

it('adding/removing aria-hidden without an appElement will try to fallback to document.body', () => {
ariaAppHider.documentNotReadyOrSSRTesting();
Expand Down
7 changes: 4 additions & 3 deletions specs/helper.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react';
import ReactDOM from 'react-dom';
import Modal from '../src/components/Modal';
import Modal, { bodyOpenClassName } from '../src/components/Modal';
import TestUtils from 'react-dom/test-utils';

const divStack = [];
Expand All @@ -19,8 +19,8 @@ if (!(String.prototype.hasOwnProperty('includes'))) {
* open class.
* @return {Boolean}
*/
export const isBodyWithReactModalOpenClass = () =>
document.body.className.includes('ReactModal__Body--open');
export const isBodyWithReactModalOpenClass = (bodyClass = bodyOpenClassName) =>
document.body.className.includes(bodyClass);

/**
* Returns a rendered dom element by class.
Expand Down Expand Up @@ -109,6 +109,7 @@ export const renderModal = function(props, children, callback) {
const currentDiv = document.createElement('div');
divStack.push(currentDiv);
document.body.appendChild(currentDiv);

return ReactDOM.render(
<Modal {...props}>{children}</Modal>
, currentDiv, callback);
Expand Down
39 changes: 7 additions & 32 deletions src/components/Modal.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import React, { Component } from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import ExecutionEnvironment from 'exenv';
import elementClass from 'element-class';
import ModalPortal from './ModalPortal';
import * as ariaAppHider from '../helpers/ariaAppHider';
import * as refCount from '../helpers/refCount';
import SafeHTMLElement from '../helpers/safeHTMLElement';

const EE = ExecutionEnvironment;
const renderSubtreeIntoContainer = ReactDOM.unstable_renderSubtreeIntoContainer;
export const portalClassName = 'ReactModalPortal';
export const bodyOpenClassName = 'ReactModal__Body--open';

const SafeHTMLElement = EE.canUseDOM ? window.HTMLElement : {};
const renderSubtreeIntoContainer = ReactDOM.unstable_renderSubtreeIntoContainer;

function getParentElement(parentSelector) {
return parentSelector();
Expand Down Expand Up @@ -62,8 +60,8 @@ export default class Modal extends Component {

static defaultProps = {
isOpen: false,
portalClassName: 'ReactModalPortal',
bodyOpenClassName: 'ReactModal__Body--open',
portalClassName,
bodyOpenClassName,
ariaHideApp: true,
closeTimeoutMS: 0,
shouldCloseOnOverlayClick: true,
Expand Down Expand Up @@ -99,10 +97,9 @@ export default class Modal extends Component {
this.node = document.createElement('div');
this.node.className = this.props.portalClassName;

if (this.props.isOpen) refCount.add(this);

const parent = getParentElement(this.props.parentSelector);
parent.appendChild(this.node);

this.renderPortal(this.props);
}

Expand All @@ -111,8 +108,6 @@ export default class Modal extends Component {
// Stop unnecessary renders if modal is remaining closed
if (!this.props.isOpen && !isOpen) return;

if (isOpen) refCount.add(this);
if (!isOpen) refCount.remove(this);
const currentParent = getParentElement(this.props.parentSelector);
const newParent = getParentElement(newProps.parentSelector);

Expand All @@ -133,12 +128,6 @@ export default class Modal extends Component {
componentWillUnmount() {
if (!this.node) return;

refCount.remove(this);

if (this.props.ariaHideApp) {
ariaAppHider.show(this.props.appElement);
}

const state = this.portal.state;
const now = Date.now();
const closesAt = state.isOpen && this.props.closeTimeoutMS
Expand All @@ -160,23 +149,9 @@ export default class Modal extends Component {
ReactDOM.unmountComponentAtNode(this.node);
const parent = getParentElement(this.props.parentSelector);
parent.removeChild(this.node);

if (refCount.count() === 0) {
elementClass(document.body).remove(this.props.bodyOpenClassName);
}
}

renderPortal = props => {
if (props.isOpen || refCount.count() > 0) {
elementClass(document.body).add(this.props.bodyOpenClassName);
} else {
elementClass(document.body).remove(this.props.bodyOpenClassName);
}

if (props.ariaHideApp) {
ariaAppHider.toggle(props.isOpen, props.appElement);
}

this.portal = renderSubtreeIntoContainer(this, (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that bugs me. Not sure what happens with this.portal when a modal is open and close too many times. I'll make some tests.
When Modal is unmounted, can this property leaks?

<ModalPortal defaultStyles={Modal.defaultStyles} {...props} />
), this.node);
Expand Down
43 changes: 43 additions & 0 deletions src/components/ModalPortal.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import React, { Component } from 'react';
import { PropTypes } from 'prop-types';
import elementClass from 'element-class';
import * as focusManager from '../helpers/focusManager';
import scopeTab from '../helpers/scopeTab';
import * as ariaAppHider from '../helpers/ariaAppHider';
import * as refCount from '../helpers/refCount';
import SafeHTMLElement from '../helpers/safeHTMLElement';

// so that our CSS is statically analyzable
const CLASS_NAMES = {
Expand Down Expand Up @@ -38,6 +42,9 @@ export default class ModalPortal extends Component {
PropTypes.string,
PropTypes.object
]),
bodyOpenClassName: PropTypes.string,
ariaHideApp: PropTypes.bool,
appElement: PropTypes.instanceOf(SafeHTMLElement),
onAfterOpen: PropTypes.func,
onRequestClose: PropTypes.func,
closeTimeoutMS: PropTypes.number,
Expand Down Expand Up @@ -67,6 +74,15 @@ export default class ModalPortal extends Component {
}

componentWillReceiveProps(newProps) {
if (process.env.NODE_ENV !== "production") {
if (newProps.bodyOpenClassName !== this.props.bodyOpenClassName) {
// eslint-disable-next-line no-console
console.warn(
'React-Modal: "bodyOpenClassName" prop has been modified. ' +
'This may cause unexpected behavior when multiple modals are open.'
);
}
}
// Focus only needs to be set once when the modal is being opened
if (!this.props.isOpen && newProps.isOpen) {
this.setFocusAfterRender(true);
Expand All @@ -84,6 +100,7 @@ export default class ModalPortal extends Component {
}

componentWillUnmount() {
this.beforeClose();
clearTimeout(this.closeTimer);
}

Expand All @@ -99,12 +116,37 @@ export default class ModalPortal extends Component {
this.content = content;
}

beforeOpen() {
const { appElement, ariaHideApp, bodyOpenClassName } = this.props;
refCount.add(bodyOpenClassName);
// Add body class
elementClass(document.body).add(bodyOpenClassName);
// Add aria-hidden to appElement
if (ariaHideApp) {
ariaAppHider.hide(appElement);
}
}

beforeClose() {
const { appElement, ariaHideApp, bodyOpenClassName } = this.props;
refCount.remove(bodyOpenClassName);
// Remove class if no more modals are open
if (refCount.count(bodyOpenClassName) === 0) {
elementClass(document.body).remove(bodyOpenClassName);
}
// Reset aria-hidden attribute if all modals have been removed
if (ariaHideApp && refCount.totalCount() < 1) {
ariaAppHider.show(appElement);
}
}

afterClose = () => {
focusManager.returnFocus();
focusManager.teardownScopedFocus();
}

open = () => {
this.beforeOpen();
if (this.state.afterOpen && this.state.beforeClose) {
clearTimeout(this.closeTimer);
this.setState({ beforeClose: false });
Expand All @@ -122,6 +164,7 @@ export default class ModalPortal extends Component {
}

close = () => {
this.beforeClose();
if (this.props.closeTimeoutMS > 0) {
this.closeWithTimeout();
} else {
Expand Down
5 changes: 0 additions & 5 deletions src/helpers/ariaAppHider.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@ export function show(appElement) {
(appElement || globalElement).removeAttribute('aria-hidden');
}

export function toggle(shouldHide, appElement) {
const apply = shouldHide ? hide : show;
apply(appElement);
}

export function documentNotReadyOrSSRTesting() {
globalElement = null;
}
Expand Down
26 changes: 15 additions & 11 deletions src/helpers/refCount.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@
const modals = [];
const modals = {};

export function add(element) {
if (modals.indexOf(element) === -1) {
modals.push(element);
export function add(bodyClass) {
// Set variable and default if none
if (!modals[bodyClass]) {
modals[bodyClass] = 0;
}
modals[bodyClass] += 1;
}

export function remove(element) {
const index = modals.indexOf(element);
if (index === -1) {
return;
export function remove(bodyClass) {
if (modals[bodyClass]) {
modals[bodyClass] -= 1;
}
modals.splice(index, 1);
}

export function count() {
return modals.length;
export function count(bodyClass) {
return modals[bodyClass];
}

export function totalCount() {
return Object.keys(modals).reduce((acc, curr) => acc + modals[curr], 0);
}
7 changes: 7 additions & 0 deletions src/helpers/safeHTMLElement.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import ExecutionEnvironment from 'exenv';

const EE = ExecutionEnvironment;

const SafeHTMLElement = EE.canUseDOM ? window.HTMLElement : {};

export default SafeHTMLElement;