Skip to content

Conversation

@TatuLund
Copy link
Contributor

@TatuLund TatuLund commented Oct 2, 2025

Modified ResizeObserver to use requestAnimationFrame for better performance in Safari.

Modified ResizeObserver to use requestAnimationFrame for better performance in Safari.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2025

Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Is there any particular case where you were able to observe performance problems? If so, please create an issue first. I understand that it might be difficult to cover the change with a unit test but ideally we would have at least a test to cover timings change.

// Use requestAnimationFrame to avoid performance issues with Safari
this.__resizeObserver = new ResizeObserver(() => {
const ironListAdapter = this;
requestAnimationFrame(() => ironListAdapter._resizeHandler());
Copy link
Member

Choose a reason for hiding this comment

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

A debouncer would be preferable here, since it also eliminates unnecessary resize handler calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would work too.

Copy link
Contributor

@vursen vursen left a comment

Choose a reason for hiding this comment

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

Calling _resizeHandler right inside the ResizeObserver callback has performance advantages. Layout is already computed at that stage, but the frame hasn't been painted, so reading such properties as scrollWidth or using getComputedStyle shouldn't generally cause a reflow as long as the layout has not been dirtied again. We will lose this benefit if _resizeHandler is deferred until the next frame.

Also, delaying _resizeHandler to the next frame may not fix the issue and could just make it asynchronous.

We need a reproducible example to confirm that this change actually fixes the issue.

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