[Terra-Button-Group] Fixes accessibility issues of button-group (#3857)

packages/terra-button-group/CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+* Fixed
+  * Fixed visual focus when using screenreaders.
+
+ * Added
+   * Added cyclic behavior when navigating through buttons with a keyboard.
+
 ## 3.67.0 - (August 11, 2023)
 
 * Changed

packages/terra-button-group/src/ButtonGroup.jsx

@@ -3,7 +3,9 @@ import PropTypes from 'prop-types';
 import classNames from 'classnames';
 import classNamesBind from 'classnames/bind';
 import ThemeContext from 'terra-theme-context';
-import { KEY_RIGHT, KEY_LEFT } from 'keycode-js';
+import {
+  KEY_RIGHT, KEY_LEFT, KEY_UP, KEY_DOWN,
+} from 'keycode-js';
 import ButtonGroupButton from './ButtonGroupButton';
 import ButtonGroupUtils from './ButtonGroupUtils';
 import styles from './ButtonGroup.module.scss';
@@ -60,22 +62,53 @@ class ButtonGroup extends React.Component {
   handleKeyDown(event, idx) {
     const allBtns = this.btnGrpRef.querySelectorAll('[data-terra-button-group-button]');
     let key = idx;
-
-    if (event.keyCode === KEY_RIGHT && allBtns[key + 1]) {
-      key += 1;
-      while (allBtns[key] && allBtns[key].hasAttribute('disabled')) {
+    if ((event.keyCode === KEY_RIGHT || event.keyCode === KEY_DOWN)) {
+      if (idx === allBtns.length - 1) {
+        key = 0;
+        if (allBtns[key].hasAttribute('disabled')) {
+          key += 1;
+        }
+      } else {
         key += 1;
+        if (allBtns[key].hasAttribute('disabled')) {
+          key += 1;
+          if (key === allBtns.length) {
+            key = 0;
+            if (allBtns[key].hasAttribute('disabled')) {
+              key += 1;
+            }
+          }
+        }
       }
       if (allBtns[key]) allBtns[key].focus();
     }
-
-    if (event.keyCode === KEY_LEFT && allBtns[key - 1]) {
-      key -= 1;
+    if ((event.keyCode === KEY_LEFT || event.keyCode === KEY_UP)) {
+      if (idx === 0) {
+        key = allBtns.length - 1;
+        if (allBtns[key].hasAttribute('disabled')) {
+          key -= 1;
+        }
+      } else {
+        key -= 1;
+        if (allBtns[key].hasAttribute('disabled')) {
+          if (key === 0) {
+            key = allBtns.length - 1;
+            if (allBtns[key].hasAttribute('disabled')) {
+              key -= 1;
+            }
+          } else {
+            key -= 1;
+          }
+        }
+      }
       while (allBtns[key] && allBtns[key].hasAttribute('disabled')) {
         key -= 1;
       }
       if (allBtns[key]) allBtns[key].focus();
     }
+    if (event.keyCode === KEY_UP || event.keyCode === KEY_DOWN) {
+      event.preventDefault();
+    }
   }
 
   wrapKeyDown(item, idx) {
@@ -123,17 +156,17 @@ class ButtonGroup extends React.Component {
 
     const allButtons = children ? [] : undefined;
     // eslint-disable-next-line no-nested-ternary
-    const btnRole = onChange ? isMultiSelect ? 'checkbox' : 'radio' : 'button';
+    const btnRole = onChange ? (isMultiSelect ? 'checkbox' : 'radio') : '';
 
     React.Children.forEach(children, (child, index) => {
       const isSelected = selectedKeys.indexOf(child.key) > -1;
       const cloneChild = React.cloneElement(child, {
-        role: btnRole,
+        role: btnRole || undefined,
         onClick: this.wrapOnClick(child),
         onKeyDown: this.wrapKeyDown(child, index),
         className: cx([{ 'is-selected': isSelected && !child.props.isDisabled }, child.props.className]),
-        'aria-pressed': btnRole === 'button' && !child.props.isDisabled ? isSelected : undefined,
-        'aria-checked': btnRole !== 'button' && !child.props.isDisabled ? isSelected : undefined,
+        'aria-pressed': btnRole === '' && !child.props.isDisabled ? isSelected : undefined,
+        'aria-checked': btnRole !== '' && !child.props.isDisabled ? isSelected : undefined,
       });
       allButtons.push(cloneChild);
     });

packages/terra-button-group/src/ButtonGroupButton.jsx

@@ -88,7 +88,8 @@ class ButtonGroupButton extends React.Component {
   handleKeyUp(event) {
     // Apply focus styles for keyboard navigation.
     // The onFocus event doesn't get triggered in some browsers, hence, the focus state needs to be managed here.
-    if (event.nativeEvent.keyCode === KeyCode.KEY_TAB || event.nativeEvent.keyCode === KeyCode.KEY_LEFT || event.nativeEvent.keyCode === KeyCode.KEY_RIGHT) {
+    const validKeys = new Set([KeyCode.KEY_TAB, KeyCode.KEY_LEFT, KeyCode.KEY_RIGHT, KeyCode.KEY_UP, KeyCode.KEY_DOWN]);
+    if (validKeys.has(event.keyCode)) {
       this.setState({ focused: true });
       this.shouldShowFocus = true;
     }

packages/terra-button-group/tests/jest/__snapshots__/ButtonGroup.test.jsx.snap

@@ -24,7 +24,6 @@ exports[`correctly applies the theme context className 1`] = `
         key="1"
         onClick={[Function]}
         onKeyDown={[Function]}
-        role="button"
         text="Button 1"
       >
         <Button
@@ -41,7 +40,6 @@ exports[`correctly applies the theme context className 1`] = `
           onFocus={[Function]}
           onKeyDown={[Function]}
           onKeyUp={[Function]}
-          role="button"
           text="Button 1"
           type="button"
           variant="neutral"
@@ -58,7 +56,6 @@ exports[`correctly applies the theme context className 1`] = `
             onKeyDown={[Function]}
             onKeyUp={[Function]}
             onMouseDown={[Function]}
-            role="button"
             type="button"
           >
             <span
@@ -80,7 +77,6 @@ exports[`correctly applies the theme context className 1`] = `
         key="2"
         onClick={[Function]}
         onKeyDown={[Function]}
-        role="button"
         text="Button 2"
       >
         <Button
@@ -97,7 +93,6 @@ exports[`correctly applies the theme context className 1`] = `
           onFocus={[Function]}
           onKeyDown={[Function]}
           onKeyUp={[Function]}
-          role="button"
           text="Button 2"
           type="button"
           variant="neutral"
@@ -114,7 +109,6 @@ exports[`correctly applies the theme context className 1`] = `
             onKeyDown={[Function]}
             onKeyUp={[Function]}
             onMouseDown={[Function]}
-            role="button"
             type="button"
           >
             <span
@@ -202,7 +196,6 @@ exports[`should only render enabled buttons as toggle buttons 1`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
   <ButtonGroupButton
@@ -212,7 +205,6 @@ exports[`should only render enabled buttons as toggle buttons 1`] = `
     key="2"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 2"
   />
   <ButtonGroupButton
@@ -221,7 +213,6 @@ exports[`should only render enabled buttons as toggle buttons 1`] = `
     key="dis"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Disabled Button"
   />
 </div>
@@ -239,7 +230,6 @@ exports[`should render a block button group with children 1`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
   <ButtonGroupButton
@@ -249,7 +239,6 @@ exports[`should render a block button group with children 1`] = `
     key="2"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 2"
   />
   <ButtonGroupButton
@@ -259,7 +248,6 @@ exports[`should render a block button group with children 1`] = `
     key="3"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 3"
   />
 </div>
@@ -277,7 +265,6 @@ exports[`should render a button group with children 1`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
 </div>
@@ -295,7 +282,6 @@ exports[`should render a button group with children selected 1`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
   <ButtonGroupButton
@@ -305,7 +291,6 @@ exports[`should render a button group with children selected 1`] = `
     key="2"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 2"
   />
 </div>
@@ -330,7 +315,6 @@ exports[`should select a button 1`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
   <ButtonGroupButton
@@ -340,7 +324,6 @@ exports[`should select a button 1`] = `
     key="2"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 2"
   />
 </div>
@@ -358,7 +341,6 @@ exports[`should select a button 2`] = `
     key="1"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 1"
   />
   <ButtonGroupButton
@@ -368,7 +350,6 @@ exports[`should select a button 2`] = `
     key="2"
     onClick={[Function]}
     onKeyDown={[Function]}
-    role="button"
     text="Button 2"
   />
 </div>

packages/terra-button-group/tests/wdio/button-group-spec.js

@@ -120,6 +120,26 @@ Terra.describeViewports('Button Group', ['huge'], () => {
       browser.keys('ArrowLeft');
       Terra.validates.element('Button-1 focused');
     });
+
+    it('Navigates from first button to last button using left arrow', () => {
+      browser.keys('ArrowLeft');
+      expect($('#button4').isFocused()).toBeTruthy();
+    });
+
+    it('Navigates from last button to first button using right arrow', () => {
+      browser.keys('ArrowRight');
+      expect($('#button1').isFocused()).toBeTruthy();
+    });
+
+    it('Navigates from first button to last button using up arrow', () => {
+      browser.keys('ArrowUp');
+      expect($('#button4').isFocused()).toBeTruthy();
+    });
+
+    it('Navigates from last button to first button using down arrow', () => {
+      browser.keys('ArrowDown');
+      expect($('#button1').isFocused()).toBeTruthy();
+    });
   });
 
   describe('Button Focus', () => {

packages/terra-core-docs/CHANGELOG.md

@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+* Changed
+  * Updated `terra-button-group` tests.
+
 * Removed
   * Removed custom styles of `terra-arrange` examples to make whole content visible.
 

packages/terra-core-docs/src/terra-dev-site/test/button-group/ButtonGroupMultiSelect.test.jsx

@@ -27,10 +27,10 @@ class ButtonGroupMultiSelect extends React.Component {
           selectedKeys={this.state.selectedKeys}
           isMultiSelect
         >
-          <ButtonGroup.Button text="Multi-Select 1" key="1" />
-          <ButtonGroup.Button text="Multi-Select 2" key="2" />
-          <ButtonGroup.Button text="Multi-Select 3" key="3" />
-          <ButtonGroup.Button text="Multi-Select 4" key="4" />
+          <ButtonGroup.Button id="button1" text="Multi-Select 1" key="1" />
+          <ButtonGroup.Button id="button2" text="Multi-Select 2" key="2" />
+          <ButtonGroup.Button id="button3" text="Multi-Select 3" key="3" />
+          <ButtonGroup.Button id="button4" text="Multi-Select 4" key="4" />
         </ButtonGroup>
       </div>
     );