From 81158c00e71665057da6842ce166e1b67683c98e Mon Sep 17 00:00:00 2001 From: Rakha Kanz Kautsar Date: Mon, 8 Jul 2024 18:06:41 +0800 Subject: [PATCH 1/3] [Avatar] fix flashing when image is already cached --- .yarn/versions/4905f29b.yml | 5 + packages/react/avatar/src/Avatar.stories.tsx | 44 +++++ packages/react/avatar/src/Avatar.test.tsx | 167 ++++++++++++++----- packages/react/avatar/src/Avatar.tsx | 43 +++-- 4 files changed, 203 insertions(+), 56 deletions(-) create mode 100644 .yarn/versions/4905f29b.yml diff --git a/.yarn/versions/4905f29b.yml b/.yarn/versions/4905f29b.yml new file mode 100644 index 000000000..50d060d51 --- /dev/null +++ b/.yarn/versions/4905f29b.yml @@ -0,0 +1,5 @@ +releases: + "@radix-ui/react-avatar": patch + +declined: + - primitives diff --git a/packages/react/avatar/src/Avatar.stories.tsx b/packages/react/avatar/src/Avatar.stories.tsx index 0ee66a2bf..28a1f70dd 100644 --- a/packages/react/avatar/src/Avatar.stories.tsx +++ b/packages/react/avatar/src/Avatar.stories.tsx @@ -1,9 +1,11 @@ import { css } from '../../../../stitches.config'; import * as Avatar from '@radix-ui/react-avatar'; +import React from 'react'; export default { title: 'Components/Avatar' }; const src = 'https://picsum.photos/id/1005/400/400'; +const srcAlternative = 'https://picsum.photos/id/1006/400/400'; const srcBroken = 'https://broken.link.com/broken-pic.jpg'; export const Styled = () => ( @@ -33,6 +35,18 @@ export const Styled = () => ( + +

Changing image src

+ + {(src) => ( + + + + JS + + + )} + ); @@ -58,6 +72,18 @@ export const Chromatic = () => ( + +

Changing image src

+ + {(src) => ( + + + + JS + + + )} + ); Chromatic.parameters = { chromatic: { disable: false, delay: 1000 } }; @@ -113,3 +139,21 @@ const AvatarIcon = () => ( /> ); + +function SourceChanger({ + sources, + children, +}: { + sources: string[]; + children: (src: string) => React.ReactElement; +}) { + const [src, setSrc] = React.useState(sources[0]); + React.useEffect(() => { + const interval = setInterval(() => { + const nextIndex = (sources.indexOf(src) + 1) % sources.length; + setSrc(sources[nextIndex]); + }, 1000); + return () => clearInterval(interval); + }, [sources, src]); + return children(src); +} diff --git a/packages/react/avatar/src/Avatar.test.tsx b/packages/react/avatar/src/Avatar.test.tsx index 4e71f84ed..bd1d353a2 100644 --- a/packages/react/avatar/src/Avatar.test.tsx +++ b/packages/react/avatar/src/Avatar.test.tsx @@ -2,11 +2,13 @@ import { axe } from 'jest-axe'; import type { RenderResult } from '@testing-library/react'; import { render, waitFor } from '@testing-library/react'; import * as Avatar from '@radix-ui/react-avatar'; +import { HTMLAttributeReferrerPolicy } from 'react'; const ROOT_TEST_ID = 'avatar-root'; const FALLBACK_TEXT = 'AB'; const IMAGE_ALT_TEXT = 'Fake Avatar'; const DELAY = 300; +const cache = new Set(); describe('given an Avatar with fallback and no image', () => { let rendered: RenderResult; @@ -27,32 +29,26 @@ describe('given an Avatar with fallback and no image', () => { describe('given an Avatar with fallback and a working image', () => { let rendered: RenderResult; let image: HTMLElement | null = null; - const orignalGlobalImage = window.Image; + const originalGlobalImage = window.Image; + const ui = (src?: string) => ( + + {FALLBACK_TEXT} + + + ); beforeAll(() => { - (window.Image as any) = class MockImage { - onload: () => void = () => {}; - src: string = ''; - constructor() { - setTimeout(() => { - this.onload(); - }, DELAY); - return this; - } - }; + (window.Image as any) = MockImage; }); afterAll(() => { - window.Image = orignalGlobalImage; + window.Image = originalGlobalImage; + jest.restoreAllMocks(); }); beforeEach(() => { - rendered = render( - - {FALLBACK_TEXT} - - - ); + cache.clear(); + rendered = render(ui('/test.png')); }); it('should render the fallback initially', () => { @@ -74,6 +70,65 @@ describe('given an Avatar with fallback and a working image', () => { image = await rendered.findByAltText(IMAGE_ALT_TEXT); expect(image).toBeInTheDocument(); }); + + it('does not leak event listeners', async () => { + rendered.unmount(); + const addEventListenerSpy = jest.spyOn(window.Image.prototype, 'addEventListener'); + const removeEventListenerSpy = jest.spyOn(window.Image.prototype, 'removeEventListener'); + rendered = render(ui('/test.png')); + rendered.unmount(); + expect(addEventListenerSpy.mock.calls.length).toEqual(removeEventListenerSpy.mock.calls.length); + }); + + it('can handle changing src', async () => { + image = await rendered.findByRole('img'); + expect(image).toBeInTheDocument(); + rendered.rerender(ui('/test2.png')); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + image = await rendered.findByRole('img'); + expect(image).toBeInTheDocument(); + }); + + it('should render the image immediately after it is cached', async () => { + image = await rendered.findByRole('img'); + expect(image).toBeInTheDocument(); + + rendered.unmount(); + rendered = render(ui('/test.png')); + image = rendered.queryByRole('img'); + expect(image).toBeInTheDocument(); + }); + + it('should not render image with no src', async () => { + rendered.rerender(ui()); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + rendered.unmount(); + rendered = render(ui()); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + }); + + it('should not render image with empty string as src', async () => { + rendered.rerender(ui('')); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + rendered.unmount(); + rendered = render(ui('')); + image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + }); + + it('should show fallback if image has no data', async () => { + rendered.unmount(); + const spy = jest.spyOn(window.Image.prototype, 'naturalWidth', 'get'); + spy.mockReturnValue(0); + rendered = render(ui('/test.png')); + const fallback = rendered.queryByText(FALLBACK_TEXT); + expect(fallback).toBeInTheDocument(); + spy.mockRestore(); + }); }); describe('given an Avatar with fallback and delayed render', () => { @@ -103,39 +158,38 @@ describe('given an Avatar with fallback and delayed render', () => { describe('given an Avatar with an image that only works when referrerPolicy=no-referrer', () => { let rendered: RenderResult; - const orignalGlobalImage = window.Image; + const originalGlobalImage = window.Image; + const ui = (src?: string, referrerPolicy?: HTMLAttributeReferrerPolicy) => ( + + {FALLBACK_TEXT} + + + ); beforeAll(() => { - (window.Image as any) = class MockImage { - onload: () => void = () => {}; - onerror: () => void = () => {}; - src: string = ''; + (window.Image as any) = class MockNoReferrerImage extends MockImage { referrerPolicy: string | undefined; - constructor() { + onSrcChange() { setTimeout(() => { if (this.referrerPolicy === 'no-referrer') { - this.onload(); + this.dispatchEvent(new Event('load')); } else { - this.onerror(); + this.dispatchEvent(new Event('error')); } }, DELAY); - return this; } }; }); afterAll(() => { - window.Image = orignalGlobalImage; + window.Image = originalGlobalImage; + jest.restoreAllMocks(); }); describe('referrerPolicy=no-referrer', () => { beforeEach(() => { - rendered = render( - - {FALLBACK_TEXT} - - - ); + cache.clear(); + rendered = render(ui('/test.png', 'no-referrer')); }); it('should render the fallback initially', () => { @@ -161,12 +215,8 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r describe('referrerPolicy=origin', () => { beforeEach(() => { - rendered = render( - - {FALLBACK_TEXT} - - - ); + cache.clear(); + rendered = render(ui('/test.png', 'origin')); }); it('should render the fallback initially', () => { @@ -187,3 +237,40 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r }); }); }); + + +class MockImage extends EventTarget { + _src: string = ''; + + constructor() { + super(); + return this; + } + + get src() { + return this._src; + } + + set src(src: string) { + if (!src) { + return; + } + this._src = src; + this.onSrcChange(); + } + + get complete() { + return !this.src || cache.has(this.src); + } + + get naturalWidth() { + return this.complete ? 300 : 0; + } + + onSrcChange() { + setTimeout(() => { + this.dispatchEvent(new Event('load')); + cache.add(this.src); + }, DELAY); + } +} diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index 53841b7f0..f0abcf13b 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -116,35 +116,46 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ +function setImageSrcAndGetInitialState(image: HTMLImageElement, src?: string): ImageLoadingStatus { + if (!src) { + return 'error'; + } + if (image.src !== src) { + image.src = src; + } + return image.complete && image.naturalWidth > 0 ? 'loaded' : 'loading'; +} + function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) { - const [loadingStatus, setLoadingStatus] = React.useState('idle'); + const image = React.useRef(new window.Image()); + const [loadingStatus, setLoadingStatus] = React.useState(() => + setImageSrcAndGetInitialState(image.current, src) + ); useLayoutEffect(() => { - if (!src) { - setLoadingStatus('error'); - return; - } - - let isMounted = true; - const image = new window.Image(); + setLoadingStatus(setImageSrcAndGetInitialState(image.current, src)); + }, [src]); + useLayoutEffect(() => { const updateStatus = (status: ImageLoadingStatus) => () => { - if (!isMounted) return; setLoadingStatus(status); }; - setLoadingStatus('loading'); - image.onload = updateStatus('loaded'); - image.onerror = updateStatus('error'); - image.src = src; + const img = image.current; + + const handleLoad = updateStatus('loaded'); + const handleError = updateStatus('error'); + img.addEventListener('load', handleLoad); + img.addEventListener('error', handleError); if (referrerPolicy) { - image.referrerPolicy = referrerPolicy; + img.referrerPolicy = referrerPolicy; } return () => { - isMounted = false; + img.removeEventListener('load', handleLoad); + img.removeEventListener('error', handleError); }; - }, [src, referrerPolicy]); + }, [referrerPolicy]); return loadingStatus; } From 308d236b34f52dd5ea34207f3984ea332b15ebb1 Mon Sep 17 00:00:00 2001 From: Rakha Kanz Kautsar Date: Mon, 19 Aug 2024 12:14:09 +0800 Subject: [PATCH 2/3] chore: rename to resolveLoadingStatus --- packages/react/avatar/src/Avatar.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index f0abcf13b..75459a900 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -116,7 +116,7 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ -function setImageSrcAndGetInitialState(image: HTMLImageElement, src?: string): ImageLoadingStatus { +function resolveLoadingStatus(image: HTMLImageElement, src?: string): ImageLoadingStatus { if (!src) { return 'error'; } @@ -129,11 +129,11 @@ function setImageSrcAndGetInitialState(image: HTMLImageElement, src?: string): I function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) { const image = React.useRef(new window.Image()); const [loadingStatus, setLoadingStatus] = React.useState(() => - setImageSrcAndGetInitialState(image.current, src) + resolveLoadingStatus(image.current, src) ); useLayoutEffect(() => { - setLoadingStatus(setImageSrcAndGetInitialState(image.current, src)); + setLoadingStatus(resolveLoadingStatus(image.current, src)); }, [src]); useLayoutEffect(() => { From 375638db3168b9aef6c4e80caa67d674318abe7d Mon Sep 17 00:00:00 2001 From: Rakha Kanz Kautsar Date: Fri, 23 Aug 2024 16:26:31 +0800 Subject: [PATCH 3/3] fix(avatar): ssr support --- packages/react/avatar/src/Avatar.test.tsx | 57 ++++++++++++++++++----- packages/react/avatar/src/Avatar.tsx | 39 +++++++++++++--- ssr-testing/app/avatar/page.tsx | 1 + 3 files changed, 78 insertions(+), 19 deletions(-) diff --git a/packages/react/avatar/src/Avatar.test.tsx b/packages/react/avatar/src/Avatar.test.tsx index bd1d353a2..640c638a5 100644 --- a/packages/react/avatar/src/Avatar.test.tsx +++ b/packages/react/avatar/src/Avatar.test.tsx @@ -2,7 +2,8 @@ import { axe } from 'jest-axe'; import type { RenderResult } from '@testing-library/react'; import { render, waitFor } from '@testing-library/react'; import * as Avatar from '@radix-ui/react-avatar'; -import { HTMLAttributeReferrerPolicy } from 'react'; +import { HTMLAttributeReferrerPolicy, ReactElement } from 'react'; +import { renderToString } from 'react-dom/server'; const ROOT_TEST_ID = 'avatar-root'; const FALLBACK_TEXT = 'AB'; @@ -11,22 +12,28 @@ const DELAY = 300; const cache = new Set(); describe('given an Avatar with fallback and no image', () => { - let rendered: RenderResult; - - beforeEach(() => { - rendered = render( - - {FALLBACK_TEXT} - - ); - }); + const ui = ( + + {FALLBACK_TEXT} + + ); it('should have no accessibility violations', async () => { + const rendered = render(ui); expect(await axe(rendered.container)).toHaveNoViolations(); }); + + it('should work with SSR', () => { + const container = document.createElement('div'); + document.body.appendChild(container); + container.innerHTML = renderToString(ui); + const rendered = render(ui, { hydrate: true, container }); + const fallback = rendered.queryByText(FALLBACK_TEXT); + expect(fallback).toBeInTheDocument(); + }); }); -describe('given an Avatar with fallback and a working image', () => { +describe('given an Avatar with fallback and an image', () => { let rendered: RenderResult; let image: HTMLElement | null = null; const originalGlobalImage = window.Image; @@ -129,6 +136,32 @@ describe('given an Avatar with fallback and a working image', () => { expect(fallback).toBeInTheDocument(); spy.mockRestore(); }); + + describe('SSR', () => { + function renderAndHydrate(ui: ReactElement) { + const container = document.createElement('div'); + document.body.appendChild(container); + container.innerHTML = renderToString(ui); + return render(ui, { hydrate: true, container }); + } + + it('can render with working image', async () => { + const rendered = renderAndHydrate(ui('/test.png')); + let image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + + image = await rendered.findByRole('img'); + expect(image).toBeInTheDocument(); + }); + + it('can render with no src', () => { + const rendered = renderAndHydrate(ui()); + const image = rendered.queryByRole('img'); + expect(image).not.toBeInTheDocument(); + const fallback = rendered.queryByText(FALLBACK_TEXT); + expect(fallback).toBeInTheDocument(); + }); + }); }); describe('given an Avatar with fallback and delayed render', () => { @@ -169,6 +202,7 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r beforeAll(() => { (window.Image as any) = class MockNoReferrerImage extends MockImage { referrerPolicy: string | undefined; + onSrcChange() { setTimeout(() => { if (this.referrerPolicy === 'no-referrer') { @@ -238,7 +272,6 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r }); }); - class MockImage extends EventTarget { _src: string = ''; diff --git a/packages/react/avatar/src/Avatar.tsx b/packages/react/avatar/src/Avatar.tsx index 75459a900..056af2a79 100644 --- a/packages/react/avatar/src/Avatar.tsx +++ b/packages/react/avatar/src/Avatar.tsx @@ -116,7 +116,10 @@ AvatarFallback.displayName = FALLBACK_NAME; /* -----------------------------------------------------------------------------------------------*/ -function resolveLoadingStatus(image: HTMLImageElement, src?: string): ImageLoadingStatus { +function resolveLoadingStatus(image: HTMLImageElement | null, src?: string): ImageLoadingStatus { + if (!image) { + return 'idle'; + } if (!src) { return 'error'; } @@ -127,21 +130,30 @@ function resolveLoadingStatus(image: HTMLImageElement, src?: string): ImageLoadi } function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) { - const image = React.useRef(new window.Image()); + const isHydrated = useIsHydrated() + const image = React.useRef(isHydrated ? new window.Image() : null); + const img = (() => { + if (!isHydrated) return null; + if (!image.current) { + image.current = new window.Image(); + } + return image.current; + })(); + const [loadingStatus, setLoadingStatus] = React.useState(() => - resolveLoadingStatus(image.current, src) + resolveLoadingStatus(img, src) ); useLayoutEffect(() => { - setLoadingStatus(resolveLoadingStatus(image.current, src)); - }, [src]); + setLoadingStatus(resolveLoadingStatus(img, src)); + }, [img, src]); useLayoutEffect(() => { const updateStatus = (status: ImageLoadingStatus) => () => { setLoadingStatus(status); }; - const img = image.current; + if (!img) return; const handleLoad = updateStatus('loaded'); const handleError = updateStatus('error'); @@ -155,10 +167,23 @@ function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttribut img.removeEventListener('load', handleLoad); img.removeEventListener('error', handleError); }; - }, [referrerPolicy]); + }, [img, referrerPolicy]); return loadingStatus; } + +function subscribe() { + return () => {}; +} + +function useIsHydrated() { + return React.useSyncExternalStore( + subscribe, + () => true, + () => false + ); +} + const Root = Avatar; const Image = AvatarImage; const Fallback = AvatarFallback; diff --git a/ssr-testing/app/avatar/page.tsx b/ssr-testing/app/avatar/page.tsx index 599d6e200..b5cff903f 100644 --- a/ssr-testing/app/avatar/page.tsx +++ b/ssr-testing/app/avatar/page.tsx @@ -5,6 +5,7 @@ export default function Page() { return ( A + ); }