square / maker

Maker Design System by Square
https://square.github.io/maker/styleguide/latest-stable/
Other
63 stars 14 forks source link

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

Closed kevinlee11 closed 10 months ago

kevinlee11 commented 10 months ago

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.

github-actions[bot] commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

github-actions[bot] commented 10 months ago

:tada: This PR is included in version 19.6.4 :tada:

The release is available on:

Your semantic-release bot :package::rocket: