diff --git a/.changeset/moody-monkeys-swim.md b/.changeset/moody-monkeys-swim.md new file mode 100644 index 0000000000..d3a16e0a2d --- /dev/null +++ b/.changeset/moody-monkeys-swim.md @@ -0,0 +1,9 @@ +--- +"@radix-ui/react-avatar": minor +"radix-ui": minor +--- + +Fixed several edge cases with Avatar's loading state: +- An avatar's fallback would not be displayed again if its image component unmounted. This is now fixed. +- Rendering multiple `Avatar.Image` components per `Avatar.Root` was never supported and results in buggy, unpredictable behavior. We now warn about this in development. +- Zero-sized images were treated as `loading`, meaning that `onLoadingStatusChange` is never called once loaded. A zero-sized image now triggers an `error` status on load. diff --git a/packages/react/avatar/src/avatar.test.tsx b/packages/react/avatar/src/avatar.test.tsx index eb21be35f9..28d52accc1 100644 --- a/packages/react/avatar/src/avatar.test.tsx +++ b/packages/react/avatar/src/avatar.test.tsx @@ -132,6 +132,26 @@ describe('given an Avatar with fallback and an image', () => { expect(image).not.toBeInTheDocument(); }); + it('should render the fallback again after a loaded image unmounts', async () => { + const conditionalUi = (showImage: boolean) => ( + + {FALLBACK_TEXT} + {showImage ? : null} + + ); + rendered.unmount(); + rendered = render(conditionalUi(true)); + + image = await rendered.findByRole('img'); + expect(image).toBeInTheDocument(); + expect(rendered.queryByText(FALLBACK_TEXT)).not.toBeInTheDocument(); + + rendered.rerender(conditionalUi(false)); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + expect(rendered.queryByText(FALLBACK_TEXT)).toBeInTheDocument(); + }); + it('should show fallback if image has no data', async () => { rendered.unmount(); const spy = vi.spyOn(window.Image.prototype, 'naturalWidth', 'get'); @@ -142,6 +162,63 @@ describe('given an Avatar with fallback and an image', () => { spy.mockRestore(); }); + it('should show the fallback if a loaded image reports no natural size', async () => { + rendered.unmount(); + const spy = vi.spyOn(window.Image.prototype, 'naturalWidth', 'get').mockReturnValue(0); + const onLoadingStatusChange = vi.fn(); + rendered = render( + + {FALLBACK_TEXT} + + , + ); + + // The image dispatches `load`, but because its natural size is 0 we treat it as an error. + await waitFor(() => expect(onLoadingStatusChange).toHaveBeenCalledWith('error')); + expect(rendered.queryByRole('img')).not.toBeInTheDocument(); + expect(rendered.queryByText(FALLBACK_TEXT)).toBeInTheDocument(); + spy.mockRestore(); + }); + + describe('onLoadingStatusChange', () => { + const renderWithCallback = (src?: string) => { + const onLoadingStatusChange = vi.fn(); + rendered.unmount(); + rendered = render( + + {FALLBACK_TEXT} + + , + ); + return onLoadingStatusChange; + }; + + it('is called with "loaded" once the image loads', async () => { + const onLoadingStatusChange = renderWithCallback('/test.png'); + await rendered.findByRole('img'); + expect(onLoadingStatusChange).toHaveBeenCalledWith('loaded'); + }); + + it('is called with "error" when there is no src', async () => { + const onLoadingStatusChange = renderWithCallback(); + await waitFor(() => expect(onLoadingStatusChange).toHaveBeenCalledWith('error')); + }); + + it('is not called with "idle" while the image is mounted', async () => { + const onLoadingStatusChange = renderWithCallback('/test.png'); + await rendered.findByRole('img'); + expect(onLoadingStatusChange).not.toHaveBeenCalledWith('idle'); + }); + }); + describe('SSR', () => { afterEach(cleanup); @@ -215,6 +292,8 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r onSrcChange() { setTimeout(() => { if (this.referrerPolicy === 'no-referrer') { + // Mirror real browsers: mark the image complete before `load` fires. + cache.add(this.src); this.dispatchEvent(new Event('load')); } else { this.dispatchEvent(new Event('error')); @@ -283,6 +362,46 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r }); }); +describe('given an Avatar with multiple images (development)', () => { + let warnSpy: ReturnType; + + beforeEach(() => { + vi.stubEnv('NODE_ENV', 'development'); + warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + cleanup(); + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + it('warns that multiple images are not supported', async () => { + render( + + {FALLBACK_TEXT} + + + , + ); + + await waitFor(() => + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('multiple were detected')), + ); + }); + + it('does not warn for a single image', () => { + render( + + {FALLBACK_TEXT} + + , + ); + + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); + class MockImage extends EventTarget { _src: string = ''; @@ -313,8 +432,10 @@ class MockImage extends EventTarget { onSrcChange() { setTimeout(() => { - this.dispatchEvent(new Event('load')); + // Mirror real browsers: the image is `complete` (and `naturalWidth` is + // populated) before the `load` event fires. cache.add(this.src); + this.dispatchEvent(new Event('load')); }, DELAY); } } diff --git a/packages/react/avatar/src/avatar.tsx b/packages/react/avatar/src/avatar.tsx index dee2df22c6..c5c5b816c0 100644 --- a/packages/react/avatar/src/avatar.tsx +++ b/packages/react/avatar/src/avatar.tsx @@ -3,7 +3,6 @@ import { createContextScope } from '@radix-ui/react-context'; import { useCallbackRef } from '@radix-ui/react-use-callback-ref'; import { useLayoutEffect } from '@radix-ui/react-use-layout-effect'; import { Primitive } from '@radix-ui/react-primitive'; -import { useIsHydrated } from '@radix-ui/react-use-is-hydrated'; import type { Scope } from '@radix-ui/react-context'; @@ -20,9 +19,16 @@ type ImageLoadingStatus = 'idle' | 'loading' | 'loaded' | 'error'; type AvatarContextValue = { imageLoadingStatus: ImageLoadingStatus; - onImageLoadingStatusChange(status: ImageLoadingStatus): void; + setImageLoadingStatus: React.Dispatch>; + imageCount: number; + setImageCount: React.Dispatch>; }; +const STATIC_IMAGE_COUNT_STATE: [number, React.Dispatch>] = [ + 0, + () => void 0, +]; + const [AvatarProvider, useAvatarContext] = createAvatarContext(AVATAR_NAME); type AvatarElement = React.ComponentRef; @@ -33,11 +39,15 @@ const Avatar = React.forwardRef( (props: ScopedProps, forwardedRef) => { const { __scopeAvatar, ...avatarProps } = props; const [imageLoadingStatus, setImageLoadingStatus] = React.useState('idle'); + const [imageCount, setImageCount] = useImageCount(); + return ( @@ -61,16 +71,27 @@ interface AvatarImageProps extends PrimitiveImageProps { const AvatarImage = React.forwardRef( (props: ScopedProps, forwardedRef) => { - const { __scopeAvatar, src, onLoadingStatusChange = () => {}, ...imageProps } = props; + const { __scopeAvatar, src, onLoadingStatusChange, ...imageProps } = props; const context = useAvatarContext(IMAGE_NAME, __scopeAvatar); - const imageLoadingStatus = useImageLoadingStatus(src, imageProps); + useUpdateImageCount(context.setImageCount); + + const imageLoadingStatus = useImageLoadingStatus(src, { + referrerPolicy: imageProps.referrerPolicy, + crossOrigin: imageProps.crossOrigin, + loadingStatus: context.imageLoadingStatus, + setLoadingStatus: context.setImageLoadingStatus, + }); const handleLoadingStatusChange = useCallbackRef((status: ImageLoadingStatus) => { - onLoadingStatusChange(status); - context.onImageLoadingStatusChange(status); + onLoadingStatusChange?.(status); }); + const loadingStatusRef = React.useRef(imageLoadingStatus); + useLayoutEffect(() => { - if (imageLoadingStatus !== 'idle') { + const previousLoadingStatus = loadingStatusRef.current; + loadingStatusRef.current = imageLoadingStatus; + + if (imageLoadingStatus !== previousLoadingStatus) { handleLoadingStatusChange(imageLoadingStatus); } }, [imageLoadingStatus, handleLoadingStatusChange]); @@ -117,71 +138,90 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ -function resolveLoadingStatus(image: HTMLImageElement | null, src?: string): ImageLoadingStatus { - if (!image) { - return 'idle'; - } - if (!src) { - return 'error'; - } - if (image.src !== src) { - image.src = src; - } - return image.complete && image.naturalWidth > 0 ? 'loaded' : 'loading'; -} - function useImageLoadingStatus( src: string | undefined, - { referrerPolicy, crossOrigin }: AvatarImageProps, + { + loadingStatus, + setLoadingStatus, + referrerPolicy, + crossOrigin, + }: { + referrerPolicy: React.HTMLAttributeReferrerPolicy | undefined; + crossOrigin: string | undefined; + loadingStatus: ImageLoadingStatus; + setLoadingStatus: React.Dispatch>; + }, ) { - const isHydrated = useIsHydrated(); - const imageRef = React.useRef(null); - const image = (() => { - if (!isHydrated) return null; - if (!imageRef.current) { - imageRef.current = new window.Image(); - } - return imageRef.current; - })(); - - const [loadingStatus, setLoadingStatus] = React.useState(() => - resolveLoadingStatus(image, src), - ); - useLayoutEffect(() => { - setLoadingStatus(resolveLoadingStatus(image, src)); - }, [image, src]); + if (!src) { + setLoadingStatus('error'); + return; + } - useLayoutEffect(() => { - const updateStatus = (status: ImageLoadingStatus) => () => { - setLoadingStatus(status); + const image = new window.Image(); + const handleLoad = (event: Event) => { + const image = event.currentTarget as HTMLImageElement; + setLoadingStatus(getImageLoadingStatus(image)); }; - - if (!image) return; - - const handleLoad = updateStatus('loaded'); - const handleError = updateStatus('error'); + const handleError = () => setLoadingStatus('error'); image.addEventListener('load', handleLoad); image.addEventListener('error', handleError); if (referrerPolicy) { image.referrerPolicy = referrerPolicy; } - if (typeof crossOrigin === 'string') { - image.crossOrigin = crossOrigin; - } + image.crossOrigin = crossOrigin ?? null; + image.src = src; + setLoadingStatus(getImageLoadingStatus(image)); return () => { image.removeEventListener('load', handleLoad); image.removeEventListener('error', handleError); + setLoadingStatus('idle'); }; - }, [image, crossOrigin, referrerPolicy]); + }, [src, crossOrigin, referrerPolicy, setLoadingStatus]); return loadingStatus; } -const Root = Avatar; -const Image = AvatarImage; -const Fallback = AvatarFallback; +function getImageLoadingStatus(image: HTMLImageElement) { + return image.complete ? (image.naturalWidth > 0 ? 'loaded' : 'error') : 'loading'; +} + +// Image count is only used in development to warn about multiple images, which +// is unsupported. Gating behind a development flag to avoid performance +// overhead in production is safe because useState is guaranteed to run +// consistently in the same environment. +// oxlint-disable react-hooks/rules-of-hooks +function useImageCount() { + let state = STATIC_IMAGE_COUNT_STATE; + if (process.env.NODE_ENV === 'development') { + state = React.useState(0); + const [imageCount] = state; + const hasWarnedRef = React.useRef(false); + React.useEffect(() => { + if (imageCount > 1 && !hasWarnedRef.current) { + hasWarnedRef.current = true; + console.warn( + 'Avatar: Only one `Avatar.Image` component should be rendered per `Avatar.Root`, but multiple were detected. This will lead to unexpected behavior.', + ); + } + }, [imageCount]); + } + + return state; +} + +function useUpdateImageCount(setImageCount: React.Dispatch>) { + if (process.env.NODE_ENV === 'development') { + React.useEffect(() => { + setImageCount((imageCount) => imageCount + 1); + return () => { + setImageCount((imageCount) => imageCount - 1); + }; + }, [setImageCount]); + } +} +// oxlint-enable react-hooks/rules-of-hooks export { createAvatarScope, @@ -190,8 +230,8 @@ export { AvatarImage, AvatarFallback, // - Root, - Image, - Fallback, + Avatar as Root, + AvatarImage as Image, + AvatarFallback as Fallback, }; export type { AvatarProps, AvatarImageProps, AvatarFallbackProps };