[Controls] Fix timeslider focus outline and Shift-Tab behavior#230852
[Controls] Fix timeslider focus outline and Shift-Tab behavior#230852Zacqary wants to merge 4 commits intoelastic:mainfrom
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.
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.
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
onKeyDownfunction to the input popover does not override the originalonKeyDown. It will always callclosePopoverif 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 === trueto theclosePopoverfunction. - "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.
@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
|
|
Closing in favor of elastic/eui#8950, it'll be cleaner to wait for the EUI fix instead of reverting our workaround. |
Pull request was closed
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.