github-mirrors/kibana
1
0
Fork 0
mirror of https://github.com/elastic/kibana.git synced 2025-04-24 01:38:56 -04:00
Code Issues Projects Releases Packages Wiki Activity

[Dashboard] [Navigation] Fix mount point bug (#150507)

Browse source 
Closes https://github.com/elastic/kibana/issues/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:


![WithAndWithoutPortal](https://user-images.githubusercontent.com/8698078/217674254-cd27470b-fcce-4cb9-b960-51ea2b8a68c8.png)

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)
This commit is contained in:
Hannah Mudge 2023-02-09 11:32:32 -07:00 • committed by GitHub
parent 26eddec708
commit 5d4c880573
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
1 changed files with 42 additions and 38 deletions
Download patch file Download diff file Expand all files Collapse all files

80
src/plugins/dashboard/public/dashboard_app/top_nav/dashboard_top_nav.tsx
View file

@ -16,7 +16,6 @@ import {
} from '@kbn/presentation-util-plugin/public';
import { ViewMode } from '@kbn/embeddable-plugin/public';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { TopNavMenuProps } from '@kbn/navigation-plugin/public';
import { EuiHorizontalRule } from '@elastic/eui';
import {
@ -68,7 +67,6 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
} = pluginServices.getServices();
const isLabsEnabled = uiSettings.get(UI_SETTINGS.ENABLE_LABS_UI);
const { setHeaderActionMenu, onAppLeave } = useDashboardMountContext();
/**
* Unpack dashboard state from redux
*/
@ -187,10 +185,9 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
setIsLabsShown,
});
const getNavBarProps = (): TopNavMenuProps => {
const visibilityProps = useMemo(() => {
const shouldShowNavBarComponent = (forceShow: boolean): boolean =>
(forceShow || isChromeVisible) && !fullScreenMode;
const shouldShowFilterBar = (forceHide: boolean): boolean =>
!forceHide && (filterManager.getFilters().length > 0 || !fullScreenMode);
@ -202,46 +199,15 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
const showFilterBar = shouldShowFilterBar(Boolean(embedSettings?.forceHideFilterBar));
const showQueryBar = showQueryInput || showDatePicker || showFilterBar;
const showSearchBar = showQueryBar || showFilterBar;
const topNavConfig = viewMode === ViewMode.EDIT ? editModeTopNavConfig : viewModeTopNavConfig;
const badges =
hasUnsavedChanges && viewMode === ViewMode.EDIT
? [
{
'data-test-subj': 'dashboardUnsavedChangesBadge',
badgeText: unsavedChangesBadgeStrings.getUnsavedChangedBadgeText(),
color: 'success',
},
]
: undefined;
return {
query,
badges,
savedQueryId,
showTopNavMenu,
showSearchBar,
showFilterBar,
showSaveQuery,
showQueryInput,
showDatePicker,
screenTitle: title,
useDefaultBehaviors: true,
appName: LEGACY_DASHBOARD_APP_ID,
visible: viewMode !== ViewMode.PRINT,
indexPatterns: allDataViews,
config: showTopNavMenu ? topNavConfig : undefined,
setMenuMountPoint: embedSettings ? undefined : setHeaderActionMenu,
className: fullScreenMode ? 'kbnTopNavMenu-isFullScreen' : undefined,
onQuerySubmit: (_payload, isUpdate) => {
if (isUpdate === false) {
dashboardContainer.forceRefresh();
}
},
onSavedQueryIdChange: (newId: string | undefined) => {
dispatch(setSavedQueryId(newId));
},
};
};
}, [embedSettings, filterManager, fullScreenMode, isChromeVisible, viewMode]);
UseUnmount(() => {
dashboardContainer.clearOverlays();
@ -255,7 +221,45 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
ref={dashboardTitleRef}
tabIndex={-1}
>{`${getDashboardBreadcrumb()} - ${dashboardTitle}`}</h1>
<TopNavMenu {...getNavBarProps()} />
<TopNavMenu
{...visibilityProps}
query={query}
screenTitle={title}
useDefaultBehaviors={true}
indexPatterns={allDataViews}
savedQueryId={savedQueryId}
showSaveQuery={showSaveQuery}
appName={LEGACY_DASHBOARD_APP_ID}
visible={viewMode !== ViewMode.PRINT}
setMenuMountPoint={embedSettings || fullScreenMode ? undefined : setHeaderActionMenu}
className={fullScreenMode ? 'kbnTopNavMenu-isFullScreen' : undefined}
config={
visibilityProps.showTopNavMenu
? viewMode === ViewMode.EDIT
? editModeTopNavConfig
: viewModeTopNavConfig
: undefined
}
badges={
hasUnsavedChanges && viewMode === ViewMode.EDIT
? [
{
'data-test-subj': 'dashboardUnsavedChangesBadge',
badgeText: unsavedChangesBadgeStrings.getUnsavedChangedBadgeText(),
color: 'success',
},
]
: undefined
}
onQuerySubmit={(_payload, isUpdate) => {
if (isUpdate === false) {
dashboardContainer.forceRefresh();
}
}}
onSavedQueryIdChange={(newId: string | undefined) => {
dispatch(setSavedQueryId(newId));
}}
/>
{viewMode !== ViewMode.PRINT && isLabsEnabled && isLabsShown ? (
<PresentationUtilContextProvider>
<LabsFlyout solutions={['dashboard']} onClose={() => setIsLabsShown(false)} />