[EuiResizableContainer] Fix issue where `onResizeEnd` becomes stale when renders occur between resize start and end #7468

davismcphee · 2024-01-18T22:51:40Z

Summary

This PR fixes an issue where onResizeEnd becomes a stale closure when renders occur between resize start and end. This can result in an outdated version of a consumer's onResizeEnd callback being called on resize end.

QA

General checklist

Browser QA
~~- [ ] Checked in both light and dark modes~~
- [ ] Checked in mobile
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
- Checked in Chrome, Safari,~ Edge~, and Firefox
Docs site QA - N/A, bugfix
Code quality checklist
- Added or updated jest ~~and cypress tests~~
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

…n renders occur between resize start and end

src-docs/src/views/resizable_container/resizable_container_callbacks.tsx

davismcphee · 2024-01-18T23:11:27Z

src/components/resizable_container/resizable_container.tsx

@@ -134,10 +138,10 @@ export const EuiResizableContainer: FunctionComponent<
    keyMoveDirection?: KeyMoveDirection;
  }>({});

-  const resizeEnd = useCallback(() => {
+  const resizeEnd = useStableCallback(() => {


For testing, swap this back to useCallback and the resize callbacks example in the docs should fail to reset on resize end.

cee-chen · 2024-01-23T17:59:51Z

Heya Davis! I was going to take over this PR and apply the change to all EUI function components that add/remove window events, if that's cool, and also use EUI's existing useLatest hook!

davismcphee · 2024-01-24T03:18:12Z

@cee-chen That's great to hear, thanks for looking into it! And by all means, the PR is all yours 🙂

…tale when renders occur between resize start and end" This reverts commit 60c32b7.

- should fail in current prod

…callbacks

cee-chen · 2024-01-24T19:21:07Z

So it turns out this is actually the only component with this problem - all other components either are class components, where it's not an issue, or they use useEffect instead of useCallback which negates several of the issues specific to this instance (e.g. the callback itself sent to the window event listener doesn't depend on a changing reference to a function).

TBH, we should potentially consider switching this component to useEffect or useEffectEvent once that becomes available, but that's a larger refactor for later vs. this quicker one.

@davismcphee let me know if the switch to useLatest and the test I wrote look good to you at a glance - if so, I'll go ahead and merge this! Thanks so much again for catching this behavior!

kibanamachine · 2024-01-24T19:44:17Z

Preview staging links for this PR:

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

elasticmachine · 2024-01-24T19:44:24Z

💚 Build Succeeded

Buildkite Build
Commit: 1b59e32

History

💚 Build #1293 succeeded d59edd8

davismcphee

So it turns out this is actually the only component with this problem

Even better!

Pulled and tested locally, and confirmed the changes resolve the issue and the test catches the issue as expected. And I'm definitely in favour of using useLatest over a bespoke utility for this. I'd give my approval, but I can't since I opened the PR originally 😁 That said, the changes LGTM and I'm looking forward to removing our workaround in Kibana after the next EUI upgrade 👍 Thanks!

cee-chen · 2024-01-25T21:59:51Z

Fantastic, thanks a million!!

`v92.1.1`⏩`v92.2.1` --- ## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1) **Bug fixes** - Removed unintentional i18n tokens in prior release that should not have been exported ## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0) - Updated `EuiFlyoutResizable` with new optional `onResize` callback ([#7464](elastic/eui#7464)) **Bug fixes** - Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could become a stale closure when renders occured between resize start and end, resulting in an outdated version of a consumer's `onResizeEnd` callback being called ([#7468](elastic/eui#7468)) - Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear button click ([#7473](elastic/eui#7473)) - Fixed `EuiContextMenu`'s panel titles & items to not show underlines on hover for non-interactive elements ([#7474](elastic/eui#7474)) **Deprecations** - Remove unused public `EuiHue` and `EuiSaturation` subcomponent exports. Use the parent `EuiColorPicker` component instead ([#7460](elastic/eui#7460)) - Remove unused public `EuiCommentTimeline` subcomponent export. Use the parent `EuiComment` or `EuiCommentList` components instead. ([#7467](elastic/eui#7467))

…76030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in #174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in #175849. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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)

…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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)

`v92.1.1`⏩`v92.2.1` --- ## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1) **Bug fixes** - Removed unintentional i18n tokens in prior release that should not have been exported ## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0) - Updated `EuiFlyoutResizable` with new optional `onResize` callback ([elastic#7464](elastic/eui#7464)) **Bug fixes** - Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could become a stale closure when renders occured between resize start and end, resulting in an outdated version of a consumer's `onResizeEnd` callback being called ([elastic#7468](elastic/eui#7468)) - Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear button click ([elastic#7473](elastic/eui#7473)) - Fixed `EuiContextMenu`'s panel titles & items to not show underlines on hover for non-interactive elements ([elastic#7474](elastic/eui#7474)) **Deprecations** - Remove unused public `EuiHue` and `EuiSaturation` subcomponent exports. Use the parent `EuiColorPicker` component instead ([elastic#7460](elastic/eui#7460)) - Remove unused public `EuiCommentTimeline` subcomponent export. Use the parent `EuiComment` or `EuiCommentList` components instead. ([elastic#7467](elastic/eui#7467))

…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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)

`v92.1.1`⏩`v92.2.1` --- ## [`v92.2.1`](https://github.com/elastic/eui/releases/v92.2.1) **Bug fixes** - Removed unintentional i18n tokens in prior release that should not have been exported ## [`v92.2.0`](https://github.com/elastic/eui/releases/v92.2.0) - Updated `EuiFlyoutResizable` with new optional `onResize` callback ([elastic#7464](elastic/eui#7464)) **Bug fixes** - Fixed an issue in `EuiResizableContainer` where `onResizeEnd` could become a stale closure when renders occured between resize start and end, resulting in an outdated version of a consumer's `onResizeEnd` callback being called ([elastic#7468](elastic/eui#7468)) - Fixed `EuiTextArea` to correctly fire `onChange` callbacks on clear button click ([elastic#7473](elastic/eui#7473)) - Fixed `EuiContextMenu`'s panel titles & items to not show underlines on hover for non-interactive elements ([elastic#7474](elastic/eui#7474)) **Deprecations** - Remove unused public `EuiHue` and `EuiSaturation` subcomponent exports. Use the parent `EuiColorPicker` component instead ([elastic#7460](elastic/eui#7460)) - Remove unused public `EuiCommentTimeline` subcomponent export. Use the parent `EuiComment` or `EuiCommentList` components instead. ([elastic#7467](elastic/eui#7467))

…astic#176030) ## Summary This PR reverts the `onResizeEnd` stable callback workaround introduced in elastic#174955 to account for an EUI bug now that elastic/eui#7468 has been merged and Kibana was upgraded to v92.2.1 in elastic#175849. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] 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)

davismcphee added 2 commits January 18, 2024 18:37

[EuiResizableContainer] Fix issue where onResizeEnd becomes stale whe…

60c32b7

…n renders occur between resize start and end

Add changelog

d59edd8

davismcphee commented Jan 18, 2024

View reviewed changes

src-docs/src/views/resizable_container/resizable_container_callbacks.tsx Outdated Show resolved Hide resolved

davismcphee marked this pull request as ready for review January 18, 2024 23:04

davismcphee requested a review from a team as a code owner January 18, 2024 23:04

davismcphee commented Jan 18, 2024

View reviewed changes

cee-chen added 5 commits January 24, 2024 09:29

Merge branch 'main' into resizable-container-stale-callbacks

62b186c

Revert "[EuiResizableContainer] Fix issue where onResizeEnd becomes s…

4945e2a

…tale when renders occur between resize start and end" This reverts commit 60c32b7.

Add unit test for callback memoization regressions

02b8c6d

- should fail in current prod

[EuiResizableContainer] Add useLatest ref wrappers around consumer …

8bb3f35

…callbacks

Copy tweaks

1b59e32

davismcphee commented Jan 25, 2024

View reviewed changes

cee-chen approved these changes Jan 25, 2024

View reviewed changes

cee-chen merged commit fca1f5e into elastic:main Jan 25, 2024
7 checks passed

davismcphee deleted the resizable-container-stale-callbacks branch January 25, 2024 22:08

cee-chen mentioned this pull request Jan 29, 2024

Upgrade EUI to v92.2.1 elastic/kibana#175849

Merged

davismcphee mentioned this pull request Feb 1, 2024

[ResizableLayout] Remove onResizeEnd stable callback workaround elastic/kibana#176030

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiResizableContainer] Fix issue where `onResizeEnd` becomes stale when renders occur between resize start and end #7468

[EuiResizableContainer] Fix issue where `onResizeEnd` becomes stale when renders occur between resize start and end #7468

davismcphee commented Jan 18, 2024 •

edited by cee-chen

Loading

davismcphee Jan 18, 2024

cee-chen commented Jan 23, 2024

davismcphee commented Jan 24, 2024

cee-chen commented Jan 24, 2024 •

edited

Loading

kibanamachine commented Jan 24, 2024

elasticmachine commented Jan 24, 2024 •

edited

Loading

davismcphee left a comment

cee-chen commented Jan 25, 2024

[EuiResizableContainer] Fix issue where onResizeEnd becomes stale when renders occur between resize start and end #7468

[EuiResizableContainer] Fix issue where onResizeEnd becomes stale when renders occur between resize start and end #7468

Conversation

davismcphee commented Jan 18, 2024 • edited by cee-chen Loading

Summary

QA

General checklist

davismcphee Jan 18, 2024

Choose a reason for hiding this comment

cee-chen commented Jan 23, 2024

davismcphee commented Jan 24, 2024

cee-chen commented Jan 24, 2024 • edited Loading

kibanamachine commented Jan 24, 2024

elasticmachine commented Jan 24, 2024 • edited Loading

💚 Build Succeeded

History

davismcphee left a comment

Choose a reason for hiding this comment

cee-chen commented Jan 25, 2024

[EuiResizableContainer] Fix issue where `onResizeEnd` becomes stale when renders occur between resize start and end #7468

[EuiResizableContainer] Fix issue where `onResizeEnd` becomes stale when renders occur between resize start and end #7468

davismcphee commented Jan 18, 2024 •

edited by cee-chen

Loading

cee-chen commented Jan 24, 2024 •

edited

Loading

elasticmachine commented Jan 24, 2024 •

edited

Loading