Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Avatar] Fix flashing when image is already cached #3008

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[Avatar] Fix flashing when image is already cached #3008

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81158c0
[Avatar] fix flashing when image is already cached
rkkautsar Jul 8, 2024
308d236
chore: rename to resolveLoadingStatus
rkkautsar Aug 19, 2024
375638d
fix(avatar): ssr support
rkkautsar Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .yarn/versions/4905f29b.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
releases:
"@radix-ui/react-avatar": patch

declined:
- primitives
44 changes: 44 additions & 0 deletions packages/react/avatar/src/Avatar.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 = () => (
Expand Down Expand Up @@ -33,6 +35,18 @@ export const Styled = () => (
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>Changing image src</h1>
<SourceChanger sources={[src, srcAlternative, srcBroken]}>
{(src) => (
<Avatar.Root className={rootClass()}>
<Avatar.Image className={imageClass()} alt="John Smith" src={src} />
<Avatar.Fallback delayMs={300} className={fallbackClass()}>
JS
</Avatar.Fallback>
</Avatar.Root>
)}
</SourceChanger>
</>
);

Expand All @@ -58,6 +72,18 @@ export const Chromatic = () => (
<AvatarIcon />
</Avatar.Fallback>
</Avatar.Root>

<h1>Changing image src</h1>
<SourceChanger sources={[src, srcAlternative, srcBroken]}>
{(src) => (
<Avatar.Root className={rootClass()}>
<Avatar.Image className={imageClass()} alt="John Smith" src={src} />
<Avatar.Fallback delayMs={300} className={fallbackClass()}>
JS
</Avatar.Fallback>
</Avatar.Root>
)}
</SourceChanger>
</>
);
Chromatic.parameters = { chromatic: { disable: false, delay: 1000 } };
Expand Down Expand Up @@ -113,3 +139,21 @@ const AvatarIcon = () => (
/>
</svg>
);

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);
}
167 changes: 127 additions & 40 deletions packages/react/avatar/src/Avatar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();

describe('given an Avatar with fallback and no image', () => {
let rendered: RenderResult;
Expand All @@ -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) => (
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
<Avatar.Image src={src} alt={IMAGE_ALT_TEXT} />
</Avatar.Root>
);

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(
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
<Avatar.Image src="/test.jpg" alt={IMAGE_ALT_TEXT} />
</Avatar.Root>
);
cache.clear();
rendered = render(ui('/test.png'));
});

it('should render the fallback initially', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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) => (
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
<Avatar.Image src={src} alt={IMAGE_ALT_TEXT} referrerPolicy={referrerPolicy} />
</Avatar.Root>
);

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(
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
<Avatar.Image src="/test.jpg" alt={IMAGE_ALT_TEXT} referrerPolicy="no-referrer" />
</Avatar.Root>
);
cache.clear();
rendered = render(ui('/test.png', 'no-referrer'));
});

it('should render the fallback initially', () => {
Expand All @@ -161,12 +215,8 @@ describe('given an Avatar with an image that only works when referrerPolicy=no-r

describe('referrerPolicy=origin', () => {
beforeEach(() => {
rendered = render(
<Avatar.Root data-testid={ROOT_TEST_ID}>
<Avatar.Fallback>{FALLBACK_TEXT}</Avatar.Fallback>
<Avatar.Image src="/test.jpg" alt={IMAGE_ALT_TEXT} referrerPolicy="origin" />
</Avatar.Root>
);
cache.clear();
rendered = render(ui('/test.png', 'origin'));
});

it('should render the fallback initially', () => {
Expand All @@ -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);
}
}
43 changes: 27 additions & 16 deletions packages/react/avatar/src/Avatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,35 +116,46 @@ AvatarFallback.displayName = FALLBACK_NAME;

/* -----------------------------------------------------------------------------------------------*/

function resolveLoadingStatus(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<ImageLoadingStatus>('idle');
const image = React.useRef(new window.Image());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one more thing I noticed. This will error in SSR. We want to ensure that hydration is complete before initializing the image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my suggested fix:

  • Check if the app is hydrated on the client (this can either be a new function in the utils package or included in this file for now)
  • If the app is not hydrated (SSR + first client render) initialize the image ref as null
  • Initialize the state as "loading" pre-hydration to prevent client/server mismatch
function resolveLoadingStatus(image: HTMLImageElement | null, src?: string): ImageLoadingStatus {
  if (!src) {
    return 'error';
  }

  if (!image) {
    // image is not initialized during SSR. Ensures the initial state is
    // 'loading' on both the server and the client to prevent hydration errors.
    return 'loading';
  }

  if (image.src !== src) {
    image.src = src;
  }
  return image.complete && image.naturalWidth > 0 ? 'loaded' : 'loading';
}

function useImageLoadingStatus(src?: string, referrerPolicy?: React.HTMLAttributeReferrerPolicy) {
  const isHydrated = useIsHydrated();
  const image = React.useRef<HTMLImageElement | null>(isHydrated ? new window.Image() : null);
  const [loadingStatus, setLoadingStatus] = React.useState<ImageLoadingStatus>(() =>
    resolveLoadingStatus(image.current, src)
  );

  useLayoutEffect(() => {
    setLoadingStatus(resolveLoadingStatus(image.current, src));
  }, [src]);

  useLayoutEffect(() => {
    // We can safely assume that image.current is not null because hydration is
    // complete by the time this effect runs
    const img = image.current!;

    const updateStatus = (status: ImageLoadingStatus) => () => {
      setLoadingStatus(status);
    };

    const handleLoad = updateStatus('loaded');
    const handleError = updateStatus('error');
    img.addEventListener('load', handleLoad);
    img.addEventListener('error', handleError);
    if (referrerPolicy) {
      img.referrerPolicy = referrerPolicy;
    }

    return () => {
      img.removeEventListener('load', handleLoad);
      img.removeEventListener('error', handleError);
    };
  }, [referrerPolicy]);

  return loadingStatus;
}

function subscribe() {
  return () => {};
}

function useIsHydrated() {
  return React.useSyncExternalStore(
    subscribe,
    () => true,
    () => false
  );
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! also TIL useSyncExternalStore is the recommended way to detect hydration. I'll make the change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chaance done, I added a test for SSR and make sure it works in the ssr-testing app too. Need to make some change from your suggestion but overall it works. One thing is that the SSR testing will log this error:

 Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://reactjs.org/link/uselayouteffect-ssr for common fixes.

since we use useLayoutEffect for Avatar. It's just a warning from jest though, and the Avatar component server rendered to its fallback component just fine as tested with unit test and manually checked in the ssr-testing app

const [loadingStatus, setLoadingStatus] = React.useState<ImageLoadingStatus>(() =>
resolveLoadingStatus(image.current, src)
);

useLayoutEffect(() => {
if (!src) {
setLoadingStatus('error');
return;
}

let isMounted = true;
const image = new window.Image();
setLoadingStatus(resolveLoadingStatus(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;
}
Expand Down