fix(common): skip zero-height warning for detached fill-mode images by TheSaiEaranti · Pull Request #69955 · angular/angular

TheSaiEaranti · 2026-07-26T20:13:47Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/contributing-docs/commit-message-guidelines.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.dev application / infrastructure changes
Other... Please describe:

What is the current behavior?

assertNonZeroRenderedHeight emits NG02952 ("the height of the fill-mode image is zero") whenever a fill-mode image fires load with clientHeight === 0. A memory-cached image can fire load before its embedded view is attached to the document — for example when the same ngSrc appears in more than one @defer block and later instances are served from memory cache — and a detached element always reports a zero height. The warning then fires in bursts and points developers at container CSS that is not broken (the issue includes document-level capture-phase evidence that every warned image was detached at dispatch time and laid out correctly one tick later).

Issue Number: #69636

What is the new behavior?

The zero-height check only warns when img.isConnected is true, where a zero clientHeight is meaningful. Two tests pin both directions: a connected fill-mode image with zero rendered height still warns, and a detached one no longer does. The new detached-image test fails on main.

One design note for review: this skips the check for detached-at-load images rather than deferring it until the element connects, so an image that loads detached and later attaches into a genuinely zero-height container will not warn. If the team prefers re-checking on attachment, I'm happy to rework in that direction. Prior attempt #69809 proposed the same guard but was closed the same day (CLA); this implementation and its tests were written independently, and the positive-direction test is new.

Does this PR introduce a breaking change?

Yes
No

Other information

Dev-mode-only diagnostic; no production behavior change. pnpm test packages/common/test:test passes with the fix and fails on main with the new test.

JeanMeche · 2026-07-26T20:42:39Z

There is a subtle but significant issue with this approach: it introduces a false negative (blind spot) for images that load from the cache.

The Issue

By adding && img.isConnected inside the one-time load event listener, the directive completely skips the zero-height check for any image that fires its load event while detached.

Because the load listener deregisters itself immediately after firing, if an image is loaded very quickly (e.g., from the browser's memory cache), the load event can fire during the component's creation phase, before Angular has attached the element to the DOM.

For these cached images:

The load event fires.
img.isConnected is false.
The warning is skipped.
The image is attached to the DOM a few milliseconds later.
If the developer actually forgot to style the fill mode wrapper (resulting in a genuine 0-height image), they will never see the warning.

Essentially, this PR inadvertently disables the NG02952 zero-height warning for any image that is already in the browser's cache.

Potential Alternatives

To fix the false positive without losing the warning for cached images, the check might need to be deferred until the element is actually participating in the layout.

Some alternative approaches:

Defer the check: If !img.isConnected or clientHeight === 0, wait for a macrotask (like setTimeout) or use requestAnimationFrame to check the height slightly later when layout has settled.
ResizeObserver: Use a ResizeObserver to monitor the image. Once it receives a dimension greater than 0, disconnect the observer. If it stays at 0 after being connected to the DOM for a certain period, throw the warning.

TheSaiEaranti · 2026-07-26T20:51:31Z

You're right — the guard traded the false positive for a blind spot, since the one-shot load listener means a detached-at-load image never gets re-checked. Reworked in 3263f53 along your first suggested line:

When the image is not connected at load time, the check re-schedules itself (setTimeout) until the element is connected, then evaluates clientHeight once. A genuinely zero-height fill image now warns right after it attaches — the cached-image case keeps its warning.
The pending recheck is cancelled in the existing DestroyRef.onDestroy, so a @defer view that is destroyed before ever attaching neither warns nor leaks the timer.
I initially used requestAnimationFrame, but the check also runs in non-browser dev environments where it isn't defined (the package's own bazel test env, and dev-mode SSR), so setTimeout it is.

Tests now cover the full space: connected + zero height warns immediately (unchanged), detached → attached with a height stays silent, detached → attached still zero-height warns (the case the previous revision lost), and destruction cancels the deferred check.

On the ResizeObserver alternative: happy to go that way if you prefer, though the initial-observation semantics have their own blind spot for elements whose box is 0×0 when observation starts, which is exactly the broken case — the polling recheck seemed the smaller, more predictable dev-only machinery.

SkyZeroZx · 2026-07-26T21:25:11Z

+    // document, and a detached element always reports a zero height. Defer the
+    // check until the element is connected and participates in layout.
+    if (!img.isConnected) {
+      recheckTimeout = setTimeout(check);


I'm not entirely sure that using setTimeout is the correct approach. Based on what was mentioned, we can use requestFrameAnimation. While it's not supported in all environments/browsers, I think we can copy (or perhaps export) the ones used in idle_service.ts. (This won't affect the final bundle; as far as I understand, this only runs under ngDevMode.)

angular/packages/core/src/defer/idle_service.ts

Lines 21 to 30 in 5ad8231

type RequestIdle = typeof requestIdleCallback;

const _requestIdleCallback = () =>

(typeof requestIdleCallback !== 'undefined'

? requestIdleCallback

: (cb: VoidFunction) => setTimeout(cb) as unknown as number

).bind(globalThis) as RequestIdle;

const _cancelIdleCallback = () =>

(typeof requestIdleCallback !== 'undefined' ? cancelIdleCallback : clearTimeout).bind(globalThis);

TheSaiEaranti · 2026-07-26T21:30:33Z

Good call — done in the latest commit. I'd gone to plain setTimeout because requestAnimationFrame isn't defined in the package's node test environment (the first version crashed component cleanup there), but the idle_service.ts shim pattern is the right resolution: requestAnimationFrame where available, setTimeout fallback otherwise, both bound to globalThis like the requestIdleCallback shims. I copied the pattern locally rather than exporting from core to keep the change self-contained — happy to switch to a shared export if a maintainer prefers. The spec's flush helper uses the same feature detection, so the tests hold in both the bazel node env and real browsers.

SkyZeroZx · 2026-07-26T21:48:00Z

You can also please rebase your 4 commits so that there's only one.

You can fetch the latest changes, run an interactive rebase on upstream/main to squash your commits, and then force push your branch.

TheSaiEaranti · 2026-07-26T23:35:26Z

Done, squashed to a single commit and rebased on the latest main. The commit message describes the final approach (deferred recheck with the shimmed requestAnimationFrame). Tests are green locally, the CI run for the new head just needs a maintainer to approve it.

JeanMeche · 2026-07-27T04:04:59Z

+/**
+ * Shims for `requestAnimationFrame` and `cancelAnimationFrame` for environments
+ * where those functions are not available, mirroring the `requestIdleCallback`
+ * shims in `core/src/defer/idle_service.ts`. Only used by dev-mode checks.
+ */
+const _requestAnimationFrame = () =>
+  (typeof requestAnimationFrame !== 'undefined'
+    ? requestAnimationFrame
+    : (cb: VoidFunction) => setTimeout(cb) as unknown as number
+  ).bind(globalThis) as typeof requestAnimationFrame;
+
+const _cancelAnimationFrame = () =>
+  (typeof cancelAnimationFrame !== 'undefined' ? cancelAnimationFrame : clearTimeout).bind(
+    globalThis,
+  );
+


We don't need such shims. The framework itself uses requestAnimationFrame for scheduling change detection.

Potential Alternatives
To fix the false positive without losing the warning for cached images, the check might need to be deferred until the element is actually participating in the layout.

Some alternative approaches:

Defer the check: If !img.isConnected or clientHeight === 0, wait for a macrotask (like setTimeout) or use requestAnimationFrame to check the height slightly later when layout has settled.

According to the Gemini review (if I'm not mistaken), it indicates that if we need to differentiate due to the potential delays of load events, wouldn't it be correct to use raF to allow this in compatible browsers and use setTimeout as a fallback?

I was reviewing and found the following; I understand it would still be necessary since if we used requestAnimationFrame directly it might not work in Safari or other environments that don't support it.

angular/packages/core/src/util/callback_scheduler.ts

Lines 37 to 66 in 5ad8231

export function scheduleCallbackWithRafRace(callback: Function): () => void {

let timeoutId: number;

let animationFrameId: number;

function cleanup() {

callback = noop;

try {

if (animationFrameId !== undefined && typeof cancelAnimationFrame === 'function') {

cancelAnimationFrame(animationFrameId);

}

if (timeoutId !== undefined) {

clearTimeout(timeoutId);

}

} catch {

// Clearing/canceling can fail in tests due to the timing of functions being patched and unpatched

// Just ignore the errors - we protect ourselves from this issue by also making the callback a no-op.

}

}

timeoutId = setTimeout(() => {

callback();

cleanup();

}) as unknown as number;

if (typeof requestAnimationFrame === 'function') {

animationFrameId = requestAnimationFrame(() => {

callback();

cleanup();

});

}

return () => cleanup();

}

TheSaiEaranti · 2026-07-27T04:32:07Z

Makes sense, shims removed in 910c303. The scheduling now calls requestAnimationFrame directly.

One detail worth explaining since it's why the shims appeared in the first place: the packages/common node test environment defines requestAnimationFrame but not cancelAnimationFrame, so the first version of this crashed component cleanup there. Rather than shimming around it, destruction now stops the recheck loop with a flag and never calls cancelAnimationFrame at all. A pending frame callback after destroy just returns early, which for a dev-mode-only check seemed like the simplest correct shape. It also answers the fallback question, since scheduling relies on the same rAF assumption the framework already makes.

Still a single commit, rebased, tests and tslint green.

JeanMeche · 2026-07-27T04:34:49Z

-    removeErrorListenerFn();
+  let stopped = false;
+  const check = () => {
+    if (stopped) {


you can use

Suggested change

if (stopped) {

if (destroyRef.destroyed) {

… is connected A memory-cached image can fire its `load` event before the embedded view is attached to the document, for example when the same `ngSrc` appears in more than one `@defer` block. A detached element always reports a zero height, so `assertNonZeroRenderedHeight` emitted false-positive NG02952 warnings that point developers at container CSS that is not broken. Instead of evaluating `clientHeight` at load time, re-schedule the check with `requestAnimationFrame` until the element is connected and participates in layout. The recheck stops once `destroyRef.destroyed` is set, rather than calling `cancelAnimationFrame`, which some test environments do not define. A genuinely zero-height fill image still warns once it attaches, including images served from the browser cache. Fixes angular#69636

TheSaiEaranti · 2026-07-27T05:18:11Z

Nice, that's cleaner. Applied in c347e1f, the local flag is gone and the recheck reads destroyRef.destroyed directly. Tests and tslint green, still one commit.

pullapprove Bot requested a review from kirjs July 26, 2026 20:13

angular-robot Bot added the area: common Issues related to APIs in the @angular/common package label Jul 26, 2026

ngbot Bot added this to the Backlog milestone Jul 26, 2026

SkyZeroZx reviewed Jul 26, 2026

View reviewed changes

TheSaiEaranti force-pushed the fix-ngoptimizedimage-detached-warning branch from 7359ceb to cf77672 Compare July 26, 2026 23:35

JeanMeche reviewed Jul 27, 2026

View reviewed changes

TheSaiEaranti force-pushed the fix-ngoptimizedimage-detached-warning branch from cf77672 to 910c303 Compare July 27, 2026 04:31

JeanMeche reviewed Jul 27, 2026

View reviewed changes

TheSaiEaranti force-pushed the fix-ngoptimizedimage-detached-warning branch from 910c303 to c347e1f Compare July 27, 2026 05:17

TheSaiEaranti force-pushed the fix-ngoptimizedimage-detached-warning branch from c347e1f to c7e0312 Compare July 27, 2026 05:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(common): skip zero-height warning for detached fill-mode images - #69955

fix(common): skip zero-height warning for detached fill-mode images#69955
TheSaiEaranti wants to merge 1 commit into
angular:mainfrom
TheSaiEaranti:fix-ngoptimizedimage-detached-warning

TheSaiEaranti commented Jul 26, 2026

Uh oh!

JeanMeche commented Jul 26, 2026

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

SkyZeroZx Jul 26, 2026 •

edited

Loading

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

SkyZeroZx commented Jul 26, 2026

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

JeanMeche Jul 27, 2026

Uh oh!

SkyZeroZx Jul 27, 2026 •

edited

Loading

Uh oh!

TheSaiEaranti commented Jul 27, 2026

Uh oh!

JeanMeche Jul 27, 2026

Uh oh!

TheSaiEaranti commented Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	type RequestIdle = typeof requestIdleCallback;

	const _requestIdleCallback = () =>
	(typeof requestIdleCallback !== 'undefined'
	? requestIdleCallback
	: (cb: VoidFunction) => setTimeout(cb) as unknown as number
	).bind(globalThis) as RequestIdle;

	const _cancelIdleCallback = () =>
	(typeof requestIdleCallback !== 'undefined' ? cancelIdleCallback : clearTimeout).bind(globalThis);

	export function scheduleCallbackWithRafRace(callback: Function): () => void {
	let timeoutId: number;
	let animationFrameId: number;
	function cleanup() {
	callback = noop;
	try {
	if (animationFrameId !== undefined && typeof cancelAnimationFrame === 'function') {
	cancelAnimationFrame(animationFrameId);
	}
	if (timeoutId !== undefined) {
	clearTimeout(timeoutId);
	}
	} catch {
	// Clearing/canceling can fail in tests due to the timing of functions being patched and unpatched
	// Just ignore the errors - we protect ourselves from this issue by also making the callback a no-op.
	}
	}
	timeoutId = setTimeout(() => {
	callback();
	cleanup();
	}) as unknown as number;
	if (typeof requestAnimationFrame === 'function') {
	animationFrameId = requestAnimationFrame(() => {
	callback();
	cleanup();
	});
	}

	return () => cleanup();
	}

Uh oh!

Conversation

TheSaiEaranti commented Jul 26, 2026

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

JeanMeche commented Jul 26, 2026

The Issue

Potential Alternatives

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

SkyZeroZx Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

SkyZeroZx commented Jul 26, 2026

Uh oh!

TheSaiEaranti commented Jul 26, 2026

Uh oh!

JeanMeche Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

SkyZeroZx Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheSaiEaranti commented Jul 27, 2026

Uh oh!

JeanMeche Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

TheSaiEaranti commented Jul 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SkyZeroZx Jul 26, 2026 •

edited

Loading

SkyZeroZx Jul 27, 2026 •

edited

Loading