Skip to content
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

[Security Solution] Fix some Prebuilt Rules Cypress tests not running in CI #191978

Merged
merged 9 commits into from
Sep 13, 2024
Original file line number Diff line number Diff line change
@@ -196,7 +196,7 @@ export const RuleDetailsFlyout = ({
paddingSize="l"
data-test-subj={dataTestSubj}
aria-labelledby={prebuiltRulesFlyoutTitleId}
ownFocus
Copy link
Contributor

@alexwizp alexwizp Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that this is the correct change. This table is partially (or completely) covered by the open Flyout, which means, from an a11y perspective, we should not allow setting focus on it.

Let's ask EUI. @cee-chen / @1Copenut please give us your input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the importance of the functionality for quick preview, but also I think we need to follow a consistent style for when it is appropriate to use ownFocus and when it is not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikitaindik I have the same concern. It's better to follow current UX across Kibana. I'm pretty sure the test you mentioned was written some time ago when the flyout didn't own focus by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EuiFlyout does set ownFocus={true} by default, so unless this component now explicitly sets ownFocus={false} this component should be behaving the same way as before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry disregard my previous unhelpful comment, I was looking only at the line highlighted and not at the full diff, I see this is setting ownFocus={false}.

ownFocus={false} flyouts still move/trap focus, so the flyout content still remains accessible for keyboard users, although the content below it does not: https://eui.elastic.co/#/layout/flyout#without-ownfocus

Alexey, does the above documentation help explain ownFocus usage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cee-chen Yep, that's exactly what I mean. That case doesn't seem like something where we should set ownFocus={false}. The default behavior will provide the best UX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input folks! I have tried both options myself and found that when tabbing using the keyboard the focus seems to behave the same. With either ownFocus={false} or ownFocus={true} it stays within the flyout and top page menu. Please tell me if I misunderstood your concern.

Also I have discussed this with our PO (@approksiu) and design team today, showed them both options - with and without the backdrop. And we have concluded that for our use case (quickly switching between rules) it would be desirable if there's no backdrop and table items are still clickable with a mouse.

Copy link
Contributor

@alexwizp alexwizp Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly support the unification of UX, but this is your decision. Please keep in mind that this will lead to the following issues:

  1. When switching between table elements, the focus will not move to the EuiFlyout as expected. In fact, the focus goes to the element under EuiFlyout, which makes keyboard navigation very problematic.
  2. There may be incorrect behaviour of the main content scrolling when using keyboard navigation inside the EuiFlyout. See same problem we have here fix: [Obs Infrastructure > Hosts][KEYBOARD]: Host flyout has buggy scrolling when I expand the State dropdown menu #192372 . I've tested for your case and issue is here.

I'm not insisting, I just want to highlight and explain why we removed ownFocus={false}

@approksiu just fyi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for highlighting these 2 issues @alexwizp! I have discussed these with the team and we've decided to revert to the default behaviour (with the dark backdrop). Made the change in this commit: d1cadfa

ownFocus={false}
>
<EuiFlyoutHeader>
<EuiTitle size="m">
Loading