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

[EuiHeader] Increase z-index value #8325

Merged
merged 19 commits into from
Feb 28, 2025
Merged

Conversation

acstll
Copy link
Contributor

@acstll acstll commented Feb 14, 2025

Summary

Resolves #8206

Note

Please refer to the original issue in order to better understand the problem

To summarize the problem: in a Project View in Kibana (e.g. Serverless) when a Flyout is open, the navigation in Collapsible Nav Beta is reachable via keyboard, even though it stands behind the Flyout's overlay (screenshot).

We have evaluated more than one way to solve the issue. Most of them have problems. The one presented here feels like the least fragile (it won't shouldn't create other problems down the road) and the least complex.

Instead of excluding the navigation from the focusable elements in the Header (e.g. adding the inert attribute to it while the Flyout is open) —as suggested in the original issue—, this PR goes a different way: it brings the navigation up above, on top of the Flyout's overlay, so it's OK for it to be focusable.

It's also worth mentioning that this is the way the Classic view works: if you toggle the navigation open with a Flyout open, it goes on top of the overlay and it's reachable.

The z-index of the Header has been changed from 1000 to 2000. But it remains below everything else (e.g. modals) it was below before. The relationship between the Header's z-index and other components whose z-index is relative to it, remains the same. Only the z-index relationship between Header and Flyout is changed.

Only the z-index of the fixed Header has been increased by 1, leaving everything else unaffected.

Here's a recording demoing the idea
Screen.Recording.2025-02-12.at.22.43.59.z-index.mov

The one remarkable side-effect, and at first glance the only, of the change is the Header's shadow appearing over the Flyout (screenshot below).

Shadow appearing:

Before (current) After (with the fix)
Screenshot 2025-02-24 at 14 13 12 Screenshot 2025-02-24 at 14 13 49

QA

https://eui.elastic.co/pr_8325/#/layout/header/elastic-pattern-project

Important

The example header_elastic_pattern_project.tsx is there only for testing purposes, not meant to be merged!

To test this locally

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
    • If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@acstll acstll self-assigned this Feb 14, 2025
This should be either reverted or refactored/improved in order to keep it
@acstll
Copy link
Contributor Author

acstll commented Feb 24, 2025

I think the fix is solid now. I want to add a test.

@acstll acstll marked this pull request as ready for review February 25, 2025 06:39
@acstll acstll requested a review from a team as a code owner February 25, 2025 06:39
@mgadewoll mgadewoll self-requested a review February 25, 2025 08:17
@mgadewoll
Copy link
Contributor

The one remarkable side-effect, and at first glance the only, of the change is the Header's shadow appearing over the Flyout

@acstll I think this is a drawback we have to accept but I'd suggest to get an approval of design on this too.

@acstll
Copy link
Contributor Author

acstll commented Feb 25, 2025

@acstll I think this is a drawback we have to accept but I'd suggest to get an approval of design on this too.

Definitely, I'm requesting a review from Ryan 🙏 (@ryankeairns I remember you mentioned a suggestion regarding the shadow…?)

@acstll acstll requested review from ryankeairns and removed request for ryankeairns February 25, 2025 12:35
@acstll
Copy link
Contributor Author

acstll commented Feb 25, 2025

@ryankeairns I'll request a review again in a bit when this is ready-ready 😁

On mobile: the EuiCollapsibleNavBeta flyout is portalled, so the stacking context is determined by the overlay mask (EuiOverlayMask), making the flyouts fight each other with the same z-index (1000 from levels.maskBelowHeader) — we do nothing
@acstll
Copy link
Contributor Author

acstll commented Feb 26, 2025

@mgadewoll I think I addressed all pending changes 🤓

  • updated changelog 192efcc
  • improved flyout story to include focusable items cf1f070
  • checked mobile behaviour 7686c05
  • test collapsible nav in flyout shards cdebce0

@acstll acstll requested a review from mgadewoll February 26, 2025 12:19
It was meant for manual testing and debugging only
@acstll acstll requested a review from mgadewoll February 26, 2025 18:13
Needs a bit of investigation why it's failing on React 16 and 17
@acstll acstll requested a review from ryankeairns February 27, 2025 06:33
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Thanks for digging into this! The solution is technically definitely the best value for impact. Let's wait for design thoughts from @ryankeairns before merging though.

@ryankeairns
Copy link
Contributor

It's hard to argue with the accessibility gain. Visually, I believe it is a bit of a downgrade but not enough to stop from proceeding. As a mitigating effort, let's decrease the size of the shadow on the header.

Changing this line to euiShadowXSmall

https://github.com/elastic/eui/blob/887efb88c88bc930a7348af2f4c84ed4c7502b32/packages/eui/src/components/header/header.styles.ts#L41C9-L41C23

...which results in...

CleanShot 2025-02-27 at 13 50 17@2x

@mgadewoll
Copy link
Contributor

It's hard to argue with the accessibility gain. Visually, I believe it is a bit of a downgrade but not enough to stop from proceeding. As a mitigating effort, let's decrease the size of the shadow on the header.

Changing this line to euiShadowXSmall

https://github.com/elastic/eui/blob/887efb88c88bc930a7348af2f4c84ed4c7502b32/packages/eui/src/components/header/header.styles.ts#L41C9-L41C23

@ryankeairns To confirm: we're fine changing it generally for EuiHeader, correct? Or should it be for fixed headers only?

And an additional side thought: We'll change the shadows for Borealis soon (issue) which will remove euiShadowXSmall - will the new euiShadowSmall work fine then anyway?

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @acstll

@ryankeairns
Copy link
Contributor

It's hard to argue with the accessibility gain. Visually, I believe it is a bit of a downgrade but not enough to stop from proceeding. As a mitigating effort, let's decrease the size of the shadow on the header.
Changing this line to euiShadowXSmall
https://github.com/elastic/eui/blob/887efb88c88bc930a7348af2f4c84ed4c7502b32/packages/eui/src/components/header/header.styles.ts#L41C9-L41C23

@ryankeairns To confirm: we're fine changing it generally for EuiHeader, correct? Or should it be for fixed headers only?

Yes. (Do we differentiate the style per position today?)

And an additional side thought: We'll change the shadows for Borealis soon (issue) which will remove euiShadowXSmall - will the new euiShadowSmall work fine then anyway?

🤔 Are we removing XSmall or flat? I thought it was the latter... in any case, I still feel this change - on this PR - is helpful for the interim/Amsterdam.

@mgadewoll
Copy link
Contributor

@ryankeairns Thanks for the confirmation!

Yes. (Do we differentiate the style per position today?)

No, we don't. If we could distinguish fixed headers while a flyout is open, that might be an idea for adjusting it conditionally, but currently that's not a thing.

🤔 Are we removing XSmall or flat? I thought it was the latter... in any case, I still feel this change - on this PR - is helpful for the interim/Amsterdam.

I thought both, but looking at the specs here again it does define xs still so ... maybe? 😅 🤷‍♀️

@mgadewoll
Copy link
Contributor

@acstll I already added the change and will go ahead and merge your PR. 🚀

@mgadewoll mgadewoll merged commit 01d7566 into elastic:main Feb 28, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Focusable Site Navigation in Flyouts After Recent Update
5 participants