kibana
kibana copied to clipboard
elastic
fix: Rules > Detection (SEIM) Rules][SCREEN READER]: Snooze notifications modal is not trapping VoiceOver + Safari properly
Closes: https://github.com/elastic/security-team/issues/8716
Description
The Detection Rules table has a Snooze notifications modal that is not trapping focus consistently in Safari + VoiceOver. The focus is being "bounced" to the <main> container when I traverse using Ctrl + Opt + RIGHT_ARROW or quick arrow navigation methods. These are the default VO navigation methods. This is not affecting Chrome + JAWS or Firefox + NVDA. Screenshot and MOV attached below.
Steps to recreate
- Open Detection Rules (SIEM)
- Enable VoiceOver (only use Safari for this test)
- Tab to the first Snooze Notification bell icon and press
Ctrl + Opt + Enterto open it - Navigate through the modal by pressing
Ctrl + Opt + Right_Arrow - Verify when your focus should be on the "days" dropdown, it pops out to the main container, escaping the modal
What was done?
- Focus trap logic has been implemented to ensure that focus returns to the correct location after executing onRuleChanged.
Screen:
https://github.com/elastic/kibana/assets/20072247/1c8951df-828d-4c7d-8b2c-2ea5a4d5e400
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)
:green_heart: Build Succeeded
- Buildkite Build
- Commit: 5954d233221eb576b9e44616dd417c070c9ab33b
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 |
|---|---|---|---|
triggersActionsUi |
1.6MB | 1.6MB | +1.0KB |
History
- :green_heart: Build #210425 succeeded 9a6aa261f3898ce380a816f384bb4b2691ad792a
- :green_heart: Build #209589 succeeded 0db0393cebd7e551a2f81a6654c614ed2b8e1727
- :green_heart: Build #209556 succeeded ec76542c246e0870c0e0d21b56da4cf60694e66d
- :green_heart: Build #209359 succeeded bf5ca83056a7f43bdf925686ddb4aeb1c2577c08
- :green_heart: Build #209124 succeeded 5ce7a319e88ebaeac6f5b2a226c7241f647240d2
- :green_heart: Build #208902 succeeded 99342586625ec6a3ad8d68aa4151f45ace5366ca
To update your PR or re-run it, just comment with:
@elasticmachine merge upstream
Also, I noticed that if I set a schedule with the mouse the button will be focused and show a tooltip. Is this a behavior we want to do for users who do not navigate with the keyboard? @mdefazio Any opinion on this?
https://github.com/elastic/kibana/assets/7871006/2059ab2b-df0a-4aca-a85c-751fea3c0966
@cnasikas Please correct me if I'm wrong. If I understand the business logic correctly, the Notify column can have one of three states (buttons): scheduledSnoozeButton, unsnoozedButton, and indefiniteSnoozeButton.
After calling onRuleChanged, the state changes, and the button where the EuiPopover was originally opened gets re-rendered to reflect the new state. This is what causes the problem, as the browser loses the current element and returns the focus to document.body.
How does that solution work? : As I mentioned at the beginning, in the UI we can have only one of the three buttons at a time. This means that if I set a single reference to all three buttons, this reference will ultimately point to the currently rendered element. Then, after calling onRuleChanged, we should wait a bit for the state to be rendered (requestAnimationFrame helps with that) and simply return focus to new rendered item.
Regarding the Tooltip question, its behavior has indeed changed. The button remains in focus, and this is a condition to show that tooltip. I don't know is we need somehow fix that or not.
Thank you @alexwizp for your detailed answer and your patience with me.
This means that if I set a single reference to all three buttons, this reference will ultimately point to the currently rendered element
This was not clear to me. Thanks for clarifying!
Regarding the Tooltip question, its behavior has indeed changed. The button remains in focus, and this is a condition to show that tooltip. I don't know is we need somehow fix that or not.
I am ok with the change. Let's wait for @mdefazio to hear his opinion on this before merging. Other than that LGTM!
@cnasikas Thanks for the ping. I don't think showing the tooltip/keeping focus on the button is bad here. And since this is our default button behavior anyway, let's wait on doing anything extra here.
