Simplify useResizeObserver

[jsnajdr]

I've been investigating a bug in WordPress.com where Gutenberg pattern previews trigger a ResizeObserver infinite loop:

ResizeObserver loop completed with undelivered notifications.

I started by looking at how useResizeObserver works, and discovered that it's unnecessarily complex. I decided to remove the unneeded features, which ended up with an almost complete rewrite.

• the only place that uses the useResizeObserver hook is the useResizeAware hook that creates a helper element with ResizeObserver attached to it. This usage doesn't use any opts, so they can be removed. Only the contentBoxSize entry is ever used, so we don't need to support any other. We also don't use the custom round function.
• the useResolvedElement and its special refOrElement option is not needed. I'm replacing that with simple logic that mounts the ResizeElement component, stores a local ref to the inner div, and then creates the observer in a layout effect.
• the didUnmountRef can also be removed, because React 18 no longer complains when setState is called after unmount.

What do you think?

[github-actions[bot]]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: DaniGuardiola <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ciampo]

cc @DaniGuardiola in particular who's also been looking at useResizeObserver code recently (see https://github.com/WordPress/gutenberg/pull/62946#discussion_r1658962084)

[jsnajdr]

The useResizeObserver that @DaniGuardiola proposed in #62946 is used like:

useResizeObserver( element, ( element ) => { ... } );

It starts observing the element resizes, and will invoke the callback whenever the ResizeObserver reports a resize. It ignores the entries passed to the ResizeObserver callback. Instead of using the provided sizes, you are supposed to measure the element yourself.

There's also a fireOnObserve option. When enabled, the callback will be called also on mount. That can be helpful for keeping track of the element size even when it didn't resize yet:

const [ size, setSize ] = useState();
useResizeObserver( element, ( element ) => setSite( measure( element ) ) );

Here the size state contains valid size info right after mount (in the on-mount effect).

Curiously, the useResizeObserver in compose will report { width: null, height: null } size until a resize really happens.

If we wanted, I think we could have both hooks: the basic one that fires a callback on resize, and then another one that builds on top of the first one, adding the overlay element and measuring that element specifically.

[ciampo]

The useResizeObserver that @DaniGuardiola proposed in https://github.com/WordPress/gutenberg/pull/62946 is used like [...] Instead of using the provided sizes, you are supposed to measure the element yourself.

@DaniGuardiola , I don't remember if there was a specific reason why you chose to measure the element yourself, instead of using the measurements from the RO. Using the measumerements from the RO could result in better runtime performance.

There's also a fireOnObserve option. When enabled, the callback will be called also on mount. That can be helpful for keeping track of the element size even when it didn't resize yet:

Is there ever a scenario in which we wouldn't want the hook to fire on mount? On paper that sounds like a feature that we'd benefit from, regardless.

If we wanted, I think we could have both hooks: the basic one that fires a callback on resize, and then another one that builds on top of the first one, adding the overlay element and measuring that element specifically.

That is something that we should explore, although not urgent IMO.

[DaniGuardiola]

I'll elaborate tomorrow as today I am AFK due to being sick, but in short:

• I measured independently because the specific use-case required something not provided by RO entries.
• My utility, iirc, forwards the RO entries, so that's completely doable.
• I investigated and it turns out RO is supposed to also trigger initially, so as long as that is true, my hook can be simplified.
• My hook is reactive to "element", which was a requirement (a ref is not stateful and therefore would fail said requirement). I believe these utils could be based upon my hook so we could get both things. Create the sizing element internally, obtain it's hook, and let my hook abstract the RO as needed. Would only need to accept RO options if it doesn't already, I don't remember.

I'll take a closer look tomorrow :)

[jsnajdr]

Is there ever a scenario in which we wouldn't want the hook to fire on mount?

Turns out that the answer is no, because ResizeObserver always fires an observation right after the .observe function is called. You always get an "initial size" without any extra work.

I measured independently because the specific use-case required something not provided by RO entries.

el.getBoundingClientRect should be equivalent to entry.borderBoxSize, but there is one important exception: ResizeObserver always ignores CSS transforms. If your div has transform: scale(2), then ResizeObserver will report the untransformed size, while getBoundingClientRect() will return a rectangle that's twice as big.

But ResizeObserver won't inform you when the transform changes, so the transformed size from .getBoundingClientRect is not 100% reliable.

The spec summarizes it as:

[jsnajdr]

I added one more improvement: the ResizeObserver callback provides an array of entries, in case multiple resizes happened before the callback had the chance to run. But so far we've been looking only at entries[ 0 ]. If there were further entries, newer and more up-to-date, we were ignoring them. The measured size can become stale. I'm replacing that with a loop that processes all entries.

All examples in the spec and on MDN do a loop like this. Maybe we could look at just the last entry, but I decided to follow the examples and process each entry fully.

I think this PR is now ready to be merged. If you don't have any major objections, let's approve and ship it.

[ciampo]

el.getBoundingClientRect should be equivalent to entry.borderBoxSize, but there is one important exception: ResizeObserver always ignores CSS transforms. If your div has transform: scale(2), then ResizeObserver will report the untransformed size, while getBoundingClientRect() will return a rectangle that's twice as big.

But ResizeObserver won't inform you when the transform changes, so the transformed size from .getBoundingClientRect is not 100% reliable.

Yup, and that is (I believe) one of the main reasons why @DaniGuardiola doesn't use the RO's measurements in the implementation linked above — because we need those measurements to work properly even when transforms are applied.

I think this PR is now ready to be merged.

Code is LGTM, but let's wait for @DaniGuardiola 's last round of feedback, since he mentioned

I'll take a closer look tomorrow :)

[DaniGuardiola]

Sorry for not commenting yesterday, but I wanted to finish up a PR I'm about to put up where I've simplified my hook, and I believe it should be possible to conciliate that version with this effort. I'll share more in a bit, once that's ready :)

[DaniGuardiola]

@tyxla to clarify, my upcoming PR does not touch these files, and rather it updates my util along with the Tabs indicator animation (and the upcoming ToggleGroupControl animation which is done and depends on these changes too). The goal is to merge that PR to then compare with what's going on in this PR and see if we can finally unify everything under here.

I can't write more details for now (heading to lunch) but the PR is done. Will undraft once I can write more details in its description, but you can look at the refactor I did of the utils for now: https://github.com/WordPress/gutenberg/pull/64926

[DaniGuardiola]

Update: PR is ready for review (https://github.com/WordPress/gutenberg/pull/64926).

[DaniGuardiola]

I created a PR that targets this one (so that it can be merged after this one) that clarifies what I intend to add: https://github.com/WordPress/gutenberg/pull/64943 (useEvent and useObserveElementSize).

Then, a final follow-up would be to remove the util from the components package to de-duplicate (see https://github.com/WordPress/gutenberg/pull/64926 for the last version of the utility I mean, which is exactly the same as useObserveElementSize but named useResizeObserver).