Fix Firefox flash of white on next/image with placeholder=blur (#…

…35889) ## History In PR #24153, `placeholder=blur` was added and it set `element.style.backgroundImage = 'none'` instead of using react state to re-render. Then in PR #25916, a delay was added to handle removing the blur placeholder. Then in PR #25994, `img.decode()` was utilized but we found this caused problems in Firefox in #26011. ## Today This PR changes the the blur placeholder removal to use react state to re-render. This _should_ prevent Firefox from erroring although we should probably keep the catch() just in case. This was pointed out when React 18 caused subtle differences in Firefox in this comment #30128 (comment)
vercel · Apr 5, 2022 · cec388c · cec388c
1 parent 4a7ab34
commit cec388c
Show file tree

Hide file tree

Showing 2 changed files with 120 additions and 71 deletions.
diff --git a/packages/next/client/image.tsx b/packages/next/client/image.tsx
@@ -1,4 +1,11 @@
-import React, { useRef, useEffect, useContext, useMemo } from 'react'
+import React, {
+  useRef,
+  useEffect,
+  useCallback,
+  useContext,
+  useMemo,
+  useState,
+} from 'react'
 import Head from '../shared/lib/head'
 import {
   ImageConfigComplete,
@@ -66,6 +73,10 @@ type OnLoadingComplete = (result: {
 
 type ImgElementStyle = NonNullable<JSX.IntrinsicElements['img']['style']>
 
+type ImgElementWithDataProp = HTMLImageElement & {
+  'data-loaded-src': string | undefined
+}
+
 export interface StaticImageData {
   src: string
   height: number
@@ -131,11 +142,15 @@ type ImageElementProps = Omit<ImageProps, 'src'> & {
   imgStyle: ImgElementStyle
   blurStyle: ImgElementStyle
   isLazy: boolean
-  imgRef: React.RefObject<HTMLImageElement>
   loading: LoadingValue
   config: ImageConfig
   unoptimized: boolean
   loader: ImageLoader
+  placeholder: PlaceholderValue
+  onLoadingCompleteRef: React.MutableRefObject<OnLoadingComplete | undefined>
+  setBlurComplete: (b: boolean) => void
+  setIntersection: (img: HTMLImageElement | null) => void
+  isVisible: boolean
 }
 
 function getWidths(
@@ -270,70 +285,59 @@ function defaultImageLoader(loaderProps: ImageLoaderProps) {
 // See https://stackoverflow.com/q/39777833/266535 for why we use this ref
 // handler instead of the img's onLoad attribute.
 function handleLoading(
-  imgRef: React.RefObject<HTMLImageElement>,
+  img: ImgElementWithDataProp,
   src: string,
   layout: LayoutValue,
   placeholder: PlaceholderValue,
-  onLoadingCompleteRef: React.MutableRefObject<OnLoadingComplete | undefined>
+  onLoadingCompleteRef: React.MutableRefObject<OnLoadingComplete | undefined>,
+  setBlurComplete: (b: boolean) => void
 ) {
-  const handleLoad = () => {
-    const img = imgRef.current
-    if (!img) {
+  if (!img || img.src === emptyDataURL || img['data-loaded-src'] === src) {
+    return
+  }
+  img['data-loaded-src'] = src
+  const p = 'decode' in img ? img.decode() : Promise.resolve()
+  p.catch(() => {}).then(() => {
+    if (!img.parentNode) {
+      // Exit early in case of race condition:
+      // - onload() is called
+      // - decode() is called but incomplete
+      // - unmount is called
+      // - decode() completes
       return
     }
-    if (img.src !== emptyDataURL) {
-      const p = 'decode' in img ? img.decode() : Promise.resolve()
-      p.catch(() => {}).then(() => {
-        if (!imgRef.current) {
-          return
-        }
-        loadedImageURLs.add(src)
-        if (placeholder === 'blur') {
-          img.style.filter = ''
-          img.style.backgroundSize = ''
-          img.style.backgroundImage = ''
-          img.style.backgroundPosition = ''
-        }
-        if (onLoadingCompleteRef.current) {
-          const { naturalWidth, naturalHeight } = img
-          // Pass back read-only primitive values but not the
-          // underlying DOM element because it could be misused.
-          onLoadingCompleteRef.current({ naturalWidth, naturalHeight })
-        }
-        if (process.env.NODE_ENV !== 'production') {
-          if (img.parentElement?.parentElement) {
-            const parent = getComputedStyle(img.parentElement.parentElement)
-            if (!parent.position) {
-              // The parent has not been rendered to the dom yet and therefore it has no position. Skip the warnings for such cases.
-            } else if (layout === 'responsive' && parent.display === 'flex') {
-              warnOnce(
-                `Image with src "${src}" may not render properly as a child of a flex container. Consider wrapping the image with a div to configure the width.`
-              )
-            } else if (
-              layout === 'fill' &&
-              parent.position !== 'relative' &&
-              parent.position !== 'fixed' &&
-              parent.position !== 'absolute'
-            ) {
-              warnOnce(
-                `Image with src "${src}" may not render properly with a parent using position:"${parent.position}". Consider changing the parent style to position:"relative" with a width and height.`
-              )
-            }
-          }
-        }
-      })
+    loadedImageURLs.add(src)
+    if (placeholder === 'blur') {
+      setBlurComplete(true)
     }
-  }
-  if (imgRef.current) {
-    if (imgRef.current.complete) {
-      // If the real image fails to load, this will still remove the placeholder.
-      // This is the desired behavior for now, and will be revisited when error
-      // handling is worked on for the image component itself.
-      handleLoad()
-    } else {
-      imgRef.current.onload = handleLoad
+    if (onLoadingCompleteRef?.current) {
+      const { naturalWidth, naturalHeight } = img
+      // Pass back read-only primitive values but not the
+      // underlying DOM element because it could be misused.
+      onLoadingCompleteRef.current({ naturalWidth, naturalHeight })
     }
-  }
+    if (process.env.NODE_ENV !== 'production') {
+      if (img.parentElement?.parentElement) {
+        const parent = getComputedStyle(img.parentElement.parentElement)
+        if (!parent.position) {
+          // The parent has not been rendered to the dom yet and therefore it has no position. Skip the warnings for such cases.
+        } else if (layout === 'responsive' && parent.display === 'flex') {
+          warnOnce(
+            `Image with src "${src}" may not render properly as a child of a flex container. Consider wrapping the image with a div to configure the width.`
+          )
+        } else if (
+          layout === 'fill' &&
+          parent.position !== 'relative' &&
+          parent.position !== 'fixed' &&
+          parent.position !== 'absolute'
+        ) {
+          warnOnce(
+            `Image with src "${src}" may not render properly with a parent using position:"${parent.position}". Consider changing the parent style to position:"relative" with a width and height.`
+          )
+        }
+      }
+    }
+  })
 }
 
 export default function Image({
@@ -352,6 +356,7 @@ export default function Image({
   objectFit,
   objectPosition,
   onLoadingComplete,
+  onError,
   loader = defaultImageLoader,
   placeholder = 'empty',
   blurDataURL,
@@ -417,6 +422,7 @@ export default function Image({
     isLazy = false
   }
 
+  const [blurComplete, setBlurComplete] = useState(false)
   const [setIntersection, isIntersected, resetIntersected] =
     useIntersection<HTMLImageElement>({
       rootRef: lazyRoot,
@@ -641,7 +647,7 @@ export default function Image({
       : layoutStyle
   )
   const blurStyle =
-    placeholder === 'blur'
+    placeholder === 'blur' && !blurComplete
       ? {
           filter: 'blur(20px)',
           backgroundSize: objectFit || 'cover',
@@ -744,7 +750,6 @@ export default function Image({
   const onLoadingCompleteRef = useRef(onLoadingComplete)
 
   const previousImageSrc = useRef<string | StaticImport>(src)
-  const imgRef = useRef<HTMLImageElement>(null)
   useEffect(() => {
     onLoadingCompleteRef.current = onLoadingComplete
   }, [onLoadingComplete])
@@ -754,13 +759,8 @@ export default function Image({
       resetIntersected()
       previousImageSrc.current = src
     }
+  }, [resetIntersected, src])
 
-    setIntersection(imgRef.current)
-  }, [setIntersection, resetIntersected, src])
-
-  useEffect(() => {
-    handleLoading(imgRef, srcString, layout, placeholder, onLoadingCompleteRef)
-  }, [srcString, layout, placeholder, isVisible])
   const imgElementArgs = {
     isLazy,
     imgAttributes,
@@ -771,13 +771,16 @@ export default function Image({
     className,
     imgStyle,
     blurStyle,
-    imgRef,
     loading,
     config,
     unoptimized,
     placeholder,
     loader,
     srcString,
+    onLoadingCompleteRef,
+    setBlurComplete,
+    setIntersection,
+    isVisible,
     ...rest,
   }
   return (
@@ -846,13 +849,17 @@ const ImageElement = ({
   imgStyle,
   blurStyle,
   isLazy,
-  imgRef,
   placeholder,
   loading,
   srcString,
   config,
   unoptimized,
   loader,
+  onLoadingCompleteRef,
+  setBlurComplete,
+  setIntersection,
+  onError,
+  isVisible,
   ...rest
 }: ImageElementProps) => {
   return (
@@ -866,8 +873,50 @@ const ImageElement = ({
         decoding="async"
         data-nimg={layout}
         className={className}
-        ref={imgRef}
         style={{ ...imgStyle, ...blurStyle }}
+        ref={useCallback(
+          (img: ImgElementWithDataProp) => {
+            setIntersection(img)
+            if (img?.complete) {
+              handleLoading(
+                img,
+                srcString,
+                layout,
+                placeholder,
+                onLoadingCompleteRef,
+                setBlurComplete
+              )
+            }
+          },
+          [
+            setIntersection,
+            srcString,
+            layout,
+            placeholder,
+            onLoadingCompleteRef,
+            setBlurComplete,
+          ]
+        )}
+        onLoad={(event) => {
+          const img = event.currentTarget as ImgElementWithDataProp
+          handleLoading(
+            img,
+            srcString,
+            layout,
+            placeholder,
+            onLoadingCompleteRef,
+            setBlurComplete
+          )
+        }}
+        onError={(event) => {
+          if (placeholder === 'blur') {
+            // If the real image fails to load, this will still remove the placeholder.
+            setBlurComplete(true)
+          }
+          if (onError) {
+            onError(event)
+          }
+        }}
       />
       {(isLazy || placeholder === 'blur') && (
         <noscript>

diff --git a/test/integration/image-component/default/pages/invalid-loader.js b/test/integration/image-component/default/pages/invalid-loader.js
@@ -32,15 +32,15 @@ const Page = () => {
         width={100}
         height={100}
         loader={({ src, width }) =>
-          `https://example.com${src}?width=${width * 2}`
+          `https://example.vercel.sh${src}?width=${width * 2}`
         }
       />
       <Image
         id="width-querystring-size"
         src="/test.tiff"
         width={100}
         height={100}
-        loader={({ src }) => `https://example.com${src}?size=medium`}
+        loader={({ src }) => `https://example.vercel.sh${src}?size=medium`}
       />
       <footer>footer</footer>
     </div>