[EuiAccordion] Improve keyboard accessibility of tabbable children (#…

…7064)

src/components/accordion/__snapshots__/accordion.test.tsx.snap

@@ -218,6 +218,59 @@ exports[`EuiAccordion behavior opens when div is clicked if element is a div 1`]
 </div>
 `;
 
+exports[`EuiAccordion behavior restores tabbable children on accordion open 1`] = `
+<div
+  class="euiAccordion__children emotion-euiAccordion__children"
+>
+  <button>
+    tabbable item one
+  </button>
+  <input
+    placeholder="tabbable item two"
+    type="text"
+  />
+  <a
+    href="#"
+  >
+    tabbable item three
+  </a>
+  <div
+    tabindex="0"
+  >
+    tabbable item four
+  </div>
+</div>
+`;
+
+exports[`EuiAccordion behavior sets tabbable children to \`tabIndex={-1}\` when accordions are closed 1`] = `
+<div
+  class="euiAccordion__children emotion-euiAccordion__children"
+>
+  <button
+    tabindex="-1"
+  >
+    tabbable item one
+  </button>
+  <input
+    placeholder="tabbable item two"
+    tabindex="-1"
+    type="text"
+  />
+  <a
+    href="#"
+    tabindex="-1"
+  >
+    tabbable item three
+  </a>
+  <div
+    data-original-tabindex="0"
+    tabindex="-1"
+  >
+    tabbable item four
+  </div>
+</div>
+`;
+
 exports[`EuiAccordion is rendered 1`] = `
 <div
   aria-label="aria-label"

src/components/accordion/accordion.test.tsx

@@ -7,6 +7,7 @@
  */
 
 import React from 'react';
+import { fireEvent } from '@testing-library/react';
 import { mount } from 'enzyme';
 import { shouldRenderCustomStyles } from '../../test/internal';
 import { requiredProps } from '../../test/required_props';
@@ -281,5 +282,39 @@ describe('EuiAccordion', () => {
 
       expect(childWrapper).toBe(document.activeElement);
     });
+
+    it('sets tabbable children to `tabIndex={-1}` when accordions are closed', () => {
+      const { container } = render(
+        <EuiAccordion id={getId()}>
+          <button>tabbable item one</button>
+          <input type="text" placeholder="tabbable item two" />
+          <a href="#">tabbable item three</a>
+          <div tabIndex={0}>tabbable item four</div>
+        </EuiAccordion>
+      );
+      const children = container.querySelector('.euiAccordion__children')!;
+
+      expect(children.querySelectorAll('[tabindex="-1"]')).toHaveLength(4);
+      expect(children).toMatchSnapshot();
+    });
+
+    it('restores tabbable children on accordion open', () => {
+      const { container, getByTestSubject } = render(
+        <EuiAccordion
+          id={getId()}
+          buttonProps={{ 'data-test-subj': 'trigger' }}
+        >
+          <button>tabbable item one</button>
+          <input type="text" placeholder="tabbable item two" />
+          <a href="#">tabbable item three</a>
+          <div tabIndex={0}>tabbable item four</div>
+        </EuiAccordion>
+      );
+      fireEvent.click(getByTestSubject('trigger'));
+      const children = container.querySelector('.euiAccordion__children')!;
+
+      expect(children.querySelectorAll('[tabindex="-1"]')).toHaveLength(0);
+      expect(children).toMatchSnapshot();
+    });
   });
 });

src/components/accordion/accordion.tsx

@@ -8,6 +8,7 @@
 
 import React, { Component, HTMLAttributes, ReactNode } from 'react';
 import classNames from 'classnames';
+import { tabbable, FocusableElement } from 'tabbable';
 
 import { CommonProps } from '../common';
 
@@ -108,9 +109,13 @@ export type EuiAccordionProps = CommonProps &
     isDisabled?: boolean;
   };
 
+type EuiAccordionState = {
+  isOpen: boolean;
+};
+
 export class EuiAccordionClass extends Component<
   WithEuiThemeProps & EuiAccordionProps,
-  { isOpen: boolean }
+  EuiAccordionState
 > {
   static defaultProps = {
     initialIsOpen: false,
@@ -125,21 +130,24 @@ export class EuiAccordionClass extends Component<
 
   childContent: HTMLDivElement | null = null;
   childWrapper: HTMLDivElement | null = null;
+  tabbableChildren: FocusableElement[] | null = null;
 
   state = {
     isOpen: this.props.forceState
       ? this.props.forceState === 'open'
       : this.props.initialIsOpen!,
   };
 
+  get isOpen() {
+    return this.props.forceState
+      ? this.props.forceState === 'open'
+      : this.state.isOpen;
+  }
+
   setChildContentHeight = () => {
-    const { forceState } = this.props;
     requestAnimationFrame(() => {
       const height =
-        this.childContent &&
-        (forceState ? forceState === 'open' : this.state.isOpen)
-          ? this.childContent.clientHeight
-          : 0;
+        this.childContent && this.isOpen ? this.childContent.clientHeight : 0;
       this.childWrapper &&
         this.childWrapper.setAttribute(
           'style',
@@ -150,17 +158,33 @@ export class EuiAccordionClass extends Component<
 
   componentDidMount() {
     this.setChildContentHeight();
+    if (!this.isOpen) this.preventTabbing();
   }
 
-  componentDidUpdate() {
+  componentDidUpdate(
+    prevProps: EuiAccordionProps,
+    prevState: EuiAccordionState
+  ) {
     this.setChildContentHeight();
+
+    if (
+      (prevProps.forceState === 'open' && this.props.forceState === 'closed') ||
+      (prevState.isOpen === true && this.state.isOpen === false)
+    ) {
+      this.preventTabbing();
+    }
+    if (
+      (prevProps.forceState === 'closed' && this.props.forceState === 'open') ||
+      (prevState.isOpen === false && this.state.isOpen === true)
+    ) {
+      this.enableTabbing();
+    }
   }
 
   onToggle = () => {
     const { forceState } = this.props;
     if (forceState) {
-      this.props.onToggle &&
-        this.props.onToggle(forceState === 'open' ? false : true);
+      this.props.onToggle?.(forceState === 'open' ? false : true);
     } else {
       this.setState(
         (prevState) => ({
@@ -170,12 +194,49 @@ export class EuiAccordionClass extends Component<
           if (this.state.isOpen && this.childWrapper) {
             this.childWrapper.focus();
           }
-          this.props.onToggle && this.props.onToggle(this.state.isOpen);
+          this.props.onToggle?.(this.state.isOpen);
         }
       );
     }
   };
 
+  // When accordions are closed, tabbable children should not be present in the tab order
+  preventTabbing = () => {
+    if (this.childContent) {
+      // Re-check for children on every close - content can change dynamically
+      this.tabbableChildren = tabbable(this.childContent);
+
+      this.tabbableChildren.forEach((element) => {
+        // If the element has an existing `tabIndex` set, make sure we can restore it
+        const originalTabIndex = element.getAttribute('tabIndex');
+        if (originalTabIndex) {
+          element.setAttribute('data-original-tabindex', originalTabIndex);
+        }
+
+        element.setAttribute('tabIndex', '-1');
+      });
+    }
+  };
+
+  enableTabbing = () => {
+    // If no tabbable children were set, we don't need to re-enable anything
+    if (this.tabbableChildren) {
+      this.tabbableChildren.forEach((element) => {
+        const originalTabIndex = element.getAttribute('data-original-tabindex');
+        if (originalTabIndex) {
+          // If the element originally had an existing `tabIndex` set, restore it
+          element.setAttribute('tabIndex', originalTabIndex);
+          element.removeAttribute('data-original-tabindex');
+        } else {
+          // If not, remove the tabIndex property
+          element.removeAttribute('tabIndex');
+        }
+      });
+      // Cleanup - unset the list of children
+      this.tabbableChildren = null;
+    }
+  };
+
   setChildContentRef = (node: HTMLDivElement | null) => {
     this.childContent = node;
   };
@@ -214,8 +275,6 @@ export class EuiAccordionClass extends Component<
       ...rest
     } = this.props;
 
-    const isOpen = forceState ? forceState === 'open' : this.state.isOpen;
-
     // Force button element to be a legend if the element is a fieldset
     const ButtonElement = Element === 'fieldset' ? 'legend' : _ButtonElement;
     const buttonElementIsFocusable = ButtonElement === 'button';
@@ -229,7 +288,7 @@ export class EuiAccordionClass extends Component<
     const classes = classNames(
       'euiAccordion',
       {
-        'euiAccordion-isOpen': isOpen,
+        'euiAccordion-isOpen': this.isOpen,
       },
       className
     );
@@ -252,7 +311,7 @@ export class EuiAccordionClass extends Component<
     const iconButtonClasses = classNames(
       'euiAccordion__iconButton',
       {
-        'euiAccordion__iconButton-isOpen': isOpen,
+        'euiAccordion__iconButton-isOpen': this.isOpen,
         'euiAccordion__iconButton--right': _arrowDisplay === 'right',
       },
       arrowProps?.className
@@ -276,13 +335,13 @@ export class EuiAccordionClass extends Component<
     const childWrapperStyles = euiAccordionChildWrapperStyles(theme);
     const cssChildWrapperStyles = [
       childWrapperStyles.euiAccordion__childWrapper,
-      isOpen && childWrapperStyles.isOpen,
+      this.isOpen && childWrapperStyles.isOpen,
     ];
 
     const iconButtonStyles = euiAccordionIconButtonStyles(theme);
     const cssIconButtonStyles = [
       iconButtonStyles.euiAccordion__iconButton,
-      isOpen && iconButtonStyles.isOpen,
+      this.isOpen && iconButtonStyles.isOpen,
       _arrowDisplay === 'right' && iconButtonStyles.arrowRight,
       arrowProps?.css,
     ];
@@ -312,7 +371,7 @@ export class EuiAccordionClass extends Component<
           iconType="arrowRight"
           onClick={this.onToggle}
           aria-controls={id}
-          aria-expanded={isOpen}
+          aria-expanded={this.isOpen}
           aria-labelledby={buttonId}
           tabIndex={buttonElementIsFocusable ? -1 : 0}
           isDisabled={isDisabled}
@@ -364,7 +423,7 @@ export class EuiAccordionClass extends Component<
         css={cssButtonStyles}
         aria-controls={id}
         // `aria-expanded` is only a valid attribute on interactive controls - axe-core throws a violation otherwise
-        aria-expanded={ButtonElement === 'button' ? isOpen : undefined}
+        aria-expanded={ButtonElement === 'button' ? this.isOpen : undefined}
         onClick={isDisabled ? undefined : this.onToggle}
         type={ButtonElement === 'button' ? 'button' : undefined}
         disabled={ButtonElement === 'button' ? isDisabled : undefined}

src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.stories.tsx

@@ -47,7 +47,9 @@ export const Playground: Story = {
 
 export const EdgeCaseTesting: Story = {
   render: ({ ...args }) => (
-    <EuiCollapsibleNavBeta>
+    // Since we're not using EuiFlyoutBody/Footer, the flex display isn't needed
+    // and causes item vertical margins to not collapse
+    <EuiCollapsibleNavBeta className="eui-displayBlock">
       <EuiCollapsibleNavItem {...args} href="#" title="Link with no icon" />
       <EuiCollapsibleNavItem
         {...args}

upcoming_changelogs/7064.md

@@ -0,0 +1,3 @@
+**Bug fixes**
+
+- Fixed `EuiAccordion` to remove tabbable children from sequential keyboard navigation when the accordion is closed