fix(tool_tip): stop propagation on escape key down

[weronikaolejniczak]

Summary

When we have EuiIconTip (consequently, EuiTooltip) within EuiFlyout and with the tooltip open we click "Escape" key, the flyout gets closed. The reason is, the escape keydown event is propagated from the tooltip and captured by the flyout as its parent. Simple event.stopPropagation() inside EuiTooltip seemed to do the trick.

closes #8130

QA

Test path:

1. Run Storybook locally: yarn storybook.
2. Find the component EuiFlyout.
3. Close the flyout with the escape key.
4. Change the EuiFlyoutBody to contain a EuiToolTip as children.
5. Focus the tooltip and close it with the Escape key. The flyout should not close, the tooltip - yes.
6. With the tooltip closed, click Escape key again to close the flyout.

Screen.Recording.2024-11-27.at.12.43.26.mov

Manual QA of other focus-trap elements

EuiModal

Screen.Recording.2024-11-27.at.12.37.48.mov

EuiPopover

Screen.Recording.2024-11-27.at.12.42.35.mov

EuiDataGrid fullscreen

Screen.Recording.2024-11-27.at.12.44.14.mov

General checklist

• Browser QA
  • Checked in both light and dark modes (not applicable)
  • Checked in mobile (not applicable)
  • Checked in Chrome, Safari, Edge, and Firefox
  • Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA
  • Added documentation (not applicable)
  • Props have proper autodocs (using @default if default values are missing) and playground toggles (not applicable)
  • Checked Code Sandbox works for any docs examples (not applicable)
• Code quality checklist
  • Added or updated jest and cypress tests
  • Updated visual regression tests (not applicable)
• 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
  • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

[cee-chen]

This LGTM (awesome work with the E2E test!) - but I'll defer to @mgadewoll for any final comments since I believe she solved a similar bug/issue recently. My only other thought is we might also want to either QA or write a test for the following other focus trap components:

• modals
• popovers
• various fullscreen modes (EuiDataGrid fullscreen, EuiImage fullscreen, EuiCodeBlock fullscreen(?) - not sure that last one can even have a tooltip)

[weronikaolejniczak]

Thanks for the great suggestion, Cee! 🌈 I focused so much on fixing that one test case I forgot about the other overlay elements that might suffer the same issue.

I updated the description with the screen recordings from the manual QA. EuiImage and EuiCodeBlock don't seem to allow for additional children in the fullscreen mode.

One doubt I have - is the stopPropagation in the tooltip the only point-of-failure for all these cases? If so, is it okay that we're adding (rather) expensive Cypress tests for each affected component? We could switch to RTL but then we need to remember it works with a dumb-downed DOM implementation, maybe not the best fit for asserting keyboard navigation and overlay elements.

If we agree to add the automated tests after all, I'll work on that first thing Monday!

[cee-chen]

I'm fine with skipping the automated Cypress tests for each focus trap use case - flyouts are likely our biggest use-case in any case. EuiDataGrid might be the second most likely use-case in production. I'm also fine with manual QA only for the cases I noted, I just want to make sure someone tested it :) It might also be nice for future devs to add an inline comment next to the flyout Cypress test noting which other edge cases to manually QA.

[mgadewoll]

One thing: This means the Escape key will never propagate. We might rather want to stop propagation only conditionally when it's visible.
If the tooltip is closed it would be expected that Escape closes the Flyout, otherwise users would first need to move off the tooltip anchor to be able to close it or would even be stuck in wrapper elements where there is no other element to navigate too. 🤔

static - blocking

Screen.Recording.2024-11-25.at.10.14.42.mov

conditional

Screen.Recording.2024-11-25.at.10.15.13.mov

[weronikaolejniczak]

@mgadewoll that's true, the way I understood it is if the tooltip trigger is focused, the Escape shouldn't close the wrapping element. Is it something we can consult with an accessibility contractor? Or maybe it's something obvious that I'm not aware about 😅

[mgadewoll]

While the tooltip is open, I'd agree that it should not close any wrapping element. But while it's closed I'd think it's more useful to free up the Escape key and prevent blocking.

I would see the behavior as follows:

• trigger is focused -> tooltip opens (active tooltip)
• Escape is pressed -> tooltip closes (inactive tooltip) -> trigger stays focused as that's where the DOM focus was all the time already (no new focus on trigger)
• Enter is pressed -> opens tooltip (active tooltip)
  OR
• Escape is pressed -> closes containing element (e.g. flyout or similar)

@alexwizp Do you have any insights here?

[weronikaolejniczak]

@mgadewoll this absolutely makes sense to me! I'll let @alexwizp confirm and I'll push the changes. I also think it'd be good to cover both paths with an automated test.

[weronikaolejniczak]

@cee-chen @mgadewoll I conditionally call stopPropagation (I leveraged the this.state.visible bool), I moved the tests to the EuiToolTip suite and added EuiModal and EuiPopover cases, and I re-tested manually all the cases mentioned before by @cee-chen and updated the description with the recordings.

I'd appreciate a re-review 🙏🏻 and thanks again for the awesome suggestions!

[cee-chen]

@weronikaolejniczak It looks like the added Cypress test is failing on React 17/16 which is odd 👀 You may want to yarn test-cypress-dev --react-version=17 to debug what's going on

[weronikaolejniczak]

@cee-chen I’m not exactly sure why but two tests for EuiFlyout and EuiModal were failing on React 16 and 17 because they couldn’t find the tooltip element. I tried switching from realMount to mount which would make the tests for the old versions pass but for 18 fail. Something must have changed between versions, leading to different focus behavior.

The solution was to focus the tool-tip trigger directly instead of tabbing into it to assure consistent behavior between versions. Otherwise, the tabbing would start and end at different elements depending on the version.

[mgadewoll]

@cee-chen I’m not exactly sure why but two tests for EuiFlyout and EuiModal were failing on React 16 and 17 because they couldn’t find the tooltip element. I tried switching from realMount to mount which would make the tests for the old versions pass but for 18 fail. Something must have changed between versions, leading to different focus behavior.

The solution was to focus the tool-tip trigger directly instead of tabbing into it to assure consistent behavior between versions. Otherwise, the tabbing would start and end at different elements depending on the version.

@weronikaolejniczak For testing with key presses, I generally would prefer to be able to use proper realPress() to move to the tested component instead of direct focus() to ensure all keyboard functionality/flow works as expected.
From what I see, the issue is that realMount adds a click on mount to move focus to the DOM.
Since we're testing components here that have default click-away behavior, the tests fail because the components close because of the click.
We could enhance the realMount to accept options to define the initial click position to manually adjust it where needed BUT that would likely become annoying for positioning it properly.

Instead we could still use mount() but use focus() to programmatically define the start position on the container element (considering other focus to that element is tested in the component specific tests anyway) and proceed with realPress()?

// example for Flyout
cy.mount(
  <Flyout>
    <EuiToolTip content="Tooltip text here" data-test-subj="tool_tip">
      <EuiButton data-test-subj="tool_tip_trigger">Show tooltip</EuiButton>
    </EuiToolTip>
  </Flyout>
);
cy.get('[data-test-subj="flyout"]').focus(); // move focus to flyout
cy.get('[data-test-subj="tool_tip"]').should('not.exist');

cy.repeatRealPress('Tab', 2);
cy.get('[data-test-subj="tool_tip"]').should('exist');

cy.realPress('Escape');
cy.get('[data-test-subj="tool_tip"]').should('not.exist');
cy.get('[data-test-subj="flyout"]').should('exist');

cy.realPress('Escape');
cy.get('[data-test-subj="flyout"]').should('not.exist');

[weronikaolejniczak]

@mgadewoll that's a fantastic suggestion, thank you, Lene! 🙌🏻 I pushed the changes for all test cases, could you check?

[kibanamachine]

Preview staging links for this PR:

• Docs site: https://eui.elastic.co/pr_8140/
• Storybook: https://eui.elastic.co/pr_8140/storybook
• New docs: https://eui.elastic.co/pr_8140/new-docs

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: f020bb0

History

• 💚 Build #3154 succeeded 6dc6d35
• 💔 Build #3152 failed a696364
• 💔 Build #3145 failed 56ad6ed
• 💚 Build #3134 succeeded 61d6291
• 💚 Build #3123 succeeded 7acdd8b
• 💚 Build #3110 succeeded ba8708d

[mgadewoll]

🚢 🐈‍⬛ Thanks for the additional changes! Updates LGTM 👍