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

[TrapFocus] Fix error restoring focus when activeElement is null #15967

Merged
merged 2 commits into from
May 31, 2019

Conversation

ryancogswell
Copy link
Contributor

@ryancogswell ryancogswell commented May 30, 2019

document.activeElement is null in IE 11 after unmounting the element that had focus.

You can see this behavior by putting the following into an html page:

<input id="testInput" autofocus>
<script>
function afterTimeout() {
        // input element has focus
	console.log("afterTimeout", document.activeElement);
	var testInput = document.getElementById("testInput");
	testInput.parentNode.removeChild(testInput);
        // null in IE 11, body element in Chrome
	console.log("after removeChild", document.activeElement);
}
function handleLoaded() {
        // body element has focus
	console.log("handleLoaded", document.activeElement);
	setTimeout(afterTimeout);
}
window.addEventListener("load", handleLoaded);
</script>

The use case where we saw this involved clicking a button in one dialog which closed that dialog (unmounting the button that was just clicked and was therefore the focus element) and then opened a new dialog. Closing the new dialog then caused an error since lastFocus.current was null.

`document.activeElement` is null in IE 11 after unmounting the element that had focus.

You can see this behavior by putting the following into an html page:

```
<input id="testInput" autofocus>
<script>
function afterTimeout() {
	console.log("afterTimeout", document.activeElement);
	var testInput = document.getElementById("testInput");
	testInput.parentNode.removeChild(testInput);
        // null in IE 11, body element in Chrome
	console.log("after removeChild", document.activeElement);
}
function handleLoaded() {
	console.log("handleLoaded", document.activeElement);
	setTimeout(afterTimeout);
}
window.addEventListener("load", handleLoaded);
</script>
```

The use case where we saw this was clicking a button in one dialog which closed that dialog (unmounting the button that was just clicked and was therefore the focus element) and then opened a new dialog. Closing the new dialog then caused an error since `lastFocus.current` was null.
@mui-pr-bot
Copy link

mui-pr-bot commented May 30, 2019

Details of bundle changes.

Comparing: b3cace6...3192905

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 315,417 315,428 86,411 86,412
@material-ui/core/Paper 0.00% 0.00% 67,921 67,921 20,184 20,184
@material-ui/core/Paper.esm 0.00% 0.00% 61,217 61,217 18,981 18,981
@material-ui/core/Popper 0.00% 0.00% 28,728 28,728 10,345 10,345
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,376 2,376
@material-ui/core/TrapFocus +0.29% 🔺 +0.13% 🔺 3,744 3,755 1,573 1,575
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,978 15,978 5,787 5,787
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 975 975
@material-ui/lab 0.00% 0.00% 138,854 138,854 42,662 42,662
@material-ui/styles 0.00% 0.00% 51,386 51,386 15,193 15,193
@material-ui/system 0.00% 0.00% 14,463 14,463 4,181 4,181
Button 0.00% 0.00% 83,901 83,901 25,459 25,459
Modal +0.05% 🔺 +0.03% 🔺 20,331 20,342 6,685 6,687
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 55,977 55,977 14,042 14,042
docs.main 0.00% 0.00% 648,929 648,940 204,667 204,667
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 289,551 289,562 82,725 82,727

Generated by 🚫 dangerJS against 3192905

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label May 30, 2019
@oliviertassinari
Copy link
Member

@ryancogswell Well spotted, could you update the description? I wish we can remove this defensive logic when we drop IE 11 support. It will help our future us.

@oliviertassinari oliviertassinari added the component: FocusTrap The React component. label May 30, 2019
@ryancogswell
Copy link
Contributor Author

could you update the description?

@oliviertassinari Sure. What would you like it to be?

@oliviertassinari
Copy link
Member

Maybe we could say that the whole branch logic should be removed with IE 11 support.

@ryancogswell
Copy link
Contributor Author

Well spotted

We're updating to v4 in production this weekend, so one of our team members encountered this in our app during testing. For now, we're working around it by specifying disableRestoreFocus on the second dialog.

@oliviertassinari oliviertassinari merged commit 9a57708 into mui:master May 31, 2019
@ryancogswell ryancogswell deleted the patch-1 branch May 31, 2019 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: FocusTrap The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants