fix: Rules > Detection (SEIM) Rules][SCREEN READER]: Snooze notifications modal is not trapping VoiceOver + Safari properly

[alexwizp]

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

1. Open Detection Rules (SIEM)
2. Enable VoiceOver (only use Safari for this test)
3. Tab to the first Snooze Notification bell icon and press Ctrl + Opt + Enter to open it
4. Navigate through the modal by pressing Ctrl + Opt + Right_Arrow
5. Verify when your focus should be on the "days" dropdown, it pops out to the main container, escaping the modal

What was done?

1. Focus trap logic has been implemented to ensure that focus returns to the correct location after executing onRuleChanged.

Screen:

Screen.Recording.2024-05-06.at.15.18.34.mov

[alexwizp]

/ci

[elasticmachine]

Pinging @elastic/response-ops (Team:ResponseOps)

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[elasticmachine]

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

[cnasikas]

What is the reason for adding this timeout?

[alexwizp]

@cnasikas the rendering cycle after calling await onRuleChanged(...args) must complete for the reference to focusTrapButtonRef to be updated. It might work without setTimeout, but I can't be certain it will work stable

[cnasikas]

Having a timeout in the callback without React being aware may cause various issues. For example, the timeout will not be cleaned if the user navigates to another page and the component unmounts. Moving the focusTrapButtonRef.current?.focus() to a useEffect that will trigger every time the rule is updated seems more appropriate. I am curious about the UX of a user using the mouse. Will it focus the button also for them?

[alexwizp]

@cnasikas, thank you for your suggestion. At the moment, I'm not entirely certain if it will be effective, probably not. I'll need some time to consider alternative approaches. But for now not sure that we can fix that differently

@1Copenut fyi

[cnasikas]

@JiaweiWu proposed the usage of requestAnimationFrame. It is being used by the EUI library (https://github.com/elastic/eui/blob/main/packages/eui/src/components/inline_edit/inline_edit_form.tsx#L242) Also, could autofocus work?

[alexwizp]

Thank you @cnasikas and @JiaweiWu. The option with requestAnimationFrame seems to be working.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 5954d23

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

• 💚 Build #210425 succeeded 9a6aa26
• 💚 Build #209589 succeeded 0db0393
• 💚 Build #209556 succeeded ec76542
• 💚 Build #209359 succeeded bf5ca83
• 💚 Build #209124 succeeded 5ce7a31
• 💚 Build #208902 succeeded 9934258

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[cnasikas]

I noticed that we pass the same focusTrapButtonRef to all buttons. Is that supported or does the reference hold the last button only?

[cnasikas]

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?

Screen.Recording.2024-05-20.at.12.06.20.PM.mov

[alexwizp]

@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.

[cnasikas]

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!

[mdefazio]

@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.