Fix data-permanent to preserve element attributes, not just descendants#64622
Fix data-permanent to preserve element attributes, not just descendants#64622
Conversation
Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
|
@copilot Have you followed the steps in src/Components/AGENTS.MD? |
Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
Yes, I've now followed the steps in AGENTS.MD. In commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where the data-permanent attribute only prevented child content synchronization during enhanced navigation, but still synchronized attributes on the element itself. The fix moves the isDataPermanentElement check to occur before attribute synchronization, ensuring both attributes and content are preserved for elements marked with data-permanent.
Key Changes:
- Modified TypeScript logic to skip all synchronization (attributes and content) for
data-permanentelements - Added unit tests verifying attribute preservation on
data-permanentelements - Added E2E test confirming dynamically added CSS classes survive enhanced navigation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/Components/Web.JS/src/Rendering/DomMerging/DomSync.ts |
Moved isDataPermanentElement check before attribute synchronization to preserve both attributes and content for permanent elements |
src/Components/Web.JS/test/DomSync.test.ts |
Added two unit tests verifying attributes are preserved: one for existing attributes and one for dynamically added attributes |
src/Components/test/E2ETest/ServerRenderingTests/EnhancedNavigationTest.cs |
Added E2E test verifying dynamically added CSS class is preserved across multiple enhanced navigation events |
src/Components/test/testassets/Components.TestServer/RazorComponents/Pages/EnhancedNav/PageThatPreservesContent.razor |
Extended test page to include element for attribute preservation testing with dynamically added CSS class |
|
/azp run aspnetcore-ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| <div id="preserved-content" data-permanent></div> | ||
| <div id="non-preserved-content"></div> | ||
| <div id="preserved-attributes" data-permanent></div> |
There was a problem hiding this comment.
nit: I expect we can reuse the preserved-content element and save two new lines of code in a test :)
| 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 |
There was a problem hiding this comment.
But that issue was fixed with #50453 already at the time of posting this PR.
There was a problem hiding this comment.
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.
|
|
||
| // 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); |
There was a problem hiding this comment.
Should we check the initial state in this test to confirm that the attribute is dynamically added?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
if the razor file got changed and the update was not dynamic anymore. But that's maybe too much caution. Ignore
Fix data-permanent to preserve element attributes, not just descendants
Description
The
data-permanentattribute was only preventing child content synchronization during enhanced navigation, but still synchronized attributes on the element itself. This caused JS-modified attributes (like dynamically added classes) to be lost.Changes:
isDataPermanentElementcheck intreatAsMatch()to occur before attribute synchronization, skipping both attribute and content sync for permanent elementsdata-permanentelements, including dynamically added attributesElementsWithDataPermanentAttribute_HavePreservedAttributes) to verify dynamically added CSS classes are preserved after enhanced navigationPageThatPreservesContent.razortest component to include attribute preservation scenarioFixes #51021
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.