-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| <div id="preserved-content" data-permanent></div> | ||
| <div id="non-preserved-content"></div> | ||
| <div id="preserved-attributes" data-permanent></div> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I expect we can reuse the |
||
|
|
||
| <InteractiveNavigationComponent @rendermode="@_interactiveNavigationRenderMode" /> | ||
|
|
||
|
|
@@ -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++; | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.