Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EuiResizableContainer] Initialize after DOM is ready #4447

Merged
merged 7 commits into from
Jan 28, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jan 25, 2021

Summary

Fixes a problem discovered in elastic/kibana#86934.
EuiResizableContainer is initially hidden (display: none;) and overlaid with a loading screen while data is loaded. When the data is loaded, the visualization is rendered onto a target DOM element via ref reference inside the EuiResizablePanel. EuiResizableContainer is mounted at this point but rerenders when it becomes visible and gains calculable size dimensions. That rerender changes/remounts the target div and alters the ref, effectively disconnecting the visualization from the DOM.

The solution is to not initialize EuiResizableContainer until it is visible/has calculable dimensions. There are likely other ways to solve this problem in the implementation (e.g., not usingdisplay: none;; watching for ref changes), but we have the information readily available in component state to prevent this kind of unnecessary rerender/remount.

Docs change is a rudimentary example that will be reverted prior to merging. Comment out the real changes to see the problem. Alternatively, use the linked branch to test in the true Kibana environment.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs
- [ ] Added documentation

- [ ] Added or updated jest tests

  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4447/

@cchaos
Copy link
Contributor

cchaos commented Jan 25, 2021

I don't know if this is related or not, but I have an issue in my usage of the component where I'm setting an initialSize=15 which only accepts a number as percentage value, then I also set a pixel value minSize=60px. But on initial load, it doesn't respect that minSize. So If the window is skinny, and 15% of the width is less than 60px the panel will be less than 60px.

@sulemanof
Copy link
Contributor

actually, I'm not sure wee need this check to render the content of EuiResizableContainer

image

so just removing the check !!reducerState.containerSize resolves the problem.
This also potentially resolves possible problem if a consumer uses display: none of the container manually, or reduces the container size (width or height) to 0 manually.

I also do not see any necessity to use resize observer useResizeObserver, and it makes possible to remove next LOC:

image

I created a pr to this branch with the clean up thompsongl#14
It seems to work fine, I didn't notice any regression at examples page.
Could you please take a look?

@thompsongl
Copy link
Contributor Author

thompsongl commented Jan 26, 2021

So If the window is skinny, and 15% of the width is less than 60px the panel will be less than 60px.

This sound like an unrelated bug, but one that should be fairly easy to fix. Spoke too soon; might need to do a follow-up issue/PR.

I also do not see any necessity to use resize observer useResizeObserver

This goes back to a decision I made after seeing that the size of the container is recalculated every time a panel is resized (many times over the course of a panel drag event). The resize observer instead calculates the size of the container 1) once initially and 2) only when the container itself changes sizes or goes from hidden to visible, all of which are much more rare than panel resize events. By eliminating "goes from hidden to visible", it's mostly just a window resize event that would cause a recalculation.

Another reason, although not currently in use, is to keep containerSize in state so that other parts of the component can use the value, including exposing it to consumers through the render prop (like the toggle function).

removing the check !!reducerState.containerSize

I agree, it looks like this check can be removed.

@thompsongl
Copy link
Contributor Author

Update: To fix the problem that @cchaos found, we'll need to retain the initContainer method. It's the only place that will allow for a minSize vs. initialSize determination based on the container size gaining calculable dimensions.

So, I'd like to move forward without the changes proposed in thompsongl#14. Going to attempt solving minSize vs. initialSize, but that may be a follow-up.

@sulemanof
Copy link
Contributor

So, I'd like to move forward without the changes proposed in thompsongl#14. Going to attempt solving minSize vs. initialSize, but that may be a follow-up.

I'm okay with leaving the subscription to resize and the container size in reducer, but I would insist on removing the check !!reducerState.containerSize in render.
Actually, this is the only change we need in the PR to get rid of the issue and guarantee the DOM elements inside the container will always have initial references.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4447/

@thompsongl
Copy link
Contributor Author

jenkins test this

Flaky context menu test

@thompsongl
Copy link
Contributor Author

Opened #4453 to track the (mostly unrelated) minSize vs. initialSize issue. No reason to block fixing the lazy load case while we work out the logic necessary.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4447/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM; Pulled & tested change locally by verifying bug is reproduced in the modified example when the fix is disabled, and verified the example works as expected with the fix.

Thank you @thompsongl ! ❤️

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4447/

@thompsongl thompsongl merged commit 4d8afb1 into elastic:master Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants