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.

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

[NEXT-943] Broken shimmer effect in next/image #47639

Closed
1 task done
ArianHamdi opened this issue Mar 29, 2023 · 10 comments · Fixed by #53442
Closed
1 task done

[NEXT-943] Broken shimmer effect in next/image #47639

ArianHamdi opened this issue Mar 29, 2023 · 10 comments · Fixed by #53442
Assignees
Labels
Image (next/image) Related to Next.js Image Optimization. locked

Comments

@ArianHamdi
Copy link
Contributor

ArianHamdi commented Mar 29, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: win32
      Arch: x64
      Version: Windows 10 Pro
    Binaries:
      Node: 16.19.0
      npm: N/A
      Yarn: N/A
      pnpm: 7.29.0
    Relevant packages:
      next: 13.2.4
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

Image optimization (next/image, next/legacy/image)

Link to the code that reproduces this issue

https://github.com/vercel/next.js/blob/canary/examples/image-component/pages/shimmer.tsx

To Reproduce

Open the following link: https://image-component.nextjs.gallery/shimmer. The shimmer effect is currently broken.

Describe the Bug

In the next/image component, the shimmer effect appears blurry compared to the next/legacy/image component where it works fine https://image-legacy-component.nextjs.gallery/shimmer. This is because next.js 13 always creates a blurry version of the blurDataUrl, which can be redundant and result in a broken shimmer effect.

Expected Behavior

It would be helpful to have an option to use blurDataUrl without modification for cases where the shimmer effect is needed. Additionally, the placeholder prop can have more options beyond just empty and blur. Perhaps, we can rename blurDataUrl to placeholderDataUrl

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-943

@ArianHamdi ArianHamdi added the bug Issue was opened via the bug report template. label Mar 29, 2023
@github-actions github-actions bot added the Image (next/image) Related to Next.js Image Optimization. label Mar 29, 2023
@ishaqibrahimbot
Copy link
Contributor

Hey @ArianHamdi, I agree there should be an option to use the placeholder url as-is, instead of adding a blur to it. I've had scenarios where I just want to show a stock background image without a blur but it's not possible without custom css.

This is a simple enough feature to add and one I'm quite willing to do so, so I'm just wondering about how the prop interface can change to accommodate this.

  • We can't rename blurDataUrl to placeholderDataUrl cause that is a breaking change, this has to be something that can be adopted without breaking anything
  • An option is to add a 3rd option to the placeholder prop - this can be named default (or something else). This will ensure the blur filter is not applied to the placeholder image passed into blurDataUrl. However the issue with this is that now the blurDataUrl prop is not accurately named since it can take an image that will show up without the blur. This can be a compromise until we can introduce breaking changes in a major release in the future.
  • Yet another option is to just add the 3rd option as mentioned above to the placeholder prop and add another prop for the no-blur placeholder url. But I feel that this is excessive.

@styfle would be great to hear your thoughts about this.

@ArianHamdi
Copy link
Contributor Author

Hey @ishaqibrahimbot, thanks for your attention.

An option is to add a 3rd option to the placeholder prop - this can be named default (or something else). This will ensure the blur filter is not applied to the placeholder image passed into blurDataUrl. However the issue with this is that now the blurDataUrl prop is not accurately named since it can take an image that will show up without the blur. This can be a compromise until we can introduce breaking changes in a major release in the future.

I believe it would be better if the functionality of the prop and its name were consistent and aligned.

Yet another option is to just add the 3rd option as mentioned above to the placeholder prop and add another prop for the no-blur placeholder url. But I feel that this is excessive.

This is a good option that does not cause any breaking changes and avoids any confusion with the prop name. However, there is a concern if we decide to add more options to the placeholder prop in the future, this would require adding another prop for that option, such as smthDataUrl , which I do not think is ideal.

In my opinion, the cleanest approach would be to add a placeholderDataUrl prop and modify the functionality based on the selected option in the placeholder prop.

What do you think about keeping blurDataUrl and adding placeholderDataUrl as a new prop? We could include a warning about blurDataUrl in the next major version to encourage users to transition to the new prop.

@styfle styfle added kind: bug and removed bug Issue was opened via the bug report template. labels Apr 4, 2023
@styfle styfle changed the title Broken shimmer effect in next/image [NEXT-943] Broken shimmer effect in next/image Apr 4, 2023
@styfle styfle self-assigned this Apr 4, 2023
@styfle
Copy link
Member

styfle commented Apr 5, 2023

The working deployment is using Next.js 12 which is basically next/legacy/image in 13. So my assumption here is that next/image has been failing to animate the blurDataUrl since Next.js 13 when the blur effect was changed from CSS to SVG.

Perhaps there is a way to continue using the SVG blur effect but still allow animation.

It would be helpful to have an option to use blurDataUrl without modification

This could work although it wouldn't fix existing apps that upgrade from 12 to 13 and it would make the surface area of next/image a bit bigger.

Maybe we could add support for something like <Image placeholder="data:..." width={100} height={100} />.

@ishaqibrahimbot
Copy link
Contributor

@styfle yep I'm trying to debug the issue around the svg animation not kicking in. Will report here if I have a breakthrough and have figured out how to fix the svg animation.

Also you're suggesting that we add the ability to directly pass placeholder image data in the placeholder prop? How does that work with the current empty and blur options?

@Thinkscape
Copy link

Thinkscape commented Jul 7, 2023

👍 Supporting a placeholder: React.ReactNode similar to <Suspense fallback={<p>Loading...</p>} /> would be ideal.

Currently I had to fall back to a kludge with state, onLoadingComplete and putting the placeholder position:absolute on top of the image.

Example use case:

<Image
  width={200}
  height={200}
  placeholder={<Shimmer variant="image" />}
  alt={"Keep it simple"}
  src={src}
/>

@arturbien
Copy link
Contributor

First of all I agree that we should rename the props at some point from:
<Image blurDataURL={...} placeholder={"blur"} />
To:
<Image placeholderDataURL={...} placeholderType={"blur"} />
Or even:
<Image placeholderURL={...} placeholderType={"blur"} />

The last option makes more sense when we consider that a URL pointing to regular image can also be used as a placeholder (like placeholderURL="/test.png")

Intermediate solution

Perhaps there is a way to continue using the SVG blur effect but still allow animation.

@styfle I've tried to pass an animated SVG to the SVG we're generating but the animation won't work - it looks like passing animated SVG as data URI to the underlying <image /> element breaks the animation.

I've looked into how it can be fixed and found a simple solution that won't introduce any breaking changes, but another placeholder value instead - placeholder = "blur" | "empty" | "normal".

When normal placeholder is used, the blurDataURL is directly passed to <img /> background-image, so there's no effect (blur) applied on top of it. If devs decide to make a complex placeholder like a combination of LQIP and shimmer they'll still be able to do it - by passing the complex animated SVG they've created as a normal placeholder.

So for for the apps with a broken shimmer effect all that would have to be done to fix the issue is to replace placeholder="blur" with placeholder="normal"

If this approach seems sensible to you I already have a branch with the changes needed to make that work

@styfle
Copy link
Member

styfle commented Jul 28, 2023

So for for the apps with a broken shimmer effect all that would have to be done to fix the issue is to replace placeholder="blur" with placeholder="normal"

That sounds like you're suggesting <Image blurDataURL="data..." placeholder="normal" />.

The existing blurDataURL feels out of place in this case because its not blur. I'm also not a fan of "normal" since it doesn't tell you what will happen.

That's why my suggestion above was <Image placeholder="data:..." width={100} height={100} /> since I think we can do it without introducing a new prop or relying on blurDataURL. I think this will be easier to comprehend what will happen.

The implementation (pseudocode) might look like:

const background = props.placeholder.startsWith('data:') ? props.placeholder : getBlurSvg(props.blurDataURL)

@arturbien
Copy link
Contributor

If we want to go this route then we should probably do something like this:

let background = null
if (placeholder !== "empty") {
   background = props.placeholder === "blur" ? getBlurSvg(props.blurDataURL) : placeholder 
}

This will allow us to do:
<Image placeholder="/placeholder-image.svg" width={100} height={100} />

I'll submit a PR early next week

@styfle
Copy link
Member

styfle commented Jul 28, 2023

<Image placeholder="/placeholder-image.svg" width={100} height={100} />

We should not allow that usage. It must be a data uri because we don't want to incur another http request. The purpose of a placeholder is to ensure there is something there before the actual image loads.

It should look like

<Image placeholder="data:image/..." width={100} height={100} />

@kodiakhq kodiakhq bot closed this as completed in #53442 Aug 11, 2023
kodiakhq bot pushed a commit that referenced this issue Aug 11, 2023
Adds support for base64-encoded `placeholder`. Enables using placeholders without the "blur" effect.

Fixes #47639
- [x] Add support for DataURL placeholder
- [x] Add tests
- [x] Update docs

Co-authored-by: Steven <229881+styfle@users.noreply.github.com>
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Image (next/image) Related to Next.js Image Optimization. locked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants