Fix: Keyboard navigation should only focus meaningful elements #970
Conversation
|
Verified that @jstoffan has signed the CLA. Thanks for the pull request! |
| @@ -33,8 +33,8 @@ | |||
| </div> | |||
| </div> | |||
| </div> | |||
| <div class="bp" tabindex="0" data-testid="bp"> | |||
| <div class="bp-content" tabindex="0" data-testid="bp-content"> | |||
| <div class="bp" data-testid="bp" tabindex="-1"> | |||
There was a problem hiding this comment.
Do both of these need to be programmatically focusable? It looks like BaseViewer focuses on .bp-content when autoFocus is enabled.
There was a problem hiding this comment.
The ContentPreview Element auto-focuses on .bp and the Preview SDK auto-focuses on .bp-content.
src/lib/viewers/doc/DocBaseViewer.js
Outdated
| setTimeout(() => this.resize(), THUMBNAILS_SIDEBAR_TRANSITION_TIME); | ||
| setTimeout(() => { | ||
| this.resize(); | ||
| this.thumbnailsSidebarEl.style = style; |
There was a problem hiding this comment.
Seems like setting style directly as a string is discouraged (https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style#Setting_styles).
Also, does this affect the animation transition at all?
There was a problem hiding this comment.
It does appear to affect the hidden -> visible transition. I'll update it to remove the display: none immediately rather than via setTimeout for that case. Good catch!
| @@ -232,9 +232,9 @@ class ThumbnailsSidebar { | |||
| * @return {HTMLElement} - thumbnail anchor element | |||
| */ | |||
| createThumbnailNav() { | |||
| const thumbnailNav = document.createElement('a'); | |||
| const thumbnailNav = document.createElement('button'); | |||
There was a problem hiding this comment.
Does button work better than a in this case?
There was a problem hiding this comment.
Anchors without an href attribute are considered placeholders rather than links and aren't included in the tab order (source). Since we just want to response to a user action rather than navigating, a button seems more semantically correct and also obviates the need for tabindex="0"
|
By adding the |
jeremypress
changed the title
No description provided.