-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[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
base: main
Are you sure you want to change the base?
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
onKeyDown: (event) => { | ||
if (event.key === 'Tab' && event.shiftKey) { | ||
lockPopoverOpen$.next(true); | ||
requestIdleCallback(() => lockPopoverOpen$.next(false)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 originalonKeyDown
. It will always callclosePopover
if you hit Tab on the last tabbable element, and it doesn't send any arguments toclosePopover
. An observable is the only way I could figure out to communicateevent.shiftKey === true
to theclosePopover
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
There was a problem hiding this comment.
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
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
Summary
To test:
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.