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

fix: [Obs Infrastructure > Hosts][KEYBOARD]: Host flyout has buggy scrolling when I expand the State dropdown menu #192372

Merged
merged 9 commits into from
Sep 29, 2024

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Sep 9, 2024

Closes: https://github.com/elastic/observability-accessibility/issues/98

Description

The Obs Applications > Hosts > Host popout menu has a buggy keyboard navigation behavior when I expand the State dropdown. If I press Down_Arrow before tabbing into the State dropdown, the entire page scrolls. This feels like a bug because I was not expecting the page to shift. It creates an unexpected scroll event, so treating it as a keyboard nav bug. I've attached an MOV file to show the behavior.

Steps to recreate

  1. Open the Obs Hosts view
  2. Click double arrows next to a host in the table
  3. Click the Processes tab in the flyout
  4. Tab through to the State dropdown and press Enter
  5. Press Down_Arrow to see the entire page scroll, but not the State menu options
  6. PressTab to "drop into" the State menu
  7. Press Down_Arrow again to see the menu scroll correctly

What was changed?:

  1. Removed ownFocus={false} from EuiFlyout. This restores the default user experience (UX) for EuiFlyout and resolves the mentioned issue.

@alexwizp
Copy link
Contributor Author

alexwizp commented Sep 9, 2024

/ci

@alexwizp alexwizp marked this pull request as ready for review September 9, 2024 13:45
@alexwizp alexwizp requested a review from a team as a code owner September 9, 2024 13:45
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Sep 9, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-accessibility (Project:Accessibility)

@alexwizp alexwizp added v8.16.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Sep 9, 2024
@alexwizp
Copy link
Contributor Author

Hi @elastic/obs-ux-infra_services-team Could you please review the new UX after removing the ownFocus={false} attribute? If you're ok with it, I'll update the tests and prepare a PR for merging. My thought is that we should proceed with this... but it is a bit related to the following discussion: #191978 (comment)

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

@alexwizp alexwizp requested a review from a team as a code owner September 11, 2024 13:38
@alexwizp alexwizp marked this pull request as draft September 13, 2024 12:02
@alexwizp
Copy link
Contributor Author

/ci

@alexwizp alexwizp marked this pull request as ready for review September 27, 2024 18:31
@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 27, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 258ea73
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192372-258ea73c2001

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
infra 1.6MB 1.6MB -12.0B

History

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

@alexwizp alexwizp merged commit fb79799 into elastic:main Sep 29, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants