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.

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
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
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
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
35 changes: 6 additions & 29 deletions src/components/accordion/accordion.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,31 +47,25 @@ 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', () => {
it('maintains focus when the button is clicked', () => {
cy.realMount(<EuiAccordion {...sharedProps} />);
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(<EuiAccordion {...sharedProps} />);
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', () => {
Expand Down Expand Up @@ -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 (
<EuiAccordion
{...sharedProps}
onToggle={(open) => setAccordionOpen(open)}
forceState={accordionOpen ? 'open' : 'closed'}
/>
);
};
cy.realMount(<ControlledComponent />);

cy.contains('Click me to toggle').realClick();
expectChildrenIsFocused();
});
});
});
});
13 changes: 0 additions & 13 deletions src/components/accordion/accordion.test.tsx
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,6 @@ exports[`EuiCollapsibleNavGroup when isCollapsible is true accepts accordion pro
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -321,7 +320,6 @@ exports[`EuiCollapsibleNavGroup when isCollapsible is true will render an accord
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ exports[`EuiCollapsibleNavAccordion renders as a sub item 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -119,7 +118,6 @@ exports[`EuiCollapsibleNavAccordion renders as a top level item 1`] = `
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ exports[`EuiCollapsibleNavItem renders a top level accordion if items exist 1`]
inert=""
role="group"
style="block-size: 0;"
tabindex="-1"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down
Loading