Skip to content

fix(core): don't reload iframes and media during hydration #71874

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

Open
jsaguet wants to merge 1 commit into
base: main
Choose a base branch
from
+542 −11
Open

fix(core): don't reload iframes and media during hydration #71874

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
21 changes: 17 additions & 4 deletions packages/core/src/render3/dom_node_manipulation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export function setElementAttribute(
name: string,
value: any,
sanitizer: SanitizerFn | null | undefined,
skipIfUnchanged = false,
) {
if (value == null) {
if (sanitizer != null) {
Expand All @@ -127,7 +128,9 @@ export function setElementAttribute(
const strValue =
sanitizer == null ? renderStringify(value) : sanitizer(value, tagName || '', name);

renderer.setAttribute(element, name, strValue as string, namespace);
if (!skipIfUnchanged || element.getAttribute(name) !== String(strValue)) {
renderer.setAttribute(element, name, strValue as string, namespace);
}
}
}

Expand Down Expand Up @@ -166,12 +169,22 @@ function writeDirectClass(renderer: Renderer, element: RElement, newValue: strin
}
}

/** Sets up the static DOM attributes on an `RNode`. */
export function setupStaticAttributes(renderer: Renderer, element: RElement, tNode: TNode) {
/**
* Sets up the static DOM attributes on an `RNode`.
*
* @param isHydrated Whether the element was reused during hydration. Its resource URL is then left
* alone if it already has the value, because the browser would load the resource again.
*/
export function setupStaticAttributes(
renderer: Renderer,
element: RElement,
tNode: TNode,

@JeanMeche JeanMeche Nov 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AGENT: Running setupStaticAttributes with skipIfUnchanged during hydration (creation mode) compares the SSR DOM against the static tNode attributes/classes/styles and overwrites them whenever they differ. Because creation mode runs before directive ngOnInit and update-mode bindings ([class.*], [style.*], [attr.*]), any attribute, class, or style that was legitimately augmented on the server by a directive or binding will differ from the static tNode value and get clobbered during hydration:

  1. Combined static + dynamic class / style: For <div class="static" [class.active]="true">, the SSR HTML has class="static active". In creation mode, element.getAttribute('class') !== classes ('static active' !== 'static') evaluates to true, so writeDirectClass clobbers the attribute back to 'static', only for the update pass to re-add 'active' immediately after—resulting in two unnecessary DOM mutations during hydration.
  2. Directive-modified attributes (e.g. NgOptimizedImage in NgOptimizedImage makes the browser select each image source again during hydration #71873): For <img ngSrc="hero.jpg" width="400" height="300" sizes="50vw">, SSR renders sizes="auto, 50vw". Here native.getAttribute('sizes') !== '50vw' is true, so setupStaticAttributes overwrites sizes back to '50vw', and NgOptimizedImage.ngOnInit() then writes 'auto, 50vw' again.

When hydrating an existing element (!wasLastNodeCreated()), the server has already serialized its static attributes, classes, and styles. Have we considered skipping setupStaticAttributes altogether when !wasLastNodeCreated() (aside from isAnimationProp properties which aren't serialized as HTML attributes)? That would avoid all the getAttribute reads on static attributes and prevent clobbering directive/binding state from SSR.

@jsaguet jsaguet Nov 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Those writes still happen, as they do today: static active differs from the template, so it's replaced, and [class.active] adds the class back.

I'd keep the comparison rather than skip setupStaticAttributes altogether, because skipping is what #71200 does, and your review there lists what it breaks. With the comparison, a server-only class or a value from an older deployment is still corrected, animation properties are still set, and an ngSkipHydration host is still written when it differs. Root hosts aren't touched.

The cost is that the sizes case stays as it is. #71873 tracks that one.

@JeanMeche JeanMeche Nov 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AGENT: Thanks for updating the PR and scoping down the dynamic property/attribute checks—narrowing isFirstWriteOfRenderedResource to video, audio, iframe, embed, and object[data] makes the property-binding side much safer and avoids the pitfalls with generic DOM property reflection and custom elements.

Regarding setupStaticAttributes:

Fair point about the subtle regressions identified in #71200 (such as root hosts, animation properties, and ngSkipHydration).

However, comparing element.getAttribute('class') !== classes and element.getAttribute('style') !== styles in creation mode has a fundamental limitation: it only checks static classes/styles against the SSR DOM, but the SSR DOM legitimately contains classes and styles added by update bindings ([class.*], [style.*]) and directives. On any element combining static classes with dynamic bindings (e.g. <div class="card" [class.active]="isActive">), getAttribute('class') is 'card active', which doesn't match 'card', so it still overwrites the class back to 'card' in creation mode, only for [class.active] to re-add 'active' right after.

Rather than trying to partially patch class and style reconciliation here (which is a much broader problem with styling pipelines and directives like in #71873):

What do you think about narrowing this PR specifically to the media/resource loading bug (#71872)?

  1. Keep your narrowed dynamic src / data check in shared.ts (isFirstWriteOfRenderedResource).
  2. In setupStaticAttributes / setUpAttributes, guard only static src / data for these media elements (video, audio, iframe, embed, object), leaving class and style untouched.

That gives a very clean separation of concerns:

@jsaguet jsaguet Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. I narrowed the PR as you suggested and squashed it into one commit:

  • Static values: only src on iframe, embed, video, and audio, and data on object, are compared. class and style are written as before.
  • Bindings: the same check, through one shared isResourceAttribute.

I opened #71971 for the cost of the other writes, with the measurements and the class/style problem you described.

isHydrated = false,
) {
const {mergedAttrs, classes, styles} = tNode;

if (mergedAttrs !== null) {
setUpAttributes(renderer, element, mergedAttrs);
setUpAttributes(renderer, element, mergedAttrs, isHydrated ? tNode.value : null);
}

if (classes !== null) {
Expand Down
35 changes: 32 additions & 3 deletions packages/core/src/render3/instructions/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
ENVIRONMENT,
FLAGS,
HEADER_OFFSET,
HYDRATION,
INJECTOR,
LView,
LViewFlags,
Expand All @@ -72,6 +73,7 @@ import {
wasLastNodeCreated,
} from '../state';
import {NO_CHANGE} from '../tokens';
import {isResourceAttribute} from '../util/attrs_utils';
import {INTERPOLATION_DELIMITER} from '../util/misc_utils';
import {getComponentLViewByIndex, getNativeByTNode, unwrapLView} from '../util/view_utils';

Expand Down Expand Up @@ -318,7 +320,12 @@ export function setDomProperty<T>(
// TODO(v23): Remove this workaround once we can introduce a breaking change
value = unwrapImgVideoSrcValue(propName, element as RElement, value);

renderer.setProperty(element as RElement, propName, value);
if (
!isFirstWriteOfRenderedResource(tNode, lView, propName) ||
(element as RElement).getAttribute(propName) !== String(value)
) {
renderer.setProperty(element as RElement, propName, value);
}
} else if (tNode.type & TNodeType.AnyContainer) {
// If the node is a container and the property didn't
// match any of the inputs or schemas we should throw.
Expand All @@ -328,6 +335,19 @@ export function setDomProperty<T>(
}
}

/**
* Whether a binding is about to write the resource URL of an element for the first time in a
* hydrated view. Setting that URL makes the browser load the resource again, even if the value
* doesn't change: an `<iframe>` reloads and a `<video>` restarts.
*/
function isFirstWriteOfRenderedResource(tNode: TNode, lView: LView, name: string): boolean {
return (
lView[HYDRATION] !== null &&
(lView[FLAGS] & LViewFlags.FirstLViewPass) !== 0 &&
isResourceAttribute(tNode.value, name)
);
}

/**
* This function allows us to workaround a breaking change introduced by #71095
* src attributes/bindings used to be sanitized which was responsible for:
Expand Down Expand Up @@ -557,7 +577,16 @@ export function elementAttributeInternal(
const element = getNativeByTNode(tNode, lView) as RElement;
// TODO(v23): Remove this workaround once we can introduce a breaking change
value = unwrapImgVideoSrcValue(name, element, value, true);
setElementAttribute(lView[RENDERER], element, namespace, tNode.value, name, value, sanitizer);
setElementAttribute(
lView[RENDERER],
element,
namespace,
tNode.value,
name,
value,
sanitizer,
namespace == null && isFirstWriteOfRenderedResource(tNode, lView, name),
);
}

/**
Expand Down Expand Up @@ -615,7 +644,7 @@ export function elementLikeStartShared(
// It's important that this runs before we've instantiated the directives.
const isElement = tNode.type === TNodeType.Element;
if (isElement) {
setupStaticAttributes(lView[RENDERER], native as RElement, tNode);
setupStaticAttributes(lView[RENDERER], native as RElement, tNode, !wasLastNodeCreated());

// any immediate children of a component or template container must be pre-emptively
// monkey-patched with the component view data so that the element can be inspected
Expand Down
27 changes: 24 additions & 3 deletions packages/core/src/render3/util/attrs_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,16 @@ import {RElement} from '../interfaces/renderer_dom';
* @param renderer The renderer to be used
* @param native The element that the attributes will be assigned to
* @param attrs The attribute array of values that will be assigned to the element
* @param hydratedTagName The tag name of an element reused during hydration, whose resource URL
* is left alone if it already has the value (see `isResourceAttribute`).
* @returns the index value that was last accessed in the attributes array
*/
export function setUpAttributes(renderer: Renderer, native: RElement, attrs: TAttributes): number {
export function setUpAttributes(
renderer: Renderer,
native: RElement,
attrs: TAttributes,
hydratedTagName: string | null = null,
): number {
let i = 0;
while (i < attrs.length) {
const value = attrs[i];
Expand All @@ -62,10 +69,14 @@ export function setUpAttributes(renderer: Renderer, native: RElement, attrs: TAt
// attrName is string;
const attrName = value as string;
const attrVal = attrs[++i];
// Standard attributes
// Standard attributes. `attrVal` can be a `TrustedScriptURL`, hence `String` below.
if (isAnimationProp(attrName)) {
renderer.setProperty(native, attrName, attrVal);
} else {
} else if (
hydratedTagName === null ||
!isResourceAttribute(hydratedTagName, attrName) ||
native.getAttribute(attrName) !== String(attrVal)
) {
renderer.setAttribute(native, attrName, attrVal as string);
}
i++;
Expand All @@ -79,6 +90,16 @@ export function setUpAttributes(renderer: Renderer, native: RElement, attrs: TAt
return i;
}

/**
* Whether setting the attribute `name` makes a `tagName` element load its resource again, even
* if the value doesn't change: an `<iframe>` reloads and a `<video>` restarts.
*/
export function isResourceAttribute(tagName: string | null, name: string): boolean {
return name === 'src'
? tagName === 'iframe' || tagName === 'embed' || tagName === 'video' || tagName === 'audio'
: name === 'data' && tagName === 'object';
}

/**
* Test whether the given value is a marker that indicates that the following
* attribute values in a `TAttributes` array are only the names of attributes,
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isForwardRef",
"isFunction",
"isInI18nBlock",
Expand All @@ -644,6 +645,7 @@
"isPositive",
"isPromise",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSkipHydrationRootTNode",
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/bundling/create_component/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,7 @@
"isPositive",
"isPromise",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSubscribable",
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/bundling/defer/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,7 @@
"isPositive",
"isPromise",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSkipHydrationRootTNode",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isFormControl",
"isFormControlState",
"isForwardRef",
Expand Down Expand Up @@ -773,6 +774,7 @@
"isPropertyUpdated",
"isReadableStreamLike",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSkipHydrationRootTNode",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/forms_signals/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isForwardRef",
"isFunction",
"isInI18nBlock",
Expand Down Expand Up @@ -787,6 +788,7 @@
"isReadableStreamLike",
"isRefreshingViews",
"isRelevantSelectMutation",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSchemaOrSchemaFn",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,6 +736,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isFormControlState",
"isForwardRef",
"isFunction",
Expand Down Expand Up @@ -766,6 +767,7 @@
"isPropertyUpdated",
"isReadableStreamLike",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSignal",
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/bundling/http/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@
"isReadableStreamLike",
"isRefreshingViews",
"isResolved",
"isResourceAttribute",
"isRootView",
"isScheduler",
"isSchedulerTick",
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/bundling/hydration/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,7 @@
"isPromise2",
"isReadableStreamLike",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isScheduler",
"isSchedulerTick",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/i18n/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,6 +610,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isForwardRef",
"isFunction",
"isI18nText",
Expand All @@ -626,6 +627,7 @@
"isPositive",
"isPromise",
"isRefreshingViews",
"isResourceAttribute",
"isRootTemplateMessage",
"isRootView",
"isSchedulerTick",
Expand Down
2 changes: 2 additions & 0 deletions packages/core/test/bundling/router/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,7 @@
"isErrorLike",
"isExistingProvider",
"isFactoryProvider",
"isFirstWriteOfRenderedResource",
"isForwardRef",
"isFunction",
"isFunction2",
Expand Down Expand Up @@ -871,6 +872,7 @@
"isRedirectingEvent",
"isRedirectingNavigationCancelingError",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isScheduler",
"isSchedulerTick",
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@
"isPositive",
"isPromise",
"isRefreshingViews",
"isResourceAttribute",
"isRootView",
"isSchedulerTick",
"isSubscribable",
Expand Down
Loading
Loading