Merge pull request #1894 from hypothesis/fix-tab-keyboard-select

Support selecting tabs with the keyboard

src/sidebar/components/selection-tabs.js

@@ -13,24 +13,26 @@ import SvgIcon from './svg-icon';
 /**
  *  Display name of the tab and annotation count.
  */
-function Tab({
-  children,
-  count,
-  isWaitingToAnchor,
-  onChangeTab,
-  selected,
-  type,
-}) {
+function Tab({ children, count, isWaitingToAnchor, isSelected, onSelect }) {
+  const selectTab = () => {
+    if (!isSelected) {
+      onSelect();
+    }
+  };
+
   return (
     <button
       className={classnames('selection-tabs__type', {
-        'is-selected': selected,
+        'is-selected': isSelected,
       })}
-      onMouseDown={onChangeTab.bind(this, type)}
-      onTouchStart={onChangeTab.bind(this, type)}
+      // Listen for `onMouseDown` so that the tab is selected when _pressed_
+      // as this makes the UI feel faster. Also listen for `onClick` as a fallback
+      // to enable selecting the tab via other input methods.
+      onClick={selectTab}
+      onMouseDown={selectTab}
       role="tab"
       tabIndex="0"
-      aria-selected={selected.toString()}
+      aria-selected={isSelected.toString()}
     >
       {children}
       {count > 0 && !isWaitingToAnchor && (
@@ -39,6 +41,7 @@ function Tab({
     </button>
   );
 }
+
 Tab.propTypes = {
   /**
    * Child components.
@@ -49,22 +52,17 @@ Tab.propTypes = {
    */
   count: propTypes.number.isRequired,
   /**
-   * Are there any annotations still waiting to anchor?
-   */
-  isWaitingToAnchor: propTypes.bool.isRequired,
-  /**
-   * Callback when this tab is active with type as a parameter.
+   * Is this tab currently selected?
    */
-  onChangeTab: propTypes.func.isRequired,
+  isSelected: propTypes.bool.isRequired,
   /**
-   * Is this tab currently selected?
+   * Are there any annotations still waiting to anchor?
    */
-  selected: propTypes.bool.isRequired,
+  isWaitingToAnchor: propTypes.bool.isRequired,
   /**
-   * The type value for this tab. One of
-   * 'annotation', 'note', or 'orphan'.
+   * Callback to invoke when this tab is selected.
    */
-  type: propTypes.oneOf(['annotation', 'note', 'orphan']).isRequired,
+  onSelect: propTypes.func.isRequired,
 };
 
 /**
@@ -87,9 +85,9 @@ function SelectionTabs({ isLoading, settings }) {
 
   const isThemeClean = settings.theme === 'clean';
 
-  const selectTab = function(type) {
+  const selectTab = tabId => {
     store.clearSelectedAnnotations();
-    store.selectTab(type);
+    store.selectTab(tabId);
   };
 
   const showAnnotationsUnavailableMessage =
@@ -111,18 +109,16 @@ function SelectionTabs({ isLoading, settings }) {
         <Tab
           count={annotationCount}
           isWaitingToAnchor={isWaitingToAnchorAnnotations}
-          selected={selectedTab === uiConstants.TAB_ANNOTATIONS}
-          type={uiConstants.TAB_ANNOTATIONS}
-          onChangeTab={selectTab}
+          isSelected={selectedTab === uiConstants.TAB_ANNOTATIONS}
+          onSelect={() => selectTab(uiConstants.TAB_ANNOTATIONS)}
         >
           Annotations
         </Tab>
         <Tab
           count={noteCount}
           isWaitingToAnchor={isWaitingToAnchorAnnotations}
-          selected={selectedTab === uiConstants.TAB_NOTES}
-          type={uiConstants.TAB_NOTES}
-          onChangeTab={selectTab}
+          isSelected={selectedTab === uiConstants.TAB_NOTES}
+          onSelect={() => selectTab(uiConstants.TAB_NOTES)}
         >
           Page Notes
         </Tab>
@@ -130,9 +126,8 @@ function SelectionTabs({ isLoading, settings }) {
           <Tab
             count={orphanCount}
             isWaitingToAnchor={isWaitingToAnchorAnnotations}
-            selected={selectedTab === uiConstants.TAB_ORPHANS}
-            type={uiConstants.TAB_ORPHANS}
-            onChangeTab={selectTab}
+            isSelected={selectedTab === uiConstants.TAB_ORPHANS}
+            onSelect={() => selectTab(uiConstants.TAB_ORPHANS)}
           >
             Orphans
           </Tab>

src/sidebar/components/test/selection-tabs-test.js

@@ -205,6 +205,49 @@ describe('SelectionTabs', function() {
     });
   });
 
+  const findButton = (wrapper, label) =>
+    wrapper.findWhere(
+      el => el.type() === 'button' && el.text().includes(label)
+    );
+
+  [
+    { label: 'Annotations', tab: uiConstants.TAB_ANNOTATIONS },
+    { label: 'Page Notes', tab: uiConstants.TAB_NOTES },
+    { label: 'Orphans', tab: uiConstants.TAB_ORPHANS },
+  ].forEach(({ label, tab }) => {
+    it(`should change the selected tab when "${label}" tab is clicked`, () => {
+      // Pre-select a different tab than the one we are about to click.
+      fakeStore.getState.returns({
+        selection: {
+          selectedTab: 'other-tab',
+        },
+      });
+
+      // Make the "Orphans" tab appear.
+      fakeStore.orphanCount.returns(1);
+      const wrapper = createComponent({});
+
+      findButton(wrapper, label).simulate('click');
+
+      assert.calledOnce(fakeStore.clearSelectedAnnotations);
+      assert.calledWith(fakeStore.selectTab, tab);
+    });
+  });
+
+  it('does not change the selected tab if it is already selected', () => {
+    fakeStore.getState.returns({
+      selection: {
+        selectedTab: uiConstants.TAB_NOTES,
+      },
+    });
+    const wrapper = createComponent({});
+
+    findButton(wrapper, 'Page Notes').simulate('click');
+
+    assert.notCalled(fakeStore.clearSelectedAnnotations);
+    assert.notCalled(fakeStore.selectTab);
+  });
+
   it(
     'should pass a11y checks',
     checkAccessibility({