Restore Flyout System feature in EUI

[tsullivan]

Summary

Closes #9173
Re-closes https://github.com/elastic/kibana-team/issues/2060

The EUI Flyout System was reverted a short time ago because of a blocking performance issue. This PR restores the feature and introduces a fix for the performance bug. The details of the performance bug fix are below.

This fix can be tested in a Kibana draft PR: elastic/kibana#233806.
Run gh pr checkout 233806

Why are we making this change?

Restoring a feature that had to be temporarily pulled out of EUI, and including a performance fix to resolve the issue that caused it to be reverted.

Performance improvement #

The previous implementation had inefficient code for adding the window resize event listeners: there was a circular dependency problem in src/components/flyout/manager/layout_mode.ts:

1. An event listener attaches to window resize events and updates the windowWidth state variable
2. A useEffect recalculates the layout mode and calls the reducer setMode action, which updates the Flyout Manger's state
3. The state update causes the Flyout Manager provider to re-create its context
4. setMode would be re-created because it depends on context
5. Because it depends on setMode, the useEffect triggers again and performs the layout mode calculation: this is a return back to Step 2 -> INFINITE LOOP

The fix was to refactor code to depend on stable references rather than the entire context. Primarily: the setMode callback now just depends on dispatch, which never changes. Secondarily, the large useEffect was broken up to re-run only when the specific values we care about are changed.

• Profile taken from Kibana before the fix
  page becomes unresponsive after brief window resizing:
  
• Profile testing the same use case after the fix
  page stays responsive after extensive window resizing:
  

NOTE: after the fix, when testing in Kibana, we still see the "JS Event Listeners" metric go up in Kibana after window resize, but this doesn't happen when testing the EUI storybook in isolation.

Screenshots #

The original EUI Flyout System screenshots can be found in #9068 (comment)

Impact to users

The original "Impact to users" and description of the breaking change can be found in #9068 (comment)

QA

Remove or strikethrough items that do not apply to your PR.

General checklist

• Browser QA
  • [ ] Checked in both light and dark modes
  • [ ] Checked in both MacOS and Windows high contrast modes
    • (emulate forced colors if you do not have access to a Windows machine.)
  • [ ] Checked in mobile
  • [ ] Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
• Docs site QA
  • [ ] Added documentation
  • [ ] Props have proper autodocs (using @default if default values are missing) and playground toggles
  • [ ] Checked Code Sandbox works for any docs examples
• Code quality checklist
  • Added or updated jest and cypress tests
  • [ ] Updated visual regression tests
• 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)
  • [ ] If the changes unblock an issue in a different repo, smoke tested carefully (see Testing EUI features in Kibana ahead of time)
• 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)

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 4aa837e
• Documentation website
• Storybook

History

• 💚 Build #1844 succeeded 8441401
• 💚 Build #1843 succeeded 34690cd
• 💚 Build #1842 succeeded e966888
• 💚 Build #1841 succeeded 53df4c0
• 💚 Build #1840 succeeded fe52040
• 💚 Build #1839 succeeded 3e0d623

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 4aa837e

History

• 💚 Build #5149 succeeded 8441401
• 💔 Build #5148 failed 34690cd
• 💔 Build #5146 failed 53df4c0
• 💔 Build #5145 failed fe52040
• 💚 Build #5144 succeeded 3e0d623

[tsullivan]

Hi @tkajtoch @weronikaolejniczak can we prioritize review of this? Compared to what has already been merged to main and reverted, the primary changes are the performance improvements.

[weronikaolejniczak]

Hey, @tsullivan 👋🏻 I can definitely take a look but I'd still wait for Tomasz when he's back from PTO on Wednesday. The thing is, we'd be able to release Flyouts next week the earliest anyways.

Lmk if you need a sanity check from me or there's no need and you prefer to wait for Tomasz 😄

[tsullivan]

Thanks @weronikaolejniczak I would appreciate a sanity check or whatever help you can offer. There are other PRs that need to be targeted to the feature branch, and since this one is to be the basis of the feature branch, those PRs are held up.

[tkajtoch]

Sorry folks, I did review the PR on Friday, but apparently forgot to submit it. Here it is:

I updated the base branch to target the new feat/flyout-system, which is now synced with main. The changes look and work as expected. I can confirm that the infinite recursion fix does the job and that the number of JS Event Listeners is not related to these changes. The number goes up in prod and is related to mouse hover/enter events. I couldn't find the exact cause of it, but it's definitely not the Flyout System, and it's very unlikely it's caused by EUI.

The test changes and pub-sub changes look good as well. LGTM 🚢