Fix: Keyboard navigation should only focus meaningful elements (#970)

box · Apr 1, 2019 · cb6fe25 · cb6fe25
1 parent 8d5cbb8
commit cb6fe25
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 16 deletions.
diff --git a/src/lib/ThumbnailsSidebar.js b/src/lib/ThumbnailsSidebar.js
@@ -232,9 +232,9 @@ class ThumbnailsSidebar {
      * @return {HTMLElement} - thumbnail anchor element
      */
     createThumbnailNav() {
-        const thumbnailNav = document.createElement('a');
+        const thumbnailNav = document.createElement('button');
         thumbnailNav.className = CLASS_BOX_PREVIEW_THUMBNAIL_NAV;
-        thumbnailNav.tabIndex = '0';
+        thumbnailNav.type = 'button';
         return thumbnailNav;
     }
 

diff --git a/src/lib/_common.scss b/src/lib/_common.scss
@@ -163,10 +163,6 @@ $header-height: 48px;
     }
 }
 
-.bp.bp-loaded .bp-content {
-    transition: left 300ms cubic-bezier(.4, 0, .2, 1);
-}
-
 .accessibility-hidden {
     left: -9999px;
     position: absolute;

diff --git a/src/lib/constants.js b/src/lib/constants.js
@@ -35,8 +35,11 @@ export const CLASS_BOX_PREVIEW_NOTIFICATION = 'bp-notification';
 export const CLASS_BOX_PREVIEW_NOTIFICATION_WRAPPER = 'bp-notifications-wrapper';
 export const CLASS_BOX_PREVIEW_TOGGLE_OVERLAY = 'bp-toggle-overlay';
 export const CLASS_BOX_PREVIEW_THEME_DARK = 'bp-theme-dark';
-export const CLASS_BOX_PREVIEW_THUMBNAILS_OPEN = 'bp-thumbnails-open';
+export const CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE = 'bp-thumbnails-close';
+export const CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE = 'bp-thumbnails-close-active';
 export const CLASS_BOX_PREVIEW_THUMBNAILS_CONTAINER = 'bp-thumbnails-container';
+export const CLASS_BOX_PREVIEW_THUMBNAILS_OPEN = 'bp-thumbnails-open';
+export const CLASS_BOX_PREVIEW_THUMBNAILS_OPEN_ACTIVE = 'bp-thumbnails-open-active';
 export const CLASS_ELEM_KEYBOARD_FOCUS = 'bp-has-keyboard-focus';
 export const CLASS_FULLSCREEN = 'bp-is-fullscreen';
 export const CLASS_FULLSCREEN_UNSUPPORTED = 'bp-fullscreen-unsupported';

diff --git a/src/lib/shell.html b/src/lib/shell.html
@@ -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">
+        <div class="bp-content" data-testid="bp-content" tabindex="-1">
             <div class="bp-loading-wrapper">
                 <div class="bp-loading">
                     <div class="bp-icon bp-icon-file"></div>

diff --git a/src/lib/viewers/doc/DocBaseViewer.js b/src/lib/viewers/doc/DocBaseViewer.js
@@ -22,7 +22,10 @@ import {
     ENCODING_TYPES,
     CLASS_BOX_PREVIEW_THUMBNAILS_CONTAINER,
     ANNOTATOR_EVENT,
-    CLASS_BOX_PREVIEW_THUMBNAILS_OPEN
+    CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE,
+    CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE,
+    CLASS_BOX_PREVIEW_THUMBNAILS_OPEN,
+    CLASS_BOX_PREVIEW_THUMBNAILS_OPEN_ACTIVE
 } from '../../constants';
 import { checkPermission, getRepresentation } from '../../file';
 import { appendQueryParams, createAssetUrlCreator, getMidpoint, getDistance, getClosestPageToPinch } from '../../util';
@@ -1349,10 +1352,14 @@ class DocBaseViewer extends BaseViewer {
         let eventName;
         if (!this.thumbnailsSidebar.isOpen) {
             this.rootEl.classList.remove(CLASS_BOX_PREVIEW_THUMBNAILS_OPEN);
+            this.rootEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE);
+            this.rootEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE);
             metricName = USER_DOCUMENT_THUMBNAIL_EVENTS.CLOSE;
             eventName = 'thumbnailsClose';
         } else {
+            this.rootEl.classList.remove(CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE);
             this.rootEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAILS_OPEN);
+            this.rootEl.classList.add(CLASS_BOX_PREVIEW_THUMBNAILS_OPEN_ACTIVE);
             metricName = USER_DOCUMENT_THUMBNAIL_EVENTS.OPEN;
             eventName = 'thumbnailsOpen';
         }
@@ -1361,7 +1368,13 @@ class DocBaseViewer extends BaseViewer {
         this.emit(eventName);
 
         // Resize after the CSS animation to toggle the sidebar is complete
-        setTimeout(() => this.resize(), THUMBNAILS_SIDEBAR_TRANSITION_TIME);
+        setTimeout(() => {
+            this.resize();
+
+            // Remove the active classes to allow the container to be transitioned properly
+            this.rootEl.classList.remove(CLASS_BOX_PREVIEW_THUMBNAILS_CLOSE_ACTIVE);
+            this.rootEl.classList.remove(CLASS_BOX_PREVIEW_THUMBNAILS_OPEN_ACTIVE);
+        }, THUMBNAILS_SIDEBAR_TRANSITION_TIME);
     }
 
     /**

diff --git a/src/lib/viewers/doc/_docBase.scss b/src/lib/viewers/doc/_docBase.scss
@@ -4,7 +4,7 @@ $thumbnail-border-radius: 3px;
 // Accounts for the 1px border on the right so the remaining width is actually 225
 $thumbnail-sidebar-width: 226px;
 
-@mixin breakpoint-medium {
+@mixin bp-breakpoint-medium {
     @media (min-width: 600px) { @content; }
 }
 
@@ -90,11 +90,54 @@ $thumbnail-sidebar-width: 226px;
         }
     }
 
-    @include breakpoint-medium {
+    @include bp-breakpoint-medium {
+        $bp-thumbnails-animation: 300ms cubic-bezier(.4, 0, .2, 1);
+
+        @keyframes bp-thumbnails-open {
+            0% {
+                display: none;
+                transform: translateX(-$thumbnail-sidebar-width);
+            }
+
+            1% {
+                display: flex;
+                transform: translateX(-$thumbnail-sidebar-width);
+            }
+
+            100% {
+                transform: translateX(0);
+            }
+        }
+
+        &.bp-loaded {
+            .bp-content {
+                transition: left $bp-thumbnails-animation;
+            }
+        }
+
         .bp-thumbnails-container {
-            display: flex;
-            transform: translateX(-$thumbnail-sidebar-width);
-            transition: transform 300ms cubic-bezier(.4, 0, .2, 1);
+            display: none;
+            transition: transform $bp-thumbnails-animation;
+        }
+
+        &,
+        &.bp-thumbnails-close,
+        &.bp-thumbnails-close-active {
+            .bp-thumbnails-container {
+                transform: translateX(-$thumbnail-sidebar-width);
+            }
+        }
+
+        &.bp-thumbnails-close {
+            .bp-thumbnails-container {
+                display: none;
+            }
+        }
+
+        &.bp-thumbnails-close-active {
+            .bp-thumbnails-container {
+                display: flex;
+            }
         }
 
         &.bp-thumbnails-open {
@@ -103,10 +146,17 @@ $thumbnail-sidebar-width: 226px;
             }
 
             .bp-thumbnails-container {
+                display: flex;
                 transform: translateX(0);
             }
         }
 
+        &.bp-thumbnails-open-active {
+            .bp-thumbnails-container {
+                animation: bp-thumbnails-open $bp-thumbnails-animation;
+            }
+        }
+
         .bp-controls-btn.bp-toggle-thumbnails-icon {
             display: block;
         }