Modal does not have a focus trap #600 #734
Conversation
|
Hi @rlucke |
|
Use Run test server using develop.opencast.org as backend: Specify a different backend like stable.opencast.org: It may take a few seconds for the interface to spin up. |
|
This pull request is deployed at test.admin-interface.opencast.org/734/2024-06-25_09-02-25/ . |
|
This pull request has conflicts ☹ |
There was a problem hiding this comment.
From my testing this seems to generally work, but there still are some problems. Below is an assortment of observations. Testing done with Firefox 121.
I can escape the trap if I press tab before the modal has finished loading, but that should not constitute a common scenario.
I can escape the "Start Task" modal if i mash tab for long enough. Same with "Recording Details" modal. There might be more, I did not go through all of them.
If anything this PR goes to show that we really want our own Modal wrapper component thingy :D
Out of interest, why did you choose to implement the focus trap yourself instead of using a library like focus-trap-react?
| @@ -0,0 +1,37 @@ | |||
|
|
|||
| export const focusTrap = (modalRef: any, focusEneabled: boolean, setFocusEneabled: Function) => { | |||
There was a problem hiding this comment.
Eneabled is incorrect spelling. Should be Enabled.
| @@ -0,0 +1,37 @@ | |||
|
|
|||
| export const focusTrap = (modalRef: any, focusEneabled: boolean, setFocusEneabled: Function) => { | |||
There was a problem hiding this comment.
Would be great if we could avoid any for the modalRef. At least something like React.MutableRefObject<unknown> should work, although I'm actually not sure if that is any better.
| @@ -12,6 +13,14 @@ const GroupDetailsModal = ({ | |||
| groupName | |||
| }: any) => { | |||
| const { t } = useTranslation(); | |||
| const closeButtonRef = React.useRef(null); | |||
There was a problem hiding this comment.
This ref looks like it is going unused. Did you mean to pass it to focusTrap([...])?
|
|
||
| export const focusTrap = (modalRef: any, focusEneabled: boolean, setFocusEneabled: Function) => { | ||
| const modalElement = modalRef.current as HTMLElement | null; | ||
| const closeButton = document.getElementsByClassName('close-modal')[0] as HTMLElement; |
There was a problem hiding this comment.
Intuitively I'd say that this should also be a ref that is passed to this function, but maybe that is not reasonable?
| const focusableElements = modalElement.querySelectorAll( | ||
| 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' | ||
| ); | ||
| const firstElement = focusableElements[0] as HTMLElement; |
There was a problem hiding this comment.
Is querySelectorAll guaranteed to return a non-empty list?
add focus trap to modals
fixes #600