From 861baa1a6e68ff8ceba6811af9800e61e719a4aa Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Thu, 10 Mar 2022 13:56:41 -0700 Subject: [PATCH 1/7] red/green cypress test for context menu tab behaviour --- .../context_menu/context_menu_panel.spec.tsx | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/components/context_menu/context_menu_panel.spec.tsx b/src/components/context_menu/context_menu_panel.spec.tsx index 90f34bd6da1..3f4a6dc45b2 100644 --- a/src/components/context_menu/context_menu_panel.spec.tsx +++ b/src/components/context_menu/context_menu_panel.spec.tsx @@ -116,6 +116,32 @@ describe('EuiContextMenuPanel', () => { }); }); }); + + describe('tab key', () => { + beforeEach(() => { + cy.mount(); + }); + + it('tab key focuses the first menu item', () => { + cy.focused().should('have.attr', 'class', 'euiContextMenuPanel'); + cy.realPress('Tab'); + cy.focused().should('have.attr', 'data-test-subj', 'itemA'); + }); + + it('subsequently, tab key focuses the next menu item', () => { + cy.focused().should('have.attr', 'class', 'euiContextMenuPanel'); + cy.repeatRealPress('Tab'); + cy.focused().should('have.attr', 'data-test-subj', 'itemB'); + }); + + it('shift+tab key focuses the previous menu item', () => { + cy.focused().should('have.attr', 'class', 'euiContextMenuPanel'); + cy.repeatRealPress('Tab'); + cy.focused().should('have.attr', 'data-test-subj', 'itemB'); + cy.realPress(['Shift', 'Tab']); + cy.focused().should('have.attr', 'data-test-subj', 'itemA'); + }); + }); }); describe('Automated accessibility check', () => { From ef2d0895390e6733f20c6867e91f3001d907481c Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 14 Mar 2022 13:16:41 -0700 Subject: [PATCH 2/7] Fix EuiContextMenu tabbing issue --- .../context_menu/context_menu_panel.tsx | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index bea56afe95d..a752817d3c3 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -161,17 +161,20 @@ export class EuiContextMenuPanel extends Component { if (this.props.items && this.props.items.length) { switch (event.key) { case cascadingMenuKeys.TAB: - // We need to sync up with the user if s/he is tabbing through the items. - const focusedItemIndex = this.state.menuItems.indexOf( - document.activeElement as HTMLElement - ); - - this.setState({ - focusedItemIndex: - focusedItemIndex >= 0 && - focusedItemIndex < this.state.menuItems.length - ? focusedItemIndex - : undefined, + setTimeout(() => { + // NOTE: document.activeElement is stale if not wrapped in a setTimeout + const focusedItemIndex = this.state.menuItems.indexOf( + document.activeElement as HTMLElement + ); + + // We need to sync up with the user if s/he is tabbing through the items. + this.setState({ + focusedItemIndex: + focusedItemIndex >= 0 && + focusedItemIndex < this.state.menuItems.length + ? focusedItemIndex + : undefined, + }); }); break; From 8f37c0d737fbb80e1819a109d257930031479e05 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 14 Mar 2022 13:30:16 -0700 Subject: [PATCH 3/7] Add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d75c6b1491..94dd51f966b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ - Updated `testenv` mock for `EuiIcon` to render `aria-label` as text ([#5709](https://github.com/elastic/eui/pull/5709)) +**Bug fixes** + +- Fixed `EuiContextMenu` requiring two tab keypresses to advance to the next focusable menu item ([#5719](https://github.com/elastic/eui/pull/5719)) + **Breaking changes** - Removed Legacy theme including compiled CSS ([#5688](https://github.com/elastic/eui/pull/5688)) From e5c172dae6618936704864ab7d61a64c2cf46ffe Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Mon, 14 Mar 2022 13:37:02 -0700 Subject: [PATCH 4/7] slight comment tweak - removing user gender - removing 'if' statement - since I moved the comment location, it makes less sense --- src/components/context_menu/context_menu_panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index a752817d3c3..5be0007dad8 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -167,7 +167,7 @@ export class EuiContextMenuPanel extends Component { document.activeElement as HTMLElement ); - // We need to sync up with the user if s/he is tabbing through the items. + // We need to sync our internal state with the user tabbing through items this.setState({ focusedItemIndex: focusedItemIndex >= 0 && From 677b98943dd9a71f72a286a5bc21199c3ff908a9 Mon Sep 17 00:00:00 2001 From: Constance Date: Mon, 14 Mar 2022 13:47:49 -0700 Subject: [PATCH 5/7] [PR feedback] requestAnimationFrame Co-authored-by: Chandler Prall --- src/components/context_menu/context_menu_panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index 5be0007dad8..3d162d1d719 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -161,7 +161,7 @@ export class EuiContextMenuPanel extends Component { if (this.props.items && this.props.items.length) { switch (event.key) { case cascadingMenuKeys.TAB: - setTimeout(() => { + requestAnimationFrame(() => { // NOTE: document.activeElement is stale if not wrapped in a setTimeout const focusedItemIndex = this.state.menuItems.indexOf( document.activeElement as HTMLElement From 3ccc16e358d7ee5f644b739263a888f011436ea0 Mon Sep 17 00:00:00 2001 From: Constance Date: Mon, 14 Mar 2022 14:40:18 -0700 Subject: [PATCH 6/7] Update comment --- src/components/context_menu/context_menu_panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/context_menu/context_menu_panel.tsx b/src/components/context_menu/context_menu_panel.tsx index 3d162d1d719..3201acc8808 100644 --- a/src/components/context_menu/context_menu_panel.tsx +++ b/src/components/context_menu/context_menu_panel.tsx @@ -162,7 +162,7 @@ export class EuiContextMenuPanel extends Component { switch (event.key) { case cascadingMenuKeys.TAB: requestAnimationFrame(() => { - // NOTE: document.activeElement is stale if not wrapped in a setTimeout + // NOTE: document.activeElement is stale if not wrapped in requestAnimationFrame const focusedItemIndex = this.state.menuItems.indexOf( document.activeElement as HTMLElement ); From bb78489ab634eedde9ec7730b44b456c07a3a2d5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 17 Mar 2022 13:02:03 -0700 Subject: [PATCH 7/7] changelog --- CHANGELOG.md | 4 ---- upcoming_changelogs/5719.md | 3 +++ 2 files changed, 3 insertions(+), 4 deletions(-) create mode 100644 upcoming_changelogs/5719.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 258d09d1d3a..48313c4b28b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,10 +5,6 @@ - Added `compressed` prop to `EuiFilterGroup` and reduced the size of the `EuiFilterButton` notification badge ([#5717](https://github.com/elastic/eui/pull/5717)) - Increased contrast of `EuiSelectableTemplateSitewide` input text when in dark header ([#5724](https://github.com/elastic/eui/pull/5724)) -**Bug fixes** - -- Fixed `EuiContextMenu` requiring two tab keypresses to advance to the next focusable menu item ([#5719](https://github.com/elastic/eui/pull/5719)) - **Breaking changes** - Removed Legacy theme including compiled CSS ([#5688](https://github.com/elastic/eui/pull/5688)) diff --git a/upcoming_changelogs/5719.md b/upcoming_changelogs/5719.md new file mode 100644 index 00000000000..6ad206c950f --- /dev/null +++ b/upcoming_changelogs/5719.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed `EuiContextMenu` requiring two tab keypresses to advance to the next focusable menu item