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]

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 #62946 (comment))

[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]

I gave this a look, and it's impressive how much simpler the code is 👏

discovered that it's unnecessarily complex

I'd be curious to know if there were specific use cases when the current implementation was added to the repo (#40509), which caused that implementation to be that complex — maybe @youknowriad has more context around this.

The code changes look, but I assume this would need some good manual testing (js-dom based tests won't pick up any changes, I guess, and I'm not sure how well e2e tests cover this hook).

We could also tentatively merge these changes and assume that there is enough time before the next WordPress release that any blocking bugs that we don't catch while reviewing would still be flagged soon.

[ciampo]

Should we make the logic more explicit about checking that entry.contentBoxSize is an array or an object?

[ciampo]

The useResizeObserver that @DaniGuardiola proposed in #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 :)

[tyxla]

Love the cleanup 💯

This appears to work well in my tests 👍

I'm curious if we have any concerns about removing those APIs that useResizeObserver was supporting but we weren't really using in Gutenberg.

Also, let's wait for @DaniGuardiola's update / review.

@DaniGuardiola @jsnajdr you folks might want to work on this together in a single branch 😉 Why approach this separately?

[tyxla]

I don't see any use of the custom rounding function, but I assume we'll be fine with always using the default one?

[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: #64926

[DaniGuardiola]

Update: PR is ready for review (#64926).

[DaniGuardiola]

Looks good, and my needs can be covered in a follow-up PR. Just left a comment about naming and with details about what I want to contribute next.

[DaniGuardiola]

This function is not properly called in an ideal world, it should rather be named useResizeAware since it's not a direct wrapper over ResizeObserver, but rather a utility that returns an element with specific constraints that is measured, and the resulting measurements.

The unfortunate thing is that the path is named after "useResizeObserver" even though the actual exported utility is "useResizeAware", thanks to the default export which makes things nameless. Then, it is exported as "useResizeObserver" under a named export in the outer barrel file (index.js).

Regarding this utility, a lot of what you could do with a proper, thin wrapper of ResizeObserver can not be achieved with this, like what I do in the animation of Tabs and ToggleGroupControl that I worked in (see latest version in #64926).

However, it should be possible to have everything we need by moving my thin wrapper here, and then using it as a base for this utility. That way, we unlock all use cases instead of only this specific one.

Anyway, as stated above, it seems like the name is irremediably taken, unfortunately. Maybe the lower-level hook (what would be my hook) could be called useObserveElementSize(element, onUpdate, resizeObserverOptions).

Update: I added my hook in this PR - #64943

[ciampo]

Reading the comments above, it looks like there is agreement on merging these changes, and iterating once Dani's PR is merged

[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: #64943 (useEvent and useObserveElementSize).

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