From d80ea48a00cf18c7c2d12528c0948faa00600d96 Mon Sep 17 00:00:00 2001 From: Joonas Kallunki Date: Wed, 3 Oct 2018 07:37:53 +0300 Subject: [PATCH 1/5] Make sure aria hideSiblings and showSiblings don't alter current node. --- packages/material-ui/src/Modal/ModalManager.js | 4 ++-- packages/material-ui/src/Modal/manageAriaHidden.js | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js index 4595618c3d5ff3..20b16d36f23982 100644 --- a/packages/material-ui/src/Modal/ModalManager.js +++ b/packages/material-ui/src/Modal/ModalManager.js @@ -94,7 +94,7 @@ class ModalManager { this.modals.push(modal); if (this.hideSiblingNodes) { - hideSiblings(container, modal.mountNode); + hideSiblings(container, modal.mountNode, modal.modalRef); } const containerIdx = this.containers.indexOf(container); @@ -140,7 +140,7 @@ class ModalManager { } if (this.hideSiblingNodes) { - showSiblings(container, modal.mountNode); + showSiblings(container, modal.mountNode, modal.modalRef); } this.containers.splice(containerIdx, 1); this.data.splice(containerIdx, 1); diff --git a/packages/material-ui/src/Modal/manageAriaHidden.js b/packages/material-ui/src/Modal/manageAriaHidden.js index 326d2295c428b4..57ed3ab3d08c1b 100644 --- a/packages/material-ui/src/Modal/manageAriaHidden.js +++ b/packages/material-ui/src/Modal/manageAriaHidden.js @@ -4,10 +4,10 @@ function isHidable(node) { return node.nodeType === 1 && BLACKLIST.indexOf(node.tagName.toLowerCase()) === -1; } -function siblings(container, mount, callback) { +function siblings(container, mount, callback, currentNode) { mount = [].concat(mount); // eslint-disable-line no-param-reassign [].forEach.call(container.children, node => { - if (mount.indexOf(node) === -1 && isHidable(node)) { + if (mount.indexOf(node) === -1 && node !== currentNode && isHidable(node)) { callback(node); } }); @@ -20,14 +20,14 @@ export function ariaHidden(show, node) { if (show) { node.setAttribute('aria-hidden', 'true'); } else { - node.removeAttribute('aria-hidden'); + node.setAttribute('aria-hidden', 'false'); } } -export function hideSiblings(container, mountNode) { - siblings(container, mountNode, node => ariaHidden(true, node)); +export function hideSiblings(container, mountNode, currentNode) { + siblings(container, mountNode, node => ariaHidden(true, node), currentNode); } -export function showSiblings(container, mountNode) { - siblings(container, mountNode, node => ariaHidden(false, node)); +export function showSiblings(container, mountNode, currentNode) { + siblings(container, mountNode, node => ariaHidden(false, node), currentNode); } From 8e0e6d6994121b9a03e55b08b895c35a8292f786 Mon Sep 17 00:00:00 2001 From: Joonas Kallunki Date: Wed, 3 Oct 2018 07:44:42 +0300 Subject: [PATCH 2/5] Set modal aria-hidden to false instead of remove and make sure removed modal will get aria-hidden true. --- packages/material-ui/src/Modal/ModalManager.js | 2 ++ packages/material-ui/src/Modal/ModalManager.test.js | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js index 20b16d36f23982..87f3088b49016d 100644 --- a/packages/material-ui/src/Modal/ModalManager.js +++ b/packages/material-ui/src/Modal/ModalManager.js @@ -93,6 +93,7 @@ class ModalManager { modalIdx = this.modals.length; this.modals.push(modal); + ariaHidden(false, modal.modalRef); if (this.hideSiblingNodes) { hideSiblings(container, modal.mountNode, modal.modalRef); } @@ -139,6 +140,7 @@ class ModalManager { removeContainerStyle(data, container); } + ariaHidden(true, modal.modalRef); if (this.hideSiblingNodes) { showSiblings(container, modal.mountNode, modal.modalRef); } diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js index 10fec77ad6a5ae..1b17c17b2246fa 100644 --- a/packages/material-ui/src/Modal/ModalManager.test.js +++ b/packages/material-ui/src/Modal/ModalManager.test.js @@ -169,7 +169,7 @@ describe('ModalManager', () => { modalManager.add(modal3, container2); assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'true'); modalManager.remove(modal3, container2); - assert.strictEqual(mountNode2.getAttribute('aria-hidden'), null); + assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'false'); }); it('should remove aria-hidden on siblings', () => { @@ -178,7 +178,7 @@ describe('ModalManager', () => { modalManager.add(modal, container2); assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); modalManager.remove(modal, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), null); + assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'false'); }); }); }); From 9ac50c9ad843927f5a967c846a5cd4b4bc1bdd34 Mon Sep 17 00:00:00 2001 From: Joonas Kallunki Date: Thu, 4 Oct 2018 05:04:07 +0300 Subject: [PATCH 3/5] Restore removing aria-hidden, and not set it false --- packages/material-ui/src/Modal/ModalManager.test.js | 4 ++-- packages/material-ui/src/Modal/manageAriaHidden.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js index 1b17c17b2246fa..10fec77ad6a5ae 100644 --- a/packages/material-ui/src/Modal/ModalManager.test.js +++ b/packages/material-ui/src/Modal/ModalManager.test.js @@ -169,7 +169,7 @@ describe('ModalManager', () => { modalManager.add(modal3, container2); assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'true'); modalManager.remove(modal3, container2); - assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'false'); + assert.strictEqual(mountNode2.getAttribute('aria-hidden'), null); }); it('should remove aria-hidden on siblings', () => { @@ -178,7 +178,7 @@ describe('ModalManager', () => { modalManager.add(modal, container2); assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); modalManager.remove(modal, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'false'); + assert.strictEqual(mountNode1.getAttribute('aria-hidden'), null); }); }); }); diff --git a/packages/material-ui/src/Modal/manageAriaHidden.js b/packages/material-ui/src/Modal/manageAriaHidden.js index 57ed3ab3d08c1b..85fe4b640c9311 100644 --- a/packages/material-ui/src/Modal/manageAriaHidden.js +++ b/packages/material-ui/src/Modal/manageAriaHidden.js @@ -20,7 +20,7 @@ export function ariaHidden(show, node) { if (show) { node.setAttribute('aria-hidden', 'true'); } else { - node.setAttribute('aria-hidden', 'false'); + node.removeAttribute('aria-hidden'); } } From 9ed1a8e043d8410689717e5a364f42c9ac4a40e7 Mon Sep 17 00:00:00 2001 From: Joonas Kallunki Date: Thu, 4 Oct 2018 05:58:08 +0300 Subject: [PATCH 4/5] Test for opened modal to have not aria-hidden set to true --- packages/material-ui/src/Modal/ModalManager.test.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js index 10fec77ad6a5ae..d6f20f24c71ed6 100644 --- a/packages/material-ui/src/Modal/ModalManager.test.js +++ b/packages/material-ui/src/Modal/ModalManager.test.js @@ -139,6 +139,13 @@ describe('ModalManager', () => { document.body.removeChild(container2); }); + it('should not contain aria-hidden on modal', () => { + const modal2 = document.createElement('div'); + + modalManager.add(modal2, container2); + assert.strictEqual(modal2.getAttribute('aria-hidden'), null); + }); + it('should add aria-hidden to container siblings', () => { modalManager.add({}, container2); assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); @@ -146,7 +153,7 @@ describe('ModalManager', () => { it('should add aria-hidden to previous modals', () => { const modal2 = {}; - const modal3 = {}; + const modal3 = document.createElement('div'); const mountNode2 = document.createElement('div'); modal2.mountNode = mountNode2; @@ -155,6 +162,7 @@ describe('ModalManager', () => { modalManager.add(modal3, container2); assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'true'); + assert.strictEqual(modal3.getAttribute('aria-hidden'), null); }); it('should remove aria-hidden on americas next top modal', () => { From 5f1e6f8fb689eaa381ea2ed5d6fed21e4a52bbb3 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Thu, 18 Oct 2018 10:21:54 +0200 Subject: [PATCH 5/5] let's merge --- docs/src/pages/demos/cards/ImgMediaCard.js | 2 +- .../page-layout-examples/sign-in/SignIn.js | 4 +- packages/material-ui/src/Avatar/Avatar.js | 2 +- .../ClickAwayListener/ClickAwayListener.js | 2 +- .../ClickAwayListener.test.js | 2 +- packages/material-ui/src/Dialog/Dialog.js | 2 +- .../src/FilledInput/FilledInput.js | 4 +- .../material-ui/src/IconButton/IconButton.js | 2 +- packages/material-ui/src/Input/Input.js | 4 +- .../material-ui/src/InputBase/InputBase.js | 4 +- .../src/InputBase/InputBase.test.js | 2 +- packages/material-ui/src/Modal/Modal.js | 88 +++++++++++-------- packages/material-ui/src/Modal/Modal.test.js | 16 +++- .../material-ui/src/Modal/ModalManager.js | 18 ++-- .../src/Modal/ModalManager.test.js | 57 +++++------- .../material-ui/src/Modal/manageAriaHidden.js | 20 ++--- .../src/NativeSelect/NativeSelect.js | 2 +- .../material-ui/src/utils/getDisplayName.js | 4 +- .../test/integration/Select.test.js | 2 +- 19 files changed, 126 insertions(+), 111 deletions(-) diff --git a/docs/src/pages/demos/cards/ImgMediaCard.js b/docs/src/pages/demos/cards/ImgMediaCard.js index 03f1eec122c722..78f67d9cedfb4d 100644 --- a/docs/src/pages/demos/cards/ImgMediaCard.js +++ b/docs/src/pages/demos/cards/ImgMediaCard.js @@ -14,7 +14,7 @@ const styles = { maxWidth: 345, }, media: { - // ⚠️ object-fit is not supported by IE11. + // ⚠️ object-fit is not supported by IE 11. objectFit: 'cover', }, }; diff --git a/docs/src/pages/page-layout-examples/sign-in/SignIn.js b/docs/src/pages/page-layout-examples/sign-in/SignIn.js index 5dc39a5c54cc57..1f5d468a4c0e10 100644 --- a/docs/src/pages/page-layout-examples/sign-in/SignIn.js +++ b/docs/src/pages/page-layout-examples/sign-in/SignIn.js @@ -16,7 +16,7 @@ import withStyles from '@material-ui/core/styles/withStyles'; const styles = theme => ({ layout: { width: 'auto', - display: 'block', // Fix IE11 issue. + display: 'block', // Fix IE 11 issue. marginLeft: theme.spacing.unit * 3, marginRight: theme.spacing.unit * 3, [theme.breakpoints.up(400 + theme.spacing.unit * 3 * 2)]: { @@ -37,7 +37,7 @@ const styles = theme => ({ backgroundColor: theme.palette.secondary.main, }, form: { - width: '100%', // Fix IE11 issue. + width: '100%', // Fix IE 11 issue. marginTop: theme.spacing.unit, }, submit: { diff --git a/packages/material-ui/src/Avatar/Avatar.js b/packages/material-ui/src/Avatar/Avatar.js index 3ea1771572e7fd..8ef2121684a2ad 100644 --- a/packages/material-ui/src/Avatar/Avatar.js +++ b/packages/material-ui/src/Avatar/Avatar.js @@ -31,7 +31,7 @@ export const styles = theme => ({ width: '100%', height: '100%', textAlign: 'center', - // Handle non-square image. The property isn't supported by IE11. + // Handle non-square image. The property isn't supported by IE 11. objectFit: 'cover', }, }); diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js index 26eadf739f6fd1..89e786432c0570 100644 --- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js +++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js @@ -30,7 +30,7 @@ class ClickAwayListener extends React.Component { return; } - // IE11 support, which trigger the handleClickAway even after the unbind + // IE 11 support, which trigger the handleClickAway even after the unbind if (!this.mounted) { return; } diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js index ba90fa8b52a809..fafabe7d83dff0 100644 --- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js +++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.test.js @@ -155,7 +155,7 @@ describe('', () => { }); }); - describe('IE11 issue', () => { + describe('IE 11 issue', () => { it('should not call the hook if the event is triggered after being unmounted', () => { const handleClickAway = spy(); wrapper = mount( diff --git a/packages/material-ui/src/Dialog/Dialog.js b/packages/material-ui/src/Dialog/Dialog.js index 7575b49dc5a2b2..f9470e2958c1e0 100644 --- a/packages/material-ui/src/Dialog/Dialog.js +++ b/packages/material-ui/src/Dialog/Dialog.js @@ -30,7 +30,7 @@ export const styles = theme => ({ flexDirection: 'column', margin: 48, position: 'relative', - overflowY: 'auto', // Fix IE11 issue, to remove at some point. + overflowY: 'auto', // Fix IE 11 issue, to remove at some point. // We disable the focus ring for mouse, touch and keyboard users. outline: 'none', }, diff --git a/packages/material-ui/src/FilledInput/FilledInput.js b/packages/material-ui/src/FilledInput/FilledInput.js index 8a953d0e534e3f..481b041780f41a 100644 --- a/packages/material-ui/src/FilledInput/FilledInput.js +++ b/packages/material-ui/src/FilledInput/FilledInput.js @@ -37,7 +37,7 @@ export const styles = theme => { borderBottom: `2px solid ${theme.palette.primary[light ? 'dark' : 'light']}`, left: 0, bottom: 0, - // Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242 + // Doing the other way around crash on IE 11 "''" https://github.com/cssinjs/jss/issues/242 content: '""', position: 'absolute', right: 0, @@ -59,7 +59,7 @@ export const styles = theme => { borderBottom: `1px solid ${bottomLineColor}`, left: 0, bottom: 0, - // Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242 + // Doing the other way around crash on IE 11 "''" https://github.com/cssinjs/jss/issues/242 content: '"\\00a0"', position: 'absolute', right: 0, diff --git a/packages/material-ui/src/IconButton/IconButton.js b/packages/material-ui/src/IconButton/IconButton.js index e01f9160ad0621..af6d8d7f2767db 100644 --- a/packages/material-ui/src/IconButton/IconButton.js +++ b/packages/material-ui/src/IconButton/IconButton.js @@ -16,7 +16,7 @@ export const styles = theme => ({ fontSize: theme.typography.pxToRem(24), padding: 12, borderRadius: '50%', - overflow: 'visible', // Explicitly set the default value to solve a bug on IE11. + overflow: 'visible', // Explicitly set the default value to solve a bug on IE 11. color: theme.palette.action.active, transition: theme.transitions.create('background-color', { duration: theme.transitions.duration.shortest, diff --git a/packages/material-ui/src/Input/Input.js b/packages/material-ui/src/Input/Input.js index fd2683a7249891..0751b3d0c11da5 100644 --- a/packages/material-ui/src/Input/Input.js +++ b/packages/material-ui/src/Input/Input.js @@ -31,7 +31,7 @@ export const styles = theme => { borderBottom: `2px solid ${theme.palette.primary[light ? 'dark' : 'light']}`, left: 0, bottom: 0, - // Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242 + // Doing the other way around crash on IE 11 "''" https://github.com/cssinjs/jss/issues/242 content: '""', position: 'absolute', right: 0, @@ -53,7 +53,7 @@ export const styles = theme => { borderBottom: `1px solid ${bottomLineColor}`, left: 0, bottom: 0, - // Doing the other way around crash on IE11 "''" https://github.com/cssinjs/jss/issues/242 + // Doing the other way around crash on IE 11 "''" https://github.com/cssinjs/jss/issues/242 content: '"\\00a0"', position: 'absolute', right: 0, diff --git a/packages/material-ui/src/InputBase/InputBase.js b/packages/material-ui/src/InputBase/InputBase.js index 36bc9d738730a2..1576da0f511391 100644 --- a/packages/material-ui/src/InputBase/InputBase.js +++ b/packages/material-ui/src/InputBase/InputBase.js @@ -76,7 +76,7 @@ export const styles = theme => { display: 'block', // Make the flex item shrink with Firefox minWidth: 0, - width: '100%', // Fix IE11 width issue + width: '100%', // Fix IE 11 width issue '&::-webkit-input-placeholder': placeholder, '&::-moz-placeholder': placeholder, // Firefox 19+ '&:-ms-input-placeholder': placeholder, // IE 11 @@ -222,7 +222,7 @@ class InputBase extends React.Component { } handleFocus = event => { - // Fix a bug with IE11 where the focus/blur events are triggered + // Fix a bug with IE 11 where the focus/blur events are triggered // while the input is disabled. if ( formControlState({ props: this.props, context: this.context, states: ['disabled'] }).disabled diff --git a/packages/material-ui/src/InputBase/InputBase.test.js b/packages/material-ui/src/InputBase/InputBase.test.js index 9fb9c6e5a07384..dd35575b615b89 100644 --- a/packages/material-ui/src/InputBase/InputBase.test.js +++ b/packages/material-ui/src/InputBase/InputBase.test.js @@ -84,7 +84,7 @@ describe('', () => { assert.strictEqual(handleBlur.callCount, 1); }); - // IE11 bug + // IE 11 bug it('should not respond the focus event when disabled', () => { const wrapper = shallow(); const instance = wrapper.instance(); diff --git a/packages/material-ui/src/Modal/Modal.js b/packages/material-ui/src/Modal/Modal.js index 5314c1bdea2e81..d4fd5de966250d 100644 --- a/packages/material-ui/src/Modal/Modal.js +++ b/packages/material-ui/src/Modal/Modal.js @@ -64,7 +64,8 @@ class Modal extends React.Component { componentDidUpdate(prevProps) { if (!prevProps.open && this.props.open) { - this.checkForFocus(); + // check for focus + this.lastFocus = ownerDocument(this.mountNode).activeElement; } if (prevProps.open && !this.props.open && !getHasTransition(this.props)) { @@ -100,31 +101,50 @@ class Modal extends React.Component { return null; } - handleRendered = () => { - this.autoFocus(); + handleOpen = () => { + const doc = ownerDocument(this.mountNode); + const container = getContainer(this.props.container, doc.body); - // Fix a bug on Chrome where the scroll isn't initially 0. - this.modalRef.scrollTop = 0; + this.props.manager.add(this, container); + doc.addEventListener('keydown', this.handleDocumentKeyDown); + doc.addEventListener('focus', this.enforceFocus, true); + if (this.dialogRef) { + this.handleOpened(); + } + }; + + handleRendered = () => { if (this.props.onRendered) { this.props.onRendered(); } + + if (this.props.open) { + this.handleOpened(); + } else { + const doc = ownerDocument(this.mountNode); + const container = getContainer(this.props.container, doc.body); + this.props.manager.add(this, container); + this.props.manager.remove(this); + } }; - handleOpen = () => { - const doc = ownerDocument(this.mountNode); - const container = getContainer(this.props.container, doc.body); + handleOpened = () => { + this.autoFocus(); - this.props.manager.add(this, container); - doc.addEventListener('keydown', this.handleDocumentKeyDown); - doc.addEventListener('focus', this.enforceFocus, true); + // Fix a bug on Chrome where the scroll isn't initially 0. + this.modalRef.scrollTop = 0; }; + handleDisplay = () => {}; + handleClose = () => { this.props.manager.remove(this); + const doc = ownerDocument(this.mountNode); doc.removeEventListener('keydown', this.handleDocumentKeyDown); doc.removeEventListener('focus', this.enforceFocus, true); + this.restoreLastFocus(); }; @@ -148,12 +168,8 @@ class Modal extends React.Component { }; handleDocumentKeyDown = event => { - if (!this.isTopModal() || keycode(event) !== 'esc') { - return; - } - // Ignore events that have been `event.preventDefault()` marked. - if (event.defaultPrevented) { + if (keycode(event) !== 'esc' || !this.isTopModal() || event.defaultPrevented) { return; } @@ -166,32 +182,28 @@ class Modal extends React.Component { } }; - checkForFocus = () => { - this.lastFocus = ownerDocument(this.mountNode).activeElement; - }; - enforceFocus = () => { - if (this.props.disableEnforceFocus || !this.mounted || !this.isTopModal()) { + // The Modal might not already be mounted. + if (!this.isTopModal() || this.props.disableEnforceFocus || !this.mounted || !this.dialogRef) { return; } const currentActiveElement = ownerDocument(this.mountNode).activeElement; - if (this.dialogRef && !this.dialogRef.contains(currentActiveElement)) { + if (!this.dialogRef.contains(currentActiveElement)) { this.dialogRef.focus(); } }; autoFocus() { - if (this.props.disableAutoFocus) { + // We might render an empty child. + if (this.props.disableAutoFocus || !this.dialogRef) { return; } const currentActiveElement = ownerDocument(this.mountNode).activeElement; - if (this.dialogRef && !this.dialogRef.contains(currentActiveElement)) { - this.lastFocus = currentActiveElement; - + if (!this.dialogRef.contains(currentActiveElement)) { if (!this.dialogRef.hasAttribute('tabIndex')) { warning( false, @@ -204,25 +216,24 @@ class Modal extends React.Component { this.dialogRef.setAttribute('tabIndex', -1); } + this.lastFocus = currentActiveElement; this.dialogRef.focus(); } } restoreLastFocus() { - if (this.props.disableRestoreFocus) { + if (this.props.disableRestoreFocus || !this.lastFocus) { return; } - if (this.lastFocus) { - // Not all elements in IE11 have a focus method. - // Because IE11 market share is low, we accept the restore focus being broken - // and we silent the issue. - if (this.lastFocus.focus) { - this.lastFocus.focus(); - } - - this.lastFocus = null; + // Not all elements in IE 11 have a focus method. + // Because IE 11 market share is low, we accept the restore focus being broken + // and we silent the issue. + if (this.lastFocus.focus) { + this.lastFocus.focus(); } + + this.lastFocus = null; } isTopModal() { @@ -255,12 +266,13 @@ class Modal extends React.Component { } = this.props; const { exited } = this.state; const hasTransition = getHasTransition(this.props); - const childProps = {}; if (!keepMounted && !open && (!hasTransition || exited)) { return null; } + const childProps = {}; + // It's a Transition like component if (hasTransition) { childProps.onExited = createChainedFunction(this.handleExited, children.props.onExited); @@ -414,6 +426,7 @@ Modal.propTypes = { }; Modal.defaultProps = { + BackdropComponent: Backdrop, disableAutoFocus: false, disableBackdropClick: false, disableEnforceFocus: false, @@ -424,7 +437,6 @@ Modal.defaultProps = { keepMounted: false, // Modals don't open on the server so this won't conflict with concurrent requests. manager: new ModalManager(), - BackdropComponent: Backdrop, }; export default withStyles(styles, { flip: false, name: 'MuiModal' })(Modal); diff --git a/packages/material-ui/src/Modal/Modal.test.js b/packages/material-ui/src/Modal/Modal.test.js index f2fb739565a86e..9fd64ef54ecb60 100644 --- a/packages/material-ui/src/Modal/Modal.test.js +++ b/packages/material-ui/src/Modal/Modal.test.js @@ -274,7 +274,9 @@ describe('', () => { topModalStub.returns(false); wrapper.setProps({ manager: { isTopModal: topModalStub } }); - instance.handleDocumentKeyDown(undefined); + instance.handleDocumentKeyDown({ + keyCode: keycode('esc'), + }); assert.strictEqual(topModalStub.callCount, 1); assert.strictEqual(onEscapeKeyDownSpy.callCount, 0); assert.strictEqual(onCloseSpy.callCount, 0); @@ -286,7 +288,7 @@ describe('', () => { event = { keyCode: keycode('j') }; // Not 'esc' instance.handleDocumentKeyDown(event); - assert.strictEqual(topModalStub.callCount, 1); + assert.strictEqual(topModalStub.callCount, 0); assert.strictEqual(onEscapeKeyDownSpy.callCount, 0); assert.strictEqual(onCloseSpy.callCount, 0); }); @@ -348,6 +350,16 @@ describe('', () => { ); assert.strictEqual(wrapper.contains(children), false); }); + + it('should mount', () => { + mount( + +
+ , + ); + const modalNode = document.querySelector('[data-mui-test="Modal"]'); + assert.strictEqual(modalNode.getAttribute('aria-hidden'), 'true'); + }); }); describe('prop: onExited', () => { diff --git a/packages/material-ui/src/Modal/ModalManager.js b/packages/material-ui/src/Modal/ModalManager.js index 87f3088b49016d..3b7a06326864aa 100644 --- a/packages/material-ui/src/Modal/ModalManager.js +++ b/packages/material-ui/src/Modal/ModalManager.js @@ -2,7 +2,7 @@ import css from 'dom-helpers/style'; import getScrollbarSize from 'dom-helpers/util/scrollbarSize'; import ownerDocument from '../utils/ownerDocument'; import isOverflowing from './isOverflowing'; -import { ariaHidden, hideSiblings, showSiblings } from './manageAriaHidden'; +import { ariaHidden, ariaHiddenSiblings } from './manageAriaHidden'; function findIndexOf(data, callback) { let idx = -1; @@ -93,9 +93,12 @@ class ModalManager { modalIdx = this.modals.length; this.modals.push(modal); - ariaHidden(false, modal.modalRef); + // If the modal we are adding is already in the DOM. + if (modal.modalRef) { + ariaHidden(modal.modalRef, false); + } if (this.hideSiblingNodes) { - hideSiblings(container, modal.mountNode, modal.modalRef); + ariaHiddenSiblings(container, modal.mountNode, modal.modalRef, true); } const containerIdx = this.containers.indexOf(container); @@ -140,15 +143,18 @@ class ModalManager { removeContainerStyle(data, container); } - ariaHidden(true, modal.modalRef); + // In case the modal wasn't in the DOM yet. + if (modal.modalRef) { + ariaHidden(modal.modalRef, true); + } if (this.hideSiblingNodes) { - showSiblings(container, modal.mountNode, modal.modalRef); + ariaHiddenSiblings(container, modal.mountNode, modal.modalRef, false); } this.containers.splice(containerIdx, 1); this.data.splice(containerIdx, 1); } else if (this.hideSiblingNodes) { // Otherwise make sure the next top modal is visible to a screan reader. - ariaHidden(false, data.modals[data.modals.length - 1].mountNode); + ariaHidden(data.modals[data.modals.length - 1].modalRef, false); } return modalIdx; diff --git a/packages/material-ui/src/Modal/ModalManager.test.js b/packages/material-ui/src/Modal/ModalManager.test.js index d6f20f24c71ed6..0b892f81650a27 100644 --- a/packages/material-ui/src/Modal/ModalManager.test.js +++ b/packages/material-ui/src/Modal/ModalManager.test.js @@ -30,9 +30,9 @@ describe('ModalManager', () => { let modal3; before(() => { - modal1 = {}; - modal2 = {}; - modal3 = {}; + modal1 = { modalRef: document.createElement('div') }; + modal2 = { modalRef: document.createElement('div') }; + modal3 = { modalRef: document.createElement('div') }; }); it('should add modal1', () => { @@ -122,15 +122,15 @@ describe('ModalManager', () => { }); describe('container aria-hidden', () => { - let mountNode1; + let modalRef1; let container2; beforeEach(() => { container2 = document.createElement('div'); document.body.appendChild(container2); - mountNode1 = document.createElement('div'); - container2.appendChild(mountNode1); + modalRef1 = document.createElement('div'); + container2.appendChild(modalRef1); modalManager = new ModalManager(); }); @@ -141,52 +141,43 @@ describe('ModalManager', () => { it('should not contain aria-hidden on modal', () => { const modal2 = document.createElement('div'); + modal2.setAttribute('aria-hidden', 'true'); - modalManager.add(modal2, container2); + assert.strictEqual(modal2.getAttribute('aria-hidden'), 'true'); + modalManager.add({ modalRef: modal2 }, container2); assert.strictEqual(modal2.getAttribute('aria-hidden'), null); }); it('should add aria-hidden to container siblings', () => { modalManager.add({}, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); + assert.strictEqual(container2.children[0].getAttribute('aria-hidden'), 'true'); }); it('should add aria-hidden to previous modals', () => { - const modal2 = {}; + const modal2 = document.createElement('div'); const modal3 = document.createElement('div'); - const mountNode2 = document.createElement('div'); - modal2.mountNode = mountNode2; - container2.appendChild(mountNode2); - modalManager.add(modal2, container2); - modalManager.add(modal3, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); - assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'true'); - assert.strictEqual(modal3.getAttribute('aria-hidden'), null); - }); + container2.appendChild(modal2); + container2.appendChild(modal3); - it('should remove aria-hidden on americas next top modal', () => { - const modal2 = {}; - const modal3 = {}; - const mountNode2 = document.createElement('div'); + modalManager.add({ modalRef: modal2 }, container2); + // Simulate the main React DOM true. + assert.strictEqual(container2.children[0].getAttribute('aria-hidden'), 'true'); + assert.strictEqual(container2.children[1].getAttribute('aria-hidden'), null); - modal2.mountNode = mountNode2; - container2.appendChild(mountNode2); - modalManager.add({}, container1); - modalManager.add(modal2, container2); - modalManager.add(modal3, container2); - assert.strictEqual(mountNode2.getAttribute('aria-hidden'), 'true'); - modalManager.remove(modal3, container2); - assert.strictEqual(mountNode2.getAttribute('aria-hidden'), null); + modalManager.add({ modalRef: modal3 }, container2); + assert.strictEqual(container2.children[0].getAttribute('aria-hidden'), 'true'); + assert.strictEqual(container2.children[1].getAttribute('aria-hidden'), 'true'); + assert.strictEqual(container2.children[2].getAttribute('aria-hidden'), null); }); it('should remove aria-hidden on siblings', () => { - const modal = {}; + const modal = { modalRef: container2.children[0] }; modalManager.add(modal, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), 'true'); + assert.strictEqual(container2.children[0].getAttribute('aria-hidden'), null); modalManager.remove(modal, container2); - assert.strictEqual(mountNode1.getAttribute('aria-hidden'), null); + assert.strictEqual(container2.children[0].getAttribute('aria-hidden'), 'true'); }); }); }); diff --git a/packages/material-ui/src/Modal/manageAriaHidden.js b/packages/material-ui/src/Modal/manageAriaHidden.js index 85fe4b640c9311..2ff96a2097c276 100644 --- a/packages/material-ui/src/Modal/manageAriaHidden.js +++ b/packages/material-ui/src/Modal/manageAriaHidden.js @@ -4,19 +4,17 @@ function isHidable(node) { return node.nodeType === 1 && BLACKLIST.indexOf(node.tagName.toLowerCase()) === -1; } -function siblings(container, mount, callback, currentNode) { - mount = [].concat(mount); // eslint-disable-line no-param-reassign +function siblings(container, mount, currentNode, callback) { + const blacklist = [mount, currentNode]; // eslint-disable-line no-param-reassign + [].forEach.call(container.children, node => { - if (mount.indexOf(node) === -1 && node !== currentNode && isHidable(node)) { + if (blacklist.indexOf(node) === -1 && isHidable(node)) { callback(node); } }); } -export function ariaHidden(show, node) { - if (!node) { - return; - } +export function ariaHidden(node, show) { if (show) { node.setAttribute('aria-hidden', 'true'); } else { @@ -24,10 +22,6 @@ export function ariaHidden(show, node) { } } -export function hideSiblings(container, mountNode, currentNode) { - siblings(container, mountNode, node => ariaHidden(true, node), currentNode); -} - -export function showSiblings(container, mountNode, currentNode) { - siblings(container, mountNode, node => ariaHidden(false, node), currentNode); +export function ariaHiddenSiblings(container, mountNode, currentNode, show) { + siblings(container, mountNode, currentNode, node => ariaHidden(node, show)); } diff --git a/packages/material-ui/src/NativeSelect/NativeSelect.js b/packages/material-ui/src/NativeSelect/NativeSelect.js index 29dcfccac1ee09..b3450547dbc29a 100644 --- a/packages/material-ui/src/NativeSelect/NativeSelect.js +++ b/packages/material-ui/src/NativeSelect/NativeSelect.js @@ -37,7 +37,7 @@ export const styles = theme => ({ color: 'transparent', textShadow: '0 0 0 #000', }, - // Remove IE11 arrow + // Remove IE 11 arrow '&::-ms-expand': { display: 'none', }, diff --git a/packages/material-ui/src/utils/getDisplayName.js b/packages/material-ui/src/utils/getDisplayName.js index c48ca40582a20a..08cbb522beccde 100755 --- a/packages/material-ui/src/utils/getDisplayName.js +++ b/packages/material-ui/src/utils/getDisplayName.js @@ -1,6 +1,6 @@ -// Fork of recompose/getDisplayName with added IE11 support +// Fork of recompose/getDisplayName with added IE 11 support -// Simplified polyfill for IE11 support +// Simplified polyfill for IE 11 support // https://github.com/JamesMGreene/Function.name/blob/58b314d4a983110c3682f1228f845d39ccca1817/Function.name.js#L3 const fnNameMatchRegex = /^\s*function(?:\s|\s*\/\*.*\*\/\s*)+([^(\s/]*)\s*/; export function getFunctionName(fn) { diff --git a/packages/material-ui/test/integration/Select.test.js b/packages/material-ui/test/integration/Select.test.js index f34074f240ed4e..cc1902cdd6638b 100644 --- a/packages/material-ui/test/integration/Select.test.js +++ b/packages/material-ui/test/integration/Select.test.js @@ -17,7 +17,7 @@ describe('