Fix VisualElement.ChangeVisualState() gets stuck in Selected state#35421
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35421Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35421" |
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/backport to release/10.0.1xx-sr7 |
|
Started backporting to |
|
/review |
|
/review -b feature/regression-check -p android |
1 similar comment
|
/review -b feature/regression-check -p android |
The GitHub Review API rejects comments with null body fields. The expert-reviewer agent sometimes writes findings where the body field is null/missing. Add defensive fallback: try body, then message, then content field, defaulting to '(no description)' if all are null. This fixes the '422 For properties/body, nil is not a string' error seen in pipeline run 14125163 for PR #35421. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/regression-check -p android |
MauiBot
left a comment
There was a problem hiding this comment.
🤖 Automated review — alternative fix proposed
The expert-reviewer evaluation compared the PR fix against #1 automatically generated candidates and selected try-fix-1 as the strongest fix.
Why: try-fix-1 fixes #35399 with the same correctness as the PR and preserves the #29815 Selected-state invariants, while reducing the diff to 5 files (vs 19) with zero platform-handler churn. Its auto-sync inside VisualStateManager.GoToState keeps legacy GoToState("Selected") callers (third-party CollectionView item subclasses, custom renderers) participating in framework re-imposition, eliminating the silent-break hazard present in the PR.
Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate.
Candidate diff (`try-fix-1`)
diff --git a/src/Controls/src/Core/VisualElement/VisualElement.cs b/src/Controls/src/Core/VisualElement/VisualElement.cs
index 27503d9fdd..65be364bae 100644
--- a/src/Controls/src/Core/VisualElement/VisualElement.cs
+++ b/src/Controls/src/Core/VisualElement/VisualElement.cs
@@ -1650,10 +1650,40 @@ namespace Microsoft.Maui.Controls
void OnFocused() => Focused?.Invoke(this, new FocusEventArgs(this, true));
- internal void ChangeVisualStateInternal() => ChangeVisualState();
+ // Re-entrancy gate: incremented around every framework-initiated re-evaluation
+ // of ChangeVisualState (IsEnabled / IsFocused / IsPointerOver changes, VSM groups
+ // (re)assigned, state-trigger fired). Lets the base ChangeVisualState tell
+ // "MAUI is re-evaluating because some unrelated input flipped" apart from
+ // "a derived class is delegating to base.ChangeVisualState() from its own
+ // override". The former must preserve Selected priority (#29815); the latter
+ // must not (#35399).
+ [ThreadStatic]
+ static int s_changeVisualStateDepth;
+ internal static bool IsInFrameworkChangeVisualState => s_changeVisualStateDepth > 0;
+
+ internal void ChangeVisualStateInternal()
+ {
+ s_changeVisualStateDepth++;
+ try
+ {
+ ChangeVisualState();
+ }
+ finally
+ {
+ s_changeVisualStateDepth--;
+ }
+ }
bool _isPointerOver;
+ // Authoritative "this element is currently in Selected state" flag.
+ // Populated automatically by VisualStateManager.GoToState when an external
+ // (non-re-entrant) caller transitions the element to Selected / Normal /
+ // PointerOver. Read by IsElementInSelectedState() so that framework-driven
+ // re-evaluations preserve Selected without consulting the (potentially stale)
+ // VisualStateGroup.CurrentState.
+ internal bool _isItemSelected;
+
internal bool IsPointerOver
{
get { return _isPointerOver; }
@@ -1666,7 +1696,7 @@ namespace Microsoft.Maui.Controls
_isPointerOver = value;
if (callChangeVisualState)
- ChangeVisualState();
+ ChangeVisualStateInternal();
}
/// <summary>
@@ -1680,7 +1710,12 @@ namespace Microsoft.Maui.Controls
}
else
{
- bool isSelected = this.IsElementInSelectedState();
+ // Honor the Selected-priority (#29815) only when MAUI itself is re-evaluating
+ // state (pointer / focus / IsEnabled toggled, state groups changed). When a
+ // derived class calls base.ChangeVisualState() from its own override, the
+ // override is the authority on selection — base must not re-impose Selected
+ // from a stale flag (#35399).
+ bool isSelected = IsInFrameworkChangeVisualState && this.IsElementInSelectedState();
string targetState = isSelected ? VisualStateManager.CommonStates.Selected
: (IsPointerOver ? VisualStateManager.CommonStates.PointerOver : VisualStateManager.CommonStates.Normal);
@@ -1748,7 +1783,7 @@ namespace Microsoft.Maui.Controls
if (element == null)
return;
- element.ChangeVisualState();
+ element.ChangeVisualStateInternal();
(bindable as IPropertyPropagationController)?.PropagatePropertyChanged(VisualElement.IsEnabledProperty.PropertyName);
}
@@ -1788,7 +1823,7 @@ namespace Microsoft.Maui.Controls
element.OnUnfocus();
}
- element.ChangeVisualState();
+ element.ChangeVisualStateInternal();
}
static void OnRequestChanged(BindableObject bindable, object oldvalue, object newvalue)
diff --git a/src/Controls/src/Core/VisualStateManager.cs b/src/Controls/src/Core/VisualStateManager.cs
index 6c74249d29..5a2ce12c0b 100644
--- a/src/Controls/src/Core/VisualStateManager.cs
+++ b/src/Controls/src/Core/VisualStateManager.cs
@@ -59,7 +59,11 @@ namespace Microsoft.Maui.Controls
if (newValue != null)
((VisualStateGroupList)newValue).VisualElement = visualElement;
- visualElement.ChangeVisualState();
+ // New VSM groups own their own initial state — drop the cached
+ // selection flag so we don't bleed Selected across style swaps.
+ visualElement._isItemSelected = false;
+
+ visualElement.ChangeVisualStateInternal();
UpdateStateTriggers(visualElement);
}
@@ -88,6 +92,28 @@ namespace Microsoft.Maui.Controls
/// <returns><see langword="true"/> if the transition was successful; otherwise, <see langword="false"/>.</returns>
public static bool GoToState(VisualElement visualElement, string name)
{
+ // Auto-sync the authoritative "is selected" flag whenever an *external*
+ // caller transitions the element through one of the mutually-exclusive
+ // item states (Selected vs Normal vs PointerOver). This lets every existing
+ // platform handler / Shell-flyout / IndicatorView site / third-party renderer
+ // keep calling GoToState(view, "Selected"|"Normal") unchanged while still
+ // feeding IsElementInSelectedState().
+ //
+ // Skip when re-entering from inside ChangeVisualState — that path is
+ // re-applying a derived state and must not oscillate the flag.
+ if (!VisualElement.IsInFrameworkChangeVisualState)
+ {
+ if (name == CommonStates.Selected)
+ {
+ visualElement._isItemSelected = true;
+ }
+ else if (name == CommonStates.Normal || name == CommonStates.PointerOver)
+ {
+ visualElement._isItemSelected = false;
+ }
+ // Disabled / Focused / Unfocused / Pressed / custom states leave the flag intact.
+ }
+
var context = visualElement.GetContext(VisualStateGroupsProperty);
if (context is null)
{
@@ -403,7 +429,7 @@ namespace Microsoft.Maui.Controls
void OnStatesChanged()
{
- VisualElement?.ChangeVisualState();
+ VisualElement?.ChangeVisualStateInternal();
}
public override bool Equals(object obj) => Equals(obj as VisualStateGroupList);
@@ -816,16 +842,7 @@ namespace Microsoft.Maui.Controls
internal static bool IsElementInSelectedState(this VisualElement element)
{
- var groups = VisualStateManager.GetVisualStateGroups(element);
- foreach (var group in groups)
- {
- if (group.CurrentState?.Name == VisualStateManager.CommonStates.Selected)
- {
- return true;
- }
- }
-
- return false;
+ return element._isItemSelected;
}
}
diff --git a/src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs b/src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs
index 09e74cdb3c..b73690938b 100644
--- a/src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs
+++ b/src/Controls/tests/Core.UnitTests/VisualStateManagerTests.cs
@@ -646,5 +646,164 @@ namespace Microsoft.Maui.Controls.Core.UnitTests
VisualStateManager.GoToState(button, customStateName);
Assert.Equal(localColor, button.BackgroundColor);
}
+
+ [Fact]
+ // https://github.com/dotnet/maui/issues/35399
+ public void SelectHoverDeselectRestoresPointerOverState()
+ {
+ var element = new Label();
+ var groups = CreateStateGroupsWithSelectedAndPointerOver();
+ VisualStateManager.SetVisualStateGroups(element, groups);
+
+ // 1. Select the item (simulates CollectionView selection)
+ VisualStateManager.GoToState(element, VisualStateManager.CommonStates.Selected);
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+
+ // 2. Simulate pointer hover while selected — Selected takes priority
+ SetIsPointerOver(element, true);
+ element.ChangeVisualStateInternal();
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+
+ // 3. Deselect while pointer is still hovering — should restore to PointerOver
+ VisualStateManager.GoToState(element, VisualStateManager.CommonStates.Normal);
+ element.ChangeVisualStateInternal();
+ Assert.Equal(VisualStateManager.CommonStates.PointerOver, groups[0].CurrentState.Name);
+ }
+
+ [Fact]
+ // https://github.com/dotnet/maui/issues/35399
+ public void SelectedStatePreservedAcrossMouseHover()
+ {
+ var element = new Label();
+ var groups = CreateStateGroupsWithSelectedAndPointerOver();
+ VisualStateManager.SetVisualStateGroups(element, groups);
+
+ // Select the item (simulates Shell flyout item selection)
+ VisualStateManager.GoToState(element, VisualStateManager.CommonStates.Selected);
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+
+ // Pointer enters — Selected should be preserved
+ SetIsPointerOver(element, true);
+ element.ChangeVisualStateInternal();
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+
+ // Pointer exits — Selected should still be preserved
+ SetIsPointerOver(element, false);
+ element.ChangeVisualStateInternal();
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+ }
+
+ [Fact]
+ // https://github.com/dotnet/maui/issues/35399
+ public void IsEnabledToggleWhileSelectedPreservesState()
+ {
+ var element = new Label();
+ var groups = CreateStateGroupsWithSelectedAndPointerOver();
+ VisualStateManager.SetVisualStateGroups(element, groups);
+
+ // Select the item
+ VisualStateManager.GoToState(element, VisualStateManager.CommonStates.Selected);
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+
+ // Disable — should go to Disabled
+ element.IsEnabled = false;
+ Assert.Equal(DisabledStateName, groups[0].CurrentState.Name);
+ Assert.True(element.IsElementInSelectedState()); // selection flag preserved
+
+ // Re-enable — should restore to Selected since the flag is still true
+ element.IsEnabled = true;
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+ }
+
+ [Fact]
+ // https://github.com/dotnet/maui/issues/35399
+ public void SelectingWhileDisabledStaysDisabledUntilReEnabled()
+ {
+ var element = new Label();
+ var groups = CreateStateGroupsWithSelectedAndPointerOver();
+ VisualStateManager.SetVisualStateGroups(element, groups);
+
+ // Disable first
+ element.IsEnabled = false;
+ Assert.Equal(DisabledStateName, groups[0].CurrentState.Name);
+
+ // "Select" while disabled — GoToState("Selected") sets the flag but the
+ // short-circuit in GoToState detects we're already in a different state and
+ // leaves CurrentState alone — so visual state remains Disabled.
+ VisualStateManager.GoToState(element, VisualStateManager.CommonStates.Selected);
+ Assert.True(element.IsElementInSelectedState());
+
+ // Re-enable — now it should go to Selected because the flag was preserved
+ element.IsEnabled = true;
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+ }
+
+ [Fact]
+ // https://github.com/dotnet/maui/issues/35399
+ // Regression: a derived class that overrides ChangeVisualState and calls
+ // base.ChangeVisualState() from its deselect branch must transition to Normal,
+ // even when an earlier GoToState("Selected") left the flag set.
+ public void BaseChangeVisualStateFromOverride_GoesToNormalEvenWhenFlagIsSet()
+ {
+ var element = new SelectableTestElement();
+ var groups = CreateStateGroupsWithSelectedAndPointerOver();
+ VisualStateManager.SetVisualStateGroups(element, groups);
+
+ // Select via the override's "Selected" path
+ element.IsSelected = true;
+ Assert.Equal(VisualStateManager.CommonStates.Selected, groups[0].CurrentState.Name);
+ Assert.True(element.IsElementInSelectedState());
+
+ // Deselect: override calls base.ChangeVisualState() — must drop to Normal
+ element.IsSelected = false;
+ Assert.Equal(VisualStateManager.CommonStates.Normal, groups[0].CurrentState.Name);
+ Assert.False(element.IsElementInSelectedState());
+ }
+
+ // Mirrors the SelectableBox custom control from Issue #35399's reproducer.
+ class SelectableTestElement : Label
+ {
+ public static readonly BindableProperty IsSelectedProperty = BindableProperty.Create(
+ nameof(IsSelected), typeof(bool), typeof(SelectableTestElement), false,
+ propertyChanged: (b, _, _) => ((SelectableTestElement)b).ChangeVisualState());
+
+ public bool IsSelected
+ {
+ get => (bool)GetValue(IsSelectedProperty);
+ set => SetValue(IsSelectedProperty, value);
+ }
+
+ protected internal override void ChangeVisualState()
+ {
+ if (IsSelected && IsEnabled)
+ VisualStateManager.GoToState(this, VisualStateManager.CommonStates.Selected);
+ else
+ base.ChangeVisualState();
+ }
+ }
+
+ static VisualStateGroupList CreateStateGroupsWithSelectedAndPointerOver()
+ {
+ return new VisualStateGroupList
+ {
+ new VisualStateGroup
+ {
+ Name = CommonStatesGroupName,
+ States =
+ {
+ new VisualState { Name = NormalStateName },
+ new VisualState { Name = VisualStateManager.CommonStates.Selected },
+ new VisualState { Name = VisualStateManager.CommonStates.PointerOver },
+ new VisualState { Name = DisabledStateName },
+ }
+ }
+ };
+ }
+
+ static void SetIsPointerOver(VisualElement element, bool value)
+ {
+ var field = typeof(VisualElement).GetField("_isPointerOver", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic);
+ field!.SetValue(element, value);
+ }
}
}
kubaflo
left a comment
There was a problem hiding this comment.
Could you please try the ai's suggestions?
…ck in Selected state (#35447) Backport of #35421 to release/10.0.1xx-sr7 /cc @PureWeen @Dhivya-SF4094 --------- Co-authored-by: Dhivya-SF4094 <127717131+Dhivya-SF4094@users.noreply.github.com>
|
/review -b feature/regression-check -p android |
The GitHub Review API rejects comments with null body fields. The expert-reviewer agent sometimes writes findings where the body field is null/missing. Add defensive fallback: try body, then message, then content field, defaulting to '(no description)' if all are null. This fixes the '422 For properties/body, nil is not a string' error seen in pipeline run 14125163 for PR #35421. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review -b feature/regression-check |
|
/review -b feature/refactor-copilot-yml |
|
/review -b feature/refactor-copilot-yml |
…35421) <!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue Details: When a custom control overrides ChangeVisualState() and applies the Selected state manually, calling base.ChangeVisualState()after the control is deselected causes the element to remain permanently stuck in the Selected visual state. ### Root Cause: In .NET 10.0.60, a fix for #29815 modified ChangeVisualState() to check whether the element is currently in the "Selected" VSM state before transitioning to PointerOver or Normal. This check was implemented by reading VisualStateGroup.CurrentState?.Name via IsElementInSelectedState() — creating a circular dependency. During a deselect, CurrentState is still "Selected" (it hasn't been cleared yet when ChangeVisualState() calls IsElementInSelectedState()). So isSelected = true → "Selected" is re-applied → element is stuck. ### Description of change: - VisualElement.IsItemSelected (internal property) — has an equality guard to avoid redundant recomputation and routes through ChangeVisualState() so that state priorities (Disabled > Selected > PointerOver > Normal) are always respected. - VisualStateManager.IsElementInSelectedState() — now simply returns element.IsItemSelected instead of reading CurrentState. ### Validated the behaviour in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Fixes Fixes #35399 ### Screenshots | Before | After | |---------|--------| | <video src="https://github.com/user-attachments/assets/0fe16315-5561-4b08-92bc-094b673579a9"> | <video src="https://github.com/user-attachments/assets/52a0bc40-04f3-4e1f-b314-9726ae883f08"> |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details:
When a custom control overrides ChangeVisualState() and applies the Selected state manually, calling base.ChangeVisualState()after the control is deselected causes the element to remain permanently stuck in the Selected visual state.
Root Cause:
In .NET 10.0.60, a fix for #29815 modified ChangeVisualState() to check whether the element is currently in the "Selected" VSM state before transitioning to PointerOver or Normal. This check was implemented by reading VisualStateGroup.CurrentState?.Name via IsElementInSelectedState() — creating a circular dependency.
During a deselect, CurrentState is still "Selected" (it hasn't been cleared yet when ChangeVisualState() calls IsElementInSelectedState()). So isSelected = true → "Selected" is re-applied → element is stuck.
Description of change:
Validated the behaviour in the following platforms
Fixes
Fixes #35399
Screenshots
35399_BeforeFix.mov
35399_AfterFix.mov