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

[Bug] Focusable Site Navigation in Flyouts After Recent Update #8206

Open
yansavitski opened this issue Dec 5, 2024 · 3 comments
Open

[Bug] Focusable Site Navigation in Flyouts After Recent Update #8206

yansavitski opened this issue Dec 5, 2024 · 3 comments
Assignees

Comments

@yansavitski
Copy link

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.

  • Severity: Medium to high. While there is no immediate functional issue, this breaks expected focus behavior and might lead to accessibility problems.

Environment and versions

  • Kibana version: > 8.17
  • Serverless

To Reproduce
Steps to reproduce the behavior:

  1. Open any flyout in Kibana where site navigation is present.
  2. Use the Tab key to navigate.
  3. Notice that focus moves to the site navigation inside the , even when interacting with the flyout.

Expected behavior
Focus should remain restricted to the flyout, and navigation elements inside

should not be included in the focusable area.

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

@cee-chen
Copy link
Contributor

cee-chen commented Dec 13, 2024

I see two obvious ways of approaching this, although there may another more elegant fix I haven't thought of:

  1. (Static) Portal the collapsible nav flyout to after the <EuiHeader> it's located in in the DOM.
    • ⚠ This will require some extra focus/tab UX to ensure that keyboard users can jump between the menu toggle button and the first menu item without having to tab through the entire EuiHeader content.
  2. Dynamically inert (or portal) the collapsible nav flyout whenever any other flyout with ownFocus/an overlay mask is rendered on the page
    • I would lean towards inert over portalling, as that feels more performant
    • Feels somewhat tricky/fragile to detect the presence of another mask/flyout however. Not totally sure what we'd use for that. A react context? Or a mutation observer?

For reference, here's the code in question that causes flyouts to include EuiHeaders and their contents in the focus order:

useEffect(() => {
if (includeFixedHeadersInFocusTrap) {
const fixedHeaderEls = document.querySelectorAll<HTMLDivElement>(
'.euiHeader[data-fixed-header]'
);
setFixedHeaders(Array.from(fixedHeaderEls));

The crux here is that we need to keep the visible / non-overlayed header content as a shard while excluding the overlayed EuiCollapsibleNavBeta.

@JasonStoltz JasonStoltz removed the ⚠️ needs validation For bugs that need confirmation as to whether they're reproducible label Jan 14, 2025
@acstll acstll self-assigned this Jan 21, 2025
@acstll
Copy link
Contributor

acstll commented Feb 6, 2025

I see two obvious ways of approaching this, although there may another more elegant fix I haven't thought of:

  1. (Static) Portal the collapsible nav flyout to after the <EuiHeader> it's located in in the DOM.

    • ⚠ This will require some extra focus/tab UX to ensure that keyboard users can jump between the menu toggle button and the first menu item without having to tab through the entire EuiHeader content.
  2. Dynamically inert (or portal) the collapsible nav flyout whenever any other flyout with ownFocus/an overlay mask is rendered on the page

    • I would lean towards inert over portalling, as that feels more performant
    • Feels somewhat tricky/fragile to detect the presence of another mask/flyout however. Not totally sure what we'd use for that. A react context? Or a mutation observer?

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 inert to an element based on some other parts of the DOM changing or a React context scares me a bit: if something fails for whatever reason and the inert attribute unexpectedly remains, the actual main navigation would be unreachable via keyboard… — still I think using inert seems like the appropriate way to handle this, if used carefully (if that makes sense).)


I have a quick and dirty, working PoC using inert but handling it within the Flyout, instead of the CollapsibleNav itself:

if includeFixedHeadersInFocusTrap is true in the Flyout, it will query for any elements inside the shards (.euiHeader[data-fixed-header]) with the attribute e.g. [data-focus-trap-shard-ignore] and apply inert to them, and remove it when the Flyout is closed

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…

https://github.com/acstll/eui/blob/357f3da7675070f56616490c2391c924a8de1a42/packages/eui/src/components/flyout/flyout.tsx#L295-L342

Note

if you would like to test it, there's a branch — do yarn start > in packages/eui and go to http://localhost:8030/#/layout/header/elastic-pattern-project-8206

Screen.Recording.2025-02-06.at.17.14.23.OK.mov

@acstll
Copy link
Contributor

acstll commented Feb 11, 2025

quick update on the previous post:

[…] is this reasoning correct or am I missing something? 😅

it's correct, both options have problems (will expand on this soon)

I have a quick and dirty, working PoC using inert but handling it within the Flyout, instead of the CollapsibleNav itself […] what do you think? could this be a way to solve it?

nope, we're discarding this idea because breaking encapsulation like that is indeed a code smell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants