fix(image): preserve fill positioning for remote images#1265
Conversation
Remote Image with fill=true currently routes through Unpic fullWidth, which renders a flowing image without data-nimg="fill" or absolute positioning. That violates Next.js's fill invariant for parent-filling layouts. Centralize fill styles and render remote fill with the same DOM contract as local fill and getImageProps. Add SSR regression coverage for remote fill.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix that correctly aligns remote fill rendering with Next.js semantics. The getFillStyle() extraction reduces duplication across all three fill paths (local, remote, getImageProps), and the test covers the regression well.
One nit and one note below — neither blocks merge.
| inset: 0, | ||
| width: "100%", | ||
| height: "100%", | ||
| objectFit: "cover", |
There was a problem hiding this comment.
Nit (pre-existing, not introduced by this PR): Next.js does not default objectFit to "cover" for fill images — it leaves it undefined and passes through the deprecated objectFit prop or lets the user set it via style. The "cover" default here is a deliberate vinext choice that's been around since before this PR, so it's fine to keep for now, but worth noting as a parity gap if someone reports that their fill image behaves differently from Next.js (e.g., they expect objectFit: "contain" to be the only active value, but vinext's "cover" base gets spread under it).
No action needed in this PR — just flagging for awareness.
| backgroundPosition: "center", | ||
| } | ||
| : undefined; | ||
| const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined; |
There was a problem hiding this comment.
Nit: blurStyle is now constructed above (lines 462-470) and passed directly to getFillStyle() on line 487, making the bg variable unused in the fill branch. It's still used by the non-fill UnpicImage constrained path on line 506, so it's not dead code overall — but it's now only relevant to the non-fill path. Consider moving the bg computation inside the if (imgWidth && imgHeight) block to make the scoping clearer. Not blocking.
| const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined; | |
| const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined; |
| expect(html).toContain("width:100%"); | ||
| expect(html).toContain("height:100%"); | ||
| expect(html).toContain('data-nimg="fill"'); | ||
| expect(html).toContain('sizes="100vw"'); |
There was a problem hiding this comment.
Nice coverage. One optional addition: you could also assert expect(html).toContain('decoding="async"') to fully match the local fill test's assertions and confirm the attribute is present on the remote fill path too. Minor.
|
Review submitted. Here's a summary: Approved with two nits (neither blocking):
The core change is correct: remote |
Overview
filllayout semantics for remotenext/imagesources.fillnow renders a plain<img>with the same fill style anddata-nimg="fill"contract used by local images andgetImageProps().Imagecomponent SSR path.packages/vinext/src/shims/image.tsx,tests/image-component.test.tsWhy
For
next/image,fillis a layout contract on the rendered<img>, not a remote-source optimization mode. Next.js applies absolute positioning anddata-nimg="fill"regardless of whether the source is local or remote, and its docs tell users to position the parent because the image itself is absolute.Imagerenderingfillmeans the DOM image fills its parent.layout="fullWidth"branch forfilland emits the same absolute-positioned<img>shape as local fill.getImageProps()should not drift.getFillStyle().fillasserting no width/height attrs, absolute positioning, defaultsizes="100vw", anddata-nimg="fill".What changed
<Image src="https://..." fill />layout="fullWidth", producingstyle="object-fit:cover;width:100%"without absolute positioning ordata-nimg="fill".<img>withposition:absolute,inset:0,width:100%,height:100%,sizes="100vw", anddata-nimg="fill".fill, loaderfill,getImageProps({ fill: true })getFillStyle()to keep the invariant in one place.Maintainer review path
packages/vinext/src/shims/image.tsxgetFillStyle()first.fillbranch and compare it with the local fill branch andgetImageProps().tests/image-component.test.tsValidation
vp test run tests/image-component.test.ts -t "renders remote fill mode with absolute positioning"vp test run tests/image-component.test.tsvp test run tests/image-component.test.ts tests/image-imports.test.ts tests/image-optimization-parity.test.tsvp checkRisk / compatibility
filluses the same plain<img>shape as local fill andgetImageProps()instead of UnpicfullWidthtransformation.fillflowing in document layout were relying on behavior that conflicts with Next.js and thefilldocs.Non-goals
fillstyles such as overridingposition,width, orheight.References
get-img-propsfill styledata-nimgdata-nimg="fill"is attached to the actual<img>.fillfillfillexpands to the parent and the image itself uses absolute positioning by default.