Fix VisualStateGroups duplicate name crash with implicit styles (#34716)

[StephaneDelcroix]

Description

Closes #34716

Fixes InvalidOperationException: VisualStateGroup Names must be unique that occurs when a Button (or any element) has explicit VisualStateManager.VisualStateGroups in XAML and an implicit style also sets VisualStateGroups with the same group name (e.g., "CommonStates").

Root Cause

All three XAML inflators (Runtime, XamlC, SourceGen) use GetValue(VisualStateGroupsProperty) + Add(group) when processing implicit collection items. When an implicit style (like the default Button style in Styles.xaml) already populated the VisualStateGroupList with a "CommonStates" group, the inflator's Add() call introduces a duplicate name, triggering the validation exception.

Fix

VisualStateGroupList.Add() now removes any existing group with the same name before adding the new one. This ensures explicit XAML groups replace style-set groups, and is backward-compatible since duplicate group names were never valid (the old behavior threw).

Changed files:

• src/Controls/src/Core/VisualStateManager.cs — Replace-on-duplicate in Add()
• src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs — Updated test to verify replace behavior
• src/Controls/tests/Xaml.UnitTests/Issues/Maui34716.xaml[.cs] — Regression tests

Tests

15 XAML unit tests + 32 Core unit tests pass:

Test	Scenario
VisualStateGroupsOnElementShouldNotThrowDuplicateNames	Direct VSG on Button (exact issue repro)
VisualStateGroupsWithImplicitStyleShouldNotThrowDuplicateNames	The bug — implicit style + explicit XAML
VisualStateGroupsViaStyleShouldNotThrow	VSG via Style Setter
MultipleVisualStateGroupsShouldNotThrow	Two groups on same element
ExplicitVisualStateGroupListShouldNotThrow	Explicit <VisualStateGroupList>
GroupWithDuplicateNameReplacesExisting	Core test: Add() replaces duplicate

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34719

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34719"

[Copilot]

Pull request overview

Adds a focused XAML unit test suite for issue #34716 to validate that VisualStateManager.VisualStateGroups inflates/compiles correctly across Runtime, XamlC, and SourceGen without triggering the “VisualStateGroup Names must be unique” exception.

Changes:

• Added a new issue reproduction XAML page containing several VisualStateGroups declaration patterns (direct on element, via Style, multiple groups, explicit VisualStateGroupList).
• Added xUnit theory tests (via XamlInflatorData) asserting the generated VisualStateGroupList contents are correct for each scenario across all inflators.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Controls/tests/Xaml.UnitTests/Issues/Maui34716.xaml	Defines the XAML scenarios covering direct, style-based, multi-group, and explicit-list VisualStateGroups patterns.
src/Controls/tests/Xaml.UnitTests/Issues/Maui34716.xaml.cs	Adds 4 inflator-parameterized xUnit tests validating group creation and basic shape (count/names/state counts).

[github-actions]

🧪 PR Test Evaluation

Overall Verdict: ✅ Tests are adequate

This is a well-structured test-only PR adding 4 XAML unit tests for VisualStateManager.VisualStateGroups parsing scenarios. Tests use the correct xUnit framework pattern for this project and cover the key repro cases from the issue.

👍 / 👎 — Was this evaluation helpful? React to let us know!

📊 Expand Full Evaluation

PR Test Evaluation Report

PR: #34719 — Add XAML unit tests for VisualStateManager.VisualStateGroups (#34716)
Test files evaluated: 2 (Maui34716.xaml, Maui34716.xaml.cs)
Fix files: 0 (test-only PR)

---

Overall Verdict

✅ Tests are adequate

The 4 XAML unit tests cover the main scenarios from issue #34716 using the correct test type (XAML unit tests) and framework (xUnit + [XamlInflatorData]). Minor improvements could add setter-value assertions, but the primary regression — duplicate group creation — is well-tested.

---

1. Fix Coverage — ✅

This is a test-only PR adding regression tests for issue #34716 (VisualStateGroups duplicate names on XAML parse). The four tests cover:

• Direct VisualStateManager.VisualStateGroups on element (exact issue repro)
• Style-based VSG via (Setter Property="VisualStateManager.VisualStateGroups")
• Multiple groups on a single element
• Explicit (VisualStateGroupList) wrapper

Each test calls new Maui34716(inflator) (runs all inflators: Runtime, XamlC, SourceGen) and verifies the groups are correctly populated without duplication, directly testing the parsing behavior described in the issue.

2. Edge Cases & Gaps — ⚠️

Covered:

• Single group with VisualStateGroupList wrapper vs. inline VisualStateGroup
• Style setter containing VSG
• Multiple named groups on one element
• Correct state/group counts

Missing:

• No assertions on the actual setter values inside states. For example, button has BackgroundColor=White for Normal and BackgroundColor=LightGray for Pressed — the test only checks groups1[0].States.Count == 2 but not that the setters are correct. Similar tests like Maui13619.xaml.cs use Assert.Equal(Colors.DarkGray, page.label.BackgroundColor). Adding a check like Assert.Equal(Colors.White, ((Setter)groups1[0].States[0].Setters[0]).Value) would strengthen the regression.
• No test for VSG on a non-Button element (e.g., Label or Grid)
• No test verifying that VisualStateManager.GoToState works correctly after parsing (ensures the parsed states are functional, not just present)

These gaps are minor since the core issue (duplicate groups) is directly tested.

3. Test Type Appropriateness — ✅

Current: XAML Unit Tests (Controls.Xaml.UnitTests)
Recommendation: Same — this is the ideal test type. The bug is in XAML parsing/XamlC compilation of VisualStateManager.VisualStateGroups, not in runtime UI behavior, making XAML unit tests the lightest and most appropriate choice.

4. Convention Compliance — ✅

• ✅ File naming: Maui34716.xaml / Maui34716.xaml.cs follows MauiXXXXX convention
• ✅ partial class Maui34716 : ContentPage matches XAML x:Class
• ✅ [Collection("Issue")] on the test class
• ✅ [Theory] + [XamlInflatorData] on all 4 test methods (runs all inflators)
• ✅ No [SetUp]/[TearDown] needed (no Application.Current or DispatcherProvider dependency)

Note: The automated script flagged Maui34716.xaml as not following MauiXXXXX pattern — this is a false positive in the script; the name correctly follows the convention.

5. Flakiness Risk — ✅ Low

All four tests are synchronous XAML unit tests with no I/O, UI interaction, animation, or async code. No Task.Delay/Thread.Sleep. No screenshot comparison. Flakiness risk is negligible.

6. Duplicate Coverage — ✅ No duplicates

The closest existing test is Maui24849.xaml.cs (VSGReturnsToNormal), which tests GoToState transitions. The new tests focus on correct initial XAML parsing of group structure — a different scenario. Maui17354.xaml.cs and Maui16960.xaml.cs test pointer/focus state transitions. No redundancy.

7. Platform Scope — ✅

XAML unit tests run all inflators (Runtime, XamlC, SourceGen) via [XamlInflatorData], covering the three XAML processing pipelines. Since the issue is in XAML parsing (cross-platform), this is the correct scope.

8. Assertion Quality — ⚠️

Test	Assertions	Quality
VisualStateGroupsOnElementShouldNotThrowDuplicateNames	Assert.Single(groups1), name check, state count	✅ Targets the exact duplicate-prevention fix
VisualStateGroupsViaStyleShouldNotThrow	Assert.Single, name check	✅ Adequate for style scenario
MultipleVisualStateGroupsShouldNotThrow	Assert.Equal(2, groups.Count), both names	✅ Good — verifies both groups present
ExplicitVisualStateGroupListShouldNotThrow	Assert.Single, name check	✅ Adequate

Assertions are specific to the group structure. They would catch if the bug regressed (e.g., groups becoming duplicated would break Assert.Single). Adding setter-value assertions would improve confidence in full parse correctness, but the current level is sufficient for the stated issue.

9. Fix-Test Alignment — ✅

The tests are directly aligned with the issue scenario: setting VisualStateManager.VisualStateGroups in XAML. The XAML file contains precisely the patterns described in issue #34716 (direct attached property, style setter, multiple groups, explicit list).

---

Recommendations

1. Optional enhancement: Add assertions on at least one setter value inside the states (e.g., Assert.Equal(Colors.White, ((Setter)groups1[0].States[0].Setters[0]).Value)) to verify the full parse tree, not just the group structure.
2. Optional enhancement: Add a test calling VisualStateManager.GoToState(page.button, "Pressed") and verifying the property update, to confirm parsed states are functional.
3. No action needed on naming convention warning — Maui34716 is correct; the script regex has a false positive.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 3 items

Integrity filtering activated and filtered the following items during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

• pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719 (pull_request_read: Resource 'pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)
• pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719 (pull_request_read: Resource 'pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719' has lower integrity than agent requires. Agent would need to drop integrity tags [approved:all unapproved:all] to trust this resource.)
• issue:SourceGen: VisualStateManager.VisualStateGroups causes 'Names must be unique' at startup #34716 (issue_read: Resource 'issue:SourceGen: VisualStateManager.VisualStateGroups causes 'Names must be unique' at startup #34716' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)

🧪 Test evaluation by Evaluate PR Tests

[github-actions]

🧪 PR Test Evaluation

Overall Verdict: ✅ Tests are adequate

The tests comprehensively cover the fix across all XAML inflators (Runtime, XamlC, SourceGen) and multiple real-world scenarios, including the exact bug reproduction path (implicit style + explicit XAML VisualStateGroups).

👍 / 👎 — Was this evaluation helpful? React to let us know!

📊 Expand Full Evaluation

PR Test Evaluation Report

PR: #34719 — Fix VisualStateGroups duplicate name crash with implicit styles
Test files evaluated: 3 (1 unit test, 2 XAML test files)
Fix files: 1 (VisualStateManager.cs)

---

Overall Verdict

✅ Tests are adequate

The fix changes VisualStateGroupList.Add() to silently replace an existing group with the same name instead of throwing. The tests directly validate this behavior through a unit test and five XAML [Theory] test methods covering all three inflators.

---

1. Fix Coverage — ✅

The tests directly exercise the changed code path:

• Unit test (GroupWithDuplicateNameReplacesExisting): Calls vsgs.Add(secondGroup) with a duplicate name and asserts Assert.Single(vsgs) + Assert.Same(secondGroup, vsgs[0]) — these assertions would fail if the fix were reverted (old behavior threw InvalidOperationException).
• XAML test VisualStateGroupsWithImplicitStyleShouldNotThrowDuplicateNames: Installs an implicit Button style with "CommonStates", then inflates a page where the XAML also declares "CommonStates" — exactly the bug scenario described in the PR. Any regression would crash or produce incorrect group counts.

---

2. Edge Cases & Gaps — ⚠️

Covered:

• Duplicate name in a fresh list (unit test)
• Explicit XAML groups with no implicit style (no-regression scenario)
• Implicit style sets "CommonStates", explicit XAML also sets "CommonStates" (real bug path)
• Element styled via Style= only (no duplication conflict)
• Two distinct group names on one element (unaffected by fix)
• Explicit VisualStateGroupList wrapping (no duplication)

Missing:

• Null or empty Name: The fix skips dedup when string.IsNullOrEmpty(item.Name). Adding two nameless groups would still cause the uniqueness error (or silent data loss). A test confirming the expected behavior for null-named groups would make the contract explicit.
• Replacement ordering: When a duplicate replaces an existing group, the new group goes to the end of the list (since Remove + Add). No test asserts the position of the replaced group if there were multiple groups before the duplicate. This is minor but could matter if callers depend on order.
• Clone behavior: The Clone() method iterates and calls Add() — since Add() now silently removes duplicates, cloning a list that had legitimate duplicates (edge case pre-fix) would silently drop entries. This is unlikely in practice, but worth noting.

---

3. Test Type Appropriateness — ✅

Current: Unit Test + XAML Tests
Recommendation: Same — these are the ideal test types.

The fix is pure C# logic in VisualStateGroupList.Add() with no platform-specific behavior, making unit tests the right choice. XAML tests additionally validate that all three inflators (Runtime, XamlC, SourceGen) correctly handle the XAML markup patterns that triggered the original crash. No device or UI tests are needed.

---

4. Convention Compliance — ✅

Unit test:

• Uses [Fact] — matches existing VisualStateManagerTests.cs conventions ✅
• Renamed from GroupNamesMustBeUniqueWithinGroupList → GroupWithDuplicateNameReplacesExisting — accurately describes the new behavior ✅

XAML tests:

• Uses xUnit [Theory] + [XamlInflatorData] — consistent with the project's XamlInflatorDataAttribute helper ✅
• [Collection("Issue")] and IDisposable teardown to reset Application.Current — correct pattern for XAML tests needing MockApplication ✅
• The automated script flagged: "Issue test file name Maui34716.xaml doesn't follow MauiXXXXX pattern" — this is a false positive. Maui34716 correctly follows MauiXXXXX (issue SourceGen: VisualStateManager.VisualStateGroups causes 'Names must be unique' at startup #34716 referenced in the PR title). ✅

---

5. Flakiness Risk — ✅ Low

Unit and XAML tests with no async code, no timing, no Appium, no platform APIs. No flakiness risk.

---

6. Duplicate Coverage — ✅ No duplicates

The previous test GroupNamesMustBeUniqueWithinGroupList was updated to reflect the new behavior rather than creating a new redundant test. The XAML tests cover distinct scenarios without overlap.

---

7. Platform Scope — ✅

The fix is in cross-platform VisualStateManager.cs. XAML tests run for all three inflators (Runtime, XamlC, SourceGen), covering all code paths that inflate XAML on any platform. No platform-specific code was changed, so device tests aren't required.

---

8. Assertion Quality — ✅

Assertions are specific and regression-catching:

Assertion	Quality
Assert.Single(vsgs)	✅ Exact count — would catch both "threw exception" and "added duplicate"
Assert.Same(secondGroup, vsgs[0])	✅ Identity check — verifies new group replaced the old one
Assert.Equal("CommonStates", groups[0].Name)	✅ Correct name after replacement
Assert.Equal(2, groups[0].States.Count)	✅ Correct states from the explicit XAML group (not the implicit style's 3 states)

The state count check in VisualStateGroupsWithImplicitStyleShouldNotThrowDuplicateNames is particularly good: the implicit style has 3 states (Normal, Pressed, Disabled) while the explicit XAML has 2 (Normal, Pressed). Assert.Equal(2, ...) proves the explicit group won, not the style's group.

---

9. Fix-Test Alignment — ✅

The fix is in VisualStateGroupList.Add() in VisualStateManager.cs. The unit test calls Add() directly. The XAML tests inflate XAML that triggers Add() via all three inflators. Perfect alignment.

---

Recommendations

1. (Optional) Add null-name edge case test: Add a unit test clarifying what happens when two groups with Name = null (or empty) are added. The current fix skips dedup for null names — making this contract explicit would prevent future confusion.
2. (Optional) Assert group position after replacement: Add a test verifying that when a duplicate replaces an existing group in a multi-group list, the resulting order is deterministic (new group at end, or in original position).

These are minor improvements — the existing tests are sufficient to prevent regressions for the reported bug.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• dc.services.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dc.services.visualstudio.com"

See Network Configuration for more information.

Note

🔒 Integrity filtering filtered 1 item

Integrity filtering activated and filtered the following item during workflow execution.
This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.

• pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719 (pull_request_read: Resource 'pr:Fix VisualStateGroups duplicate name crash with implicit styles (#34716) #34719' has lower integrity than agent requires. Agent would need to drop integrity tags [unapproved:all approved:all] to trust this resource.)

🧪 Test evaluation by Evaluate PR Tests

[kubaflo]

Code Review Summary

Fix VisualStateGroups duplicate name crash with implicit styles — Changes VisualStateGroupList.Add() to replace existing groups with the same name instead of throwing InvalidOperationException. Fixes the crash when a Button has explicit VisualStateGroups in XAML and an implicit style also sets groups with "CommonStates".

Findings

Severity	Finding
✅ Good	Fix is correct — removes duplicate before Validate() fires via WatchAddList callback, so no exception
✅ Good	Event lifecycle correct — StatesChanged properly unsubscribed on the removed group
✅ Good	5 XAML inflator tests (Runtime/XamlC/SourceGen) + 1 core unit test covering the exact bug scenario
✅ Good	No public API changes, main branch target correct for bug fix
✅ Good	Behavioral change (throw → replace) is well-justified — old behavior was always unrecoverable
🟡 Minor	_internalList.Remove(_internalList[i]) could be RemoveAt(i) to skip the redundant linear scan (micro-optimization)

Overall Assessment

✅ Looks good — clean, minimal, well-tested fix. The "last-writer-wins" replacement semantics are the right approach for a scenario that was always a crash.

---

Review performed by Copilot CLI