Skip to content

Fix data-permanent to preserve element attributes, not just descendants #64622

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
javiercn merged 3 commits into from
Jan 21, 2026
Merged

Fix data-permanent to preserve element attributes, not just descendants #64622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9fb6a51
Initial plan
Copilot Dec 3, 2025
5093e0c
Fix data-permanent to preserve element attributes, not just descendants
Copilot Dec 3, 2025
68284e4
Add E2E test for data-permanent attribute preservation
Copilot Dec 3, 2025
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
23 changes: 11 additions & 12 deletions src/Components/Web.JS/src/Rendering/DomMerging/DomSync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,22 +185,21 @@ function treatAsMatch(destination: Node, source: Node) {
break;
}
case Node.ELEMENT_NODE: {
const editableElementValue = getEditableElementValue(source as Element);
synchronizeAttributes(destination as Element, source as Element);
applyAnyDeferredValue(destination as Element);

if (isDataPermanentElement(destination as Element)) {
// The destination element's content should be retained, so we avoid recursing into it.
// The destination element's content and attributes should be retained.
} else {
const editableElementValue = getEditableElementValue(source as Element);
synchronizeAttributes(destination as Element, source as Element);
applyAnyDeferredValue(destination as Element);
synchronizeDomContentCore(destination as Element, source as Element);
}

// This is a much simpler alternative to the deferred-value-assignment logic we use in interactive rendering.
// Because this sync algorithm goes depth-first, we know all the attributes and descendants are fully in sync
// by now, so setting any "special value" property is just a matter of assigning it right now (we don't have
// to be concerned that it's invalid because it doesn't correspond to an <option> child or a min/max attribute).
if (editableElementValue !== null) {
ensureEditableValueSynchronized(destination as Element, editableElementValue);
// This is a much simpler alternative to the deferred-value-assignment logic we use in interactive rendering.
// Because this sync algorithm goes depth-first, we know all the attributes and descendants are fully in sync
// by now, so setting any "special value" property is just a matter of assigning it right now (we don't have
// to be concerned that it's invalid because it doesn't correspond to an <option> child or a min/max attribute).
if (editableElementValue !== null) {
ensureEditableValueSynchronized(destination as Element, editableElementValue);
}
}
break;
}
Expand Down
38 changes: 38 additions & 0 deletions src/Components/Web.JS/test/DomSync.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,44 @@ describe('DomSync', () => {
expect(newNodes[0].textContent).toBe('');
expect(newNodes[1].textContent).toBe('new content');
});

test('should preserve attributes on elements marked as data permanent', () => {
// Arrange: An element with data-permanent has additional attributes that differ from the new content
const destination = makeExistingContent(`<div id="myelem" class="expand" data-permanent>preserved</div>`);
const newContent = makeNewContent(`<div id="myelem" data-permanent>other content</div>`);
const oldNode = toNodeArray(destination)[0] as Element;

// Act
synchronizeDomContent(destination, newContent);
const newNode = toNodeArray(destination)[0] as Element;

// Assert: The element is the same, content is preserved, and attributes are preserved
expect(newNode).toBe(oldNode);
expect(newNode.textContent).toBe('preserved');
expect(newNode.getAttribute('class')).toBe('expand');
expect(newNode.getAttribute('id')).toBe('myelem');
});

test('should preserve dynamically added attributes on elements marked as data permanent', () => {
// Arrange: Simulates the scenario from the issue where JS mutates an element with data-permanent
const destination = makeExistingContent(`<div id="myelem" data-permanent></div>`);
const oldNode = toNodeArray(destination)[0] as Element;

// User adds a class via JS
oldNode.classList.add('expand');
expect(oldNode.classList.contains('expand')).toBe(true);

// Enhanced nav returns equivalent content
const newContent = makeNewContent(`<div id="myelem" data-permanent></div>`);

// Act
synchronizeDomContent(destination, newContent);
const newNode = toNodeArray(destination)[0] as Element;

// Assert: The expand class should be retained
expect(newNode).toBe(oldNode);
expect(newNode.classList.contains('expand')).toBe(true);
});
});

test('should remove value if neither source nor destination has one', () => {
Expand Down
33 changes: 33 additions & 0 deletions src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,39 @@ public void ElementsWithoutDataPermanentAttribute_DoNotHavePreservedContent()
Browser.Equal("", () => Browser.Exists(By.Id("non-preserved-content")).Text);
}

[Fact]
public void ElementsWithDataPermanentAttribute_HavePreservedAttributes()
{
Navigate($"{ServerPathBase}/nav");
Browser.Equal("Hello", () => Browser.Exists(By.TagName("h1")).Text);

Browser.Exists(By.TagName("nav")).FindElement(By.LinkText("Preserve content")).Click();
Browser.Equal("Page that preserves content", () => Browser.Exists(By.TagName("h1")).Text);

// Required until https://github.com/dotnet/aspnetcore/issues/50424 is fixed
Copy link
Copy Markdown
Member

@ilonatommy ilonatommy Jan 20, 2026

Choose a reason for hiding this comment

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

But that issue was fixed with #50453 already at the time of posting this PR.

Copy link
Copy Markdown
Member

@javiercn javiercn Jan 20, 2026

Choose a reason for hiding this comment

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

It has just copied from https://github.com/dotnet/aspnetcore/pull/64622/changes#diff-6257fe399feef2288438ea53e07d98e7c4857ef31001680078770686b1477929R487.

I'll leave it like it is for now, but it might be a good task for the AI to look through all these comments in the codebase and react accordingly when the fixes are already done.

Browser.Navigate().Refresh();

Browser.Exists(By.Id("refresh-with-refresh"));

Browser.Click(By.Id("start-listening"));

// Verify the dynamically added class exists before enhanced nav
var preservedAttributesElement = Browser.Exists(By.Id("preserved-attributes"));
Browser.True(() => preservedAttributesElement.GetAttribute("class")?.Contains("dynamically-added-class") == true);
Copy link
Copy Markdown
Member

@ilonatommy ilonatommy Jan 20, 2026

Choose a reason for hiding this comment

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

Should we check the initial state in this test to confirm that the attribute is dynamically added?

Copy link
Copy Markdown
Member

@javiercn javiercn Jan 20, 2026

Choose a reason for hiding this comment

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

I'm not sure what you mean, the attribute isn't in the initial markup, I expect that to be enough (how else would it be there)

Copy link
Copy Markdown
Member

@ilonatommy ilonatommy Jan 21, 2026

Choose a reason for hiding this comment

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

if the razor file got changed and the update was not dynamic anymore. But that's maybe too much caution. Ignore


Browser.Click(By.Id("refresh-with-refresh"));
AssertEnhancedUpdateCountEquals(1);

// Verify the dynamically added class is preserved after enhanced nav
Browser.True(() => preservedAttributesElement.GetAttribute("class")?.Contains("dynamically-added-class") == true);

Browser.Click(By.Id("refresh-with-refresh"));
AssertEnhancedUpdateCountEquals(2);

// Verify the dynamically added class is still preserved after another enhanced nav
Browser.True(() => preservedAttributesElement.GetAttribute("class")?.Contains("dynamically-added-class") == true);
}

[Fact]
public void EnhancedNavNotUsedForNonBlazorDestinations()
{
Expand Down
3 changes: 3 additions & 0 deletions ...ts/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageThatPreservesContent.razor
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

<div id="preserved-content" data-permanent></div>
<div id="non-preserved-content"></div>
<div id="preserved-attributes" data-permanent></div>
Copy link
Copy Markdown
Member

@maraf maraf Jan 20, 2026

Choose a reason for hiding this comment

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

nit: I expect we can reuse the preserved-content element and save two new lines of code in a test :)


<InteractiveNavigationComponent @rendermode="@_interactiveNavigationRenderMode" />

Expand All @@ -18,9 +19,11 @@
//# sourceURL=preserve-content.js
const preservedContent = document.getElementById('preserved-content');
const nonPreservedContent = document.getElementById('non-preserved-content');
const preservedAttributes = document.getElementById('preserved-attributes');

preservedContent.textContent = 'Preserved content';
nonPreservedContent.textContent = 'Non preserved content';
preservedAttributes.classList.add('dynamically-added-class');

const onEnhancedLoad = (ev) => {
window.enhancedPageUpdateCount++;
Expand Down
Loading