Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiAccordion] Update component logic to retain focus on <button> when clicked #7696

Merged
merged 5 commits into from
Apr 18, 2024
Merged

[EuiAccordion] Update component logic to retain focus on <button> when clicked #7696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0473b4
Removed the logic for moving focus to child container.
1Copenut Apr 17, 2024
03b4edb
Updated unit tests and snapshots.
1Copenut Apr 17, 2024
2a16967
Updated Cypress functional test.
1Copenut Apr 17, 2024
fba7170
Added changelog for PR 7696.
1Copenut Apr 17, 2024
5ff2e60
simplify tests and add comment block with explanation
cee-chen Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/upcoming/7696.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Accessibility**

- Updated `EuiAccordion` to keep focus on accordion trigger instead of moving to content on click/keypress
23 changes: 0 additions & 23 deletions src/components/accordion/__snapshots__/accordion.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ exports[`EuiAccordion behavior closes when clicked twice 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -97,7 +96,6 @@ exports[`EuiAccordion behavior does not open when isDisabled 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -150,7 +148,6 @@ exports[`EuiAccordion behavior opens when clicked once 1`] = `
id="22"
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -203,7 +200,6 @@ exports[`EuiAccordion behavior opens when div is clicked if element is a div 1`]
id="24"
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -259,7 +255,6 @@ exports[`EuiAccordion is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -311,7 +306,6 @@ exports[`EuiAccordion isDisabled is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -346,7 +340,6 @@ exports[`EuiAccordion props arrowDisplay none is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -396,7 +389,6 @@ exports[`EuiAccordion props arrowDisplay right is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -448,7 +440,6 @@ exports[`EuiAccordion props arrowProps is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -502,7 +493,6 @@ exports[`EuiAccordion props buttonContent is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -552,7 +542,6 @@ exports[`EuiAccordion props buttonContentClassName is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -600,7 +589,6 @@ exports[`EuiAccordion props buttonElement is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -652,7 +640,6 @@ exports[`EuiAccordion props buttonProps is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -703,7 +690,6 @@ exports[`EuiAccordion props buttonProps paddingSize l 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -754,7 +740,6 @@ exports[`EuiAccordion props buttonProps paddingSize m 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -805,7 +790,6 @@ exports[`EuiAccordion props buttonProps paddingSize s 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -853,7 +837,6 @@ exports[`EuiAccordion props element is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -910,7 +893,6 @@ exports[`EuiAccordion props extraAction is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -960,7 +942,6 @@ exports[`EuiAccordion props forceState closed is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -1013,7 +994,6 @@ exports[`EuiAccordion props forceState open is rendered 1`] = `
id="17"
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -1066,7 +1046,6 @@ exports[`EuiAccordion props initialIsOpen is rendered 1`] = `
id="12"
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -1129,7 +1108,6 @@ exports[`EuiAccordion props isLoading is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children euiAccordion__children-isLoading emotion-euiAccordion__children-isLoading"
Expand Down Expand Up @@ -1188,7 +1166,6 @@ exports[`EuiAccordion props isLoadingMessage is rendered 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children euiAccordion__children-isLoading emotion-euiAccordion__children-isLoading"
Expand Down
86 changes: 8 additions & 78 deletions src/components/accordion/accordion.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
/// <reference types="cypress-real-events" />
/// <reference types="../../../cypress/support" />

import React, { useState } from 'react';
import React from 'react';
import { EuiAccordion, EuiAccordionProps } from './index';

const sharedProps: EuiAccordionProps = {
Expand Down Expand Up @@ -47,88 +47,18 @@ describe('EuiAccordion', () => {
cy.realMount(<EuiAccordion {...sharedProps} />);
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');
});
});

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', () => {
cy.realMount(<EuiAccordion {...sharedProps} />);
cy.get('.euiAccordion__arrow').realClick();
expectChildrenIsFocused();
});

it('focuses the accordion content when the button is clicked', () => {
cy.realMount(<EuiAccordion {...sharedProps} />);
cy.realPress('Tab');
cy.focused().contains('Click me to toggle').realPress('Enter');
expectChildrenIsFocused();
});

describe('forceState', () => {
it('does not focus the accordion when `forceState` prevents the accordion from opening', () => {
cy.realMount(<EuiAccordion {...sharedProps} forceState="closed" />);

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 (
<>
<button
data-test-subj="toggleForceState"
onClick={() => setAccordionOpen(!accordionOpen)}
>
Control accordion
</button>
<EuiAccordion
{...sharedProps}
forceState={accordionOpen ? 'open' : 'closed'}
/>
</>
);
};
cy.realMount(<ControlledComponent />);

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 (
<EuiAccordion
{...sharedProps}
onToggle={(open) => setAccordionOpen(open)}
forceState={accordionOpen ? 'open' : 'closed'}
/>
);
};
cy.realMount(<ControlledComponent />);

cy.contains('Click me to toggle').realClick();
expectChildrenIsFocused();
});
});
// 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(<EuiAccordion {...sharedProps} />);
cy.get('.euiAccordion__button').realClick();
cy.focused().should('have.class', 'euiAccordion__button');
});
});
13 changes: 0 additions & 13 deletions src/components/accordion/accordion.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,18 +319,5 @@ describe('EuiAccordion', () => {
expect(onToggleHandler).toBeCalled();
expect(onToggleHandler).toBeCalledWith(false);
});

it('moves focus to the content when expanded', () => {
const component = mount(<EuiAccordion id={getId()} />);
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);
});
});
});
23 changes: 0 additions & 23 deletions src/components/accordion/accordion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,40 +152,18 @@ 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) => ({
isOpen: !prevState.isOpen,
}),
() => {
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() {
Expand Down Expand Up @@ -256,7 +234,6 @@ export class EuiAccordionClass extends Component<
isLoading={isLoading}
isLoadingMessage={isLoadingMessage}
isOpen={this.isOpen}
accordionChildrenRef={this.accordionChildrenRef}
>
{children}
</EuiAccordionChildren>
Expand Down
5 changes: 0 additions & 5 deletions src/components/accordion/accordion_children/accordion_children.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import React, {
FunctionComponent,
HTMLAttributes,
Ref,
useCallback,
useMemo,
useState,
Expand All @@ -32,14 +31,12 @@ type _EuiAccordionChildrenProps = HTMLAttributes<HTMLDivElement> &
'role' | 'children' | 'paddingSize' | 'isLoading' | 'isLoadingMessage'
> & {
isOpen: boolean;
accordionChildrenRef: Ref<HTMLDivElement>;
};
export const EuiAccordionChildren: FunctionComponent<
_EuiAccordionChildrenProps
> = ({
role,
children,
accordionChildrenRef,
paddingSize,
isLoading,
isLoadingMessage,
Expand Down Expand Up @@ -90,9 +87,7 @@ export const EuiAccordionChildren: FunctionComponent<
className="euiAccordion__childWrapper"
css={wrapperCssStyles}
style={heightInlineStyle}
ref={accordionChildrenRef}
role={role}
tabIndex={-1}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt safe to remove the tabindex here because the only time it was being called was from our parent component to set focus programmatically. Given that's what we don't want anymore, seemed the right play.

// @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
>
Expand Down
Loading
Loading