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

perf(image): scrolling large number of images in Safari #572

Merged
merged 2 commits into from
Dec 8, 2023

Conversation

kevinlee11
Copy link
Contributor

@kevinlee11 kevinlee11 commented Dec 8, 2023

Describe the problem this PR addresses

Safari seems to really struggle with getImageDimensions, specifically calling offsetHeight/offsetWidth which triggers a style recalculation. If you're scrolling a list that has images (e.g. OO on mobile view), it just causes the main thread to get frozen.

Screenshot 2023-12-08 at 3 14 12 AM
Screenshot 2023-12-08 at 3 14 45 AM

Describe the changes in this PR

Try to preload the image dimensions in the mounted event instead of waiting of the onLoaded event which can cause the main thread to freeze for a significant amount of time.

Also added logic to just skip getting the image dimensions if the shape is square.

@kevinlee11 kevinlee11 requested a review from a team as a code owner December 8, 2023 08:17
Copy link

github-actions bot commented Dec 8, 2023

Deployed Styleguide and Lab.

Notes
  1. Links may take a few minutes to update after PR is opened or commit is pushed.
  2. Links may become invalidated after PR is merged or closed.
  3. Links for all releases and open PRs can be found on the Maker Deploys page.

@pretzelhammer
Copy link
Collaborator

it kinda looks like you've created an ad-hoc implementation of lodash's throttle/debounce, is it possible to just use those functions directly instead?

@kevinlee11
Copy link
Contributor Author

it kinda looks like you've created an ad-hoc implementation of lodash's throttle/debounce, is it possible to just use those functions directly instead?

I think the issue with throttle/debounce is it would only trigger once, and I'm not as confident to use an arbitrary number value on mounted and hope after the throttle/debounce that the offesetHeight/offsetWidth is available. Because those aren't conditional that I can see?

It would just be the equivalent of doing

		const getImageDimensionsFn = () => {
			setTimeout(() => {
			    if (!this.height || !this.width) {
				this.getImageDimensionsTimeout = setTimeout(getImageDimensionsFn, timeoutValue);
			    }
			, 1000}
		};
		if (this.shouldGetImageDimensions) {
			this.$nextTick(getImageDimensionsFn);
		}

@kevinlee11
Copy link
Contributor Author

For the short term I'd also be fine just forging ahead with the shouldGetImageDimensions check, it'd be enough to test out the performance relief on several sites as they all use the square shape.

@privatenumber privatenumber changed the title fix(image): perf fix for safari when scrolling large number of images perf(image): scrolling large number of images in Safari Dec 8, 2023
@kevinlee11 kevinlee11 merged commit 3869377 into master Dec 8, 2023
4 checks passed
@kevinlee11 kevinlee11 deleted the safari-image-fix branch December 8, 2023 16:16
Copy link

github-actions bot commented Dec 8, 2023

🎉 This PR is included in version 19.6.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

2 participants