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

[ModalManager] Fix aria-hidden of modal current node #13082

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/src/pages/demos/cards/ImgMediaCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
};
Expand Down
4 changes: 2 additions & 2 deletions docs/src/pages/page-layout-examples/sign-in/SignIn.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)]: {
Expand All @@ -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: {
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Avatar/Avatar.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ describe('<ClickAwayListener />', () => {
});
});

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(
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Dialog/Dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/FilledInput/FilledInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/IconButton/IconButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/Input/Input.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions packages/material-ui/src/InputBase/InputBase.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/InputBase/InputBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('<InputBase />', () => {
assert.strictEqual(handleBlur.callCount, 1);
});

// IE11 bug
// IE 11 bug
it('should not respond the focus event when disabled', () => {
const wrapper = shallow(<InputBase disabled />);
const instance = wrapper.instance();
Expand Down
88 changes: 50 additions & 38 deletions packages/material-ui/src/Modal/Modal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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();
};

Expand All @@ -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;
}

Expand All @@ -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,
Expand All @@ -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() {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -414,6 +426,7 @@ Modal.propTypes = {
};

Modal.defaultProps = {
BackdropComponent: Backdrop,
disableAutoFocus: false,
disableBackdropClick: false,
disableEnforceFocus: false,
Expand All @@ -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);
16 changes: 14 additions & 2 deletions packages/material-ui/src/Modal/Modal.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,9 @@ describe('<Modal />', () => {
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);
Expand All @@ -286,7 +288,7 @@ describe('<Modal />', () => {
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);
});
Expand Down Expand Up @@ -348,6 +350,16 @@ describe('<Modal />', () => {
);
assert.strictEqual(wrapper.contains(children), false);
});

it('should mount', () => {
mount(
<Modal keepMounted open={false}>
<div />
</Modal>,
);
const modalNode = document.querySelector('[data-mui-test="Modal"]');
assert.strictEqual(modalNode.getAttribute('aria-hidden'), 'true');
});
});

describe('prop: onExited', () => {
Expand Down
Loading