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] Timeline bar on the rules table page causes broken focus #152344

Closed
maximpn opened this issue Feb 28, 2023 · 9 comments
Closed
Assignees
Labels
bug Fixes for quality problems that affect the customer experience impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team Team:Threat Hunting Security Solution Threat Hunting Team v8.8.0

Comments

@maximpn
Copy link
Contributor

maximpn commented Feb 28, 2023

Relates to: #151244

Description

EUI team worked on improving accessibility and with EUI v75.0.0 release focus is handling another way. After that upgrade we encountered a problem of failing tests in main and had to skip failing tests. Later investigation and consultation with EUI team revealed that we have an EuiFlyout shown at the rules table (see a screenshot).

image

One can see Add Integrations button becomes focused after a page reload and body element has a class euiBody--hasFlyout. Such behavior causes e2e tests failing since Cypress tries typing in a field but focus gets transferred to another element.

The timeline bar's implementation should be changed to avoid displaying an EuiFlyout which causes the problem.

@maximpn maximpn added bug Fixes for quality problems that affect the customer experience Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team 8.8 candidate labels Feb 28, 2023
@elasticmachine
Copy link
Contributor

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

@maximpn maximpn added the Team:Threat Hunting Security Solution Threat Hunting Team label Feb 28, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@maximpn
Copy link
Contributor Author

maximpn commented Feb 28, 2023

@MadameSheema @michaelolo24 I've noticed this behavior, please triage.

@michaelolo24
Copy link
Contributor

Hey @maximpn this has always been disabled on the rules table page, but something may have changed recently in this file or the path name for that page may have changed to cause this issue: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/common/utils/timeline/use_show_timeline_for_path.ts

We'll get a fix in, thanks!

@michaelolo24 michaelolo24 added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. v8.7.0 and removed triage_needed labels Mar 6, 2023
@michaelolo24
Copy link
Contributor

@machadoum or @semd , quick question. Should the rules table page, SECURITY_DETECTIONS_RULES_MANAGEMENT_URL, from https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/cypress/urls/navigation.ts have an equivalent link in https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/management/links.ts ?

if so, @maximpn it might involve adding SECURITY_DETECTIONS_RULES_MANAGEMENT_URL as a sublink to

with the hideTimeline: true flag added

@machadoum
Copy link
Member

machadoum commented Mar 9, 2023

I believe the timeline started appearing on the rules page when the tabs were introduced to the URL. So to make it work as expected, we have to configure the new links here. I would also update the RULES_PATH entry because it links to a redirect page. But be aware that updating links config might affect how they are displayed in the side nav and global search.

@christineweng
Copy link
Contributor

@maximpn looks like this behavior no longer appears on main, and I saw that the tests are re-enabled. Could you confirm this is fixed?

@maximpn
Copy link
Contributor Author

maximpn commented May 24, 2023

@christineweng I've checked this problem on main and it's not fixed. I had to add force: true to type() command to circumvent the problem and re-enable the tests. While the e2e tests has to work without forcing any action.

As far as I see rule search bar isn't focusable from the first click (as you can see on the video below). This is the main problem as cy.get(...).type(...) doesn't work and requires either force: true or click() before type(). While type() command clicks on the input.

Screen.Recording.2023-05-24.at.12.47.58.mov

But it works as expected when I removed the timeline bar by removing this code block (as you can see on the video below).

Screen.Recording.2023-05-24.at.12.48.48.mov

A similar search bar field on the Shared Exception Lists page works fine (gets focused by clicking once) as the page doesn't contain the timeline bar.

I also noticed that .euiBody--hasFlyout class doesn't appear on the body element so it might be changed in a new EUI version and it's rather was a side effect than the root cause.

kqualters-elastic added a commit that referenced this issue May 25, 2023
… issue, causing focus issues (#158392)

## Summary

This pr removes some code added 2 years ago to work around an eui issue
with the global search bar and timeline focus, was causing issues in
some cases, like in #152344 none
of this code is needed, as the original bug no longer exists.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))

Co-authored-by: Kibana Machine <[email protected]>
maximpn added a commit that referenced this issue May 30, 2023
…s tests (#158560)

**Relates to:** #152344, #152470
**Depends on:** #158392

## Summary

Removes `force` option from Cypress actions like `type()`, `click()` and etc. It helps to catch UI quirks instead of silently ignoring them.

## Details

After merging back of #158392 it's possible to remove `force: true` flag from the Cypress actions used in Detection rules tests. Based on the discussion in #152470 it's bette to avoid forced actions and leave a comment if there is no other way than forcing an action.
@maximpn
Copy link
Contributor Author

maximpn commented May 30, 2023

This bug was fixed by #158392, closing it.

@maximpn maximpn closed this as completed May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team Team:Threat Hunting Security Solution Threat Hunting Team v8.8.0
Projects
None yet
Development

No branches or pull requests

5 participants