[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for `watchedItemProps` #5880

cee-chen · 2022-05-05T23:56:09Z

Summary

I am once again asking you all to look at my memes:

Me fixing a single EuiContextMenu issue: Ah yes I know what the problem is, it's a quick 30 min fix
Me several hours and many E2E tests later: Haha yes, I've basically successfully rewritten the component

I strongly recommend reviewing by commit, although I will also try and lay out the rabbit hole I descended into below:

I thought you fixed this in #5760, what happened?

The fix in #5760 was implemented only for Escape key usage, because I basically forgot that consumers can manually close popovers from within the popover 🤦

So while the above PR is still generally useful for keyboard users and scenarios where focus gets stranded from popovers, this fix applies to EuiContextMenuPanel specifically and should hopefully be the final fix (knock on wood).

OK so why is this bug happening?

Essentially the issue is we need to let our focus trap library do its thing and stop messing up its returnFocus logic by setting focus too early (see this previous comment: #5760 (comment)).

So we detect when EuiContextMenuPanel is in a popover, and then I added a delay when it's in a popover to not run its updateFocus() call until EuiPopover has finished doing its thing. Boom, no more focus hijacking!

Wait but why the huge refactor then

OK so here's the deal. I thought I was basically done after 2 commits (1e3d257 and f6c99b8) and that was it was a pretty clean and straightforward fix, per the last paragraph.

I was testing it on docs site and everything was great and working. Then I wrote E2E tests for it. And by a sheer stroke of dumb luck I wrote my test component using the children prop, and not the items prop (6ef9342). And it kept failing and failing and failing. And I was like what the heck, I know this is working locally.

So then somehow (still not sure how, pretty sure a fever dream?) I realized that focus was still not returning to the popover toggle if EuiContextMenuPanel was using the children API instead of items. Why? Because of these lines right here:

eui/src/components/context_menu/context_menu_panel.tsx

Lines 400 to 403 in a5be54f

    
           // it's not possible (in any good way) to know if `children` has changed, assume they might have 
        
           if (this.props.children != null) { 
        
             return true; 
        
           }

Basically, a context menu with items updates less often and thus hijacks focus less often. children however... doesn't... so what was happening was that our focus trap was returning focus to the popover toggle button, but EuiContextMenuPanel with children was still updating focus and hijacking/stealing it back to the panel as the popover closed, and of course once the panel is removed from the DOM along with the popover, focus gets stranded, yet again.

So what was the fix?

I basically table flipped and removed everything around our shouldComponentUpdate logic. EuiContextMenuPanel now updates like a regular component, removing the need for watchedItemsProps.

To make focus work as before, I then had to:

Entirely change updateFocus() - I renamed it takeInitialFocus() made it only call once on panel init (1fb9b24).
I moved the tabbable logic that populates state.menuItems out of updateFocus() and into its own thing (40dc5c4)
I changed incrementFocusedItemIndex() to focusMenuItem() and made the method do its own .focus()ing instead of relying on updateFocus() to handle it. This restored arrow navigation to as before. (4b397b0)
There's some small other little edge cases I hardened for that are noted in either commit messages or had E2E tests written.

All the commits around this set of changes are prefixed with [!!!]. Phew, I think that's it.

Checklist

~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Updated the Figma library counterpart~~

Checked in Chrome, Safari, Edge, and Firefox
Added or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

- move it closer to updateFocus / componentDidMount for easier context between the other two methods

- move to separate method - create instance var - specify `initialPopover` and add `transitionType` check, as the popover doesn't re-initialize when moving between panels in the same popover, and we don't want this to re-fire unnecessarily - add E2E tests for popover behavior

- this makes it so EuiContextMenu doesn't call `.focus()` too early and hijack the `returnFocus` element that our focus trap dependency sets via `document.activeElement` - see elastic#5760 (comment) + Add Cypress E2E tests for popover close focus return (w/ bonus `initialFocus` regression test)

…t correctly return focus :( - this is because of `shouldComponentUpdate` - the `items` API updates focus less than `children`, so `children` is still updating/hijacking focus after the popover focus trap returns focus to the button

- replace `updateFocus` with `takeInitialFocus`, and do not continue to update/hijack focus once initial focus has been set - this removes the need to restrict how often `EuiContextMenuPanel` updates (which also requires a bunch of tedious `items` diffing that we will no longer need)

…ethod - it shouldn't be tied to the focus call anymore since the focus call no longer occurs after update, and makes more sense as a separate call + updates to logic: - do not run `tabbable` on `children` API since it won't even use the navigation - return early - use `this.backButton` instead for back button focusing, since `children` will no longer have `menuItems` - Check for a valid focusedItem - it's possible for consumers to either pass in bad indices or for `items` to update and the index to no longer exist - Add E2E tests confirming changes & new logic work as expected

- rename `incrementFocusedItemIndex` to `focusMenuItem` and change args to be a bit more human readable - instead of having the previous `updateFocus` handle up/down nav, we can simply call `.focus()` from within this method, and arrow navigation works as before - note `?.focus();` - this is important to keep as users can start mashing up/down before `tabbable` is done running and there are any menu items to focus - no specific E2E tests for this, tests should simply not be failing

kibanamachine · 2022-05-06T00:26:14Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5880/

kibanamachine · 2022-05-06T16:37:03Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5880/

thompsongl · 2022-05-06T18:55:36Z

Going to add @1Copenut as a reviewer for some extra keyboard & SR coverage

cee-chen · 2022-05-06T19:02:00Z

FWIW I thought screen reader experience was kinda medium (e.g. I feel like the back button titles should have an extra hint that it goes back to the previous panel), but it shouldn't have changed / be any worse than it currently is on prod 🤔 Might be nice to add a follow-up ticket for this some day

1Copenut

LGTM! I took your advice @constancecchen and read through the code changes one commit at a time. The comments and rationale for refactoring were very helpful.

Putting the update through its paces with keyboard and VoiceOver navigation yielded a more refined experience, esp. with VO. The focus management now reads exactly what text element has focus. It does not start reading one thing, then stop and read another as focus shifts so rapidly not to be seen, but to be heard.

Many thanks for adding all of the Cypress test cases and a regression short circuit. I think this will yield even bigger improvements for NVDA and JAWS where they use a different accessibility logic paradigm than VO.

cee-chen · 2022-05-09T16:45:13Z

Thanks a million @1Copenut! ❤️

@thompsongl, did you want to review this PR, or should I go ahead and merge with just Trevor's approval?

thompsongl · 2022-05-09T17:10:32Z

I'll review this afternoon. Thanks for the quick turnaround, @1Copenut!

thompsongl

Looking good! I like the approach to "let the focus trap do all the work"

thompsongl · 2022-05-09T18:36:10Z

src/components/context_menu/context_menu_panel.tsx

@@ -128,15 +128,17 @@ export class EuiContextMenuPanel extends Component<Props, State> {
    }
  };

-  incrementFocusedItemIndex = (amount: number) => {
+  focusMenuItem = (direction: 'up' | 'down') => {


thompsongl · 2022-05-09T18:46:02Z

src/components/context_menu/context_menu_panel.tsx

+    const hasEuiContextMenuParent = parent.classList.contains('euiContextMenu');
+
+    // It's possible to use an EuiContextMenuPanel directly in a popover without
+    // an EuiContextMenu, so we need to account for that when searching parent nodes
+    const popoverParent = hasEuiContextMenuParent
+      ? (parent?.parentNode?.parentNode as HTMLElement)
+      : (parent?.parentNode as HTMLElement);
+    if (!popoverParent) return;
+
+    const hasPopoverParent = popoverParent.classList.contains(
+      'euiPopover__panel'
+    );


Could you throw a comment in the files where these class names are created/added about being used as selectors here? To prevent accidental deletion during Emotion conversions and/or up force us to update these to something else if we want to remove the class names.

TBH, I'd love a less DOM-dependent way of figuring out whether an EuiContextMenu/Panel is inside an EuiPopover. Just spitballing, but what if we do something like React.cloneElement on popover children and checking for type === EuiContextMenu || EuiContextMenuPanel, and setting an isInPopover flag if so? We can avoid all these issues with classNames in that case.

Worth trying! Looks like we already do that kind of thing in this file:

eui/src/components/context_menu/context_menu_panel.tsx

Lines 503 to 505 in fcd9fae

return MenuItem.type === EuiContextMenuItem

? cloneElement(MenuItem, cloneProps)

: MenuItem;

I spiked this out for a bit in EuiPopover but it ended up being a bit of a ref nightmare so I gave up (due to either render order or the way popover stores refs, the panel ref is null / isn't available on EuiContextMenuPanel mount). I converted the .euiPopover__panel check to hook onto a data attribute instead instead. I left euiContextMenu alone since we haven't been removing top-level CSS classes as part of our Emotion conversion.

f8e7a5c

TBH if we ever do completely remove CSS classNames from EUI we're going to have to do a lot of grepping so I don't super see the need to add a comment, especially since EuiContextMenu is directly in the same component + E2E tests will immediately fail for this if the class name is removed.

kibanamachine · 2022-05-09T22:33:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5880/

cee-chen · 2022-05-09T23:34:46Z

jenkins test this

kibanamachine · 2022-05-10T00:03:08Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5880/

…Props prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed

* Initial commit for EUI 57.0.0 upgrade * Handle i18n changes * Resolved type errors in DatePicker and Markdown Editor * Resolve test failures in Jest Test Suite #1. Updated multiple snapshots for euiLink and euiTitle as they have been converted to Emotion * Resolved failing tests for Jest Suite #2. Updated snapshots for euiHealth, euiAvatar, euiSpacer, euiTitle, and euiLink as they have recently been converted to Emotion * Resolved failing tests for Jest Suite 3. Updated failing snapshots as EuiSpacer, EuiText, EuiCallout, EuiHorizontalRule, EuiTitle, and EuiLink have been converted to Emotion. Updated the i18n translation snapshots * Upgrade EUI verion to 58.0.0 * Resolved tests failures from Jest Test Suite 4. Updated snapshots as EuiLink, EuiTitle, EuiHorizontalRule, EuiSpace, and EuiCallout have been converted to Emotion * Resolved failing test cases for Jest Test Suite 5. Updated snapshots as EuiLoader has been converted to Emotion * Resolved failing tests in Jest Test Suite 6. Updated snapshots as EuiSpacer, EuiHorizontalRule, Eui Callout, and EuiLink have been converted to Emotion * Resolved type errors for EuiDatePicker component * Resolved type error within EuiContextMenu by removing the watchedItemProps prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed * Resolved type error within EuiContextMenu by removing the watchedItemProps prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed * Resolved type error within EuiContextMenu by removing the watchedItemProps prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed * Resolved type errors by updating the popoverPlacement prop for the EuiDatePicket component with new / valid values. A list of values were deprecated and new values were added in EUI PR #5868 (elastic/eui#5868) * Resolved type error within EuiTabs by removing instances of display: condensed as it is no longer a part of the Amsterdam theme via EUI PR #5868(elastic/eui#5868) * Remove deprecated `display` prop from EuiTabs * Deprecate `.eui-textOverflowWrap` * Deprecate EuiSuggestItem `labelDisplay` prop * [EuiStepsHorizontal] Replace deprecated `isComplete`/`isSelected` with `status` * Update last EuiStepsHorizontal `status` migration - this one was more complex than the previous commit due to existing `status` usage and conditional steps. Some amount of logic was simplified via `completedStep` * Resolved type error within EuiTabs by removing instances of display: condensed as it is no longer a part of the Amsterdam theme via EUI PR #5868(elastic/eui#5868) * Resolved failing test cases in Jest Test Suite 5. Updated snapshots as EuiTitle has been converted to Emotion * Resovlved failing test cases in Jest Test Suite 4. Updated snapshots as EuiTitle and EuiSpacer have been converted to Emotion. Resolved failing tests for EuiLink click simlulations by esuring the test is referencing the correct element. * Resolved failing test cases in Jest Test Suite 3. Updated snapshots as EuiLink, EuiSpacr, and EuiTItle have been converted to Emotion. Updated various test cases to ensure that the references to EuiLink are correct * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Resolved failing test cases in Jest Test Suite 1. Updated snapshots as EUI text utilities have been converted to Emotion. Updated referenes to EuiLink to ensure test are simulating clicks on the correct elements * Resolved failing test for Jet Test Suite 2. Updated required snapshots. Updated references to EuiLink to ensure that tests are simulating clicks on the correct elements * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Resolved failing test cases for Jest Test Suite 5. Updated references to EuiLink to ensure tests are targeting the correct element * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * Resolved failing test cases across the Jest Test Suites. Updated required snaphots for components recently converted to Emotion. Updated test cases to ensure that tests targeting EuiLink are using the correct element. * Resolved failing tests from multiple Jest test suites. Updated snapshots for components that have recently been converted to Emotion. Updated tests that reference the EuiLink component to ensure the correct element is being targeted * Updated the getEuiStepsHorizontal function. Previously, this function used the .euiStepHorizontal-isSelected class (now deprecated) to determine which step was current. The function has been updated to use the status prop. * Updated Jest integration test snapshots to account for the recent conversion of EuiLoader to Emotion * Resolved failing tests in Jest suites 2 and 4. Updated required snapshots and references for tests using EuiLink * Removed a console statement. Extracted a nested turnary operation into its own function. * Rollback new turnary function and replace it with a simple if/else * Rollback new turnary function and replace it with a simple if/else * Rollback new turnary function and replace it with a simple if/else * Rollback new turnary function and replace it with a simple if/else * [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' * Resolved failing test cases in Jest Suites 3 and 5 by updating required snapshots * revert doc_viewer_source test and snapshot changes * Take care of merge conflict in license_checker: * Reverted .render() change for analytics_no_data_page.component.test.tsx. Restored snapshot * Reverted .render() change for analytics_no_data_page.component.test.tsx. Restored snapshot Co-authored-by: Constance Chen <constance.chen@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>

cee-chen added 7 commits May 5, 2022 10:15

[misc] move componentDidUpdate fn

a5be54f

- move it closer to updateFocus / componentDidMount for easier context between the other two methods

cee-chen added the breaking change label May 5, 2022

cee-chen requested a review from thompsongl May 5, 2022 23:56

changelog

1721abf

cee-chen linked an issue May 6, 2022 that may be closed by this pull request

[EuiContextMenu] [A11y] EuiContextMenu does not place the focus correctly after submitting the action #5869

Closed

thompsongl requested a review from 1Copenut May 6, 2022 18:55

1Copenut approved these changes May 6, 2022

View reviewed changes

thompsongl reviewed May 9, 2022

View reviewed changes

cee-chen added 2 commits May 9, 2022 13:47

Merge branch 'main' into contextmenu/focus-fix

6a7a464

[PR feedback] Prefer not to hook into className for popover panel

f8e7a5c

cee-chen requested a review from thompsongl May 9, 2022 22:02

thompsongl approved these changes May 9, 2022

View reviewed changes

cee-chen enabled auto-merge (squash) May 9, 2022 22:12

cee-chen merged commit 723d0ec into elastic:main May 10, 2022

cee-chen deleted the contextmenu/focus-fix branch May 10, 2022 00:29

breehall mentioned this pull request May 18, 2022

Upgrade EUI to v58.0.0 elastic/kibana#132257

Merged

breehall added a commit to elastic/kibana that referenced this pull request May 18, 2022

Resolved type error within EuiContextMenu by removing the watchedItem…

16c308c

…Props prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed

breehall added a commit to elastic/kibana that referenced this pull request May 18, 2022

Resolved type error within EuiContextMenu by removing the watchedItem…

bd52b5b

…Props prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed

breehall added a commit to elastic/kibana that referenced this pull request May 18, 2022

Resolved type error within EuiContextMenu by removing the watchedItem…

f8a3e2e

…Props prop. It was recently deprecated in EUI PR# 5880 (elastic/eui#5880) as is no longer needed

cee-chen mentioned this pull request May 18, 2022

Upgrade EUI to v55.1.3 elastic/kibana#132451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for `watchedItemProps` #5880

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for `watchedItemProps` #5880

cee-chen commented May 5, 2022 •

edited

Loading

kibanamachine commented May 6, 2022

kibanamachine commented May 6, 2022

thompsongl commented May 6, 2022

cee-chen commented May 6, 2022 •

edited

Loading

1Copenut left a comment

cee-chen commented May 9, 2022

thompsongl commented May 9, 2022

thompsongl left a comment

thompsongl May 9, 2022

thompsongl May 9, 2022

cee-chen May 9, 2022

thompsongl May 9, 2022

cee-chen May 9, 2022

kibanamachine commented May 9, 2022

cee-chen commented May 9, 2022

kibanamachine commented May 10, 2022

	// it's not possible (in any good way) to know if `children` has changed, assume they might have
	if (this.props.children != null) {
	return true;
	}

	return MenuItem.type === EuiContextMenuItem
	? cloneElement(MenuItem, cloneProps)
	: MenuItem;

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for watchedItemProps #5880

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for watchedItemProps #5880

Conversation

cee-chen commented May 5, 2022 • edited Loading

Summary

I thought you fixed this in #5760, what happened?

OK so why is this bug happening?

Wait but why the huge refactor then

So what was the fix?

Checklist

kibanamachine commented May 6, 2022

kibanamachine commented May 6, 2022

thompsongl commented May 6, 2022

cee-chen commented May 6, 2022 • edited Loading

1Copenut left a comment

Choose a reason for hiding this comment

cee-chen commented May 9, 2022

thompsongl commented May 9, 2022

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl May 9, 2022

Choose a reason for hiding this comment

thompsongl May 9, 2022

Choose a reason for hiding this comment

cee-chen May 9, 2022

Choose a reason for hiding this comment

thompsongl May 9, 2022

Choose a reason for hiding this comment

cee-chen May 9, 2022

Choose a reason for hiding this comment

kibanamachine commented May 9, 2022

cee-chen commented May 9, 2022

kibanamachine commented May 10, 2022

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for `watchedItemProps` #5880

[EuiContextMenuPanel] Fix popover toggle focus restoration issues and remove need for `watchedItemProps` #5880

cee-chen commented May 5, 2022 •

edited

Loading

cee-chen commented May 6, 2022 •

edited

Loading