[Dashboard] [Navigation] Fix mount point bug #150507
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Heenawter
added
bug
Heenawter
force-pushed
the
fix-fullscreen-link_2023-02-07
branch
2 times, most recently
from
5da2c8f
to
404fb63
Compare
Heenawter
added
backport:prev-minor
Heenawter
force-pushed
the
fix-fullscreen-link_2023-02-07
branch
from
be4b6e7
to
9dd31c6
Compare
Heenawter
force-pushed
the
fix-fullscreen-link_2023-02-07
branch
from
9dd31c6
to
9151378
Compare
Heenawter
force-pushed
the
fix-fullscreen-link_2023-02-07
branch
from
9151378
to
dec6cf0
Compare
Heenawter
commented
Feb 9, 2023
| @@ -254,7 +220,45 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr | |||
| ref={dashboardTitleRef} | |||
| tabIndex={-1} | |||
| >{`${getDashboardBreadcrumb()} - ${dashboardTitle}`}</h1> | |||
| <TopNavMenu {...getNavBarProps()} /> | |||
| <TopNavMenu | |||
There was a problem hiding this comment.
Rather than calling and spreading the result of getNavBarProps, which goes against our usual pattern for grabbing props, I talked with @ThomThomson and we decided it was best to just put the props in directly - this makes the logic for determining the props much more clear.
This has nothing to do with the bug, it was just some code cleanup 👍
Heenawter
commented
Feb 9, 2023
| showSaveQuery={showSaveQuery} | ||
| appName={LEGACY_DASHBOARD_APP_ID} | ||
| visible={viewMode !== ViewMode.PRINT} | ||
| setMenuMountPoint={embedSettings || fullScreenMode ? undefined : setHeaderActionMenu} |
There was a problem hiding this comment.
This is where the actual bug fix is - the logic for this used to simply be:
setMenuMountPoint={embedSettings ? undefined : setHeaderActionMenu}By adding || fullScreenMode, we make it so that the portal gets unmounted every time fullscreen mode is entered, then remounted when it is exited 👍
ThomThomson
approved these changes
Feb 9, 2023
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @Heenawter |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Feb 9, 2023
Closes elastic#150371 ## Summary Oooh, man. This was a fun one! Okay, so. To try to understand this bug, we first have to understand a little bit about how the header action menu is rendered as part of the `TopNavMenu` component (in `dashboard_top_nav.tsx`). Essentially, as part of rendering `TopNavMenu`, a `MountPointPortal` is created for the `headerActionMenu` so that the items get rendered in the correct location, like so:  When there is **no** filter pill (i.e. `showSearchBar` is `false`), this `MountPointPortal` gets **unmounted** alongside the search bar as part of the `TopNavMenu` component (after all, only the dashboard viewport is getting rendered in this scenario). When you exit fullscreen mode, both the search bar and the `MountPointPortal` get re-mounted, which triggers a re-render, and so the `headerActionMenu` shows up as expected. Now, consider what happens when there **is** a filter pill (i.e. when `showSearchBar` is `true`). Because the search bar still needs to get rendered, neither it **nor** the `MountPointPortal` get unmounted - instead, the search bar simply gets re-rendered while the portal gets updated to point at an element that no longer exists (since the place where the `headerActionMenu` normally gets rendered is no longer present). Now, when you exit fullscreen mode, neither element needs to get re-mounted - they simply need to get re-rendered. Everything works great for the search bar, but the `headerActionMenu` seems to be rendering itself **before** the portal gets a chance to point to the correct place again - so, it doesn't show up unless a re-render of `TopNavMenu` is manually triggered (like, say, by removing the filter pill): https://user-images.githubusercontent.com/8698078/217868558-7eb73df8-3417-4d5a-ab5d-e7c9138532a9.mov So essentially, from what I can understand, this bug comes down to a race condition between the `MountPointPortal` telling the `headerActionMenu` **where** to render and the `TopNavMenu` actually rendering. Since the `MountPointPortal` **is not necessary** in fullscreen mode, regardless of if `showSearchBar` is `true` or `false`, the best way to prevent this bug is to simply ensure that it **always** gets unmounted when entering fullscreen mode - this ensures that the rendering always happens in the order that it is meant to. This can be accomplished by setting the `MenuMountPoint` to `undefined` when the dashboard enters fullscreen mode. As far as why this bug only happened on **click** and not when the escape key was hit... For some reason (React magic 🤷), hitting the escape key causes two renders of the `TopNavMenu` component whereas clicking the button only causes a single render. This means that, by the time the second render occurs when hitting escape, `MountPointPortal` has had a chance to update - hence, why the menu shows up where it's supposed to. ### Checklist - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 5d4c880)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Feb 9, 2023
# Backport This will backport the following commits from `main` to `8.7`: - [[Dashboard] [Navigation] Fix mount point bug (#150507)](#150507) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"Heenawter@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-02-09T18:32:32Z","message":"[Dashboard] [Navigation] Fix mount point bug (#150507)\n\nCloses #150371 Summary\r\n\r\nOooh, man. This was a fun one! \r\n\r\nOkay, so. To try to understand this bug, we first have to understand a\r\nlittle bit about how the header action menu is rendered as part of the\r\n`TopNavMenu` component (in `dashboard_top_nav.tsx`). Essentially, as\r\npart of rendering `TopNavMenu`, a `MountPointPortal` is created for the\r\n`headerActionMenu` so that the items get rendered in the correct\r\nlocation, like so:\r\n\r\n\r\n\r\n\r\nWhen there is **no** filter pill (i.e. `showSearchBar` is `false`), this\r\n`MountPointPortal` gets **unmounted** alongside the search bar as part\r\nof the `TopNavMenu` component (after all, only the dashboard viewport is\r\ngetting rendered in this scenario). When you exit fullscreen mode, both\r\nthe search bar and the `MountPointPortal` get re-mounted, which triggers\r\na re-render, and so the `headerActionMenu` shows up as expected.\r\n\r\nNow, consider what happens when there **is** a filter pill (i.e. when\r\n`showSearchBar` is `true`). Because the search bar still needs to get\r\nrendered, neither it **nor** the `MountPointPortal` get unmounted -\r\ninstead, the search bar simply gets re-rendered while the portal gets\r\nupdated to point at an element that no longer exists (since the place\r\nwhere the `headerActionMenu` normally gets rendered is no longer\r\npresent). Now, when you exit fullscreen mode, neither element needs to\r\nget re-mounted - they simply need to get re-rendered. Everything works\r\ngreat for the search bar, but the `headerActionMenu` seems to be\r\nrendering itself **before** the portal gets a chance to point to the\r\ncorrect place again - so, it doesn't show up unless a re-render of\r\n`TopNavMenu` is manually triggered (like, say, by removing the filter\r\npill):\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/217868558-7eb73df8-3417-4d5a-ab5d-e7c9138532a9.mov\r\n\r\n\r\nSo essentially, from what I can understand, this bug comes down to a\r\nrace condition between the `MountPointPortal` telling the\r\n`headerActionMenu` **where** to render and the `TopNavMenu` actually\r\nrendering. Since the `MountPointPortal` **is not necessary** in\r\nfullscreen mode, regardless of if `showSearchBar` is `true` or `false`,\r\nthe best way to prevent this bug is to simply ensure that it **always**\r\ngets unmounted when entering fullscreen mode - this ensures that the\r\nrendering always happens in the order that it is meant to. This can be\r\naccomplished by setting the `MenuMountPoint` to `undefined` when the\r\ndashboard enters fullscreen mode.\r\n\r\nAs far as why this bug only happened on **click** and not when the\r\nescape key was hit... For some reason (React magic 🤷), hitting the\r\nescape key causes two renders of the `TopNavMenu` component whereas\r\nclicking the button only causes a single render. This means that, by the\r\ntime the second render occurs when hitting escape, `MountPointPortal`\r\nhas had a chance to update - hence, why the menu shows up where it's\r\nsupposed to.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"5d4c88057355062f92f01f570a0efc42e25b3483","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Feature:Dashboard","release_note:fix","Team:Presentation","loe:days","impact:high","Team:SharedUX","backport:prev-minor","v8.7.0","v8.8.0"],"number":150507,"url":"https://github.com/elastic/kibana/pull/150507","mergeCommit":{"message":"[Dashboard] [Navigation] Fix mount point bug (#150507)\n\nCloses #150371 Summary\r\n\r\nOooh, man. This was a fun one! \r\n\r\nOkay, so. To try to understand this bug, we first have to understand a\r\nlittle bit about how the header action menu is rendered as part of the\r\n`TopNavMenu` component (in `dashboard_top_nav.tsx`). Essentially, as\r\npart of rendering `TopNavMenu`, a `MountPointPortal` is created for the\r\n`headerActionMenu` so that the items get rendered in the correct\r\nlocation, like so:\r\n\r\n\r\n\r\n\r\nWhen there is **no** filter pill (i.e. `showSearchBar` is `false`), this\r\n`MountPointPortal` gets **unmounted** alongside the search bar as part\r\nof the `TopNavMenu` component (after all, only the dashboard viewport is\r\ngetting rendered in this scenario). When you exit fullscreen mode, both\r\nthe search bar and the `MountPointPortal` get re-mounted, which triggers\r\na re-render, and so the `headerActionMenu` shows up as expected.\r\n\r\nNow, consider what happens when there **is** a filter pill (i.e. when\r\n`showSearchBar` is `true`). Because the search bar still needs to get\r\nrendered, neither it **nor** the `MountPointPortal` get unmounted -\r\ninstead, the search bar simply gets re-rendered while the portal gets\r\nupdated to point at an element that no longer exists (since the place\r\nwhere the `headerActionMenu` normally gets rendered is no longer\r\npresent). Now, when you exit fullscreen mode, neither element needs to\r\nget re-mounted - they simply need to get re-rendered. Everything works\r\ngreat for the search bar, but the `headerActionMenu` seems to be\r\nrendering itself **before** the portal gets a chance to point to the\r\ncorrect place again - so, it doesn't show up unless a re-render of\r\n`TopNavMenu` is manually triggered (like, say, by removing the filter\r\npill):\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/217868558-7eb73df8-3417-4d5a-ab5d-e7c9138532a9.mov\r\n\r\n\r\nSo essentially, from what I can understand, this bug comes down to a\r\nrace condition between the `MountPointPortal` telling the\r\n`headerActionMenu` **where** to render and the `TopNavMenu` actually\r\nrendering. Since the `MountPointPortal` **is not necessary** in\r\nfullscreen mode, regardless of if `showSearchBar` is `true` or `false`,\r\nthe best way to prevent this bug is to simply ensure that it **always**\r\ngets unmounted when entering fullscreen mode - this ensures that the\r\nrendering always happens in the order that it is meant to. This can be\r\naccomplished by setting the `MenuMountPoint` to `undefined` when the\r\ndashboard enters fullscreen mode.\r\n\r\nAs far as why this bug only happened on **click** and not when the\r\nescape key was hit... For some reason (React magic 🤷), hitting the\r\nescape key causes two renders of the `TopNavMenu` component whereas\r\nclicking the button only causes a single render. This means that, by the\r\ntime the second render occurs when hitting escape, `MountPointPortal`\r\nhas had a chance to update - hence, why the menu shows up where it's\r\nsupposed to.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"5d4c88057355062f92f01f570a0efc42e25b3483"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/150507","number":150507,"mergeCommit":{"message":"[Dashboard] [Navigation] Fix mount point bug (#150507)\n\nCloses #150371 Summary\r\n\r\nOooh, man. This was a fun one! \r\n\r\nOkay, so. To try to understand this bug, we first have to understand a\r\nlittle bit about how the header action menu is rendered as part of the\r\n`TopNavMenu` component (in `dashboard_top_nav.tsx`). Essentially, as\r\npart of rendering `TopNavMenu`, a `MountPointPortal` is created for the\r\n`headerActionMenu` so that the items get rendered in the correct\r\nlocation, like so:\r\n\r\n\r\n\r\n\r\nWhen there is **no** filter pill (i.e. `showSearchBar` is `false`), this\r\n`MountPointPortal` gets **unmounted** alongside the search bar as part\r\nof the `TopNavMenu` component (after all, only the dashboard viewport is\r\ngetting rendered in this scenario). When you exit fullscreen mode, both\r\nthe search bar and the `MountPointPortal` get re-mounted, which triggers\r\na re-render, and so the `headerActionMenu` shows up as expected.\r\n\r\nNow, consider what happens when there **is** a filter pill (i.e. when\r\n`showSearchBar` is `true`). Because the search bar still needs to get\r\nrendered, neither it **nor** the `MountPointPortal` get unmounted -\r\ninstead, the search bar simply gets re-rendered while the portal gets\r\nupdated to point at an element that no longer exists (since the place\r\nwhere the `headerActionMenu` normally gets rendered is no longer\r\npresent). Now, when you exit fullscreen mode, neither element needs to\r\nget re-mounted - they simply need to get re-rendered. Everything works\r\ngreat for the search bar, but the `headerActionMenu` seems to be\r\nrendering itself **before** the portal gets a chance to point to the\r\ncorrect place again - so, it doesn't show up unless a re-render of\r\n`TopNavMenu` is manually triggered (like, say, by removing the filter\r\npill):\r\n\r\n\r\n\r\nhttps://user-images.githubusercontent.com/8698078/217868558-7eb73df8-3417-4d5a-ab5d-e7c9138532a9.mov\r\n\r\n\r\nSo essentially, from what I can understand, this bug comes down to a\r\nrace condition between the `MountPointPortal` telling the\r\n`headerActionMenu` **where** to render and the `TopNavMenu` actually\r\nrendering. Since the `MountPointPortal` **is not necessary** in\r\nfullscreen mode, regardless of if `showSearchBar` is `true` or `false`,\r\nthe best way to prevent this bug is to simply ensure that it **always**\r\ngets unmounted when entering fullscreen mode - this ensures that the\r\nrendering always happens in the order that it is meant to. This can be\r\naccomplished by setting the `MenuMountPoint` to `undefined` when the\r\ndashboard enters fullscreen mode.\r\n\r\nAs far as why this bug only happened on **click** and not when the\r\nescape key was hit... For some reason (React magic 🤷), hitting the\r\nescape key causes two renders of the `TopNavMenu` component whereas\r\nclicking the button only causes a single render. This means that, by the\r\ntime the second render occurs when hitting escape, `MountPointPortal`\r\nhas had a chance to update - hence, why the menu shows up where it's\r\nsupposed to.\r\n\r\n\r\n### Checklist\r\n\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"5d4c88057355062f92f01f570a0efc42e25b3483"}}]}] BACKPORT--> Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Feb 10, 2023
* main: (115 commits) [Custom branding] Add custom logo to space selector (elastic#150284) [api-docs] 2023-02-10 Daily api_docs build (elastic#150831) [ci] build next docs in PRs when relevant files change (elastic#149991) [codeowners] allow overrides to take higher precedence (elastic#150821) [docs] Remove kibDevDocsOpsPluginDiscovery (elastic#150788) [Fleet] Fix max 20 installed integrations returned from Fleet API (elastic#150780) [maps] fix Changing resolutions on Heat map layer throws error in console (elastic#150761) fixes Failing ES Promotion: X-Pack API Integration Tests x-pack/test/api_integration/apis/maps/get_grid_tile.js (elastic#150768) [Synthetics] adjust overview scrolling e2e (elastic#150774) [Security Solution] Fixes bulk close alerts from exception flyout type bug (elastic#150765) Upgrade EUI to v74.1.0 (elastic#150235) [skip ci] Fix labeling for Infrastructure UI (elastic#150571) [Enterprise Search] Move pipelines modal to flyout (elastic#150727) [Security Solution] fix flaky endpoint tests (elastic#150652) Fixes the space selector page layout (elastic#150503) [Dashboard] [Navigation] Fix mount point bug (elastic#150507) [Infrastructure UI] Track host cloud provider on table entry click (elastic#150685) [Dashboard Usability] Moves scrollbar to panel section (elastic#145628) [Maps] fixes Kibana maps shows MVT borders if the geometry border style is greater than 1 (elastic#150497) [Cloud Posture][Dashboard] dashboard re-design enhancements (elastic#150394) ...
\r\n\r\nWhen){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #150371
Summary
Oooh, man. This was a fun one!
Okay, so. To try to understand this bug, we first have to understand a little bit about how the header action menu is rendered as part of the
TopNavMenucomponent (indashboard_top_nav.tsx). Essentially, as part of renderingTopNavMenu, aMountPointPortalis created for theheaderActionMenuso that the items get rendered in the correct location, like so:When there is no filter pill (i.e.
showSearchBarisfalse), thisMountPointPortalgets unmounted alongside the search bar as part of theTopNavMenucomponent (after all, only the dashboard viewport is getting rendered in this scenario). When you exit fullscreen mode, both the search bar and theMountPointPortalget re-mounted, which triggers a re-render, and so theheaderActionMenushows up as expected.Now, consider what happens when there is a filter pill (i.e. when
showSearchBaristrue). Because the search bar still needs to get rendered, neither it nor theMountPointPortalget unmounted - instead, the search bar simply gets re-rendered while the portal gets updated to point at an element that no longer exists (since the place where theheaderActionMenunormally gets rendered is no longer present). Now, when you exit fullscreen mode, neither element needs to get re-mounted - they simply need to get re-rendered. Everything works great for the search bar, but theheaderActionMenuseems to be rendering itself before the portal gets a chance to point to the correct place again - so, it doesn't show up unless a re-render ofTopNavMenuis manually triggered (like, say, by removing the filter pill):Screen.Recording.2023-02-09.at.9.05.32.AM.mov
So essentially, from what I can understand, this bug comes down to a race condition between the
MountPointPortaltelling theheaderActionMenuwhere to render and theTopNavMenuactually rendering. Since theMountPointPortalis not necessary in fullscreen mode, regardless of ifshowSearchBaristrueorfalse, the best way to prevent this bug is to simply ensure that it always gets unmounted when entering fullscreen mode - this ensures that the rendering always happens in the order that it is meant to. This can be accomplished by setting theMenuMountPointtoundefinedwhen the dashboard enters fullscreen mode.As far as why this bug only happened on click and not when the escape key was hit... For some reason (React magic 🤷), hitting the escape key causes two renders of the
TopNavMenucomponent whereas clicking the button only causes a single render. This means that, by the time the second render occurs when hitting escape,MountPointPortalhas had a chance to update - hence, why the menu shows up where it's supposed to.Checklist
For maintainers