Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Browse the repository at this point in the history
# 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":"[email protected]"},"sourceCommit":{"committedDate":"2023-02-09T18:32:32Z","message":"[Dashboard] [Navigation] Fix mount point bug (#150507)\n\nCloses https://github.com/elastic/kibana/issues/150371\r\n\r\n## 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 https://github.com/elastic/kibana/issues/150371\r\n\r\n## 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 https://github.com/elastic/kibana/issues/150371\r\n\r\n## 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 <[email protected]>
\r\n\r\nWhen){kind=link}
- Loading branch information
1 parent
ea84378
commit fab0ddc