Skip to content

Commit

Permalink
[8.x] fix dashboard grid item performs 2 DOM queries every render (#1…
Browse files Browse the repository at this point in the history
…99390) (#200648)

# Backport

This will backport the following commits from `main` to `8.x`:
- [fix dashboard grid item performs 2 DOM queries every render
(#199390)](#199390)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-18T18:32:53Z","message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","v9.0.0","ci:project-deploy-observability","Team:obs-ux-management","backport:version","v8.17.0"],"number":199390,"url":"https://github.com/elastic/kibana/pull/199390","mergeCommit":{"message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199390","number":199390,"mergeCommit":{"message":"fix
dashboard grid item performs 2 DOM queries every render
(#199390)\n\nCloses
https://github.com/elastic/kibana/issues/199361\r\n\r\nWhile
investigating, I found that fetching DOM element with
id\r\n`app-fixed-viewport` is a common pattern. I created the
hook\r\n`useAppFixedViewport` to consolidate this logic into a single
location.\r\nThe hook only performs the DOM look-up on first render and
then avoids\r\nthe DOM look-up on each additional
render.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"9f545039ab42e95fd1d3d0518da4df6a8d040177"}},{"branch":"8.x","label":"v8.17.0","labelRegex":"^v8.17.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
  • Loading branch information
nreese authored Nov 18, 2024
1 parent 25f42b7 commit 7e5eeb3
Show file tree
Hide file tree
Showing 27 changed files with 148 additions and 18 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@
"@kbn/core-preboot-server": "link:packages/core/preboot/core-preboot-server",
"@kbn/core-preboot-server-internal": "link:packages/core/preboot/core-preboot-server-internal",
"@kbn/core-provider-plugin": "link:test/plugin_functional/plugins/core_provider_plugin",
"@kbn/core-rendering-browser": "link:packages/core/rendering/core-rendering-browser",
"@kbn/core-rendering-browser-internal": "link:packages/core/rendering/core-rendering-browser-internal",
"@kbn/core-rendering-server-internal": "link:packages/core/rendering/core-rendering-server-internal",
"@kbn/core-root-browser-internal": "link:packages/core/root/core-root-browser-internal",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { I18nStart } from '@kbn/core-i18n-browser';
import type { OverlayStart } from '@kbn/core-overlays-browser';
import type { ThemeServiceStart } from '@kbn/core-theme-browser';
import { KibanaRootContextProvider } from '@kbn/react-kibana-context-root';
import { APP_FIXED_VIEWPORT_ID } from '@kbn/core-rendering-browser';
import { AppWrapper } from './app_containers';

interface StartServices {
Expand Down Expand Up @@ -68,7 +69,7 @@ export class RenderingService {
{/* The App Wrapper outside of the fixed headers that accepts custom class names from apps */}
<AppWrapper chromeVisible$={chrome.getIsVisible$()}>
{/* Affixes a div to restrict the position of charts tooltip to the visible viewport minus the header */}
<div id="app-fixed-viewport" />
<div id={APP_FIXED_VIEWPORT_ID} />

{/* The actual plugin/app */}
{appComponent}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"@kbn/core-analytics-browser-mocks",
"@kbn/core-analytics-browser",
"@kbn/core-i18n-browser",
"@kbn/core-theme-browser"
"@kbn/core-theme-browser",
"@kbn/core-rendering-browser"
],
"exclude": [
"target/**/*",
Expand Down
4 changes: 4 additions & 0 deletions packages/core/rendering/core-rendering-browser/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# @kbn/core-rendering-browser

This package contains the types and implementation for Core's browser-side rendering service.

10 changes: 10 additions & 0 deletions packages/core/rendering/core-rendering-browser/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { APP_FIXED_VIEWPORT_ID, useAppFixedViewport } from './src';
14 changes: 14 additions & 0 deletions packages/core/rendering/core-rendering-browser/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

module.exports = {
preset: '@kbn/test',
rootDir: '../../../..',
roots: ['<rootDir>/packages/core/rendering/core-rendering-browser'],
};
5 changes: 5 additions & 0 deletions packages/core/rendering/core-rendering-browser/kibana.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "shared-browser",
"id": "@kbn/core-rendering-browser",
"owner": "@elastic/kibana-core"
}
7 changes: 7 additions & 0 deletions packages/core/rendering/core-rendering-browser/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@kbn/core-rendering-browser",
"private": true,
"version": "1.0.0",
"author": "Kibana Core",
"license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0"
}
10 changes: 10 additions & 0 deletions packages/core/rendering/core-rendering-browser/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { APP_FIXED_VIEWPORT_ID, useAppFixedViewport } from './use_app_fixed_viewport';
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { useRef } from 'react';

export const APP_FIXED_VIEWPORT_ID = 'app-fixed-viewport';

export function useAppFixedViewport() {
const ref = useRef(document.getElementById(APP_FIXED_VIEWPORT_ID) ?? undefined);
return ref.current;
}
19 changes: 19 additions & 0 deletions packages/core/rendering/core-rendering-browser/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"extends": "../../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "target/types",
"types": [
"jest",
"node",
"react"
]
},
"include": [
"**/*.ts",
"**/*.tsx",
],
"kbn_references": [],
"exclude": [
"target/**/*",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
} from '@kbn/expressions-plugin/public';
import type { FieldFormat } from '@kbn/field-formats-plugin/common';
import { getOverridesFor } from '@kbn/chart-expressions-common';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { consolidateMetricColumns } from '../../common/utils';
import { DEFAULT_PERCENT_DECIMALS } from '../../common/constants';
import {
Expand Down Expand Up @@ -385,7 +386,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
[visType, visParams, containerDimensions, rescaleFactor, hasOpenedOnAggBasedEditor]
);

const fixedViewPort = document.getElementById('app-fixed-viewport');
const fixedViewPort = useAppFixedViewport();

const legendPosition = visParams.legendPosition ?? Position.Right;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"@kbn/chart-expressions-common",
"@kbn/cell-actions",
"@kbn/react-kibana-context-render",
"@kbn/core-rendering-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
} from '@kbn/visualizations-plugin/common/constants';
import { PersistedState } from '@kbn/visualizations-plugin/public';
import { getOverridesFor, ChartSizeSpec } from '@kbn/chart-expressions-common';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import type {
FilterEvent,
BrushEvent,
Expand Down Expand Up @@ -232,6 +233,7 @@ export function XYChart({
const chartRef = useRef<Chart>(null);
const chartBaseTheme = chartsThemeService.useChartsBaseTheme();
const darkMode = chartsThemeService.useDarkMode();
const appFixedViewport = useAppFixedViewport();
const filteredLayers = getFilteredLayers(layers);
const layersById = filteredLayers.reduce<Record<string, CommonXYLayerConfig>>(
(hashMap, layer) => ({ ...hashMap, [layer.layerId]: layer }),
Expand Down Expand Up @@ -767,7 +769,7 @@ export function XYChart({
>
<Chart ref={chartRef} {...getOverridesFor(overrides, 'chart')}>
<Tooltip<Record<string, string | number>, XYChartSeriesIdentifier>
boundary={document.getElementById('app-fixed-viewport') ?? undefined}
boundary={appFixedViewport}
headerFormatter={
!args.detailedTooltip && xAxisColumn
? ({ value }) => (
Expand Down
1 change: 1 addition & 0 deletions src/plugins/chart_expressions/expression_xy/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@kbn/es-query",
"@kbn/cell-actions",
"@kbn/react-kibana-context-render",
"@kbn/core-rendering-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ export function FiltersNotificationPopover({ api }: { api: FiltersNotificationAc
}
}, [api, setDisableEditButton]);

const [hasLockedHoverActions, dataViews, parentViewMode] = useBatchedOptionalPublishingSubjects(
api.hasLockedHoverActions$,
const [dataViews, parentViewMode] = useBatchedOptionalPublishingSubjects(
api.parentApi?.dataViews,
getViewModeSubject(api ?? undefined)
);
Expand All @@ -77,7 +76,7 @@ export function FiltersNotificationPopover({ api }: { api: FiltersNotificationAc
onClick={() => {
setIsPopoverOpen(!isPopoverOpen);
if (apiCanLockHoverActions(api)) {
api?.lockHoverActions(!hasLockedHoverActions);
api?.lockHoverActions(!api.hasLockedHoverActions$.value);
}
}}
data-test-subj={`embeddablePanelNotification-${api.uuid}`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,21 @@ import { Layout, Responsive as ResponsiveReactGridLayout } from 'react-grid-layo
import { ViewMode } from '@kbn/embeddable-plugin/public';

import { useBatchedPublishingSubjects } from '@kbn/presentation-publishing';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { DashboardPanelState } from '../../../../common';
import { DashboardGridItem } from './dashboard_grid_item';
import { useDashboardGridSettings } from './use_dashboard_grid_settings';
import { useDashboardApi } from '../../../dashboard_api/use_dashboard_api';
import { getPanelLayoutsAreEqual } from '../../state/diffing/dashboard_diffing_utils';
import { DASHBOARD_GRID_HEIGHT, DASHBOARD_MARGIN_SIZE } from '../../../dashboard_constants';

export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
export const DashboardGrid = ({
dashboardContainer,
viewportWidth,
}: {
dashboardContainer?: HTMLElement;
viewportWidth: number;
}) => {
const dashboardApi = useDashboardApi();

const [animatePanelTransforms, expandedPanelId, focusedPanelId, panels, useMargins, viewMode] =
Expand All @@ -51,6 +58,8 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
}
}, [expandedPanelId]);

const appFixedViewport = useAppFixedViewport();

const panelsInOrder: string[] = useMemo(() => {
return Object.keys(panels).sort((embeddableIdA, embeddableIdB) => {
const panelA = panels[embeddableIdA];
Expand All @@ -72,6 +81,8 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
const type = panels[embeddableId].type;
return (
<DashboardGridItem
appFixedViewport={appFixedViewport}
dashboardContainer={dashboardContainer}
data-grid={panels[embeddableId].gridData}
key={embeddableId}
id={embeddableId}
Expand All @@ -82,7 +93,14 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
/>
);
});
}, [expandedPanelId, panels, panelsInOrder, focusedPanelId]);
}, [
appFixedViewport,
dashboardContainer,
expandedPanelId,
panels,
panelsInOrder,
focusedPanelId,
]);

const onLayoutChange = useCallback(
(newLayout: Array<Layout & { i: string }>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { embeddableService, presentationUtilService } from '../../../services/ki
type DivProps = Pick<React.HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'children'>;

export interface Props extends DivProps {
appFixedViewport?: HTMLElement;
dashboardContainer?: HTMLElement;
id: DashboardPanelState['explicitInput']['id'];
index?: number;
type: DashboardPanelState['type'];
Expand All @@ -35,6 +37,8 @@ export interface Props extends DivProps {
export const Item = React.forwardRef<HTMLDivElement, Props>(
(
{
appFixedViewport,
dashboardContainer,
expandedPanelId,
focusedPanelId,
id,
Expand Down Expand Up @@ -92,10 +96,8 @@ export const Item = React.forwardRef<HTMLDivElement, Props>(
}
}, [id, dashboardApi, scrollToPanelId, highlightPanelId, ref, blurPanel]);

const dashboardContainerTopOffset =
(document.querySelector('.dashboardContainer') as HTMLDivElement)?.offsetTop || 0;
const globalNavTopOffset =
(document.querySelector('#app-fixed-viewport') as HTMLDivElement)?.offsetTop || 0;
const dashboardContainerTopOffset = dashboardContainer?.offsetTop || 0;
const globalNavTopOffset = appFixedViewport?.offsetTop || 0;

const focusStyles = blurPanel
? css`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const useDebouncedWidthObserver = (skipDebounce = false, wait = 100) => {
return { ref, width };
};

export const DashboardViewport = () => {
export const DashboardViewport = ({ dashboardContainer }: { dashboardContainer?: HTMLElement }) => {
const dashboardApi = useDashboardApi();
const [hasControls, setHasControls] = useState(false);
const [
Expand Down Expand Up @@ -160,7 +160,9 @@ export const DashboardViewport = () => {
otherwise, there is a race condition where the panels can end up being squashed
TODO only render when dashboardInitialized
*/}
{viewportWidth !== 0 && <DashboardGrid viewportWidth={viewportWidth} />}
{viewportWidth !== 0 && (
<DashboardGrid dashboardContainer={dashboardContainer} viewportWidth={viewportWidth} />
)}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ export class DashboardContainer
coreStart={{ chrome: coreServices.chrome, customBranding: coreServices.customBranding }}
>
<DashboardContext.Provider value={this as DashboardApi}>
<DashboardViewport />
<DashboardViewport dashboardContainer={this.domNode} />
</DashboardContext.Provider>
</ExitFullScreenButtonKibanaProvider>
</KibanaRenderContextProvider>,
Expand Down
1 change: 1 addition & 0 deletions src/plugins/dashboard/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"@kbn/core-custom-branding-browser-mocks",
"@kbn/core-mount-utils-browser",
"@kbn/visualization-utils",
"@kbn/core-rendering-browser",
],
"exclude": ["target/**/*"]
}
2 changes: 2 additions & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@
"@kbn/core-preboot-server-mocks/*": ["packages/core/preboot/core-preboot-server-mocks/*"],
"@kbn/core-provider-plugin": ["test/plugin_functional/plugins/core_provider_plugin"],
"@kbn/core-provider-plugin/*": ["test/plugin_functional/plugins/core_provider_plugin/*"],
"@kbn/core-rendering-browser": ["packages/core/rendering/core-rendering-browser"],
"@kbn/core-rendering-browser/*": ["packages/core/rendering/core-rendering-browser/*"],
"@kbn/core-rendering-browser-internal": ["packages/core/rendering/core-rendering-browser-internal"],
"@kbn/core-rendering-browser-internal/*": ["packages/core/rendering/core-rendering-browser-internal/*"],
"@kbn/core-rendering-browser-mocks": ["packages/core/rendering/core-rendering-browser-mocks"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
} from '@elastic/charts';
import { useEuiTheme } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { useBaseChartTheme } from '../../../../../../hooks/use_base_chart_theme';
import { BAR_HEIGHT } from './constants';
import { WaterfallChartChartContainer, WaterfallChartTooltip } from './styles';
Expand Down Expand Up @@ -86,6 +87,8 @@ export const WaterfallBarChart = ({
const handleProjectionClick = useMemo(() => onProjectionClick, [onProjectionClick]);
const memoizedTickFormat = useCallback(tickFormat, [tickFormat]);

const appFixedViewport = useAppFixedViewport();

return (
<WaterfallChartChartContainer
height={getChartHeight(chartData)}
Expand All @@ -96,7 +99,7 @@ export const WaterfallBarChart = ({
<Tooltip
// this is done to prevent the waterfall tooltip from rendering behind Kibana's
// stacked header when the user highlights an item at the top of the chart
boundary={document.getElementById('app-fixed-viewport') ?? undefined}
boundary={appFixedViewport}
customTooltip={CustomTooltip}
/>
<Settings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"@kbn/ebt-tools",
"@kbn/alerting-types",
"@kbn/core-chrome-browser",
"@kbn/core-rendering-browser",
"@kbn/index-lifecycle-management-common-shared"
],
"exclude": ["target/**/*"]
Expand Down
Loading

0 comments on commit 7e5eeb3

Please sign in to comment.