From 56fe8eba0a769647c2aea67d4f743f11841ec1a5 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 25 Oct 2023 10:04:22 -0700 Subject: [PATCH 1/4] Fix accordion open focus behavior to only occur on user click --- src/components/accordion/accordion.tsx | 10 ++++++++++ .../accordion_children/accordion_children.tsx | 18 +++++------------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index d51c3a9947b..875230a80ca 100644 --- a/src/components/accordion/accordion.tsx +++ b/src/components/accordion/accordion.tsx @@ -149,11 +149,20 @@ export class EuiAccordionClass extends Component< }), () => { this.props.onToggle?.(this.state.isOpen); + if (this.state.isOpen) { + this.accordionChildrenEl?.focus(); + } } ); } }; + // Used to focus the accordion children on user trigger click only (vs controlled/programmatic open) + accordionChildrenEl: HTMLDivElement | null = null; + accordionChildrenRef = (node: HTMLDivElement | null) => { + this.accordionChildrenEl = node; + }; + generatedId = htmlIdGenerator()(); render() { @@ -222,6 +231,7 @@ export class EuiAccordionClass extends Component< isLoading={isLoading} isLoadingMessage={isLoadingMessage} isOpen={this.isOpen} + accordionChildrenRef={this.accordionChildrenRef} > {children} diff --git a/src/components/accordion/accordion_children/accordion_children.tsx b/src/components/accordion/accordion_children/accordion_children.tsx index 0dca8d2b433..0c6f0a40cc1 100644 --- a/src/components/accordion/accordion_children/accordion_children.tsx +++ b/src/components/accordion/accordion_children/accordion_children.tsx @@ -9,14 +9,14 @@ import React, { FunctionComponent, HTMLAttributes, - useRef, + Ref, useCallback, useMemo, useState, } from 'react'; import classNames from 'classnames'; -import { useEuiTheme, useUpdateEffect } from '../../../services'; +import { useEuiTheme } from '../../../services'; import { EuiResizeObserver } from '../../observer/resize_observer'; import { EuiAccordionProps } from '../accordion'; @@ -32,11 +32,13 @@ type _EuiAccordionChildrenProps = HTMLAttributes & 'children' | 'paddingSize' | 'isLoading' | 'isLoadingMessage' > & { isOpen: boolean; + accordionChildrenRef: Ref; }; export const EuiAccordionChildren: FunctionComponent< _EuiAccordionChildrenProps > = ({ children, + accordionChildrenRef, paddingSize, isLoading, isLoadingMessage, @@ -67,8 +69,6 @@ export const EuiAccordionChildren: FunctionComponent< isOpen ? wrapperStyles.isOpen : wrapperStyles.isClosed, ]; - const wrapperRef = useRef(null); - /** * Update the accordion wrapper height whenever the accordion opens, and also * whenever the child content updates (which will change the height) @@ -83,21 +83,13 @@ export const EuiAccordionChildren: FunctionComponent< [isOpen, contentHeight] ); - /** - * Focus the children wrapper when the accordion is opened, - * but not if the accordion is initially open on mount - */ - useUpdateEffect(() => { - if (isOpen) wrapperRef.current?.focus(); - }, [isOpen]); - return (
Date: Wed, 25 Oct 2023 10:05:32 -0700 Subject: [PATCH 2/4] Rewrite Accordion E2E tests to be more focused and cover new focus logic --- src/components/accordion/accordion.spec.tsx | 153 ++++++++------------ 1 file changed, 59 insertions(+), 94 deletions(-) diff --git a/src/components/accordion/accordion.spec.tsx b/src/components/accordion/accordion.spec.tsx index 1c58d6ac34c..5a6e5de7ff3 100644 --- a/src/components/accordion/accordion.spec.tsx +++ b/src/components/accordion/accordion.spec.tsx @@ -10,63 +10,43 @@ /// /// -import React from 'react'; +import React, { useState } from 'react'; import { EuiAccordion, EuiAccordionProps } from './index'; -import { EuiPanel } from '../../components/panel'; -import { htmlIdGenerator } from '../../services'; -const baseProps: EuiAccordionProps = { +const sharedProps: EuiAccordionProps = { buttonContent: 'Click me to toggle', - id: htmlIdGenerator()(), + id: 'cypress-accordion', initialIsOpen: false, + children: ( + <> + Test accordion content.{' '} + + Focusable link inside content + + + ), }; -const noArrow = { arrowDisplay: 'none' }; -const noArrowProps: EuiAccordionProps = Object.assign(baseProps, noArrow); - describe('EuiAccordion', () => { - describe('Keyboard and screen reader accessibility', () => { - it('renders with required props', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. - - - ); + describe('keyboard and screen reader accessibility', () => { + it('does not tab to the arrow if the button is interactive', () => { + cy.realMount(); cy.realPress('Tab'); cy.focused().contains('Click me to toggle'); + cy.realPress('Tab'); + cy.focused().contains('Focusable link inside content'); }); - it('opens and closes on ENTER keypress', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. - - - ); + it('does tab to the arrow if the button is not interactive', () => { + cy.realMount(); cy.realPress('Tab'); - cy.focused().contains('Click me to toggle').realPress('Enter'); - cy.realPress(['Shift', 'Tab']); - cy.focused().invoke('attr', 'aria-expanded').should('equal', 'true'); - cy.realPress('Enter'); - cy.focused().invoke('attr', 'aria-expanded').should('equal', 'false'); + cy.focused().should('have.class', 'euiAccordion__arrow'); }); - it('opens and closes on SPACE keypress', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. - - - ); + it('opens and closes the accordion on keypress', () => { + cy.realMount(); cy.realPress('Tab'); - cy.focused().contains('Click me to toggle').realPress('Space'); + cy.realPress('Enter'); cy.realPress(['Shift', 'Tab']); cy.focused().invoke('attr', 'aria-expanded').should('equal', 'true'); cy.realPress('Space'); @@ -74,65 +54,50 @@ describe('EuiAccordion', () => { }); }); - describe('Props and navigation', () => { - it('should not have an arrow', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. - - - ); - cy.get('.euiAccordion__arrow').should('not.exist'); - }); + describe('focus management', () => { + const expectChildrenIsFocused = () => { + cy.focused() + .should('have.class', 'euiAccordion__childWrapper') + .should('have.attr', 'tabindex', '-1'); + }; - it('manages focus when panel is clicked', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. - - - ); - cy.get('button').contains('Click me to toggle').realClick(); - cy.focused().invoke('attr', 'tabindex').should('equal', '-1'); - cy.focused().contains('Any content inside of EuiAccordion'); + it('focuses the accordion content when the arrow is clicked', () => { + cy.realMount(); + cy.get('.euiAccordion__arrow').realClick(); + expectChildrenIsFocused(); }); - it('manages focus when panel is opened by keyboard interaction', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. We will include a link to confirm focus. - - - ); + it('focuses the accordion content when the button is clicked', () => { + cy.realMount(); cy.realPress('Tab'); cy.focused().contains('Click me to toggle').realPress('Enter'); - cy.focused().invoke('attr', 'tabindex').should('equal', '-1'); - cy.focused().contains('Any content inside of EuiAccordion'); - cy.realPress('Tab'); - cy.focused().contains('a link'); + expectChildrenIsFocused(); }); - it('manages focus when forceState is open', () => { - cy.realMount( - - - Any content inside of EuiAccordion will appear - here. We will include a link to confirm focus. - - - ); - cy.realPress('Tab'); - cy.focused().contains('Click me to toggle'); - cy.focused().invoke('attr', 'aria-expanded').should('equal', 'true'); - cy.focused().invoke('attr', 'tabindex').should('not.exist'); - cy.realPress('Tab'); - cy.focused().contains('a link'); + it('does not focus the accordion when programmatically toggled via forceState', () => { + const ControlledComponent = () => { + const [accordionOpen, setAccordionOpen] = useState(false); + return ( + <> + + + + ); + }; + cy.realMount(); + + cy.get('[data-test-subj="toggleForceState"]').realClick(); + cy.focused() + .should('not.have.class', 'euiAccordion__childWrapper') + .should('have.attr', 'data-test-subj', 'toggleForceState'); }); }); }); From 611c0ce86a3badb4b918615274ae380267adba72 Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 25 Oct 2023 10:42:37 -0700 Subject: [PATCH 3/4] Attempt to preserve focus behavior on user click/toggle even when `forceState` is true - add more comments to help clarify behavior - add more E2E tests --- src/components/accordion/accordion.spec.tsx | 66 +++++++++++++++------ src/components/accordion/accordion.tsx | 16 ++++- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/src/components/accordion/accordion.spec.tsx b/src/components/accordion/accordion.spec.tsx index 5a6e5de7ff3..40eb8538c76 100644 --- a/src/components/accordion/accordion.spec.tsx +++ b/src/components/accordion/accordion.spec.tsx @@ -74,30 +74,58 @@ describe('EuiAccordion', () => { expectChildrenIsFocused(); }); - it('does not focus the accordion when programmatically toggled via forceState', () => { - const ControlledComponent = () => { - const [accordionOpen, setAccordionOpen] = useState(false); - return ( - <> - + describe('forceState', () => { + it('does not focus the accordion when `forceState` prevents the accordion from opening', () => { + cy.realMount(); + + cy.contains('Click me to toggle').realClick(); + cy.focused() + .should('not.have.class', 'euiAccordion__childWrapper') + .contains('Click me to toggle'); + }); + + it('does not focus the accordion when programmatically toggled from outside the accordion', () => { + const ControlledComponent = () => { + const [accordionOpen, setAccordionOpen] = useState(false); + return ( + <> + + + + ); + }; + cy.realMount(); + + cy.get('[data-test-subj="toggleForceState"]').realClick(); + cy.focused() + .should('not.have.class', 'euiAccordion__childWrapper') + .should('have.attr', 'data-test-subj', 'toggleForceState'); + }); + + it('attempts to focus the accordion children when `onToggle` controls `forceState`', () => { + const ControlledComponent = () => { + const [accordionOpen, setAccordionOpen] = useState(false); + return ( setAccordionOpen(open)} forceState={accordionOpen ? 'open' : 'closed'} /> - - ); - }; - cy.realMount(); + ); + }; + cy.realMount(); - cy.get('[data-test-subj="toggleForceState"]').realClick(); - cy.focused() - .should('not.have.class', 'euiAccordion__childWrapper') - .should('have.attr', 'data-test-subj', 'toggleForceState'); + cy.contains('Click me to toggle').realClick(); + expectChildrenIsFocused(); + }); }); }); }); diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index 875230a80ca..80572d5babd 100644 --- a/src/components/accordion/accordion.tsx +++ b/src/components/accordion/accordion.tsx @@ -141,7 +141,18 @@ export class EuiAccordionClass extends Component< onToggle = () => { const { forceState } = this.props; if (forceState) { - this.props.onToggle?.(forceState === 'open' ? false : true); + const nextState = !this.isOpen; + this.props.onToggle?.(nextState); + + // If the accordion should theoretically be opened, wait a tick (allows + // consumer state to update) and attempt to focus the child content. + // NOTE: Even if the accordion does not actually open, this is fine - + // the `inert` property on the hidden children will prevent focus + if (nextState === true) { + requestAnimationFrame(() => { + this.accordionChildrenEl?.focus(); + }); + } } else { this.setState( (prevState) => ({ @@ -149,6 +160,9 @@ export class EuiAccordionClass extends Component< }), () => { this.props.onToggle?.(this.state.isOpen); + + // If the accordion is open, programmatically move focus + // from the accordion trigger to the child content if (this.state.isOpen) { this.accordionChildrenEl?.focus(); } From fdecfe28225e2bd033cfd8223f9a14b55753533c Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Wed, 25 Oct 2023 10:17:28 -0700 Subject: [PATCH 4/4] changelog --- upcoming_changelogs/7314.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 upcoming_changelogs/7314.md diff --git a/upcoming_changelogs/7314.md b/upcoming_changelogs/7314.md new file mode 100644 index 00000000000..b66ef1c3ef0 --- /dev/null +++ b/upcoming_changelogs/7314.md @@ -0,0 +1,3 @@ +**Accessibility** + +- `EuiAccordion`s no longer attempt to focus child content when the accordion is externally opened via `forceState`, but will continue to focus expanded content when users click the toggle button.