[EuiAccordion] Fix focus behavior on user interaction open/close vs `forceState` open/close #7314

cee-chen · 2023-10-25T17:09:28Z

Summary

I (partially) regressed this in #7161 when I converted EuiAccordion's constituent parts into sub-functional components. Specifically, this logic:

eui/src/components/accordion/accordion_children/accordion_children.tsx

Lines 86 to 92 in fb7c397

    
             /** 
        
              * 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]);

I thought was more React-y rather than calling a declarative .focus() when the trigger button was clicked. However, I now realize that the click behavior is necessary to determine user open vs programmatic open for predictable focus behavior. It does not necessarily make any sense for focus to jump to the accordion just because it was opened on the page by another event (e.g. async loading) - but it always makes sense for focus to move if the user made an action on the trigger button.

The new behavior in this PR only moves focus onto the child content on user click of the toggle button/arrow. However, it works for both controlled (forceState) and uncontrolled accordions.

Previous behavior (v88.x)	Production behavior (v89.x)	New behavior (Staging)

Notice that focus is never moved to the child content	Notice that focus is always moved to the child content, jumping from the external switch/toggle to the content	Notice that focus is now only moved to the child content when the accordion toggle is clicked, and is not moved to the child content when the external switch/toggle is used

QA

Go to https://eui.elastic.co/pr_7314/#/layout/accordion#controlling-toggled-state
Confirm that when toggling the Open/Closed button group control, focus is not moved away from the control to the accordion
Confirm that when toggling the accordion arrow/button, focus is moved into the accordion children on open
Smoke testing
- Confirm that all other uncontrolled accordions on the page still focus into the accordion children on open

General checklist

Browser QA
- Checked in Chrome, Safari, Edge, and Firefox
- Checked for accessibility including keyboard-only and screenreader modes
  ~~- [ ] Checked in both light and dark modes~~
  - [ ] Checked in mobile
Docs site QA - N/A
Code quality checklist
- Added or updated ~~jest and~~ cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

…rceState` is true - add more comments to help clarify behavior - add more E2E tests

cee-chen · 2023-10-25T18:02:36Z

@1Copenut Your keen accessibility eye here would be much appreciated!

kibanamachine · 2023-10-25T18:31:47Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7314/
Storybook: https://eui.elastic.co/pr_7314/storybook

elasticmachine · 2023-10-25T18:31:52Z

💚 Build Succeeded

Buildkite Build
Commit: fdecfe2

1Copenut

👍 The controlled accordion behaved perfectly in all three screen readers and keyboard navigation had zero regression. No axe violations. This one's ready to ship.

1Copenut · 2023-10-25T21:11:33Z

src/components/accordion/accordion.spec.tsx

+          .should('not.have.class', 'euiAccordion__childWrapper')
+          .contains('Click me to toggle');


I like this double assertion for certifying the forceState="closed" prevents focus being moved.

cee-chen · 2023-10-26T02:03:24Z

Woohoo! Thanks a million Trevor!

`v89.1.0`⏩`v90.0.0` The majority of changes in this PR come from: - **EuiContextMenu** being converted to Emotion (elastic/eui#7312). If your usage of `EuiContextMenu` was significantly affected, we recommend pulling down this PR and QAing it locally. - `defaultProps` being removed from some very widespread components, particularly **EuiButton**, in anticipation of React's upcoming deprecation. (elastic/eui@b7dc9b4) **NOTE**: This only affected Enzyme snapshots, and did not affect production behavior. [Commits](https://github.com/elastic/kibana/pull/170179/commits) have been broken up by component changes as well as types of changes. --- ## [`90.0.0`](https://github.com/elastic/eui/tree/v90.0.0) - Updated the `eventColor` prop on `EuiCommentEvent` to apply the color to the entire comment header. ([#7288](elastic/eui#7288)) - Updated `EuiBasicTable` and `EuiInMemoryTable` to support a new controlled selection API: `selection.selected` ([#7321](elastic/eui#7321)) **Bug fixes** - Fixed controlled `EuiFieldNumbers` not correctly updating native validity state ([#7291](elastic/eui#7291)) - Fixed `EuiListGroupItem` to pass `style` props to the wrapping `<li>` element alongside `className` and `css`. All other props will be passed to the underlying content. ([#7298](elastic/eui#7298)) - Fixed `EuiListGroupItem`'s non-transitioned transform on hover/focus ([#7298](elastic/eui#7298)) - Fixed `EuiDataGrid`s with `gridStyle.stripes` sometimes showing buggy row striping after being sorted ([#7301](elastic/eui#7301)) - Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to not conflict with `gridStyle.stripes` if dynamically updated ([#7301](elastic/eui#7301)) - Fixed `EuiDataGrid`'s `gridStyle.rowClasses` API to support multiple space-separated classes ([#7301](elastic/eui#7301)) - Fixed `EuiInputPopover` not calling `onPanelResize` callback prop ([#7305](elastic/eui#7305)) - Fixed `EuiDualRange` incorrectly positioning highlights when rendered with `showInput="inputWithPopover"` ([#7305](elastic/eui#7305)) - Fixed `EuiTabs` incorrectly wrapping text when it should instead either scroll or truncate ([#7309](elastic/eui#7309)) - `EuiContextMenu` now renders text colors correctly when used within an `EuiBottomBar` ([#7312](elastic/eui#7312)) - Fixed the width of `EuiSuperDatePicker`'s Absolute date picker ([#7313](elastic/eui#7313)) - Fixed `EuiDataGrid` cells visually cutting off overflowing content a little too quickly ([#7320](elastic/eui#7320)) **Deprecations** - Deprecated `EuiBasicTable` and `EuiInMemoryTable`'s ref `setSelection` API. Use the new `selection.selected` API instead. ([#7321](elastic/eui#7321)) **Breaking changes** - Removed `EuiPageTemplate_Deprecated`, `EuiPageSideBar_Deprecated`, and `EuiPageContent*_Deprecated` ([#7265](elastic/eui#7265)) - Removed the `ghost` color option from `EuiButton`, `EuiButtonEmpty`, and `EuiButtonIcon`. Use an `<EuiThemeProvider colorMode="dark">` wrapper and `color="text"` instead. ([#7296](elastic/eui#7296)) **Dependency updates** - Updated `refractor` to v3.6.0 ([#7127](elastic/eui#7127)) - Updated `rehype-raw` to v5.1.0 ([#7127](elastic/eui#7127)) - Updated `vfile` to v4.2.1 ([#7127](elastic/eui#7127)) **Accessibility** - `EuiContextMenu` now correctly respects reduced motion preferences ([#7312](elastic/eui#7312)) - `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. ([#7314](elastic/eui#7314)) **CSS-in-JS conversions** - Converted `EuiContextMenu`, `EuiContextMenuPanel`, and `EuiContextMenuItem` to Emotion; Removed `$euiContextMenuWidth` ([#7312](elastic/eui#7312))

cee-chen added 3 commits October 25, 2023 10:04

Fix accordion open focus behavior to only occur on user click

56fe8eb

Rewrite Accordion E2E tests to be more focused and cover new focus logic

b3ed776

Attempt to preserve focus behavior on user click/toggle even when `fo…

611c0ce

…rceState` is true - add more comments to help clarify behavior - add more E2E tests

cee-chen force-pushed the accordion/focus-fix branch from 2317adc to cf9a1b0 Compare October 25, 2023 17:44

cee-chen marked this pull request as ready for review October 25, 2023 18:02

cee-chen requested a review from a team as a code owner October 25, 2023 18:02

cee-chen requested a review from 1Copenut October 25, 2023 18:02

changelog

fdecfe2

cee-chen force-pushed the accordion/focus-fix branch from cf9a1b0 to fdecfe2 Compare October 25, 2023 18:03

cee-chen mentioned this pull request Oct 25, 2023

[Serverless/SideNav] Initial state of selected nav items should not be wrapped in an "active" area elastic/kibana#167330

Closed

1Copenut approved these changes Oct 26, 2023

View reviewed changes

cee-chen merged commit ece5046 into elastic:main Oct 26, 2023
7 checks passed

cee-chen deleted the accordion/focus-fix branch October 26, 2023 02:03

sebelga mentioned this pull request Oct 26, 2023

[Serverless nav] Accordion auto-expand state + polish work elastic/kibana#169651

Merged

cee-chen mentioned this pull request Oct 30, 2023

Upgrade EUI to v90.0.0 elastic/kibana#170179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiAccordion] Fix focus behavior on user interaction open/close vs `forceState` open/close #7314

[EuiAccordion] Fix focus behavior on user interaction open/close vs `forceState` open/close #7314

cee-chen commented Oct 25, 2023 •

edited by 1Copenut

Loading

cee-chen commented Oct 25, 2023

kibanamachine commented Oct 25, 2023

elasticmachine commented Oct 25, 2023

1Copenut left a comment

1Copenut Oct 25, 2023

cee-chen commented Oct 26, 2023

	/**
	* 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]);

		.should('not.have.class', 'euiAccordion__childWrapper')
		.contains('Click me to toggle');

[EuiAccordion] Fix focus behavior on user interaction open/close vs forceState open/close #7314

[EuiAccordion] Fix focus behavior on user interaction open/close vs forceState open/close #7314

Conversation

cee-chen commented Oct 25, 2023 • edited by 1Copenut Loading

Summary

QA

General checklist

cee-chen commented Oct 25, 2023

kibanamachine commented Oct 25, 2023

elasticmachine commented Oct 25, 2023

💚 Build Succeeded

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Oct 25, 2023

Choose a reason for hiding this comment

cee-chen commented Oct 26, 2023

[EuiAccordion] Fix focus behavior on user interaction open/close vs `forceState` open/close #7314

[EuiAccordion] Fix focus behavior on user interaction open/close vs `forceState` open/close #7314

cee-chen commented Oct 25, 2023 •

edited by 1Copenut

Loading