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

Fix height adjustment in ZoomElement in Safari #21174

Merged
merged 6 commits into from
Feb 28, 2023

Conversation

jrencz
Copy link
Contributor

@jrencz jrencz commented Feb 20, 2023

What I did

I replaced a mutation observer with the hook useResizeObserver inside ZoomElement in order to make stories using custom elements resize properly in Safari

Problem is visualized in #21138 (comment)

Problem, as described in this thread affects stories rendered with custom elements using Shadow DOM. Such stories are affected if at the moment ZoomElement measures the height, component did not yet render its contents. Then, when component in a story renders content, it doesn't trigger MutationObserver (mutation callback is not called when Shadow DOM is altered).

ResizeObserver replaces the existing MutationObserver (orignally added via #15472).

Problem, as described in the original thread was mitigated by @JReinhold with #21163, but even after that fix Safari is excluded from using native zoom and needs the fallback behavior.

How to test

See https://github.com/jrencz/storybook-lity-story-height-repro

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

Problem, as described in [this thread](storybookjs#21138 (review))
affects stories rendered with custom elements using Shadow DOM. Such
stories are affected if at the moment ZoomElement measures the height,
component did not yet render its contents. Then, when component in a
story renders content, it doesn't trigger MutationObserver (mutation
callback is not called when Shadow DOM is altered).

ResizeObserver is added next to MutationObserver (orignally added via
storybookjs#15472), not replaces it, because MutationObserver is better supported
by older browsers than ResizeObserver.

Problem, as described in the original thread was mitigated by @JReinhold
with storybookjs#21163, but even after that fix Safari is excluded from using native
`zoom` and needs the fallback behavior.
@jrencz jrencz mentioned this pull request Feb 20, 2023
5 tasks
@JReinhold JReinhold self-assigned this Feb 21, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Thanks for this! How do I test this? Do I just need a setup using web components that changes size over time?

I think we should replace the mutation observer completely with the resize observer because:

  1. Can I Use shows a great support table for it, and works for all the browsers and versions we support in 7.0.
  2. It was already introduced in another part of the UI recently as part of Tab UI Improvements #20783, so we've sort of already made it a requirement.

@jrencz
Copy link
Contributor Author

jrencz commented Feb 21, 2023

Good point. I will try to provide some reproduction example, but until I find time to get it right

  • try using @storybook/web-components-vite
  • create component and story for it (i use Lit, maybe it matters, maybe not)
  • maybe it matters that I globally use style :not(:defined) { display: none } turned out not to be relevant, see below

@jrencz jrencz requested a review from JReinhold February 21, 2023 21:21
@jrencz
Copy link
Contributor Author

jrencz commented Feb 21, 2023

@JReinhold I added a repository that reproduces the problem on beta.53 and allows to notice that ResizeObserver resolves it.

Before After
Screenshot 2023-02-21 at 22 33 58 Screenshot 2023-02-21 at 22 35 10

If there's a way to integrate that in this repo instead of having a distinct one, please guide me how can I do it the right way.

I think we should replace the mutation observer completely

Let's clarify: do you want me to replace MutationObserver with ResizeObserver it in this PR instead of just adding ResizeObserver?

@JReinhold
Copy link
Contributor

Before After
Screenshot 2023-02-21 at 22 33 58 Screenshot 2023-02-21 at 22 35 10

Great!

If there's a way to integrate that in this repo instead of having a distinct one, please guide me how can I do it the right way.

I don't think that'll be necessary in this case.

Let's clarify: do you want me to replace MutationObserver with ResizeObserver it in this PR instead of just adding ResizeObserver?

yup!

Can be tested with jrencz/storybook-lity-story-height-repro@da1369a
with combination of Zoom in/Zoom out on controls and button to alter
height of component added in that commit
@jrencz
Copy link
Contributor Author

jrencz commented Feb 22, 2023

It should be all right now. I extended the demo and found and fixed a problem, which I initially didn't notice (see commit message of c8509fb)

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your contribution!

@JReinhold JReinhold merged commit 7815aa0 into storybookjs:next Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants