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.

Sign up for GitHub

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

Closed
Zacqary wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d064b4
Add workaround for tab-away bug
Zacqary Aug 6, 2025
0ec221b
Fix timeslider focus outline offset
Zacqary Aug 6, 2025
fe119bb
Merge branch 'main' into 215846-maps-a11y-nav
Zacqary Aug 6, 2025
7d9bbcc
Merge branch 'main' into 215846-maps-a11y-nav
Zacqary Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...red/controls/public/controls/timeslider_control/components/time_slider_popover_button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ const timeSliderStyles = {
text-decoration: line-through;
color: ${euiTheme.colors.mediumShade};
}

&:focus {
outline-offset: -3px;
}
`,
};

Expand Down
17 changes: 16 additions & 1 deletion ...ins/shared/controls/public/controls/timeslider_control/get_timeslider_control_factory.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ export const getTimesliderControlFactory = (): ControlFactory<
const isPopoverOpen$ = new BehaviorSubject(false);
const hasTimeSliceSelection$ = new BehaviorSubject<boolean>(Boolean(timeslice$));

// Workaround for https://github.com/elastic/eui/issues/8949
const lockPopoverOpen$ = new BehaviorSubject(false);

const timeRangePercentage = initTimeRangePercentage(
initialState,
syncTimesliceWithTimeRangePercentage
Expand Down Expand Up @@ -328,8 +331,20 @@ export const getTimesliderControlFactory = (): ControlFactory<
/>
}
isOpen={isPopoverOpen}
closePopover={() => isPopoverOpen$.next(false)}
closePopover={() => {
// Workaround for https://github.com/elastic/eui/issues/8949
if (!lockPopoverOpen$.getValue()) isPopoverOpen$.next(false);
}}
panelPaddingSize="s"
panelProps={{
// Workaround for https://github.com/elastic/eui/issues/8949
onKeyDown: (event) => {
if (event.key === 'Tab' && event.shiftKey) {
lockPopoverOpen$.next(true);
requestIdleCallback(() => lockPopoverOpen$.next(false));
Copy link
Copy Markdown
Contributor

@nreese nreese Aug 6, 2025

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
Copy Markdown
Contributor Author

@Zacqary Zacqary Aug 6, 2025

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
Copy Markdown
Contributor

@weronikaolejniczak weronikaolejniczak Aug 7, 2025 •
edited
Loading

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

}
},
}}
>
<TimeSliderPopoverContent
isAnchored={typeof isAnchored === 'boolean' ? isAnchored : false}
Expand Down