From d76c33d4083226eed174819ceb7445db0cd5f09c Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 11:45:32 -0700 Subject: [PATCH 1/6] [misc cleanup] Optional chaining syntax --- src/components/accordion/accordion.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index dfea8f59dd3..b7bcc0410dc 100644 --- a/src/components/accordion/accordion.tsx +++ b/src/components/accordion/accordion.tsx @@ -159,8 +159,7 @@ export class EuiAccordionClass extends Component< 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,7 +169,7 @@ 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); } ); } From 493cf860bb3ad105a891ce9b8fa1e0bee5cd58c0 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 11:49:05 -0700 Subject: [PATCH 2/6] [misc cleanup] DRY out `forceState === 'open'` checks - using a JS getter --- src/components/accordion/accordion.tsx | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index b7bcc0410dc..f0a2054959b 100644 --- a/src/components/accordion/accordion.tsx +++ b/src/components/accordion/accordion.tsx @@ -132,14 +132,16 @@ export class EuiAccordionClass extends Component< : 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', @@ -213,8 +215,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'; @@ -228,7 +228,7 @@ export class EuiAccordionClass extends Component< const classes = classNames( 'euiAccordion', { - 'euiAccordion-isOpen': isOpen, + 'euiAccordion-isOpen': this.isOpen, }, className ); @@ -251,7 +251,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 @@ -275,13 +275,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, ]; @@ -311,7 +311,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} @@ -363,7 +363,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} From 2bece39526593f4f8b87814fbd429cb2c6f9fc2a Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 12:23:25 -0700 Subject: [PATCH 3/6] [EuiAccordion] Manually handle tabbable children on close & re-open --- src/components/accordion/accordion.tsx | 64 +++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index f0a2054959b..8eb69ed73a5 100644 --- a/src/components/accordion/accordion.tsx +++ b/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,6 +130,7 @@ export class EuiAccordionClass extends Component< childContent: HTMLDivElement | null = null; childWrapper: HTMLDivElement | null = null; + tabbableChildren: FocusableElement[] | null = null; state = { isOpen: this.props.forceState @@ -152,10 +158,27 @@ 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 = () => { @@ -177,6 +200,43 @@ export class EuiAccordionClass extends Component< } }; + // 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; }; From e40a7c1e49e25f381ac3cd73a6475eb92f6ad656 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 12:23:42 -0700 Subject: [PATCH 4/6] Write tests --- .../__snapshots__/accordion.test.tsx.snap | 53 +++++++++++++++++++ src/components/accordion/accordion.test.tsx | 35 ++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/components/accordion/__snapshots__/accordion.test.tsx.snap b/src/components/accordion/__snapshots__/accordion.test.tsx.snap index 5a12e7444c6..1f606027299 100644 --- a/src/components/accordion/__snapshots__/accordion.test.tsx.snap +++ b/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`] `; +exports[`EuiAccordion behavior restores tabbable children on accordion open 1`] = ` +
+ + + + tabbable item three + +
+ tabbable item four +
+
+`; + +exports[`EuiAccordion behavior sets tabbable children to \`tabIndex={-1}\` when accordions are closed 1`] = ` +
+ + + + tabbable item three + +
+ tabbable item four +
+
+`; + exports[`EuiAccordion is rendered 1`] = `
{ expect(childWrapper).toBe(document.activeElement); }); + + it('sets tabbable children to `tabIndex={-1}` when accordions are closed', () => { + const { container } = render( + + + + tabbable item three +
tabbable item four
+ + ); + 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( + + + + tabbable item three +
tabbable item four
+ + ); + fireEvent.click(getByTestSubject('trigger')); + const children = container.querySelector('.euiAccordion__children')!; + + expect(children.querySelectorAll('[tabindex="-1"]')).toHaveLength(0); + expect(children).toMatchSnapshot(); + }); }); }); From 60a5ea376dd416a46feda397583f8b0417f326d3 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 12:47:31 -0700 Subject: [PATCH 5/6] changelog --- upcoming_changelogs/7064.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 upcoming_changelogs/7064.md diff --git a/upcoming_changelogs/7064.md b/upcoming_changelogs/7064.md new file mode 100644 index 00000000000..5e10dcfa392 --- /dev/null +++ b/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 From 9a9ebc2d59cbf391f8fbe32c4893718877438564 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 9 Aug 2023 12:56:26 -0700 Subject: [PATCH 6/6] [misc] Fix `EuiCollapsibleNavItem` story - this visually broke when I removed the `eui-yScroll` child div - fixing it now --- .../collapsible_nav_item/collapsible_nav_item.stories.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.stories.tsx b/src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.stories.tsx index 9735d9ed301..5a5de350df3 100644 --- a/src/components/collapsible_nav_beta/collapsible_nav_item/collapsible_nav_item.stories.tsx +++ b/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 }) => ( - + // Since we're not using EuiFlyoutBody/Footer, the flex display isn't needed + // and causes item vertical margins to not collapse +