feat: modal dialog accessibility updates #4025
Conversation
|
I think this PR needs to be updated to keep the focus trapped inside the modal dialog while it's open. |
gkatsev
changed the title
| * | ||
| * @private | ||
| */ | ||
| focusableEls_() { |
There was a problem hiding this comment.
Would this be something we could use elsewhere? Should it be in Component?
There was a problem hiding this comment.
Should this be called focusableChildELs_?
There was a problem hiding this comment.
Is there a link that should be included in the comments here to explain how this works?
There was a problem hiding this comment.
Because using instanceof isn't great, I think it should stay here until and if we start needing it elsewhere.
focusableChildEls_ is probably more accurate.
src/js/component.js
Outdated
| */ | ||
| focus() { | ||
| this.el_.focus(); | ||
| } |
There was a problem hiding this comment.
I think we already have these in master? Does this PR need a rebase for some reason? https://github.com/videojs/video.js/blob/master/src/js/component.js#L1004
There was a problem hiding this comment.
Yup, it needs to be rebased. I had two separate PRs that both needed this.
src/js/modal-dialog.js
Outdated
| this.previouslyActiveEl_.focus(); | ||
| this.previouslyActiveEl_ = null; | ||
|
|
||
| this.off(document, 'keydown', this.handleKeyDown); |
There was a problem hiding this comment.
Should we always turn off the listener for keydown regardless of wether there is a previouslyActiveEl_?
There was a problem hiding this comment.
Potentially, though, I think we only need to off it only if one exists because that's the only time we on it.
There was a problem hiding this comment.
unless the previouslyActiveEl_ gets disposed/removed on the player for some reason between when this gets focused and blured
| */ | ||
| conditionalFocus_() { | ||
| const activeEl = document.activeElement; | ||
| const playerEl = this.player_.el_; |
There was a problem hiding this comment.
I am wondering if we even need a const for this here, since it is only used in the if statement below. Doesn't matter either way though.
There was a problem hiding this comment.
I thought it was a bit cleaner than using the expanded form twice.
There was a problem hiding this comment.
I pulled all the content from the Captions Setting Dialog to test this. Pressing the Tab key works properly (focus wraps back to the first element from the last one), but pressing Shift-Tab doesn't - focus goes from the first element to the last one (the close button), but the next Shift-Tab moves focus back to the first element again, rather than to the second-to-last element.
|
Interesting. I thought I had it working right. I'll take a look at it. |
If the modal dialog was opened and the focus was preset inside the player, move the focus to the modal dialog. When the modal dialog is closed, move the focus back to the previously active element.
gkatsev
dismissed
OwenEdwards’s stale review
Fixed it. Was pretty simple, actually. Had to make sure not to do the forward tab action when shift key was pressed. Unit tests would theoretically have caught this earlier. I'll make sure to write some before this is merged.
If the modal dialog was opened and the focus was preset inside the
player, move the focus to the modal dialog.
When the modal dialog is closed, move the focus back to the previously
active element.
When focus is inside the dialog, trap tab focus. This was inspired by https://github.com/gdkraus/accessible-modal-dialog and ally.js
Still needs: