[EuiPopover] Resolve Positioning Errors When EuiPopover is Nested Inside of EuiFlyout

[breehall]

Summary

Issue #1023 raised a positioning error for components that utilize EuiPopover when they are nested in containers like EuiFlyout. EuiInputPopover and EuiSuperSelect were specifically mentioned in the issue. The positioning error caused the popover portion of the EuiInputPopover and EuiSuperSelect to scroll with the page and detach from their sibling components.

By enabling the positioning event listener on the popover to useCapture, the components are no longer split with the user scrolls inside of a container. However, this did bring to light another issue where the popover appears to be on top of the EuiFlyout header and any additional headers on the page. It doesn't appear to be an issue with the z-index, but it may have to do with the calculation for the absolute positioning inside of popover_positioning.ts.

@cchaos @miukimiu do you have a preference on how I should tackle this from a design prospective? Per @chandlerprall there may be quite a few edge cases with this.

EuiInputPopover Positioning Resolution
https://user-images.githubusercontent.com/40739624/132523212-04a20ebe-efbc-454d-8564-fd84fa349117.mov

Checklist

• Check against all themes for compatibility in both light and dark modes
• Checked in mobile
• Checked in Chrome, Safari, Edge, and Firefox
• Props have proper autodocs and playground toggles
• Added documentation
• Checked Code Sandbox works for any docs examples
• Added or updated jest tests
• Checked for breaking changes and labeled appropriately
• Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[cla-checker-service]

❌ Author of the following commits did not sign a Contributor Agreement:
b6e1db2

Please, read and sign the above mentioned agreement if you want to contribute to this project

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5155/

[cchaos]

However, this did bring to light another issue where the popover appears to be on top of the EuiFlyout header and any additional headers on the page. It doesn't appear to be an issue with the z-index, but it may have to do with the calculation for the absolute positioning inside of popover_positioning.ts.

Yeah this is a known issues with the popovers #4756 because they are rendered in a Portal (at the bottom of the DOM). We have historically & intentionally kept the z-index high enough to pass over the header for cases where either the popover was triggered from a fixed position element like the header, or when the fixed positioned element isn't taken into account for the popover to adjust it's position.

I honestly don't have a good answer to this riddle as with most z-index/portal/fixed position issues it can be completely contextual-based. In these situations I tend to look towards similar native HTML components and how they handle these situations.

So for instance a native <select> element does seem to always ensure it's options list renders above all fixed content.

But it goes further and prohibits the scrolling of the page. I'm not sure we can do this safely because of the possible length of the popover. Where the native element seems to be more controlled by the OS to fit the contents in height:

But it also allows covering the actual select input with the options list which we cannot do because for some components like the EuiComboBox we allow for searching.

The other option would be auto-closing the popover on scroll, which we've done in EuiComboBox but I've found this to be terribly cumbersome.

So really the conclusion is that I'm not sure there's anything we can do about it at the moment without some sort of rewrite of not using Portals.

[breehall]

You're absolutely right. @chandlerprall also brought up the idea of creating a context specific for scrollable containers. This also isn't something that would be an immediate fix, but something we may think about in the future.
Should I ship the first part of this fix, or should I put this on ice until we come to a consensus on how to resolve the issue in its entirety in the future?

[cchaos]

I think this is definitely an improvement, albeit maybe not as perfect as we'd like. I'd say lets get this is in and continue to iterate.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5155/

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5155/

[cchaos]

👍 The fix looks good to me. Glad it came down to a simple parameter. I think now we need to be more vocal about it in the docs. The following is a simple suggestion, but maybe even something louder like in the Popover page docs.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5155/

[thompsongl]

Code changes LGTM!

I think this is definitely an improvement, albeit maybe not as perfect as we'd like. I'd say lets get this is in and continue to iterate.

I agree with this; the positioning context idea sounds cool, too.

When @cchaos gives her approval, go ahead and merge with "Use your administrator privileges ..." option. The CLA checker won't approve, but you're safe to merge.

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_5155/

[cchaos]

👍 LGTM!