-
Notifications
You must be signed in to change notification settings - Fork 843
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
[Bug] Focusable Site Navigation in Flyouts After Recent Update #8206
Comments
I see two obvious ways of approaching this, although there may another more elegant fix I haven't thought of:
For reference, here's the code in question that causes flyouts to include EuiHeaders and their contents in the focus order: eui/packages/eui/src/components/flyout/flyout.tsx Lines 296 to 301 in 5c40315
The crux here is that we need to keep the visible / non-overlayed header content as a shard while excluding the overlayed EuiCollapsibleNavBeta. |
I think option (1) still requires checking for the presence of a flyout as in option (2), because: if the "extra focus/tab UX" means more specifically another "focus lock" (where we listen for the tab key on the toggle button to programatically focus the first item in the portalled menu), wouldn't this "undo" (or bypass) the portalling, so the end result would be the same as if the menu was not portalled at all? — you would need to again check for the presence of the open flyout to enable this behaviour is this reasoning correct or am I missing something? 😅 (regarding option (2), applying I have a quick and dirty, working PoC using if the one thing I don't like about this is breaking the "encapsulation" (touching the DOM outside of itself — is there a name for this (anti-)pattern?), but it's similar to locking the scroll on the body when a modal is open, so it may be acceptable (it would be great if the react-focus-lock library we use, allowed this somehow, to ignore some elements within a shard) what do you think? could this be a way to solve it? code and demo below… Note if you would like to test it, there's a branch — do Screen.Recording.2025-02-06.at.17.14.23.OK.mov |
quick update on the previous post:
it's correct, both options have problems (will expand on this soon)
nope, we're discarding this idea because breaking encapsulation like that is indeed a code smell |
Describe the bug
After the recent Kibana and Serverless updates, site navigation became part of the
element. This has caused a change in focus behavior for flyouts. Previously, focus was restricted to the flyout and , with the option to exclude using the IncludeFixedHeadersInFocusTrap setting. However, since site navigation is now inside , EuiCollapsibleNavBeta is automatically include it in the focusable area, allowing users to navigate to it even when interacting with a flyout.Impact and severity
This impacts all flyouts in Kibana and related applications, as it allows users to focus on navigation elements (site navigation) unintentionally when they are supposed to remain restricted to the flyout.
Environment and versions
To Reproduce
Steps to reproduce the behavior:
Expected behavior
should not be included in the focusable area.Focus should remain restricted to the flyout, and navigation elements inside
Minimum reproducible sandbox
It is extremely helpful for us if you are able to reproduce your issue in a minimum reproducible sandbox (visit our docs site and click the CodeSandbox logo in the top-right corner).
Screenshots
![Image](https://private-user-images.githubusercontent.com/17390745/392905189-8038c345-9670-48a9-8800-23303bc85208.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkzODA2MjYsIm5iZiI6MTczOTM4MDMyNiwicGF0aCI6Ii8xNzM5MDc0NS8zOTI5MDUxODktODAzOGMzNDUtOTY3MC00OGE5LTg4MDAtMjMzMDNiYzg1MjA4LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTIlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjEyVDE3MTIwNlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWU5MzBmZmZjMGVmYzY5OThiZjAwY2RhZmFmMTZhOWY1ZDNlNTY5Y2JiMDk0MTVkNGNkMjZjZTZlN2ZhNzllYWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.EEPVrZw2KRSNmbfBTLE8DU98pdNqNLPGbm9oaUuF7lI)
The text was updated successfully, but these errors were encountered: