primitives icon indicating copy to clipboard operation
primitives copied to clipboard

[Avatar] Fix flashing when image is already cached

Open rkkautsar opened this issue 1 year ago • 6 comments

Description

Fixes https://github.com/radix-ui/primitives/issues/2044 by initializing loadingStatus with loaded if the image.complete is true, based on @lauri865 suggestion in the linked issue.

Based on MDN, image.complete will be true if:

  • Neither the src nor the srcset attribute is specified.
  • The srcset attribute is absent and the src attribute, while specified, is the empty string ("").
  • The image resource has been fully fetched and has been queued for rendering/compositing.
  • The image element has previously determined that the image is fully available and ready for use.
  • The image is "broken;" that is, the image failed to load due to an error or because image loading is disabled.

I added test case for all the cases, although I imagine the last case might be different from browser to browser. In Chrome, when image loading is disabled the load event will never fire and image.complete will always be false. To be safe I added another check for image.naturalWidth > 0 since image with no data will have naturalWidth = 0.

I also added test and story for the case when image src changing to make sure my change can handle the changing src.

Tests added: ✓ does not leak event listeners (7 ms) ✓ can handle changing src (627 ms) ✓ should render the image immediately after it is cached (327 ms) ✓ should not render image with no src (5 ms) ✓ should not render image with empty string as src (3 ms) ✓ should show fallback if image has no data (4 ms)

Storybook added:

  • Changing image src
Before After
fix-flashing-avatar-before fix-flashing-avatar-after

rkkautsar avatar Jul 09 '24 07:07 rkkautsar

Awesome, I've been waiting for this I was actually about to hack my own Avatar together myself... Will this be useable directly into Shadcn and NextUI or will we need an update from NextUI directly?

daveycodez avatar Jul 09 '24 20:07 daveycodez

@LeakedDave the same avatar flicker issue in shadcn/ui should be fixed just by bumping the radix version if this fix is released. For nextui I think the same idea can be implemented to this hook: https://github.com/nextui-org/nextui/blob/canary/packages/hooks/use-image/src/index.ts

rkkautsar avatar Jul 10 '24 12:07 rkkautsar

Hey @chaance @rkkautsar, is this PR ready to be merged? 🙏

mackermans avatar Sep 05 '24 18:09 mackermans

Hello, I just want to follow if this PR is ready?

quachthientai avatar Sep 14 '24 19:09 quachthientai

@mackermans @quachthientai need maintainer to review the change 😅

rkkautsar avatar Sep 15 '24 01:09 rkkautsar

Please do not leave comments like "bump" and "+1", they are unhelpful. At the moment this project is just me, and I've got a large backlog of issues and need to prioritize accordingly.

chaance avatar Sep 27 '24 16:09 chaance

@rkkautsar do you see an opportunity to finish this PR with the last PR comment?

jvandenaardweg avatar Oct 23 '24 12:10 jvandenaardweg

@chaance sorry just got the chance to work on this again. I applied your suggestion and tested that everything still works.

rkkautsar avatar Nov 02 '24 16:11 rkkautsar

Let's merge. This is a pre-2025 feature.

daveycodez avatar Dec 15 '24 23:12 daveycodez

Hey folks 👋 any update here? Would be great to have this merged 🙏

ovflowd avatar Dec 23 '24 01:12 ovflowd

Merge pls 🙏

mthmcalixto avatar Dec 29 '24 16:12 mthmcalixto

Pre-2025...

daveycodez avatar Apr 01 '25 01:04 daveycodez

Resolved conflicts and merged in https://github.com/radix-ui/primitives/commit/f46e3d6685ee67aebc9b4c574cb515b42d1f5418

chaance avatar Apr 08 '25 16:04 chaance