Skip to content

[Controls] Fix timeslider focus outline and Shift-Tab behavior #230852

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Aug 6, 2025

Summary

  • Makes the focus outline visible on the Timeslider control:
Before After
Screenshot 2025-08-06 at 11 56 48 AM Screenshot 2025-08-06 at 12 56 50 PM

To test:

  • Create a timeslider control
  • Tab into focus until the dates are highlighted
  • Press Enter
  • Tab to the second range handle
  • Shift-Tab back to the previous handle and ensure the popover doesn't close
  • Tab forward to the second range handle, then Tab forward again. Ensure the popover does close

Note that Shift-Tabbing all the way back to the Pin at the beginning of the popover, and then Shift-Tabbing again, will loop back to the second range handle. This is because of the focus trap on the popover, and I believe it's expected. We only want to close the popover when tabbing forward.

@Zacqary Zacqary requested a review from a team as a code owner August 6, 2025 18:03
@Zacqary Zacqary added release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:small Small Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Project:Controls accessibility: keyboard navigation Accessibility keyboard navigation and focus backport:all-open Backport to all branches that could still receive a release labels Aug 6, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

onKeyDown: (event) => {
if (event.key === 'Tab' && event.shiftKey) {
lockPopoverOpen$.next(true);
requestIdleCallback(() => lockPopoverOpen$.next(false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a workaround recommended by EUI? Could you provide some more background about why this implemenation path was selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No workaround is currently recommended by EUI. This bug wasn't tracked, I just opened the issue after discovering it.

As I explained in the issue, <EuiInputPopover> uses an onKeyDown handler to trigger closePopover when the user tabs away from it. It does it by detecting if the Tab key was pressed, and if document.activeElement is equal to the last tabbable item in the popover. It does not check to see whether the Shift key was also being held.

The reason I'm doing it in this janky way is:

  • Passing a new onKeyDown function to the input popover does not override the original onKeyDown. It will always call closePopover if you hit Tab on the last tabbable element, and it doesn't send any arguments to closePopover. An observable is the only way I could figure out to communicate event.shiftKey === true to the closePopover function.
  • "Last tabbable element" is determined using some complex logic from a library called tabbable, an EUI dependency but not a Kibana dependency, so it's not available. The best we can do without adding a new dependency is to just hard disable closing the popover if the Shift key is used.
  • Disabling the focusTrap entirely on the <EuiInputPopover> just makes it impossible to Tab into the popover at all

Happy to use a different workaround if EUI can recommend one, but this is what I got working

Copy link
Contributor

@weronikaolejniczak weronikaolejniczak Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zacqary that's a very clever workaround but of course, we don't want it! I opened a PR to check for Shift key too: elastic/eui#8950

I also noticed that overall, EuiInputPopover keyboard navigation is strange. We might need to look more deeply into this but for now the simple change ☝🏻 should at least get rid of the code on your side.

The earliest we will be able to merge this PR is next week so don't hesitate to merge this as is.

cc @nreese @elastic/eui-team

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #131 / Fleet Endpoints fleet policy secrets fleet server version requirements "after each" hook for "should store new secrets after package upgrade"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 484.5KB 484.7KB +179.0B

History

@Zacqary Zacqary enabled auto-merge (squash) August 7, 2025 22:05
@Zacqary Zacqary added backport:version Backport to applied version labels v9.2.0 v8.19.2 v9.1.1 and removed backport:all-open Backport to all branches that could still receive a release labels Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility: keyboard navigation Accessibility keyboard navigation and focus backport:version Backport to applied version labels impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort Project:Controls release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.19.2 v9.1.1 v9.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Analytics:Maps page]Timeslider unexpectedly closes
4 participants