Fix multiple calls to image onLoadingComplete()

packages/next/client/image.tsx

@@ -1,4 +1,4 @@
-import React from 'react'
+import React, { useRef, useEffect } from 'react'
 import Head from '../shared/lib/head'
 import {
   ImageConfigComplete,
@@ -8,7 +8,6 @@ import {
 } from '../server/image-config'
 import { useIntersection } from './use-intersection'
 
-const loadedImageURLs = new Set<string>()
 const allImgs = new Map<
   string,
   { src: string; priority: boolean; placeholder: string }
@@ -250,31 +249,34 @@ 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(
-  img: HTMLImageElement | null,
+  imgRef: React.RefObject<HTMLImageElement>,
   src: string,
   layout: LayoutValue,
   placeholder: PlaceholderValue,
-  onLoadingComplete?: OnLoadingComplete
+  onLoadingCompleteRef: React.MutableRefObject<OnLoadingComplete | undefined>
 ) {
-  if (!img) {
-    return
-  }
   const handleLoad = () => {
+    const img = imgRef.current
+    if (!img) {
+      return
+    }
     if (img.src !== emptyDataURL) {
       const p = 'decode' in img ? img.decode() : Promise.resolve()
       p.catch(() => {}).then(() => {
+        if (!imgRef.current) {
+          return
+        }
         if (placeholder === 'blur') {
           img.style.filter = ''
           img.style.backgroundSize = ''
           img.style.backgroundImage = ''
           img.style.backgroundPosition = ''
         }
-        loadedImageURLs.add(src)
-        if (onLoadingComplete) {
+        if (onLoadingCompleteRef.current) {
           const { naturalWidth, naturalHeight } = img
           // Pass back read-only primitive values but not the
           // underlying DOM element because it could be misused.
-          onLoadingComplete({ naturalWidth, naturalHeight })
+          onLoadingCompleteRef.current({ naturalWidth, naturalHeight })
         }
         if (process.env.NODE_ENV !== 'production') {
           if (img.parentElement?.parentElement) {
@@ -299,13 +301,15 @@ function handleLoading(
       })
     }
   }
-  if (img.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 {
-    img.onload = handleLoad
+  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
+    }
   }
 }
 
@@ -328,6 +332,7 @@ export default function Image({
   blurDataURL,
   ...all
 }: ImageProps) {
+  const imgRef = useRef<HTMLImageElement>(null)
   let rest: Partial<ImageProps> = all
   let layout: NonNullable<LayoutValue> = sizes ? 'responsive' : 'intrinsic'
   if ('layout' in rest) {
@@ -376,7 +381,7 @@ export default function Image({
     unoptimized = true
     isLazy = false
   }
-  if (typeof window !== 'undefined' && loadedImageURLs.has(src)) {
+  if (typeof window !== 'undefined' && imgRef.current?.complete) {
     isLazy = false
   }
 
@@ -504,7 +509,7 @@ export default function Image({
     }
   }
 
-  const [setRef, isIntersected] = useIntersection<HTMLImageElement>({
+  const [setIntersection, isIntersected] = useIntersection<HTMLImageElement>({
     rootMargin: lazyBoundary,
     disabled: !isLazy,
   })
@@ -657,6 +662,22 @@ export default function Image({
     [imageSizesPropName]: imgAttributes.sizes,
   }
 
+  const useLayoutEffect =
+    typeof window === 'undefined' ? React.useEffect : React.useLayoutEffect
+  const onLoadingCompleteRef = useRef(onLoadingComplete)
+
+  useEffect(() => {
+    onLoadingCompleteRef.current = onLoadingComplete
+  }, [onLoadingComplete])
+
+  useLayoutEffect(() => {
+    setIntersection(imgRef.current)
+  }, [setIntersection])
+
+  useEffect(() => {
+    handleLoading(imgRef, srcString, layout, placeholder, onLoadingCompleteRef)
+  }, [srcString, layout, placeholder, isVisible])
+
   return (
     <span style={wrapperStyle}>
       {hasSizer ? (
@@ -687,10 +708,7 @@ export default function Image({
         decoding="async"
         data-nimg={layout}
         className={className}
-        ref={(img) => {
-          setRef(img)
-          handleLoading(img, srcString, layout, placeholder, onLoadingComplete)
-        }}
+        ref={imgRef}
         style={{ ...imgStyle, ...blurStyle }}
       />
       {isLazy && (

test/integration/image-component/default/pages/on-loading-complete.js

@@ -1,74 +1,122 @@
 import { useState } from 'react'
 import Image from 'next/image'
 
-const Page = () => (
-  <div>
-    <h1>On Loading Complete Test</h1>
-    <ImageWithMessage
-      id="1"
-      src="/test.jpg"
-      layout="intrinsic"
-      width="128"
-      height="128"
-    />
-    <hr />
-    <ImageWithMessage
-      id="2"
-      src={require('../public/test.png')}
-      placeholder="blur"
-      layout="fixed"
-    />
-    <hr />
-    <ImageWithMessage
-      id="3"
-      src="/test.svg"
-      layout="responsive"
-      width="1200"
-      height="1200"
-    />
-    <hr />
-    <div style={{ position: 'relative', width: '64px', height: '64px' }}>
+const Page = () => {
+  // Hoisted state to count each image load callback
+  const [idToCount, setIdToCount] = useState({})
+  const [clicked, setClicked] = useState(false)
+
+  return (
+    <div>
+      <h1>On Loading Complete Test</h1>
+      <ImageWithMessage
+        id="1"
+        src="/test.jpg"
+        layout="intrinsic"
+        width="128"
+        height="128"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+
+      <ImageWithMessage
+        id="2"
+        src={require('../public/test.png')}
+        placeholder="blur"
+        layout="fixed"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+
+      <ImageWithMessage
+        id="3"
+        src="/test.svg"
+        layout="responsive"
+        width="1200"
+        height="1200"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+
       <ImageWithMessage
         id="4"
         src="/test.ico"
         layout="fill"
         objectFit="contain"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
       />
-    </div>
-    <hr />
-    <ImageWithMessage
-      id="5"
-      src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAAFCAYAAACAcVaiAAAAE0lEQVR42mNk+P+/ngEKGMngAADDdwx3Uz/3AAAAAABJRU5ErkJggg=="
-      layout="fixed"
-      width={64}
-      height={64}
-    />
-    <hr />
-    <div style={{ position: 'relative', width: '64px', height: '64px' }}>
+
+      <ImageWithMessage
+        id="5"
+        src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAAFCAYAAACAcVaiAAAAE0lEQVR42mNk+P+/ngEKGMngAADDdwx3Uz/3AAAAAABJRU5ErkJggg=="
+        layout="fixed"
+        width={64}
+        height={64}
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+
       <ImageWithMessage
         id="6"
         src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAMAAAAFCAYAAACAcVaiAAAAE0lEQVR42mNk+P+/ngEKGMngAADDdwx3Uz/3AAAAAABJRU5ErkJggg=="
         layout="fill"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+
+      <ImageWithMessage
+        id="7"
+        src="/test.bmp"
+        loading="eager"
+        width="400"
+        height="400"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
       />
+
+      <ImageWithMessage
+        id="8"
+        src={clicked ? '/foo/test-rect.jpg' : '/wide.png'}
+        layout={clicked ? 'fixed' : 'intrinsic'}
+        width="500"
+        height="500"
+        idToCount={idToCount}
+        setIdToCount={setIdToCount}
+      />
+      <button id="toggle" onClick={() => setClicked(!clicked)}>
+        Toggle
+      </button>
+      <div id="footer" />
     </div>
-    <div id="footer" />
-  </div>
-)
+  )
+}
 
-function ImageWithMessage({ id, ...props }) {
+function ImageWithMessage({ id, idToCount, setIdToCount, ...props }) {
   const [msg, setMsg] = useState('[LOADING]')
+  const style =
+    props.layout === 'fill'
+      ? { position: 'relative', width: '64px', height: '64px' }
+      : {}
   return (
     <>
-      <Image
-        id={`img${id}`}
-        onLoadingComplete={({ naturalWidth, naturalHeight }) =>
-          setMsg(
-            `loaded img${id} with dimensions ${naturalWidth}x${naturalHeight}`
-          )
-        }
-        {...props}
-      />
+      <div className="wrap" style={style}>
+        <Image
+          id={`img${id}`}
+          onLoadingComplete={({ naturalWidth, naturalHeight }) => {
+            let count = idToCount[id] || 0
+            count++
+            idToCount[id] = count
+            setIdToCount(idToCount)
+            const msg = `loaded ${count} img${id} with dimensions ${naturalWidth}x${naturalHeight}`
+            setMsg(msg)
+            console.log(msg)
+          }}
+          {...props}
+        />
+      </div>
       <p id={`msg${id}`}>{msg}</p>
+      <hr />
     </>
   )
 }