[EuiResizeObserver] Remove reflow/`getBoundingClientRect()` usage on resize #7575

cee-chen · 2024-03-12T23:52:46Z

Summary

See @davismcphee's comment in #7462 (comment):

It looks like useResizeObserver relies on getBoundingClientRect because contentRect only provides the content sizing for elements instead of box sizing:

eui/src/components/observer/resize_observer/resize_observer.tsx

Lines 106 to 109 in 635f860

// `entry.contentRect` provides incomplete `height` and `width` data.

// Use `getBoundingClientRect` to account for padding and border.

// https://developer.mozilla.org/en-US/docs/Web/API/DOMRectReadOnly

const { height, width } = container.getBoundingClientRect();

But I assume this code is old and isn't actually needed anymore since ResizeObserverEntry now supports borderBoxSize to access box sizing.

This is definitely a refactor/perf improvement we want across the board in our resize observer (both component and hook), hence me opening this PR.

QA

Go to https://eui.elastic.co/pr_7575/#/utilities/resize-observer
Confirm both examples print the correct dimensions on page load (compare against production)
- Note: It looks like subpixel rounding is slightly different on Chrome/FF than on Safari. IMO, this is fine and well worth the perf tradeoff
Add items and padding and confirm the measurements update as expected (also compared against production)

General checklist

Browser QA
- Checked in Chrome, Safari, Edge, and Firefox
Docs site QA - N/A
Code quality checklist - Skipped as there are no jsdom/unit tests for the resize observer API. Manual QA for now
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)~~
Designer checklist - N/A

…ClientRect()`

cee-chen · 2024-03-13T01:48:14Z

src/components/observer/resize_observer/resize_observer.tsx

-      // ResizeObserver's first call to the observation callback is scheduled in the future
-      // so find the container's initial dimensions now
-      const boundingRect = container.getBoundingClientRect();
-      setSize({
-        width: boundingRect.width,
-        height: boundingRect.height,
-      });


I've opted to remove this one-off getBoundingClientRect() call on mount because it appears to only have been added purely for EuiDataGrid (#2991 (review)) and to be completely frank, at this point EuiDataGrid has a lot more performance issues going on than a quick flash of unsized columns. The extra reflow/performance hit is, IMO, not worth it 🤷

Is this still happening? I tried to reproduce this in the EUI docs but I had the same result on the feature branch and on production. (but maybe I'm missing some context on how to reproduce it).

In any case, I'm ok with removing it. We can keep an eye out if there will be issues raised because of it.

I wasn't able to repro either but I wasn't trying super hard or analyzing frame by frame like Dave apparently was 😅 EuiDataGrid has changed a lot since then so def possible it either isn't an issue or simply isn't worth it anymore!

kibanamachine · 2024-03-13T02:15:14Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7575/
Storybook: https://eui.elastic.co/pr_7575/storybook

elasticmachine · 2024-03-13T02:15:17Z

💚 Build Succeeded

Buildkite Build
Commit: b5cdda0

History

💚 Build #1489 succeeded 6df1915

mgadewoll

🦄 LGTM!

mgadewoll · 2024-03-13T15:07:03Z

src/components/observer/resize_observer/resize_observer.tsx

-      // ResizeObserver's first call to the observation callback is scheduled in the future
-      // so find the container's initial dimensions now
-      const boundingRect = container.getBoundingClientRect();
-      setSize({
-        width: boundingRect.width,
-        height: boundingRect.height,
-      });


Is this still happening? I tried to reproduce this in the EUI docs but I had the same result on the feature branch and on production. (but maybe I'm missing some context on how to reproduce it).

In any case, I'm ok with removing it. We can keep an eye out if there will be issues raised because of it.

cee-chen · 2024-03-13T16:39:49Z

Thanks Lene!

cee-chen · 2024-03-13T16:40:29Z

@mgadewoll I probably should have checked before merging ha, but just to confirm, when you review PRs do you also complete the QA steps in the PR description, or are you doing code review only?

mgadewoll · 2024-03-13T16:47:18Z

@cee-chen I do the code review and then I follow the QA steps. 👣

cee-chen · 2024-03-13T17:04:13Z

Perfection! Thanks!

`v93.3.0`⏩ `v93.4.0` --- ## [`v93.4.0`](https://github.com/elastic/eui/releases/v93.4.0) - Added the following properties to `EuiButtonGroup`'s `options` configs: `toolTipContent`, `toolTipProps`, and `title`. These new properties allow wrapping buttons in `EuiToolTips`, and additionally customizing or disabling the native browser `title` tooltip. ([#7461](elastic/eui#7461)) - Enhanced `EuiResizeObserver` and `useResizeObserver`'s performance to not trigger page reflows on resize event ([#7575](elastic/eui#7575)) - Updated `EuiSuperUpdateButton` to support custom button text via an optional `children` prop ([#7576](elastic/eui#7576)) **Bug fixes** - Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize ([#7462](elastic/eui#7462)) - Fixed `EuiToast` title text to wrap instead of overflowing out of the container ([#7568](elastic/eui#7568)) - Fixed a visual bug with `EuiHeaderBreadcrumbs` with popovers ([#7580](elastic/eui#7580)) **Deprecations** - Deprecated `euiPalettePositive` and `euiPaletteNegative` in favour of a more culturally inclusive `euiPaletteGreen` and `euiPaletteRed` ([#7570](elastic/eui#7570)) - Deprecated all charts theme exports in favor of `@elastic/charts` exports: ([#7572](elastic/eui#7572)) - Deprecated `EUI_CHARTS_THEME_<DARK|LIGHT>` in favor of `<DARK|LIGHT>_THEME` from `@elastic/charts`. ([#7572](elastic/eui#7572)) - Deprecated `EUI_SPARKLINE_THEME_PARTIAL` in favor of `useSparklineOverrides` theme from the kibana `charts` plugin `theme` service. **Accessibility** - Updated `EuiModal` to set an `aria-modal` attribute and a default `dialog` role ([#7564](elastic/eui#7564)) - Updated `EuiConfirmModal` to set a default `alertdialog` role ([#7564](elastic/eui#7564)) - Fixed `EuiModal` and `EuiConfirmModal` to properly trap Safari+VoiceOver's virtual cursor ([#7564](elastic/eui#7564))

Update resize observer to use borderBoxSize instead of `getBounding…

c3eb4b3

…ClientRect()`

cee-chen added the performance label Mar 12, 2024

cee-chen changed the title ~~[EuiResizeObserver] Remove reflow/getBoundingClientRect() usage from resize events~~ [EuiResizeObserver] Remove reflow/getBoundingClientRect() usage on resize Mar 12, 2024

changelog

6df1915

davismcphee mentioned this pull request Mar 13, 2024

[EuiFlyout] Performance cleanup #7462

Merged

5 tasks

[hook] Remove initial getBoundingClientRect on mount

b5cdda0

cee-chen commented Mar 13, 2024

View reviewed changes

cee-chen marked this pull request as ready for review March 13, 2024 01:48

cee-chen requested a review from a team as a code owner March 13, 2024 01:48

mgadewoll approved these changes Mar 13, 2024

View reviewed changes

cee-chen merged commit 6ef79cb into elastic:main Mar 13, 2024
8 checks passed

cee-chen deleted the resize-observer/perf branch March 13, 2024 16:39

cee-chen mentioned this pull request Mar 18, 2024

Upgrade EUI to v93.4.0 elastic/kibana#178890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiResizeObserver] Remove reflow/`getBoundingClientRect()` usage on resize #7575

[EuiResizeObserver] Remove reflow/`getBoundingClientRect()` usage on resize #7575

cee-chen commented Mar 12, 2024 •

edited

Loading

cee-chen Mar 13, 2024 •

edited

Loading

mgadewoll Mar 13, 2024

cee-chen Mar 13, 2024

kibanamachine commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

mgadewoll left a comment

mgadewoll Mar 13, 2024

cee-chen commented Mar 13, 2024

cee-chen commented Mar 13, 2024

mgadewoll commented Mar 13, 2024

cee-chen commented Mar 13, 2024

	// `entry.contentRect` provides incomplete `height` and `width` data.
	// Use `getBoundingClientRect` to account for padding and border.
	// https://developer.mozilla.org/en-US/docs/Web/API/DOMRectReadOnly
	const { height, width } = container.getBoundingClientRect();

[EuiResizeObserver] Remove reflow/getBoundingClientRect() usage on resize #7575

[EuiResizeObserver] Remove reflow/getBoundingClientRect() usage on resize #7575

Conversation

cee-chen commented Mar 12, 2024 • edited Loading

Summary

QA

General checklist

cee-chen Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

mgadewoll Mar 13, 2024

Choose a reason for hiding this comment

cee-chen Mar 13, 2024

Choose a reason for hiding this comment

kibanamachine commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

💚 Build Succeeded

History

mgadewoll left a comment

Choose a reason for hiding this comment

mgadewoll Mar 13, 2024

Choose a reason for hiding this comment

cee-chen commented Mar 13, 2024

cee-chen commented Mar 13, 2024

mgadewoll commented Mar 13, 2024

cee-chen commented Mar 13, 2024

[EuiResizeObserver] Remove reflow/`getBoundingClientRect()` usage on resize #7575

[EuiResizeObserver] Remove reflow/`getBoundingClientRect()` usage on resize #7575

cee-chen commented Mar 12, 2024 •

edited

Loading

cee-chen Mar 13, 2024 •

edited

Loading