Fix: Recalculate height of ZoomElement when child element updates #15472

Tomastomaslol · 2021-07-03T04:50:19Z

What I did

Removed duplicated never called function browserSupportsCssZoom
Added an observer to "watch" for changes in child elements
Update calculated height when child elements are updated

In browsers without support for CSS zoom the height of the element needs to be known to be able to simulate zoom using transform. When the height of the child ZoomElement updates the height needs to be re-calculated.

New behavior

Kapture.2021-12-19.at.11.40.19.mp4

Current behavior

Kapture.2021-12-19.at.12.15.54.mp4

nx-cloud · 2021-07-03T04:52:13Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f7739fb. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

nx run-many --target=prepare --all --parallel=2 --max-parallel=15 -- --optimized

Sent with 💌 from NxCloud.

darleendenno · 2021-07-19T14:45:04Z

Hey @Tomastomaslol is this ready for review?

…nObserver

…368-ZoomElement-do-not-resize-on-child-resize

Tomastomaslol · 2021-12-07T03:08:04Z

Hi @darleendenno! Sorry, I took a long break from this PR to focus on other things.

I think I have figured out a decent solution to the issue described in #15368 and I'm planning to spend some more time improving the code and "productionize" it before it's ready for review.

I will let you know when I'm happy with the code and update the status of the PR. 🙂

lib/components/src/Zoom/ZoomElement.tsx

lib/components/src/Zoom/Zoom.tsx

stale · 2022-01-09T03:16:36Z

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

…368-ZoomElement-do-not-resize-on-child-resize

…oes not support CSS zoom

…368-ZoomElement-do-not-resize-on-child-resize

…ld-resize # Conflicts: # lib/store/src/autoTitle.ts

ndelangen · 2022-06-30T20:00:18Z

@ghengeveld perhaps this is something you could take a look at? This seems like exactly the type of UI/UX/DX thing you'd be keen to review/comment on?

@shilman with the upcoming work on docs in future/base, how easy is it for this to be applied there too?

Does this change still really do anything when rendering in modernInlineRendering (which will be the default in 7.0 anyway @shilman @tmeasday ?

This feature seems like a no-brainer to me: of course should the height of the container grow with the size of the story's html output! But I find it hard to judge wether this implementation will be compatible with how things will be in 7.0.

@Tomastomaslol FYI, we've sort of feature-locked 6.x and focussed our full attention on cleaning the codebase modernizing it, so it can scale better and remove the many deprecations and mistakes we made (we all make them, right?).
Hence even though this is a clearly important feature, I'm not sure we can merge soon, and I'm pretty sure if it's merged it will be for 7.0, not 6.x.

Thanks for all your hard work, energy and skill on this PR!

tmeasday · 2022-06-30T23:53:06Z

Does this change still really do anything when rendering in modernInlineRendering (which will be the default in 7.0 anyway @shilman @tmeasday ?

I'm not sure modern inline rendering really changes the behaviour of this component @ndelangen? Can you explain what you were thinking here, I'm missing something I think.

I think this PR should be considered as part of the doc blocks 2.0 work. I'm not quite sure what the exact plan is there @shilman.

ndelangen · 2022-07-01T11:25:32Z

@tmeasday with inline rendering, the size of the container will just auto-update. so there's no need to keep it in sync, because the layout engine is doing that the natural way.
That's what I was referring to.

ghengeveld · 2022-07-01T12:48:33Z

lib/components/src/Zoom/ZoomElement.tsx

+}): void => {
+  const observer = useMemo(
+    () =>
+      typeof MutationObserver === 'function'


I don't think we need this check, MutationObserver has been available in all browsers since 2014. Save for Opera Mini which frankly nobody is going to be using to view their Storybook in anyway.

I strongly agree with this, Storybook 7.0 will target chrome100. If it's supported in there, don't bother downgrading, transpiling, of feature detecting!

ghengeveld · 2022-07-01T12:51:32Z

lib/components/src/Zoom/ZoomElement.tsx

  useEffect(() => {
    if (componentWrapperRef.current) {
      setHeight(componentWrapperRef.current.getBoundingClientRect().height);
    }
  }, [scale, componentWrapperRef.current]);

+  useMutationObserver({
+    element: componentWrapperRef,
+    options: { subtree: true, childList: true },


options should be defined outside the component render function (or memoized, but that seems silly), otherwise we'll have a new object with each render, which will cause the useEffect above to disconnect and re-observe each time.

ghengeveld · 2022-07-01T12:52:53Z

Some small remarks but other than that (and PR checks) this LGTM.

ghengeveld · 2022-07-01T12:57:50Z

lib/components/src/Zoom/ZoomElement.tsx

  return (
    <ZoomElementWrapper scale={scale} height={height}>
-      <div ref={componentWrapperRef} className="innerZoomElementWrapper">
+      <div
+        ref={browserSupportsCssZoom() ? null : componentWrapperRef}


I think we can just call browserSupportsCssZoom once, outside the component render function, put it in a variable and use that here. Otherwise we'll be running this check over and over again.

ndelangen · 2022-07-01T13:04:17Z

Thanks for the review @ghengeveld 🤝

tmeasday · 2022-07-04T00:50:06Z

@tmeasday with inline rendering

@ndelangen well if you are talking about inline rendering, then that hasn't changed in 7.0. The default was to inline render already. What's changed is the mechanism to do it, which doesn't affect how the browser lays things out AFAIK. I guess this is useful for non-inline mode? Although TBH I wasn't aware that a mutation observer would work there..

…368-ZoomElement-do-not-resize-on-child-resize

…ld-resize

@JReinhold

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.

remove duplicated function browserSupportsCssZoom

1c6f503

Tomastomaslol added 2 commits December 7, 2021 13:54

update zoomElement heigth when a child element mutate using a Mutatio…

1a3f03b

…nObserver

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

5b681d1

…368-ZoomElement-do-not-resize-on-child-resize

Tomastomaslol commented Dec 7, 2021

View reviewed changes

lib/components/src/Zoom/ZoomElement.tsx Outdated Show resolved Hide resolved

Tomastomaslol commented Dec 7, 2021

View reviewed changes

lib/components/src/Zoom/ZoomElement.tsx Outdated Show resolved Hide resolved

Tomastomaslol commented Dec 7, 2021

View reviewed changes

lib/components/src/Zoom/Zoom.tsx Outdated Show resolved Hide resolved

handle scenario no MutationObserver, clean up ZoomElement

cd25d9a

Tomastomaslol changed the title ~~FIX: Removed duplicated function browserSupportsCssZoom~~ Fix: Recalculate height of ZoomElement when child element updates in browsers that does't support CSS zoom Dec 19, 2021

Tomastomaslol changed the title ~~Fix: Recalculate height of ZoomElement when child element updates in browsers that does't support CSS zoom~~ Fix: Recalculate height of ZoomElement when child element updates Dec 19, 2021

stale bot added the inactive label Jan 9, 2022

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

60cad0c

…368-ZoomElement-do-not-resize-on-child-resize

stale bot removed the inactive label Jan 18, 2022

Tomastomaslol marked this pull request as ready for review January 18, 2022 07:27

Tomastomaslol requested a review from shilman January 18, 2022 07:27

Tomastomaslol and others added 6 commits March 15, 2022 18:53

merged next into issue 15368, only add ref with observer if browser d…

8d0d673

…oes not support CSS zoom

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

926ada3

…368-ZoomElement-do-not-resize-on-child-resize

merged with remote origin next

6686b6f

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

849c7fe

…368-ZoomElement-do-not-resize-on-child-resize

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

cd01b77

…368-ZoomElement-do-not-resize-on-child-resize

Merge branch 'next' into issue-15368-ZoomElement-do-not-resize-on-chi…

cd7bce9

…ld-resize # Conflicts: # lib/store/src/autoTitle.ts

ndelangen added bug addon: docs ci: do not merge labels Jun 30, 2022

ndelangen assigned ghengeveld Jun 30, 2022

ghengeveld reviewed Jul 1, 2022

View reviewed changes

ghengeveld approved these changes Jul 1, 2022

View reviewed changes

ghengeveld reviewed Jul 1, 2022

View reviewed changes

Tomastomaslol and others added 3 commits July 8, 2022 17:39

feedback from code review

413c6df

Merge branch 'next' of github.com:storybookjs/storybook into issue-15…

f7739fb

…368-ZoomElement-do-not-resize-on-child-resize

Merge branch 'next' into issue-15368-ZoomElement-do-not-resize-on-chi…

d35ed61

…ld-resize

ndelangen removed the ci: do not merge label Sep 13, 2022

remove unnecessary check if observer is defined

d06ab1c

ndelangen merged commit 7690577 into storybookjs:next Sep 14, 2022

Tomastomaslol deleted the issue-15368-ZoomElement-do-not-resize-on-child-resize branch September 26, 2022 00:47

shilman mentioned this pull request Sep 27, 2022

In Firefox, ZoomElement component doesn't resize on child resize #15368

Closed

jrencz mentioned this pull request Feb 20, 2023

Fix height adjustment in ZoomElement in Safari #21174

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Recalculate height of ZoomElement when child element updates #15472

Fix: Recalculate height of ZoomElement when child element updates #15472

Tomastomaslol commented Jul 3, 2021 •

edited

Loading

nx-cloud bot commented Jul 3, 2021 •

edited

Loading

darleendenno commented Jul 19, 2021

Tomastomaslol commented Dec 7, 2021

stale bot commented Jan 9, 2022

ndelangen commented Jun 30, 2022

tmeasday commented Jun 30, 2022

ndelangen commented Jul 1, 2022

ghengeveld Jul 1, 2022

ndelangen Jul 1, 2022

ghengeveld Jul 1, 2022 •

edited

Loading

ghengeveld commented Jul 1, 2022

ghengeveld Jul 1, 2022

ndelangen commented Jul 1, 2022 •

edited

Loading

tmeasday commented Jul 4, 2022

Fix: Recalculate height of ZoomElement when child element updates #15472

Fix: Recalculate height of ZoomElement when child element updates #15472

Conversation

Tomastomaslol commented Jul 3, 2021 • edited Loading

What I did

New behavior

Current behavior

nx-cloud bot commented Jul 3, 2021 • edited Loading

☁️ Nx Cloud Report

darleendenno commented Jul 19, 2021

Tomastomaslol commented Dec 7, 2021

stale bot commented Jan 9, 2022

ndelangen commented Jun 30, 2022

tmeasday commented Jun 30, 2022

ndelangen commented Jul 1, 2022

ghengeveld Jul 1, 2022

Choose a reason for hiding this comment

ndelangen Jul 1, 2022

Choose a reason for hiding this comment

ghengeveld Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

ghengeveld commented Jul 1, 2022

ghengeveld Jul 1, 2022

Choose a reason for hiding this comment

ndelangen commented Jul 1, 2022 • edited Loading

tmeasday commented Jul 4, 2022

Tomastomaslol commented Jul 3, 2021 •

edited

Loading

nx-cloud bot commented Jul 3, 2021 •

edited

Loading

ghengeveld Jul 1, 2022 •

edited

Loading

ndelangen commented Jul 1, 2022 •

edited

Loading