From 7a7e7d4f933356c1137daca30a865bfa443fc779 Mon Sep 17 00:00:00 2001 From: Steven Date: Wed, 30 Nov 2022 21:30:02 -0500 Subject: [PATCH] Remove `useState` from `next/image` (#43587) This PR remove `React.useState()` from the `next/image` component. It was only used in the `onError` case and it was causing Safari to become very slow when there were many images on the same page. We were seeing 1s delay blocking the main thread when there were about 350 images on the same page. Chrome and Firefox were not slow. --- packages/next/client/image.tsx | 47 ++++++------- .../default/pages/blurry-placeholder.js | 31 --------- .../next-image-new/default/test/index.test.ts | 66 ++----------------- .../integration/production/test/index.test.js | 40 +++++------ 4 files changed, 43 insertions(+), 141 deletions(-) delete mode 100644 test/integration/next-image-new/default/pages/blurry-placeholder.js diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx index fcde56b2d4b3c3c..6a231114fc8312f 100644 --- a/packages/next/client/image.tsx +++ b/packages/next/client/image.tsx @@ -6,7 +6,6 @@ import React, { useCallback, useContext, useMemo, - useState, forwardRef, } from 'react' import Head from '../shared/lib/head' @@ -149,8 +148,6 @@ type ImageElementProps = Omit & { placeholder: PlaceholderValue onLoadRef: React.MutableRefObject onLoadingCompleteRef: React.MutableRefObject - setBlurComplete: (b: boolean) => void - setShowAltText: (b: boolean) => void } function getWidths( @@ -264,10 +261,8 @@ function getInt(x: unknown): number | undefined { function handleLoading( img: ImgElementWithDataProp, src: string, - placeholder: PlaceholderValue, onLoadRef: React.MutableRefObject, onLoadingCompleteRef: React.MutableRefObject, - setBlurComplete: (b: boolean) => void, unoptimized: boolean ) { if (!img || img['data-loaded-src'] === src) { @@ -284,9 +279,6 @@ function handleLoading( // - decode() completes return } - if (placeholder === 'blur') { - setBlurComplete(true) - } if (onLoadRef?.current) { // Since we don't have the SyntheticEvent here, // we must create one with the same shape. @@ -383,8 +375,6 @@ const ImageElement = forwardRef( loader, onLoadRef, onLoadingCompleteRef, - setBlurComplete, - setShowAltText, onLoad, onError, ...rest @@ -441,20 +431,16 @@ const ImageElement = forwardRef( handleLoading( img, srcString, - placeholder, onLoadRef, onLoadingCompleteRef, - setBlurComplete, unoptimized ) } }, [ srcString, - placeholder, onLoadRef, onLoadingCompleteRef, - setBlurComplete, onError, unoptimized, forwardedRef, @@ -465,20 +451,30 @@ const ImageElement = forwardRef( handleLoading( img, srcString, - placeholder, onLoadRef, onLoadingCompleteRef, - setBlurComplete, unoptimized ) }} onError={(event) => { - // if the real image fails to load, this will ensure "alt" is visible - setShowAltText(true) - if (placeholder === 'blur') { - // If the real image fails to load, this will still remove the placeholder. - setBlurComplete(true) + // Note: We removed React.useState() in the error case here + // because it was causing Safari to become very slow when + // there were many images on the same page. + const { style } = event.currentTarget + + if (style.color === 'transparent') { + // If src image fails to load, this will ensure "alt" is visible + style.color = '' + } + + if (placeholder === 'blur' && style.backgroundImage) { + // If src image fails to load, this will ensure the placeholder is removed + style.backgroundSize = '' + style.backgroundPosition = '' + style.backgroundRepeat = '' + style.backgroundImage = '' } + if (onError) { onError(event) } @@ -626,9 +622,6 @@ const Image = forwardRef( unoptimized = true } - const [blurComplete, setBlurComplete] = useState(false) - const [showAltText, setShowAltText] = useState(false) - const qualityInt = getInt(quality) if (process.env.NODE_ENV !== 'production') { @@ -810,12 +803,12 @@ const Image = forwardRef( objectPosition, } : {}, - showAltText ? {} : { color: 'transparent' }, + { color: 'transparent' }, style ) const blurStyle = - placeholder === 'blur' && blurDataURL && !blurComplete + placeholder === 'blur' && blurDataURL ? { backgroundSize: imgStyle.objectFit || 'cover', backgroundPosition: imgStyle.objectPosition || '50% 50%', @@ -905,8 +898,6 @@ const Image = forwardRef( srcString, onLoadRef, onLoadingCompleteRef, - setBlurComplete, - setShowAltText, ...rest, } return ( diff --git a/test/integration/next-image-new/default/pages/blurry-placeholder.js b/test/integration/next-image-new/default/pages/blurry-placeholder.js deleted file mode 100644 index 30583ad0a20a2ce..000000000000000 --- a/test/integration/next-image-new/default/pages/blurry-placeholder.js +++ /dev/null @@ -1,31 +0,0 @@ -import React from 'react' -import Image from 'next/image' - -export default function Page() { - return ( -
-

Blurry Placeholder

- - - -
- - -
- ) -} diff --git a/test/integration/next-image-new/default/test/index.test.ts b/test/integration/next-image-new/default/test/index.test.ts index 65892992a95c015..9e4ff54778a7cc2 100644 --- a/test/integration/next-image-new/default/test/index.test.ts +++ b/test/integration/next-image-new/default/test/index.test.ts @@ -599,8 +599,8 @@ function runTests(mode) { 'lazy' ) expect(await browser.elementById('blur1').getAttribute('sizes')).toBeNull() - expect(await browser.elementById('blur1').getAttribute('style')).toBe( - 'color: transparent;' + expect(await browser.elementById('blur1').getAttribute('style')).toMatch( + 'color:transparent;background-size:cover;background-position:50% 50%;background-repeat:no-repeat;' ) expect(await browser.elementById('blur1').getAttribute('height')).toBe( '400' @@ -646,8 +646,8 @@ function runTests(mode) { expect(await browser.elementById('blur2').getAttribute('loading')).toBe( 'lazy' ) - expect(await browser.elementById('blur2').getAttribute('style')).toBe( - 'color: transparent;' + expect(await browser.elementById('blur2').getAttribute('style')).toMatch( + 'color:transparent;background-size:cover;background-position:50% 50%;background-repeat:no-repeat' ) expect(await browser.elementById('blur2').getAttribute('height')).toBe( '400' @@ -1084,15 +1084,6 @@ function runTests(mode) { expect(await getComputedStyle(browser, 'img-blur', 'filter')).toBe( 'opacity(0.5)' ) - expect(await getComputedStyle(browser, 'img-blur', 'background-size')).toBe( - '30%' - ) - expect( - await getComputedStyle(browser, 'img-blur', 'background-image') - ).toMatch('iVBORw0KGgo=') - expect( - await getComputedStyle(browser, 'img-blur', 'background-position') - ).toBe('1px 2px') }) describe('Fill-mode tests', () => { let browser @@ -1187,55 +1178,6 @@ function runTests(mode) { }) } - it('should have blurry placeholder when enabled', async () => { - const html = await renderViaHTTP(appPort, '/blurry-placeholder') - const $html = cheerio.load(html) - - $html('noscript > img').attr('id', 'unused') - - expect($html('#blurry-placeholder-raw')[0].attribs.style).toContain( - `color:transparent;background-size:cover;background-position:50% 50%;background-repeat:no-repeat;background-image:url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http%3A//www.w3.org/2000/svg' viewBox='0 0 400 400'%3E%3Cfilter id='b' color-interpolation-filters='sRGB'%3E%3CfeGaussianBlur stdDeviation='20'/%3E%3C/filter%3E%3Cimage preserveAspectRatio='none' filter='url(%23b)' x='0' y='0' height='100%25' width='100%25' href='data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8P4nhDwAGuAKPn6cicwAAAABJRU5ErkJggg=='/%3E%3C/svg%3E")` - ) - - expect($html('#blurry-placeholder-with-lazy')[0].attribs.style).toContain( - `color:transparent;background-size:cover;background-position:50% 50%;background-repeat:no-repeat;background-image:url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http%3A//www.w3.org/2000/svg' viewBox='0 0 400 400'%3E%3Cfilter id='b' color-interpolation-filters='sRGB'%3E%3CfeGaussianBlur stdDeviation='20'/%3E%3C/filter%3E%3Cimage preserveAspectRatio='none' filter='url(%23b)' x='0' y='0' height='100%25' width='100%25' href='data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mO0/8/wBwAE/wI85bEJ6gAAAABJRU5ErkJggg=='/%3E%3C/svg%3E")` - ) - }) - - it('should remove blurry placeholder after image loads', async () => { - const browser = await webdriver(appPort, '/blurry-placeholder') - await check( - async () => - await getComputedStyle( - browser, - 'blurry-placeholder-raw', - 'background-image' - ), - 'none' - ) - expect( - await getComputedStyle( - browser, - 'blurry-placeholder-with-lazy', - 'background-image' - ) - ).toBe( - `url("data:image/svg+xml;charset=utf-8,%3Csvg xmlns='http%3A//www.w3.org/2000/svg' viewBox='0 0 400 400'%3E%3Cfilter id='b' color-interpolation-filters='sRGB'%3E%3CfeGaussianBlur stdDeviation='20'/%3E%3C/filter%3E%3Cimage preserveAspectRatio='none' filter='url(%23b)' x='0' y='0' height='100%25' width='100%25' href='data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mO0/8/wBwAE/wI85bEJ6gAAAABJRU5ErkJggg=='/%3E%3C/svg%3E")` - ) - - await browser.eval('document.getElementById("spacer").remove()') - - await check( - async () => - await getComputedStyle( - browser, - 'blurry-placeholder-with-lazy', - 'background-image' - ), - 'none' - ) - }) - it('should be valid HTML', async () => { let browser try { diff --git a/test/integration/production/test/index.test.js b/test/integration/production/test/index.test.js index d232f7eb3aeb66a..71351162730c0f5 100644 --- a/test/integration/production/test/index.test.js +++ b/test/integration/production/test/index.test.js @@ -1301,7 +1301,7 @@ describe('Production Usage', () => { }) } - it('should remove placeholder for next/image correctly', async () => { + it('should loading next/image correctly', async () => { const browser = await webdriver(context.appPort, '/') await browser.eval(`(function() { @@ -1312,10 +1312,16 @@ describe('Production Usage', () => { expect(await browser.eval('window.beforeNav')).toBe(1) - await check( - () => browser.elementByCss('img').getComputedCss('background-image'), - 'none' - ) + await check(async () => { + const result = await browser.eval( + `document.getElementById('static-image').width` + ) + if (result === 0) { + throw new Error('Incorrectly loaded image') + } + + return 'result-correct' + }, /result-correct/) await browser.eval(`(function() { window.beforeNav = 1 @@ -1332,22 +1338,16 @@ describe('Production Usage', () => { expect(await browser.eval('window.beforeNav')).toBe(1) - await check( - () => - browser - .elementByCss('#static-image') - .getComputedCss('background-image'), - 'none' - ) + await check(async () => { + const result = await browser.eval( + `document.getElementById('static-image').width` + ) + if (result === 0) { + throw new Error('Incorrectly loaded image') + } - for (let i = 0; i < 5; i++) { - expect( - await browser - .elementByCss('#static-image') - .getComputedCss('background-image') - ).toBe('none') - await waitFor(500) - } + return 'result-correct' + }, /result-correct/) }) dynamicImportTests(context, (p, q) => renderViaHTTP(context.appPort, p, q))