fix(image): clear blur placeholder after image load#1267
Conversation
Blur placeholders currently stay on the img background after the high resolution image finishes loading. That is wrong for transparent image formats because the low quality placeholder remains visible through the rendered pixels. The shim treated placeholder styling as static props instead of load lifecycle state. Track blur completion per src, clear local and remote placeholder backgrounds after successful load or error, and cover the transparent-image regression in an app-router browser test.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Clean fix that correctly matches Next.js blur-completion semantics. The state design (completedBlurSrc tracking the src that completed, deriving blurComplete from comparison) handles src changes correctly — if src changes, blurComplete becomes false again and the new image shows its blur placeholder until it loads. The setCompletedBlurSrc updater function avoids unnecessary re-renders when already completed for the same src. The test is well-constructed.
Two observations (neither blocking):
-
Layout effect deps:
markBlurCompleteandsrcare called/read inside theuseNonWarningLayoutEffect(line 354) but aren't in its dependency array ([placeholder, sizes, _unoptimized]). This is safe in practice because the effect only runs once (didInsertRef.currentgates it), and the existing deps were already incomplete before this PR. But if a linter ever enforces exhaustive-deps here, it'll flag it. -
Pre-existing:
loaderpath missing blur styles: Theloadercode path (line 431) doesn't render blur placeholder styles at all — it spreadsstyledirectly, noblurStyle. ThemarkBlurComplete()calls inhandleLoad/handleErrorare harmless there but have no visible effect since there's no blur background to clear. Not introduced by this PR — just flagging as a potential follow-up parity gap.
|
|
||
| const markBlurComplete = () => { | ||
| if (placeholder !== "blur") return; | ||
| setCompletedBlurSrc((current) => (current === src ? current : src)); |
There was a problem hiding this comment.
Nit: markBlurComplete captures placeholder and src from the render closure. If placeholder switches from "blur" to "empty" on the same src, the early return in markBlurComplete will prevent setCompletedBlurSrc from being called, but blurComplete remains true from the previous render (since completedBlurSrc === src still holds). This is fine because the blur background conditions also check placeholder === "blur", so it's a no-op either way. Just noting the invariant holds.
| const sanitizedBlur = imgBlurDataURL ? sanitizeBlurDataURL(imgBlurDataURL) : undefined; | ||
| const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined; | ||
| const bg = | ||
| !blurComplete && placeholder === "blur" && sanitizedBlur |
There was a problem hiding this comment.
Good — this correctly gates the blur background for the remote/UnpicImage path. The blurComplete check is consistent with the local image path at line 563.
|
Review submitted. Here's a summary: Approved — this is a clean, correct fix for the blur placeholder lifecycle. What the PR does well:
Two non-blocking observations noted in the review:
|
…ceholder-load # Conflicts: # packages/vinext/src/shims/image.tsx
Overview
placeholder="blur"background styling once the image has completed loading or errored.packages/vinext/src/shims/image.tsx,tests/e2e/app-router/nextjs-compat/image.spec.tsnext/image; no public API or config change.Why
A blur placeholder is loading UI, not part of the final rendered image. Next.js tracks blur completion separately from the image props and stops emitting the placeholder background after load. Vinext kept the background style static, which makes transparent final images permanently reveal the placeholder.
srcand suppresses blur backgrounds after load or error.background-image: noneafter load.next/imagevisible behavior should follow Next.js unless vinext intentionally diverges.What changed
placeholder="blur"local image loadsbackgroundImageremained on the<img>.Maintainer review path
packages/vinext/src/shims/image.tsxcompletedBlurSrcstate and the load/error paths that callmarkBlurComplete().<img>rendering and remote@unpic/reactrendering suppress the blur background once complete.tests/fixtures/app-basic/app/nextjs-compat/image-blur-placeholder/page.tsxnext/image, a transparent public image, and a blur data URL.tests/e2e/app-router/nextjs-compat/image.spec.tsValidation
Added a failing browser regression before the implementation, then verified it passes after the fix.
Commands run:
All commands passed. The build still prints existing virtual import externalization warnings, but exits successfully.
Risk / compatibility
next/imageblur placeholder lifecycle.Non-goals
References
!blurComplete.placeholderdocsblurDataURLdocs