Skip to content

fix(image): preserve fill positioning for remote images #1265

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.

Sign up for GitHub

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

Merged
james-elicx merged 1 commit into from
May 16, 2026
+58 −46
Merged

fix(image): preserve fill positioning for remote images #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 37 additions & 46 deletions packages/vinext/src/shims/image.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,21 @@ function isRemoteUrl(src: string): boolean {
return src.startsWith("http://") || src.startsWith("https://") || src.startsWith("//");
}

function getFillStyle(
style?: React.CSSProperties,
backgroundStyle?: React.CSSProperties,
): React.CSSProperties {
return {
position: "absolute",
inset: 0,
width: "100%",
height: "100%",
objectFit: "cover",
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

...backgroundStyle,
...style,
};
}

/**
* Resolve src, width, height, blurDataURL from Image props (string or StaticImageData).
* Shared by the Image component and getImageProps to keep behavior in sync.
Expand Down Expand Up @@ -421,24 +436,15 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
className={className}
onLoad={handleLoad}
onError={handleError}
style={
fill
? {
position: "absolute",
inset: 0,
width: "100%",
height: "100%",
objectFit: "cover",
...style,
}
: style
}
style={fill ? getFillStyle(style) : style}
{...rest}
/>
);
}

// For remote URLs, validate against remotePatterns then use @unpic/react
// For remote URLs, validate against remotePatterns. Non-fill images use
// @unpic/react for CDN URL transforms; fill uses a plain img so the DOM
// element keeps Next.js's absolute-positioned fill contract.
if (isRemoteUrl(src)) {
const validation = validateRemoteUrl(src);
if (!validation.allowed) {
Expand All @@ -453,26 +459,33 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
}

const sanitizedBlur = imgBlurDataURL ? sanitizeBlurDataURL(imgBlurDataURL) : undefined;
const blurStyle =
placeholder === "blur" && sanitizedBlur
? {
backgroundImage: `url(${sanitizedBlur})`,
backgroundSize: "cover",
backgroundRepeat: "no-repeat",
backgroundPosition: "center",
}
: undefined;
const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined;
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined;
const bg = placeholder === "blur" && sanitizedBlur ? `url(${sanitizedBlur})` : undefined;


if (fill) {
return (
<UnpicImage
<img
ref={mergedRef}
src={src}
alt={alt}
layout="fullWidth"
// `priority` is a Next.js concept — translate it to HTML attributes so
// it is never forwarded to the DOM as a non-boolean attribute, which
// would trigger React's "Received `true` for a non-boolean attribute"
// warning.
loading={priority ? "eager" : (loading ?? "lazy")}
fetchPriority={priority ? "high" : undefined}
sizes={sizes}
decoding="async"
sizes={sizes ?? "100vw"}
className={className}
background={bg}
data-nimg="fill"
onLoad={handleLoad}
onError={handleError}
ref={mergedRef}
style={getFillStyle(style, blurStyle)}
{...rest}
/>
);
}
Expand Down Expand Up @@ -561,19 +574,7 @@ const Image = forwardRef<HTMLImageElement, ImageProps>(function Image(
data-nimg={fill ? "fill" : "1"}
onLoad={handleLoad}
onError={handleError}
style={
fill
? {
position: "absolute",
inset: 0,
width: "100%",
height: "100%",
objectFit: "cover",
...blurStyle,
...style,
}
: { ...blurStyle, ...style }
}
style={fill ? getFillStyle(style, blurStyle) : { ...blurStyle, ...style }}
{...rest}
/>
);
Expand Down Expand Up @@ -684,17 +685,7 @@ export function getImageProps(props: ImageProps): {
sizes: sizes ?? (fill ? "100vw" : undefined),
className,
"data-nimg": fill ? "fill" : "1",
style: fill
? {
position: "absolute" as const,
inset: 0,
width: "100%",
height: "100%",
objectFit: "cover" as const,
...blurStyle,
...style,
}
: { ...blurStyle, ...style },
style: fill ? getFillStyle(style, blurStyle) : { ...blurStyle, ...style },
...rest,
} as React.ImgHTMLAttributes<HTMLImageElement>,
};
Expand Down
21 changes: 21 additions & 0 deletions tests/image-component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,27 @@ describe("Image SSR rendering", () => {
expect(html).toContain('sizes="100vw"');
});

it("renders remote fill mode with absolute positioning", () => {
// Ported from Next.js: test/unit/next-image-get-img-props.test.ts
// https://github.com/vercel/next.js/blob/canary/test/unit/next-image-get-img-props.test.ts
const html = ReactDOMServer.renderToString(
React.createElement(Image, {
alt: "remote fill image",
src: "https://images.unsplash.com/photo-fill",
fill: true,
}),
);
// Remote fill must preserve the same layout contract as local fill:
// the DOM img is absolutely positioned and marked as data-nimg="fill".
expect(html).not.toMatch(/width="\d+"/);
expect(html).not.toMatch(/height="\d+"/);
expect(html).toContain("position:absolute");
expect(html).toContain("width:100%");
expect(html).toContain("height:100%");
expect(html).toContain('data-nimg="fill"');
expect(html).toContain('sizes="100vw"');
Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot May 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

it("renders with custom sizes prop", () => {
const html = ReactDOMServer.renderToString(
React.createElement(Image, {
Expand Down
Loading