fix(Modal): add keyboard trap #1115
Conversation
There was a problem hiding this comment.
Hi @Kyle-Cooley thank you for jumping in! One question - Does this fix prevents the “focus-wrap” behavior while the modal is not open?
Adds a keyboard trap to fix issue #874
edd80f1 to
bd92e8f
Compare
|
Hello @asudoh. That was a good catch. I have amended my commit to check if the modal is open when trapping focus. |
|
Hi @Kyle-Cooley thank you for your quick change! Just a couple of additional things:
FYI here's the corresponding code in vanilla: https://github.com/IBM/carbon-components/blob/3e9bc26/src/components/modal/modal.js#L140-L153 |
Fixes the modal keyboard trap to be consistent with vanilla behavior.
|
I addressed the first, but I'm struggling with the second. I'm using an overflow menu (floating) in a modal. The items in the menu can be tabbed to, but when the overflow menu closes, focus is not returned to the modal. |
Makes it possible to escape keyboard trap in modals when using floating menus
This sends focus back to the overflow menu item after the menu gets closed. This is important for floating overflow menus in modals.
|
I think I've managed to address the issue, let me know if there is something else. |
There was a problem hiding this comment.
Thanks alot @Kyle-Cooley for your quick change! Looks that we are getting there - Just a couple of comments this time.
| this.setState({ open: false }, () => { | ||
| if (wasOpen) { | ||
| this.focusMenuEl(); |
There was a problem hiding this comment.
src/components/Modal/Modal.js
Outdated
| @@ -36,10 +43,21 @@ export default class Modal extends Component { | |||
| iconDescription: 'close the modal', | |||
| modalHeading: '', | |||
| modalLabel: '', | |||
| selectorsFloatingMenus: ['.bx--overflow-menu-options__btn'], | |||
There was a problem hiding this comment.
Do you think it's possible to align the list with vanilla? https://github.com/IBM/carbon-components/blob/3e9bc26/src/components/modal/modal.js#L194
Made algorithm to check for floating menus check parents as well and made the list of floating menu selectors consistent with vanilla.
6b2edbf to
18f1b62
Compare
There was a problem hiding this comment.
LGTM now 👍 - Thank you very much @Kyle-Cooley for your contribution!
|
Hi @Kyle-Cooley! Looks great, mind fixing the merge conflicts and then we can merge? |
|
Should be good now. |
|
🎉 This PR is included in version 6.19.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds a keyboard trap to fix issue #874