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/image blur placeholder with conditional rendering is taken down too early in Firefox #30128

Open
yfhui opened this issue Oct 21, 2021 · 10 comments
Labels
Image (next/image) Related to Next.js Image Optimization.

Comments

@yfhui
Copy link

yfhui commented Oct 21, 2021

What version of Next.js are you using?

11.1.2

What version of Node.js are you using?

16.9.1

What browser are you using?

Firefox

What operating system are you using?

Windows

How are you deploying your application?

Vercel

Describe the Bug

Observation:

  • next/image placeholder disappears when image is loading in Firefox
  • The image placeholder is taken down too early at the image loading phase (even before the image starts loading)
  • In other words, the onload/onLoadingComplete function is fired prematurely and hence the placeholder get unset
  • This is very obvious when the image component is setup with conditional rendering
  • With conditional rendering onload function is executed twice, first time is executed before the image is loaded (it runs when rendering variable is set to true)
  • Without conditional rendering, onload/onLoadingComplete function is still fired early but it run once. However, the time to run onLoad image is still shorter than the time to load the image

conditional rendering example:

import React from 'react'
import Image from 'next/image';
export default function MyImage() {
    console.log('image is rendered');
	let loadTime = Date.now();

    return (
        <Image
            width={1395}
            height={947}
            blurDataURL={
                "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk+M9QDwADhgGAWjR9awAAAABJRU5ErkJggg=="
            }
            placeholder="blur"
            src={"/mountains.jpg"}
            onLoadingComplete={() => {
                console.log("test", Date.now() - loadTime);
            }}
        />
    )
}
import MyImage from "../components/image";
import styles from "../styles/Home.module.css";
import { useState } from "react";

export default function Home() {
	const [mountImage, setMountImage] = useState(false);

	console.log('Home is rendered')
	return (
		<div className={styles.container}>
			<button
				onClick={() => {
					setMountImage(true);
				}}
			>
				click to load image
			</button>
			{mountImage && (
				<MyImage/>
			)}
		</div>
	);
}

log in Firefox, we could see the first time to execute onLoad function is shorter than the image loading time:
image
log in Chrome, we could see the time to execute onLoad function is longer than the image loading time(and it only run once):
image

Expected Behavior

onLoadingComplete/onload should be run once only. And placeholder exists until the image is fully loaded. (Chrome will execute onLoad once only and gives the expected behavior.)

To Reproduce

A demo is deployed to vercel. Click the button to update the conditional rendering variable. The base code is from npx create-next-app@latest. (complete repo)

https://test-nextjs-peach.vercel.app/

@yfhui yfhui added the bug Issue was opened via the bug report template. label Oct 21, 2021
@styfle styfle added kind: bug and removed bug Issue was opened via the bug report template. labels Oct 22, 2021
@styfle
Copy link
Member

styfle commented Oct 22, 2021

This might be a difference with the way Firefox resolved img.decode()

const p = 'decode' in img ? img.decode() : Promise.resolve()
p.catch(() => {}).then(() => {
if (placeholder === 'blur') {
img.style.filter = 'none'
img.style.backgroundSize = 'none'
img.style.backgroundImage = 'none'
}
loadedImageURLs.add(src)
if (onLoadingComplete) {

Would you like to submit a PR to fix it?

@yfhui
Copy link
Author

yfhui commented Oct 25, 2021

Thanks for suggesting the direction. after some investigation I think the reason for the early firing is because of the line if(!img.src.startsWith('data:'))

my discovery:

  • Chrome will execute onLoad 2 times and Firefox will execute onLoad 3 times (I could not explain why)

  • For chrome it only executes the things in the decode promise at the 2nd time because the checking of !img.src.startsWith('data:') of the first execution of the onLoad is false, so the decode promise is never registered neither it is resolved.
    image

  • however in firefox, the checking of !img.src.startsWith('data:') is true in both first and second time of the execution of onLoad function, so the decode promise is registered and resolved at the beginning
    image

  • I tried to remove !img.src.startsWith('data:') checking and Chrome and Firefox has the same behavior which at the very first time the of the onLoad execution, the handleLoad function has everything executed, placeholder is unset early before image is loaded.
    I am not sure what exactly I need to do for fixing this bug, if I know what to do, I would love to submit a PR to fix it!

Edit: Noticed that !img.src.startsWith('data:') is changed to if (img.src !== emptyDataURL) in 11.1.3 but tested with 11.1.3-canary.100, the problem still exists

@styfle styfle self-assigned this Oct 25, 2021
@styfle styfle added this to the 12.0.4 milestone Nov 5, 2021
@timneutkens timneutkens removed this from the 12.0.5 milestone Nov 17, 2021
@timneutkens timneutkens added the Image (next/image) Related to Next.js Image Optimization. label Nov 17, 2021
@kara kara assigned atcastle and unassigned kara Jan 31, 2022
@styfle
Copy link
Member

styfle commented Mar 9, 2022

Theres a similar bug reported in #34697. It seems that Firefox implements img.decode() differently so there is a Firefox bug with some discussion here https://bugzilla.mozilla.org/show_bug.cgi?id=1758035

@yfhui
Copy link
Author

yfhui commented Mar 10, 2022

That's what I suspected too, thx for you update!

@bennettdams
Copy link
Contributor

bennettdams commented Apr 3, 2022

I hope this is related, but there's also a problem of a blank image while the blur placeholder transitions to the real image.

I couldn't see the bug for the blurred placeholder in your repro, @yfhui, so here is another one that uses a remote image (from Imgur.com). You can see the bug in action:

blurred image -> blank -> final image

Animation

Deployed at: https://nextjs-image-bug.vercel.app/

Here is a reproduction repo, it just uses an image from Imgur.com and a static blur URL.

  • Firefox 98.0.2 (64-bit)
  • Next.js 12.1.4
  1. npx create-next-app@latest --ts --use-npm .
  2. Add a remote image
  3. Open the browser and disable cache an throttle the network speed
  4. npm run build && npm run start

This is only an issue in Firefox - Chrome doesn't show this blank space between the placeholder and the final image.

@yfhui
Copy link
Author

yfhui commented Apr 3, 2022

@bennettdams I noticed this issue too! but I am not sure if it's related to the Firefox bug mentioned above

@bennettdams
Copy link
Contributor

@bennettdams I noticed this issue too! but I am not sure if it's related to the Firefox bug mentioned above

It doesn't happen in Chrome, that's why I posted it in this issue - but maybe it's worth to keep this as a separate issue, maybe someone from the team could let me know?

@styfle
Copy link
Member

styfle commented Apr 4, 2022

This should be fixed in PR #35889

kodiakhq bot pushed a commit that referenced this issue Apr 5, 2022
…35889)

## History

In PR #24153, `placeholder=blur` was added and it set `element.style.backgroundImage = 'none'` instead of using react state to re-render. Then in PR  #25916, a delay was added to handle removing the blur placeholder. Then in PR #25994, `img.decode()` was utilized but we found this caused problems in Firefox in #26011.

## Today

This PR changes the the blur placeholder removal to use react state to re-render. This _should_ prevent Firefox from erroring although we should probably keep the catch() just in case. This was pointed out when React 18 caused subtle differences in Firefox in this comment #30128 (comment)
@styfle styfle changed the title next/image placeholder (with conditional rendering?) is taken down too early in Firefox next/image blur placeholder with conditional rendering is taken down too early in Firefox Apr 6, 2022
@styfle
Copy link
Member

styfle commented Apr 6, 2022

One issue #30128 (comment) is fixed can you can try it on canary with npm install next@canary.

The original issue #30128 (comment) still fails and seems to be related to be either decode() resolving immediately mentioned in #30128 (comment) or it could be img.complete is true. It requires a deeper dive.

That being said, the blur placeholder is only acting up on Firefox when its conditionally rendered. Images that are rendered during page load seem to work fine with the blur placeholder.

pavleadev pushed a commit to pavleadev/next.js that referenced this issue Mar 29, 2023
…35889)

## History

In PR #24153, `placeholder=blur` was added and it set `element.style.backgroundImage = 'none'` instead of using react state to re-render. Then in PR  #25916, a delay was added to handle removing the blur placeholder. Then in PR #25994, `img.decode()` was utilized but we found this caused problems in Firefox in #26011.

## Today

This PR changes the the blur placeholder removal to use react state to re-render. This _should_ prevent Firefox from erroring although we should probably keep the catch() just in case. This was pointed out when React 18 caused subtle differences in Firefox in this comment vercel/next.js#30128 (comment)
@Etsi0

This comment was marked as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Image (next/image) Related to Next.js Image Optimization.
Projects
None yet
Development

No branches or pull requests

8 participants