diff --git a/.changeset/fix-use-image-race-condition.md b/.changeset/fix-use-image-race-condition.md new file mode 100644 index 0000000000..eba5ae66dd --- /dev/null +++ b/.changeset/fix-use-image-race-condition.md @@ -0,0 +1,19 @@ +--- +"@heroui/use-image": patch +"@heroui/image": patch +"@heroui/avatar": patch +--- + +Fix race condition in use-image hook that caused cached images to remain invisible (stuck at opacity-0) on Firefox and Safari. + +**Root Cause:** +Event handlers (`onload`/`onerror`) were attached AFTER setting the image `src`. For cached images, the browser fires `onload` synchronously when `src` is set, causing the event to be missed. This is particularly prevalent in Firefox and Safari due to their JavaScript execution timing characteristics. + +**Solution:** +- Attach `onload`/`onerror` handlers BEFORE setting `src` +- Check both `naturalWidth` AND `naturalHeight` (per CodeRabbit review feedback on #4523) +- Handle synchronous error callbacks for failed cached images +- Include `ignoreFallback` in useCallback dependencies to prevent stale closures when prop changes dynamically +- Add comprehensive test coverage including synchronous callback scenarios and dynamic `ignoreFallback` changes + +Fixes #4534, #2259 diff --git a/packages/hooks/use-image/__tests__/use-image.test.tsx b/packages/hooks/use-image/__tests__/use-image.test.tsx index f3b8482aa6..7c3aee8630 100644 --- a/packages/hooks/use-image/__tests__/use-image.test.tsx +++ b/packages/hooks/use-image/__tests__/use-image.test.tsx @@ -13,45 +13,396 @@ describe("use-image hook", () => { mockImage.restore(); }); - it("can handle missing src", () => { - const {result} = renderHook(() => useImage({})); + describe("basic loading states", () => { + it("returns pending when no src provided", () => { + const {result} = renderHook(() => useImage({})); - expect(result.current).toEqual("pending"); + expect(result.current).toEqual("pending"); + }); + + it("returns loading then loaded for successful image load", async () => { + const {result} = renderHook(() => useImage({src: "/test.png"})); + + expect(result.current).toEqual("loading"); + mockImage.simulate("loaded"); + await waitFor(() => expect(result.current).toBe("loaded")); + }); + + it("returns loading then failed for error image", async () => { + mockImage.simulate("error"); + const {result} = renderHook(() => useImage({src: "/test.png"})); + + expect(result.current).toEqual("loading"); + await waitFor(() => expect(result.current).toBe("failed")); + }); }); - it("can handle loading image", async () => { - const {result} = renderHook(() => useImage({src: "/test.png"})); + describe("cached images", () => { + it("handles asynchronously cached images", async () => { + mockImage.simulate("loaded"); + const {result} = renderHook(() => useImage({src: "/test.png"})); + + await waitFor(() => expect(result.current).toBe("loaded")); + }); + + it("handles synchronously cached images (Firefox/Safari race condition)", async () => { + // This test simulates the race condition where cached images fire onload + // synchronously when src is set - a scenario that occurs in Firefox/Safari + mockImage.restore(); + + const originalImage = window.Image; + + // Mock Image that fires onload synchronously when src is set + // @ts-expect-error - mocking global Image + window.Image = class SyncImage { + onload: VoidFunction = () => {}; + onerror: VoidFunction = () => {}; + _src = ""; + alt = ""; + naturalWidth = 100; + naturalHeight = 100; + complete = true; + + get src() { + return this._src; + } + + set src(value: string) { + this._src = value; + // Fire onload SYNCHRONOUSLY when src is set (simulates cached image behavior) + this.onload(); + } + + hasAttribute(name: string) { + return name in this; + } + getAttribute(name: string) { + // @ts-expect-error - dynamic property access + return name in this ? this[name] : null; + } + }; + + try { + const {result} = renderHook(() => useImage({src: "/cached-image.png"})); + + // With the fix, the status should be "loaded" because handlers are attached + // BEFORE src is set, so the synchronous onload callback is caught + await waitFor(() => expect(result.current).toBe("loaded")); + } finally { + // Always restore, even if test fails + window.Image = originalImage; + } + }); + + it("handles synchronous error for cached failed images", async () => { + mockImage.restore(); + + const originalImage = window.Image; + + // Mock Image that fires onerror synchronously when src is set + // @ts-expect-error - mocking global Image + window.Image = class SyncErrorImage { + onload: VoidFunction = () => {}; + onerror: VoidFunction = () => {}; + _src = ""; + naturalWidth = 0; + naturalHeight = 0; + complete = true; - expect(result.current).toEqual("loading"); - mockImage.simulate("loaded"); - await waitFor(() => expect(result.current).toBe("loaded")); + get src() { + return this._src; + } + + set src(value: string) { + this._src = value; + // Fire onerror SYNCHRONOUSLY when src is set + this.onerror(); + } + + hasAttribute(name: string) { + return name in this; + } + getAttribute(name: string) { + // @ts-expect-error - dynamic property access + return name in this ? this[name] : null; + } + }; + + try { + const {result} = renderHook(() => useImage({src: "/broken-image.png"})); + + await waitFor(() => expect(result.current).toBe("failed")); + } finally { + window.Image = originalImage; + } + }); }); - it("can handle changing image", async () => { - const {result, rerender} = renderHook(() => useImage({src: undefined})); + describe("dynamic src changes", () => { + it("transitions from pending to loading when src changes from undefined to valid", async () => { + const {result, rerender} = renderHook(({src}: {src?: string}) => useImage({src}), { + initialProps: {src: undefined}, + }); - expect(result.current).toEqual("pending"); + expect(result.current).toEqual("pending"); - setTimeout(() => { + // Rerender with a valid src rerender({src: "/test.png"}); - }, 3000); - mockImage.simulate("loaded"); - await waitFor(() => expect(result.current).toBe("loaded")); + // Should be loading + expect(result.current).toEqual("loading"); + + // Simulate the image load + mockImage.simulate("loaded"); + await waitFor(() => expect(result.current).toBe("loaded")); + }); + + it("reloads when src changes from one valid URL to another", async () => { + const {result, rerender} = renderHook(({src}: {src?: string}) => useImage({src}), { + initialProps: {src: "/image1.png"}, + }); + + expect(result.current).toEqual("loading"); + mockImage.simulate("loaded"); + await waitFor(() => expect(result.current).toBe("loaded")); + + // Reset the mock for the new image - the new image shouldn't be "already loaded" + mockImage.restore(); + mockImage = mocks.image(); + + // Change to a different image + rerender({src: "/image2.png"}); + + // Should go back to loading for the new image + expect(result.current).toEqual("loading"); + + mockImage.simulate("loaded"); + await waitFor(() => expect(result.current).toBe("loaded")); + }); + }); + + describe("bypass options", () => { + it("returns loaded immediately when ignoreFallback is true", () => { + const {result} = renderHook(() => useImage({src: "/test.png", ignoreFallback: true})); + + expect(result.current).toEqual("loaded"); + }); + + it("returns loaded immediately when shouldBypassImageLoad is true", () => { + const {result} = renderHook(() => useImage({src: "/test.png", shouldBypassImageLoad: true})); + + // The hook returns status but ignoreFallback takes precedence in the return + // shouldBypassImageLoad makes load() return "loaded" + expect(result.current).toEqual("loaded"); + }); + + it("loads image when ignoreFallback changes from true to false", async () => { + // This test verifies that the load callback is recreated when ignoreFallback changes, + // preventing stale closure bugs where the image never loads + const {result, rerender} = renderHook( + ({src, ignoreFallback}: {src: string; ignoreFallback: boolean}) => + useImage({src, ignoreFallback}), + { + initialProps: {src: "/test.png", ignoreFallback: true}, + }, + ); + + // With ignoreFallback=true, should return "loaded" immediately without loading + expect(result.current).toEqual("loaded"); + + // Change ignoreFallback to false - should now actually load the image + rerender({src: "/test.png", ignoreFallback: false}); + + // Should be in loading state while image loads + expect(result.current).toEqual("loading"); + + // Simulate successful image load + mockImage.simulate("loaded"); + await waitFor(() => expect(result.current).toBe("loaded")); + }); + + it("returns loaded immediately when ignoreFallback changes from false to true", async () => { + // This test verifies the hook properly short-circuits when ignoreFallback becomes true + const {result, rerender} = renderHook( + ({src, ignoreFallback}: {src: string; ignoreFallback: boolean}) => + useImage({src, ignoreFallback}), + { + initialProps: {src: "/test.png", ignoreFallback: false}, + }, + ); + + // With ignoreFallback=false, should be loading + expect(result.current).toEqual("loading"); + + // Change ignoreFallback to true - should immediately return "loaded" + rerender({src: "/test.png", ignoreFallback: true}); + + // Should return "loaded" immediately due to ignoreFallback + expect(result.current).toEqual("loaded"); + }); }); - it("can handle error image", async () => { - mockImage.simulate("error"); - const {result} = renderHook(() => useImage({src: "/test.png"})); + describe("callbacks", () => { + it("calls onLoad callback when image loads", async () => { + const onLoad = jest.fn(); + + renderHook(() => useImage({src: "/test.png", onLoad})); + + mockImage.simulate("loaded"); + await waitFor(() => expect(onLoad).toHaveBeenCalled()); + }); + + it("calls onError callback when image fails", async () => { + const onError = jest.fn(); - expect(result.current).toEqual("loading"); - await waitFor(() => expect(result.current).toBe("failed")); + mockImage.simulate("error"); + renderHook(() => useImage({src: "/test.png", onError})); + + await waitFor(() => expect(onError).toHaveBeenCalled()); + }); }); - it("can handle cached image", async () => { - mockImage.simulate("loaded"); - const {result} = renderHook(() => useImage({src: "/test.png"})); + describe("image properties", () => { + it("sets crossOrigin property on image element", async () => { + mockImage.restore(); + + const originalImage = window.Image; + let capturedCrossOrigin: string | null = null; + + // @ts-expect-error - mocking global Image + window.Image = class MockImage { + onload: VoidFunction = () => {}; + onerror: VoidFunction = () => {}; + src = ""; + naturalWidth = 100; + naturalHeight = 100; + complete = false; + _crossOrigin: string | null = null; + + get crossOrigin() { + return this._crossOrigin; + } + set crossOrigin(value: string | null) { + this._crossOrigin = value; + capturedCrossOrigin = value; + } + + hasAttribute(name: string) { + return name in this; + } + getAttribute(name: string) { + // @ts-expect-error - dynamic property access + return name in this ? this[name] : null; + } + }; + + try { + renderHook(() => useImage({src: "/test.png", crossOrigin: "anonymous"})); + + expect(capturedCrossOrigin).toBe("anonymous"); + } finally { + window.Image = originalImage; + } + }); + + it("sets srcset and sizes properties on image element", async () => { + mockImage.restore(); + + const originalImage = window.Image; + let capturedSrcset: string | null = null; + let capturedSizes: string | null = null; + + // @ts-expect-error - mocking global Image + window.Image = class MockImage { + onload: VoidFunction = () => {}; + onerror: VoidFunction = () => {}; + src = ""; + naturalWidth = 100; + naturalHeight = 100; + complete = false; + _srcset = ""; + _sizes = ""; + + get srcset() { + return this._srcset; + } + set srcset(value: string) { + this._srcset = value; + capturedSrcset = value; + } + get sizes() { + return this._sizes; + } + set sizes(value: string) { + this._sizes = value; + capturedSizes = value; + } + + hasAttribute(name: string) { + return name in this; + } + getAttribute(name: string) { + // @ts-expect-error - dynamic property access + return name in this ? this[name] : null; + } + }; + + try { + renderHook(() => + useImage({ + src: "/test.png", + srcSet: "/test-2x.png 2x, /test-3x.png 3x", + sizes: "(max-width: 600px) 100vw, 50vw", + }), + ); + + expect(capturedSrcset).toBe("/test-2x.png 2x, /test-3x.png 3x"); + expect(capturedSizes).toBe("(max-width: 600px) 100vw, 50vw"); + } finally { + window.Image = originalImage; + } + }); + + it("sets loading property on image element", async () => { + mockImage.restore(); + + const originalImage = window.Image; + let capturedLoading: string | null = null; + + // @ts-expect-error - mocking global Image + window.Image = class MockImage { + onload: VoidFunction = () => {}; + onerror: VoidFunction = () => {}; + src = ""; + naturalWidth = 100; + naturalHeight = 100; + complete = false; + _loading = ""; + + get loading() { + return this._loading; + } + set loading(value: string) { + this._loading = value; + capturedLoading = value; + } + + hasAttribute(name: string) { + return name in this; + } + getAttribute(name: string) { + // @ts-expect-error - dynamic property access + return name in this ? this[name] : null; + } + }; + + try { + renderHook(() => useImage({src: "/test.png", loading: "lazy"})); - await waitFor(() => expect(result.current).toBe("loaded")); + expect(capturedLoading).toBe("lazy"); + } finally { + window.Image = originalImage; + } + }); }); }); diff --git a/packages/hooks/use-image/src/index.ts b/packages/hooks/use-image/src/index.ts index b61bbcb00f..9748ccb485 100644 --- a/packages/hooks/use-image/src/index.ts +++ b/packages/hooks/use-image/src/index.ts @@ -4,7 +4,7 @@ import type {ImgHTMLAttributes, SyntheticEvent} from "react"; -import {useRef, useState, useEffect, useCallback} from "react"; +import {useRef, useState, useCallback} from "react"; import {useIsHydrated} from "@heroui/react-utils"; import {useSafeLayoutEffect} from "@heroui/use-safe-layout-effect"; @@ -87,51 +87,75 @@ export function useImage(props: UseImageProps = {}) { const isHydrated = useIsHydrated(); - const imageRef = useRef(isHydrated ? new Image() : null); + const imageRef = useRef(null); const [status, setStatus] = useState("pending"); - useEffect(() => { - if (!imageRef.current) return; - imageRef.current.onload = (event) => { - flush(); - setStatus("loaded"); - onLoad?.(event as unknown as ImageEvent); - }; - imageRef.current.onerror = (error) => { - flush(); - setStatus("failed"); - onError?.(error as any); - }; - }, [imageRef.current]); - - const flush = () => { + const flush = useCallback(() => { if (imageRef.current) { imageRef.current.onload = null; imageRef.current.onerror = null; imageRef.current = null; } - }; + }, []); const load = useCallback((): Status => { if (!src) return "pending"; if (ignoreFallback || shouldBypassImageLoad) return "loaded"; + // Flush any previous image to avoid stale handlers + flush(); + const img = new Image(); - img.src = src; + // CRITICAL: Attach handlers BEFORE setting src to avoid race condition + // with cached images that fire onload synchronously + img.onload = (event) => { + flush(); + setStatus("loaded"); + onLoad?.(event as unknown as ImageEvent); + }; + img.onerror = (error) => { + flush(); + setStatus("failed"); + onError?.(error as any); + }; + + // Set properties AFTER handlers are attached if (crossOrigin) img.crossOrigin = crossOrigin; if (srcSet) img.srcset = srcSet; if (sizes) img.sizes = sizes; if (loading) img.loading = loading; + // Set src last - this triggers the load + img.src = src; + imageRef.current = img; - if (img.complete && img.naturalWidth) { - return "loaded"; + + // Handle already-complete images (belt and suspenders for cached images) + if (img.complete) { + // Check both naturalWidth and naturalHeight to ensure image fully loaded + if (img.naturalWidth && img.naturalHeight) { + return "loaded"; + } + + // Image is complete but has no dimensions - it failed to load + return "failed"; } return "loading"; - }, [src, crossOrigin, srcSet, sizes, onLoad, onError, loading, shouldBypassImageLoad]); + }, [ + src, + crossOrigin, + srcSet, + sizes, + onLoad, + onError, + ignoreFallback, + loading, + shouldBypassImageLoad, + flush, + ]); useSafeLayoutEffect(() => { if (isHydrated) {