From c0473b419a1a89c4bcff6d19865e82e4aa646ac9 Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 17 Apr 2024 16:32:45 -0500 Subject: [PATCH 1/5] Removed the logic for moving focus to child container. --- src/components/accordion/accordion.tsx | 23 ------------------- .../accordion_children/accordion_children.tsx | 5 ---- 2 files changed, 28 deletions(-) diff --git a/src/components/accordion/accordion.tsx b/src/components/accordion/accordion.tsx index 640576bc606..3ceb0af6bd4 100644 --- a/src/components/accordion/accordion.tsx +++ b/src/components/accordion/accordion.tsx @@ -152,16 +152,6 @@ export class EuiAccordionClass extends Component< if (forceState) { 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) => ({ @@ -169,23 +159,11 @@ 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(); - } } ); } }; - // 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() { @@ -256,7 +234,6 @@ 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 62a2d7ffad8..d119d9b725b 100644 --- a/src/components/accordion/accordion_children/accordion_children.tsx +++ b/src/components/accordion/accordion_children/accordion_children.tsx @@ -9,7 +9,6 @@ import React, { FunctionComponent, HTMLAttributes, - Ref, useCallback, useMemo, useState, @@ -32,14 +31,12 @@ type _EuiAccordionChildrenProps = HTMLAttributes & 'role' | 'children' | 'paddingSize' | 'isLoading' | 'isLoadingMessage' > & { isOpen: boolean; - accordionChildrenRef: Ref; }; export const EuiAccordionChildren: FunctionComponent< _EuiAccordionChildrenProps > = ({ role, children, - accordionChildrenRef, paddingSize, isLoading, isLoadingMessage, @@ -90,9 +87,7 @@ export const EuiAccordionChildren: FunctionComponent< className="euiAccordion__childWrapper" css={wrapperCssStyles} style={heightInlineStyle} - ref={accordionChildrenRef} role={role} - tabIndex={-1} // @ts-expect-error - inert property not yet available in React TS defs. TODO: Remove this once https://github.com/DefinitelyTyped/DefinitelyTyped/pull/60822 is merged inert={!isOpen ? '' : undefined} // Can't pass a boolean currently, Jest throws errors > From 03b4edb1d70b16c6ae5843fea5bf7efee436747e Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 17 Apr 2024 16:34:29 -0500 Subject: [PATCH 2/5] Updated unit tests and snapshots. --- .../__snapshots__/accordion.test.tsx.snap | 23 ------------------- src/components/accordion/accordion.test.tsx | 13 ----------- .../collapsible_nav_group.test.tsx.snap | 2 -- .../collapsible_nav_accordion.test.tsx.snap | 2 -- .../collapsible_nav_item.test.tsx.snap | 1 - 5 files changed, 41 deletions(-) diff --git a/src/components/accordion/__snapshots__/accordion.test.tsx.snap b/src/components/accordion/__snapshots__/accordion.test.tsx.snap index 460bc19f21f..94a560bdfc6 100644 --- a/src/components/accordion/__snapshots__/accordion.test.tsx.snap +++ b/src/components/accordion/__snapshots__/accordion.test.tsx.snap @@ -41,7 +41,6 @@ exports[`EuiAccordion behavior closes when clicked twice 1`] = ` inert="" role="group" style="block-size: 0;" - tabindex="-1" >
{ expect(onToggleHandler).toBeCalled(); expect(onToggleHandler).toBeCalledWith(false); }); - - it('moves focus to the content when expanded', () => { - const component = mount(); - const childWrapper = component.find('div[role="group"]').getDOMNode(); - - expect(childWrapper).not.toBeFalsy(); - expect(childWrapper).not.toBe(document.activeElement); - - // click the button - component.find('button').at(0).simulate('click'); - - expect(childWrapper).toBe(document.activeElement); - }); }); }); diff --git a/src/components/collapsible_nav/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap b/src/components/collapsible_nav/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap index d60f9fd81de..7f6277f2a14 100644 --- a/src/components/collapsible_nav/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap +++ b/src/components/collapsible_nav/collapsible_nav_group/__snapshots__/collapsible_nav_group.test.tsx.snap @@ -256,7 +256,6 @@ exports[`EuiCollapsibleNavGroup when isCollapsible is true accepts accordion pro inert="" role="group" style="block-size: 0;" - tabindex="-1" >
Date: Wed, 17 Apr 2024 16:34:44 -0500 Subject: [PATCH 3/5] Updated Cypress functional test. --- src/components/accordion/accordion.spec.tsx | 35 ++++----------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/components/accordion/accordion.spec.tsx b/src/components/accordion/accordion.spec.tsx index 3c03d3e76a5..66f73a47294 100644 --- a/src/components/accordion/accordion.spec.tsx +++ b/src/components/accordion/accordion.spec.tsx @@ -47,7 +47,6 @@ describe('EuiAccordion', () => { cy.realMount(); cy.realPress('Tab'); cy.realPress('Enter'); - cy.realPress(['Shift', 'Tab']); cy.focused().invoke('attr', 'aria-expanded').should('equal', 'true'); cy.realPress('Space'); cy.focused().invoke('attr', 'aria-expanded').should('equal', 'false'); @@ -55,23 +54,18 @@ describe('EuiAccordion', () => { }); describe('focus management', () => { - const expectChildrenIsFocused = () => { - cy.focused() - .should('have.class', 'euiAccordion__childWrapper') - .should('have.attr', 'tabindex', '-1'); - }; - - it('focuses the accordion content when the arrow is clicked', () => { + it('maintains focus when the button is clicked', () => { cy.realMount(); - cy.get('.euiAccordion__arrow').realClick(); - expectChildrenIsFocused(); + cy.get('.euiAccordion__button').realClick(); + cy.focused().should('have.class', 'euiAccordion__button'); }); - it('focuses the accordion content when the button is clicked', () => { + it('maintains focus when the button is pressed', () => { cy.realMount(); cy.realPress('Tab'); + cy.focused().should('have.class', 'euiAccordion__button'); cy.focused().contains('Click me to toggle').realPress('Enter'); - expectChildrenIsFocused(); + cy.focused().should('have.class', 'euiAccordion__button'); }); describe('forceState', () => { @@ -112,23 +106,6 @@ describe('EuiAccordion', () => { .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.contains('Click me to toggle').realClick(); - expectChildrenIsFocused(); - }); }); }); }); From fba7170c8279a34ff7822f5b24034c1c4f78a1f6 Mon Sep 17 00:00:00 2001 From: 1Copenut Date: Wed, 17 Apr 2024 16:47:28 -0500 Subject: [PATCH 4/5] Added changelog for PR 7696. --- changelogs/upcoming/7696.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelogs/upcoming/7696.md diff --git a/changelogs/upcoming/7696.md b/changelogs/upcoming/7696.md new file mode 100644 index 00000000000..4daea0fa436 --- /dev/null +++ b/changelogs/upcoming/7696.md @@ -0,0 +1,3 @@ +**Accessibility** + +- Updated `EuiAccordion` to keep focus on accordion trigger instead of moving to content on click/keypress From 5ff2e60ec26e3a1d1b581f299c35cb9e38eb4e4e Mon Sep 17 00:00:00 2001 From: Cee Chen Date: Thu, 18 Apr 2024 14:22:41 -0700 Subject: [PATCH 5/5] simplify tests and add comment block with explanation --- src/components/accordion/accordion.spec.tsx | 63 +++------------------ 1 file changed, 8 insertions(+), 55 deletions(-) diff --git a/src/components/accordion/accordion.spec.tsx b/src/components/accordion/accordion.spec.tsx index 66f73a47294..01593766745 100644 --- a/src/components/accordion/accordion.spec.tsx +++ b/src/components/accordion/accordion.spec.tsx @@ -10,7 +10,7 @@ /// /// -import React, { useState } from 'react'; +import React from 'react'; import { EuiAccordion, EuiAccordionProps } from './index'; const sharedProps: EuiAccordionProps = { @@ -53,59 +53,12 @@ describe('EuiAccordion', () => { }); }); - describe('focus management', () => { - it('maintains focus when the button is clicked', () => { - cy.realMount(); - cy.get('.euiAccordion__button').realClick(); - cy.focused().should('have.class', 'euiAccordion__button'); - }); - - it('maintains focus when the button is pressed', () => { - cy.realMount(); - cy.realPress('Tab'); - cy.focused().should('have.class', 'euiAccordion__button'); - cy.focused().contains('Click me to toggle').realPress('Enter'); - cy.focused().should('have.class', 'euiAccordion__button'); - }); - - 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() is flaky here and doesn't always return an element, so use document.activeElement instead - cy.then(() => { - expect(document.activeElement).not.to.have.class( - 'euiAccordion__childWrapper' - ); - }); - }); - - 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'); - }); - }); + // Note: we used to move focus automatically to the accordion contents on open/ + // toggle button click, but we no longer do so after receiving a11y audit feedback + // that the change of context was confusing/not helpful + it('maintains focus on the toggle button when opened', () => { + cy.realMount(); + cy.get('.euiAccordion__button').realClick(); + cy.focused().should('have.class', 'euiAccordion__button'); }); });