fix(core): add hydration protected elements - #52478

@arturovt as we've discussed offline, the proposed fix would not work due to potential performance issues. This extra logic should be limited to the following scenario:

• when an element is an iframe and it was hydrated
• and when we try to set the src attribute for the first time

There are 2 possible cases:

1. Static src attribute (e.g. <iframe src="...">).
2. When src attribute is set as a binding (e.g. <iframe [src]="...">).

Those 2 cases are being handled by different code paths (static case, binding case). Static attributes are applied when an element is created/hydrated and we can handle that. The binding case is more complicated: it's invoked as a part of change detection and currently there is no information whether an underlying element was created or hydrated, so we need to store more info about it (but only for iframes with src attributes).

Here is a proposed solution:

1. Add a new field to the DehydratedView interface (code) to store an initial value of the src, which it currently has in the DOM. The field might have the following shape: Map<number, string>, where number would represent a TNode index and a value is the current value of the src attribute.
2. In hydration code (here), once we find an element in the DOM, we can check whether an element is an iframe and if so, read the value of the src attribute and store it in the lView[HYDRATION] field (we've updated the type for it in Step 1). Note: the field should be null by default, so we don't create empty Maps for relatively rare cases when we need it, we can create it lazily when we come across an iframe.
3. Update the code path for static attributes (in this function) and instead of always doing renderer.setAttribute(native, attrName, attrVal as string), we can invoke a different function (for example, _setAttribute), which would do the regular call by default, but we replace its implementation if hydration is enabled using this approach. The hydration-aware implementation may look like this:

function _setAttributeWithHydrationSupport(lView: LView, tNode: TNode, tagName: string, element: RElement, name: string, value: string|TrustedHTML|TrustedScript|TrustedScriptURL, namespace?: string|null) { 
  if (
      // Are we in the process of creating/updating a view the first time?
      (lView[FLAGS] & LViewFlags.FirstLViewPass) &&
      // Do we have extra hydration info?
      (typeof lView?.[HYDRATION]?.protectedAttrs !== 'undefined') &&
      // Is it a "protected" attribute?
      isHydrationProtectedAttr(tagName, name) && 
      // Do we have a "protected" attribute on this node and whether the value is the same?
      lView[HYDRATION].protectedAttrs[tNode.index] === value) {

    // If answers to the questions above are all "yes" - exit, we do not want to reset the value,
    // since it may negatively affect performance and UX (e.g. in the `<iframe src="...">` case,
    // this would cause a reload within the iframe).
    return;
  }
  const renderer = lView[RENDERER];
  renderer.setAttribute(element, name, value, namespace);
}

The isHydrationProtectedAttr may look like this:

function isHydrationProtectedAttr(tagName: string, attrName: string): boolean {
  // Note: this list can be expanded if there are more cases.
  return tagName === 'iframe' && attrName === 'src';
}

4. For the binding case, we can now use the _setAttribute, which will use hydration-aware implementation and prevent overriding src for the first update pass.
5. Put together a few tests (in this spec) to verify that the src is not overridden during hydration.

Additional note: there might be other elements affected by the same problem, we'd like to ask if you could check the spec for elements like <audio>, <object>, <picture> and others that refer to remote resources, to see if any of them might be affected and should be included into the fix.

Please let me know if any additional information is required.

Here's the list of valid and invalid cases: elements marked in red reload resources when hydrated. We don't consider cases when the cache is disabled through devtools or the remote resource returns a 302 status code.

• ✅

<img src="/assets/img_girl.jpg" />

• ✅

<picture>
  <source media="(min-width: 650px)" srcset="/assets/img_food.jpg" />
  <source media="(min-width: 465px)" srcset="/assets/img_car.jpg" />
  <img src="/assets/img_girl.jpg"  />
</picture>

• ✅

<svg>
  <image xlink:href="/assets/angular.svg" />
</svg>

• ✅

<video controls autoplay>
  <source src="/assets/mov_bbb.mp4" type="video/mp4" />
  <source src="/assets/mov_bbb.ogg" type="video/ogg" />
</video>

• ✅

<audio controls src="/assets/t-rex-roar.mp3"></audio>

• ✅

<video controls src="/assets/friday.mp4">
  <track default kind="captions" srclang="en" src="/assets/friday.vtt" />
</video>

• ❌

<object
  type="application/pdf"
  data="/assets/In-CC0.pdf"
></object>

• ❌

<embed type="video/webm" src="/assets/flower.mp4" />

• ❌

<iframe src="https://www.youtube-nocookie.com/embed/7HVMn1TpXKQ"></iframe>

@arturovt thanks for additional information, this is very helpful. We can extend the isHydrationProtectedAttr function described in my previous comment to support <object> and <embed> cases as well. Please let us know if you might be interested in updating this PR as described in #52478 (comment) and also let us know if you have any questions about the proposed fix.

@AndrewKushnir I am interested in providing the fix; I'll need to go through your comments and start implementing it locally. I'll message you if I have any questions.

Thanks @arturovt and @AndrewKushnir for your efforts on this.

@arturovt thanks for implementing this 👍 I've added this PR to the v18 milestone.

If you get a chance, could you please try to find corresponding sections of HTML spec that talk about this specific behavior of the <iframe>, <object> and <embed> elements, so that we can include it as comments into the code for future reference?

@AndrewKushnir I went through the HTML standard and I may guess it doesn't explicitly mandate this behavior or provide detailed instructions on how user agents should handle this aspect of network requests. Because if we talk about img.src - setting the src attribute to the same value again on the img element doesn't trigger resource reload and it's a common behavior implemented by all user agents. Considering the iframe.src case, I may guess that the content of an iframe can be dynamic and it may contain interactive elements or scripts that require reloading when the source URL changes (took this statement from Google search).

I am confident that what we're doing in this PR doesn't go against the spec because we're essentially targeting that hydration case with the very first render. (Please correct me if I'm wrong...).

Out of curiosity, wanna check what engines do internally...

UPD: I examined the behavior of Safari's JSC for images and iframes.

Each HTML element class has a hook called attributeChanged.

For iframes, HTMLIFrameElement::attributeChanged triggers HTMLFrameElementBase::attributeChanged, which then determines the attribute being changed. If name == srcAttr, it invokes setLocation, which in turn calls openURL.

In the case of images, it's more intricate and aligns with our previous discussion. HTMLImageElement::attributeChanged employs a switch-case structure, and if it's case AttributeNames::srcAttr, it performs this check:

if (oldValue != newValue)
  selectImageSource(RelevantMutation::Yes);
else
  m_imageLoader->updateFromElementIgnoringPreviousErrorToSameValue();

And updateFromElementIgnoringPreviousErrorToSameValue checks whether image->allowsCaching() and does nothing if it has caching or does not have running service worker.

But you know, it's more like interesting information to keep in mind. Unfortunately, such implementation details are not included in the specification.

@AndrewKushnir made changes according to your comment.

@AndrewKushnir made changes according to your comment.

When this ships, I will celebrate for all of you

@AndrewKushnir what's left?

@arturovt @rhutchison Sorry for the delay. I've added this fix to the v19 candidate list and we'll perform code review and testing as a part of v19 preparation process.

@arturovt thanks for working on this fix 👍

We've were reviewing this change today and the conclusion was that this fix would work (and I remember that I was proposing this/similar approach earlier), but it introduces quite a lot of complexity and runtime code at perf-sensitive locations, which would be very hard to manage. @alxhub proposed a different and a very interesting approach. Instead of relying on the overwritten behavior at runtime, we can serialize more info on the server and pre-populate corresponding LView slots with the necessary attribute values during hydration. In this case, a non-hydration runtime code would not require any changes - an update of an attribute would not happen with an existing runtime logic.

Here are some extra details that might be helpful for this change.

1. The serialization logic can be added to the serializeLView function here, you can take a look how TNodes with the TNodeType.Element type are processed. The TNode would contain a tag name (tNode.value) and you'd also need an attribute/property name and value - this would require some extra code, see points below.

   • Static attributes (e.g. <iframe src="...">): those attributes (both name and value ) would be stored on a TNode at the tNode.mergedAttrs slot.
   • Bindings (e.g. <iframe [src]="...">): the value of an attribute/property would be stored in an LView, but the name is currently not stored in production mode, only in dev mode by using the storePropertyBindingMetadata function. We may potentially need a similar function (or reuse the same one?) and invoke it only when we are on the server. We can potentially bring this logic in in a similar way how it's done for attribute and property setter functions in this PR.

2. The necessary information (a list of key-value pairs) can be included into the serialized output (it'd be present in the nghData in the TransferState). For example:

nghData: {
  ...
  {  // this object represents an LView info:
    attributes: {
      2 /* element idx */: [5 /* binding idx */, 'src', '...'],
      7 /* element idx */: [-1 /* `-1` means static */, 'src', '...'],
      ...
    } 
  }
}

The idea is that we'll have a special object in a serialized object for each LView and we can use an element LView slot id as an index and represent key-value pairs as a flat array + include binding slot index (we can also optimize it and have a string with separators as a value, e.g. 2|src|https://some-url.com).

3. On the client, during hydration (in the locateOrCreateElementNodeImpl function), we can do the following:

   • check to see if we an element with a given index have special attributes
   • if yes, read the data and determine whether it's a static value or a dynamic binding
   • for static values - we'll use it to skip a "write" operation in the setupStaticAttributes (see point 4 below)
   • for dynamic bindings - restore it at LView[index]

4. For static values, we may need to do an extra update to the setupStaticAttributes function to avoid resetting the value (for attributes that were serialized as "special"). There are a few ways to do it, we can look into that if the dynamic binding case works correctly.

Please let me know if you need any extra info or have any questions, we'll be happy to help. Also, if you'd like, I can create an initial draft of the change and share with you (in this thread) as a diff.

Let's also keep this PR open for now, we'll get back to it in case the described approach doesn't work.

Thank you.

@AndrewKushnir okay, I'll need to go through the functions you mentioned locally in the playground to see what data they're working with and how they're serializing the LView.

Since this has been stale for so long, I'm going to close this. Feel free to open a new PR if you want to still see this land.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}