Skip to content

Improve VisualState order and prevent sticky Focused visual state#27477

Open
mattleibow wants to merge 9 commits intomainfrom
dev/focus-visual-states
Open

Improve VisualState order and prevent sticky Focused visual state#27477
mattleibow wants to merge 9 commits intomainfrom
dev/focus-visual-states

Conversation

@mattleibow
Copy link
Copy Markdown
Member

@mattleibow mattleibow commented Jan 30, 2025

Description of Change

Alternative to #19752

While reviewing @MartyIX's PR #19812 I discovered that the code for switching to the Focused and Unfocused states was all dependent on the control being enabled. What this results in that if you have a focused button and the visual state was some sort of border, disabling the button will not actually switch to unfocused and the visual state will remain with the border that was added when it got focused.

This PR originally copied the code logic from WinUI: https://github.com/microsoft/microsoft-ui-xaml/blob/ffe33f9b7d0e9f5a2ca3330d0ce329f09dff092b/src/dxaml/xcp/dxaml/lib/Button_Partial.cpp#L29-L60 but I have updated it to follow maybe a better visual state order. This new way is to make sure the unfocus happens first and the pointer over happens last.

For the issue in #19752, the actual reason things are wrong is not because the states are set wrong, but rather because the focus states are in the same group as the pointer over state. This means that the button can either be focused or be pointer over.

The correct way to have all these states working is to use multiple groups:

<VisualStateManager.VisualStateGroups>
  <VisualStateGroupList>
    <VisualStateGroup x:Name="CommonStates">
      <VisualState x:Name="Normal" />
      <VisualState x:Name="PointerOver" />
      <VisualState x:Name="Pressed" />
      <VisualState x:Name="Disabled" />
    </VisualStateGroup>
    <VisualStateGroup x:Name="FocusStates">
      <VisualState x:Name="Focused" />
      <VisualState x:Name="Unfocused" />
    </VisualStateGroup>
  </VisualStateGroupList>
</VisualStateManager.VisualStateGroups>

This can also be seen in other controls such as the WinUI combo box (the Button does not use a state but rather the OS focus border): https://github.com/microsoft/microsoft-ui-xaml/blob/ffe33f9b7d0e9f5a2ca3330d0ce329f09dff092b/src/controls/dev/ComboBox/ComboBox_themeresources.xaml#L472 It is also in the docs: https://learn.microsoft.com/en-us/uwp/api/windows.ui.xaml.controls.control.usesystemfocusvisuals?view=winrt-26100#examples

This is the docs for WinUI to do focus states:

To define custom focus visuals for a control, you need to provide a custom ControlTemplate. In the ControlTemplate, do the following:

  • If you're modifying a default ControlTemplate, be sure to set the UseSystemFocusVisuals property to false to turn off the system focus visuals. When set to false, the focus states in the VisualStateManager are called.
  • Define a VisualStateGroup for FocusStates.
  • In the FocusStates group, define VisualStates for Focused, Unfocused, and PointerFocused.
  • Define the focus visuals.

Another result of not having multiple groups is that sometimes unexpected things happen. If you are missing the focus states, then nothing happens when you change states. And, if you have the focus states in the same group as normal, the normal state will never apply since it will either be focused or unfocused and normal will be overwritten.

Issues Fixed

I was not able to find an open issue with the focus states "sticking" when disabling. And the issues that I have seen are just VSM improperly configured.

Maybe these:

Copilot AI review requested due to automatic review settings January 30, 2025 14:45
@mattleibow mattleibow requested a review from a team as a code owner January 30, 2025 14:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

Files not reviewed (2)
  • src/Controls/tests/TestCases.HostApp/Issues/Issue19752.xaml: Language not supported
  • src/Controls/src/Core/VisualElement/VisualElement.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)

src/TestUtils/src/UITest.Appium/Actions/AppiumMouseActions.cs:221

  • Ensure that the new MoveCursor and MoveCursorCoordinates commands are covered by tests.
CommandResponse MoveCursor(IDictionary<string, object> parameters)

src/TestUtils/src/UITest.Appium/HelperExtensions.cs:2282

  • Ensure that the new MoveCursor methods are covered by tests.
public static void MoveCursor(this IApp app, string element)

@mattleibow
Copy link
Copy Markdown
Member Author

@MartyIX had some wise words:

I just wonder what the precedence rules are for these two style groups. I mean I want "pointer-over" state to have higher precedence than "focused" state. Does it depend on the order here https://github.com/dotnet/maui/pull/27477/files#diff-f9f36395cbe8fbe5db0fb42d9eeda70e070ed5ced099553747d1236d4f05117fR16-R54 ?

@mattleibow
Copy link
Copy Markdown
Member Author

mattleibow commented Jan 30, 2025

Thanks for the wise words @MartyIX, maybe this is a better order:

var shouldFocus = IsFocused && IsEnabled;
			
// 1. unfocus first
if (!shouldFocus)
	VisualStateManager.GoToState(this, VisualStateManager.FocusStates.Unfocused);

// 2. set basic states (normal/disabled)
if (!IsEnabled)
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.Disabled);
else if (!IsPointerOver)
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.Normal);

// 3. focus
if (shouldFocus)
	VisualStateManager.GoToState(this, VisualStateManager.FocusStates.Focused);

// 4. end with pointer over
if (IsPointerOver)
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);

This is different to UWP/WPF/WinUI, so it may be better or it may cause people to get surprised coming from another XAML framework:

// 1. set basic states (normal/disabled/pointer over)
if (!IsEnabled)
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.Disabled);
else if (IsPointerOver)
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);
else
	VisualStateManager.GoToState(this, VisualStateManager.CommonStates.Normal);

// 2. override with focus
if (IsFocused && IsEnabled)
	VisualStateManager.GoToState(this, VisualStateManager.FocusStates.Focused);
else
	VisualStateManager.GoToState(this, VisualStateManager.FocusStates.Unfocused);

Any thoughts?

@mattleibow mattleibow added this to the .NET 9 SR4 milestone Jan 30, 2025
@mattleibow mattleibow added the area-xaml XAML, CSS, Triggers, Behaviors label Jan 30, 2025
@mattleibow mattleibow changed the title Always apply the Unfocued visual state when the element loses focus Improve VisualState order and prevent sticky focus Jan 30, 2025
@mattleibow mattleibow changed the title Improve VisualState order and prevent sticky focus Improve VisualState order and prevent sticky Focused visual state Jan 30, 2025
@MartyIX
Copy link
Copy Markdown
Contributor

MartyIX commented Jan 30, 2025

This is different to UWP/WPF/WinUI, so it may be better or it may cause people to get surprised coming from another XAML framework.

Could you explain how it is different exactly? I don't know the frameworks in detail.

Copy link
Copy Markdown
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

Need to verify some related UITests checking the focused VisualState etc.
image

Example:
DisablingUnfocusedButtonMovesToDisabledState

Assert.That(App.FindElement("button2").GetText(), Is.EqualTo("Disabled"))
Expected string length 8 but was 11. Strings differ at index 0.
Expected: "Disabled"
But was:  "PointerOver"

@mattleibow
Copy link
Copy Markdown
Member Author

Could you explain how it is different exactly? I don't know the frameworks in detail.

@MartyIX I updated the comment with the WinUI way so it can be seen side-by-side

@MartyIX
Copy link
Copy Markdown
Contributor

MartyIX commented Jan 31, 2025

It looks good to me. :-)

I still wonder though how will one implement styling for a button like this:

  • red ~ the button is focused and the pointer is over the button (i.e. Focused && PointerOver styles at the same time)
  • green ~ the button is just focused
  • blue ~ pointer is over the button

I think that one can make it somehow work with triggers (doc). But not with visual styles (doc). Is that right?

It's not like the scenario is super-useful. The question is more about API design and perhaps even for user-defined visual styles and their composition.

@PureWeen
Copy link
Copy Markdown
Member

PureWeen commented Mar 2, 2025

/rebase

@PureWeen PureWeen moved this from Changes Requested to Ready To Review in MAUI SDK Ongoing Mar 2, 2025
@github-actions github-actions bot force-pushed the dev/focus-visual-states branch from 4e36d9f to a1b0d56 Compare March 2, 2025 22:12
@mattleibow
Copy link
Copy Markdown
Member Author

/rebase

@github-actions github-actions bot force-pushed the dev/focus-visual-states branch from a1b0d56 to 30161dc Compare March 3, 2025 18:51
Copy link
Copy Markdown
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

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

image This failing tests are related with VisualStates, could you verify if are related with the changes?

@github-project-automation github-project-automation bot moved this from Ready To Review to Changes Requested in MAUI SDK Ongoing Mar 4, 2025
@PureWeen PureWeen modified the milestones: .NET 9 SR8, .NET 9 SR9 Jun 2, 2025
@MitchBomcanhao
Copy link
Copy Markdown

this seems like it could fix some issues we've got. please look to release it

@PureWeen PureWeen modified the milestones: .NET 9 SR9, .NET 9 SR10 Jul 3, 2025
@PureWeen PureWeen modified the milestones: .NET 9 SR10, .NET 9 SR12 Aug 4, 2025
@PureWeen PureWeen modified the milestones: .NET 9 SR12, .NET 10 SR1 Sep 10, 2025
@PureWeen PureWeen modified the milestones: .NET 10 SR1, .NET 10.0 SR2 Nov 4, 2025
@PureWeen PureWeen modified the milestones: .NET 10.0 SR2, .NET 10 SR4 Dec 13, 2025
@PureWeen PureWeen modified the milestones: .NET 10.0 SR4, .NET 10 SR5 Jan 21, 2026
@PureWeen PureWeen modified the milestones: .NET 10 SR5, .NET 10 SR6 Mar 3, 2026
@kubaflo
Copy link
Copy Markdown
Contributor

kubaflo commented Mar 5, 2026

🤖 AI Summary

📊 Expand Full Review
🔍 Pre-Flight — Context & Validation
📝 Review SessionTry and order the state changes better · dd79425

Issue: #19752 - Button does not behave properly when pointer hovers over the button because it's in focused state
PR: #27477 - Improve VisualState order and prevent sticky Focused visual state
Author: mattleibow
Platforms Affected: Windows (primary - pointer hover), cross-platform (VisualState focus ordering)
Labels: area-xaml
Files Changed: 2 fix files, 3 test files, 1 UI test helper file, 1 csproj

Issue Summary

On Windows, when a Button has keyboard focus and the mouse pointer moves over it, the PointerOver state was not applied because the Focused state (in the same CommonStates group) blocked it. The root cause was that the visual state group had Focused and PointerOver in the same group (CommonStates), and the code checked IsFocused to set state — overriding PointerOver when focus was present.

Fix Description

The PR restructures ChangeVisualState() in VisualElement.cs to apply states in a specific order:

  1. Unfocus first - if !IsFocused || !IsEnabled, go to FocusStates.Unfocused
  2. Set Disabled or Normal based on IsEnabled / IsPointerOver
  3. Apply Focused if IsFocused && IsEnabled
  4. Apply PointerOver LAST — so it can override all other states (incl. Focused)

Also introduces a new FocusStates inner class in VisualStateManager (currently internal, marked for .NET 10 promotion to public) with Focused and Unfocused constants.

Key Reviewer Feedback & Disagreements

Reviewer Finding Status
jsuarezruiz DisablingFocusedButtonMovesToDisabledState fails (gets "PointerOver" instead of "Disabled") ⚠️ UNRESOLVED
jsuarezruiz DisablingUnfocusedButtonMovesToDisabledState fails (gets "PointerOver") ⚠️ UNRESOLVED
rmarinho Code logic looks fine, but UI tests are failing ⚠️ UNRESOLVED
MartyIX Inline suggestions for comment improvements in test code Minor/cosmetic

Root Cause of Reviewer-Flagged Failures

The PR's fix applies PointerOver unconditionally at the end, even when !IsEnabled:

if (IsPointerOver)
    VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);

When Appium taps a button, the mouse cursor ends up hovering over it. When that button is then disabled (via click handler), ChangeVisualState() runs: sets Disabled, but then IsPointerOver is still true, so PointerOver overrides Disabled. This is the bug causing test failures.

Files Classification

  • Fix files: src/Controls/src/Core/VisualElement/VisualElement.cs (+27/-11), src/Controls/src/Core/VisualStateManager.cs (+8/-2)
  • Test files: TestCases.HostApp/Issues/Issue19752.xaml (+67), TestCases.HostApp/Issues/Issue19752.xaml.cs (+29), TestCases.Shared.Tests/Tests/Issues/Issue19752.cs (+125)
  • Helper: UITest.Appium/Actions/AppiumMouseActions.cs (+133/-2), UITest.Appium/HelperExtensions.cs (+58)
  • Minor: Sandbox MainPage.xaml.cs (whitespace), UITest.Appium.csproj (+1)

Test Type

UI Tests (Appium, pointer/focus interaction). Tests heavily use App.MoveCursor() - pointer-only operations not available on iOS.

Note: Issue is labeled platform/windows. Tests use pointer hover actions (MoveCursor) which are not available on iOS. Selected platform (iOS) may yield inconclusive Gate results for pointer-specific tests.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #27477 Reorder ChangeVisualState: unfocus → disabled/normal → focused → PointerOver last ⏳ PENDING (Gate) VisualElement.cs (+27/-11), VisualStateManager.cs (+8/-2) Original PR — reviewers flagging test failures suggesting PointerOver overrides Disabled

🚦 Gate — Test Verification
📝 Review SessionTry and order the state changes better · dd79425

Result: ❌ FAILED
Platform: ios
Mode: Full Verification (RequireFullVerification)

Check Expected Actual Result
Tests WITHOUT fix FAIL FAIL
Tests WITH fix PASS FAIL

Test Results WITH Fix (7 tests, 4 pass / 3 fail)

Test Result Failure Reason
InitialStateAreAllCorrect ✅ PASS
EnablingButtonMovesToNormalState ✅ PASS
HoveringOverButtonAndThenMovingOffMovesToNormalState ✅ PASS
DisablingFocusedButtonMovesToDisabledState ✅ PASS
HoveringOverButtonMovesToPointerOverState ❌ FAIL iOS has no pointer/hover support — MoveCursor not supported
PressingAndReleasingButtonMovesToPointerOverState ❌ FAIL iOS has no PointerOver after tap; expected "PointerOver" but was "Normal"
DisablingUnfocusedButtonMovesToDisabledState ❌ FAIL Expected button3 to be "Focused" after disabling button2, but was "Normal" — iOS focus doesn't auto-transfer

Analysis of Failures

2 Platform-mismatch failures (HoveringOverButtonMovesToPointerOverState, PressingAndReleasingButtonMovesToPointerOverState): These tests require pointer/mouse hover operations not available on iOS. Expected on iOS testing.

1 Functional failure (DisablingUnfocusedButtonMovesToDisabledState): Expects focus to automatically move to button3 when button2 is disabled. This focus-transfer behavior may be Windows-specific. On iOS, focus does not auto-transfer to the next element when a focused/sibling element is disabled.

Context: Windows Reviewer Feedback

Reviewers jsuarezruiz and rmarinho tested on Windows and also found failing tests:

  • DisablingFocusedButtonMovesToDisabledState: got "PointerOver" instead of "Disabled"
  • DisablingUnfocusedButtonMovesToDisabledState: got "PointerOver" instead of "Disabled"

The Windows failures are caused by a different issue: the PR applies PointerOver unconditionally at the end of ChangeVisualState() (even when !IsEnabled), so after Appium taps to disable a button, the cursor hovers over it and PointerOver overrides Disabled.

Conclusion

Gate ❌ FAILED. Both iOS testing and Windows reviewer feedback confirm tests don't pass with the fix. The PR needs changes before approval.


🔧 Fix — Analysis & Comparison
📝 Review SessionTry and order the state changes better · dd79425

Skipped: Gate phase FAILED — Fix phase not executed.

The Gate failed because tests don't pass with the PR's fix (confirmed both by iOS test run and Windows reviewer feedback). Per the workflow, Fix phase is skipped when Gate fails.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #27477 Reorder ChangeVisualState: unfocus → disabled/normal → focused → PointerOver last ❌ FAIL (Gate) VisualElement.cs (+27/-11), VisualStateManager.cs (+8/-2) Original PR — tests fail with fix on both iOS and Windows

Exhausted: N/A (skipped due to Gate failure)
Selected Fix: N/A


📋 Report — Final Recommendation
📝 Review SessionTry and order the state changes better · dd79425

⚠️ Final Recommendation: REQUEST CHANGES

Summary

PR #27477 improves the visual state ordering in ChangeVisualState() to fix the "sticky Focused state" problem when a control is focused and pointer hovers over it. The architecture (applying states in order: unfocus → disabled/normal → focused → PointerOver last) is directionally correct and the new FocusStates class in VisualStateManager is a good addition.

However, the implementation has a critical bug that causes included tests to fail on Windows (confirmed by reviewers jsuarezruiz and rmarinho) and on iOS (confirmed by Gate). The PointerOver state is applied unconditionally at the end of ChangeVisualState(), even when the control is disabled, causing PointerOver to override Disabled.

Gate Status

FAILED — Tests fail with fix on iOS (4/7 pass, 3 fail). Windows reviewers also confirm test failures.

Root Cause of Bug in the PR

The original issue (#19752): When a button is focused, PointerOver state doesn't apply because Focused and PointerOver were in the same VSM group (CommonStates) — setting one overwrites the other.

The PR's fix: Moves Focused/Unfocused to a separate FocusStates group and applies states in a specific order, with PointerOver last.

The bug introduced: The final PointerOver block is unconditional:

// Current (broken) code in VisualElement.cs:
if (IsPointerOver)
{
    VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);
}

When Appium (or a user) clicks to disable a button, the cursor remains over the button. IsPointerOver is still true, so PointerOver overrides Disabled. Expected result: "Disabled". Actual result: "PointerOver".

Required Fix

The PointerOver state should NOT be applied when the control is disabled:

// Option A: Guard with IsEnabled
if (IsPointerOver && IsEnabled)
{
    VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);
}

Or alternatively, clear IsPointerOver when the control is disabled:

// Option B: In SetPointerOver or OnIsEnabledPropertyChanged
// When IsEnabled → false, call SetPointerOver(false, callChangeVisualState: false)
// before calling ChangeVisualState

Option A is simpler and more clearly expresses the intent.

Code Review Findings

🔴 Critical Issue: PointerOver Overrides Disabled State

File: src/Controls/src/Core/VisualElement/VisualElement.cs (~line 1704)

// Bug: PointerOver applied even when !IsEnabled
if (IsPointerOver)
{
    VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);
}

Problem: A disabled control should not visually indicate PointerOver. On Windows (and macOS Catalyst), pointer events can still trigger during mouse interactions (e.g., tapping/clicking leaves cursor over the control), causing PointerOver to override Disabled.

Fix: Add && IsEnabled guard.

🟡 Suggestion: Platform-Specific Tests Need Guards

Tests that use App.MoveCursor() and expect PointerOver results (HoveringOverButtonMovesToPointerOverState, PressingAndReleasingButtonMovesToPointerOverState) are desktop-only (require pointer/hover support). They fail on iOS because iOS doesn't support pointer hover.

These tests should either:

  • Be marked with a platform-specific attribute (e.g., [Platform(TestPlatforms.Windows | TestPlatforms.MacCatalyst)])
  • Or have the test class decorated with platform restrictions

🟡 Suggestion: DisablingUnfocusedButtonMovesToDisabledState Tests Platform-Specific Focus Behavior

The assertion Assert.That(App.FindElement("button3").GetText(), Is.EqualTo("Focused")) (line 106) assumes focus automatically transfers to button3 when button2 is disabled. This focus-transfer behavior appears to be Windows/desktop specific — iOS doesn't auto-transfer focus when a sibling element is disabled. This test needs platform restriction.

✅ What Looks Good

  • Introducing FocusStates as a separate internal class (marked for .NET 10 public exposure) is architecturally correct and aligns with WinUI's approach
  • The overall state ordering rationale (unfocus → disabled/normal → focused → PointerOver) is sound and well-documented in code comments
  • Adding MoveCursor helpers to Appium (AppiumMouseActions.cs, HelperExtensions.cs) is valuable for testing pointer interactions
  • The XAML test page using two VisualStateGroups (CommonStates + FocusStates) is a good documentation of the recommended pattern
  • CommonStates.Focused aliasing to FocusStates.Focused maintains backward compatibility

PR Title & Description Review

Title: "Improve VisualState order and prevent sticky Focused visual state"

  • Acceptable, though it could be more descriptive: [Windows] VisualStateManager: Fix state ordering so PointerOver doesn't conflict with Focused

Description: Well-written with good technical depth. Missing the required NOTE block.

Required addition (prepend to top):

<!-- 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!

Changes Requested

  1. Critical: Guard PointerOver state with IsEnabled check:

    if (IsPointerOver && IsEnabled)
        VisualStateManager.GoToState(this, VisualStateManager.CommonStates.PointerOver);
  2. Required: Add platform guards to pointer-specific UI tests (or restrict the test class to desktop platforms)

  3. Minor: Add the NOTE block to the PR description


📋 Expand PR Finalization Review
Title: ✅ Good

Current: Improve VisualState order and prevent sticky Focused visual state

Description: ✅ Good

Description needs updates. See details below.

✨ Suggested PR Description

[!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!

Root Cause

VisualElement.ChangeVisualState() applied the Focused/Unfocused states only when IsEnabled was true. This meant that if a control was focused and then disabled, the method would skip updating focus states entirely. The control would remain visually stuck in the Focused state even though it was now disabled and logically cannot hold focus.

Additionally, many controls (and the default MAUI Button style) put focus states (Focused, Unfocused) in the same VisualStateGroup as Normal, Disabled, and PointerOver. Because each group can only hold one active state at a time, having focus and pointer states in a single group means they compete — Normal can be overwritten and never re-applied if focus/unfocus states fire afterward.

Description of Change

VisualElement.ChangeVisualState() — state transition order rewrite (VisualElement.cs)

The method now applies states in a deliberate order:

  1. Unfocused first — if IsEnabled is false OR the control is not focused, Unfocused is applied unconditionally. This ensures a disabled-while-focused control exits the focused state.
  2. Disabled or Normal — applied based on IsEnabled / IsPointerOver.
  3. Focused — applied only if IsFocused && IsEnabled.
  4. PointerOver last — applied after focus states so it can override all others when the user is hovering.

VisualStateManager.FocusStates inner class (VisualStateManager.cs)

A new internal FocusStates class is introduced alongside CommonStates, holding Focused and Unfocused constants. CommonStates.Focused now points to FocusStates.Focused. This enables developers to use a separate FocusStates VisualStateGroup in their XAML — which is the correct pattern to allow pointer states and focus states to coexist independently. (TODO: make public in .NET 10.)

Correct XAML pattern (two groups):

<VisualStateGroupList>
  <VisualStateGroup x:Name="CommonStates">
    <VisualState x:Name="Normal" />
    <VisualState x:Name="PointerOver" />
    <VisualState x:Name="Disabled" />
  </VisualStateGroup>
  <VisualStateGroup x:Name="FocusStates">
    <VisualState x:Name="Focused" />
    <VisualState x:Name="Unfocused" />
  </VisualStateGroup>
</VisualStateGroupList>

When focus states are in their own group, the control can simultaneously be in PointerOver (from CommonStates) and Focused (from FocusStates) without either state overwriting the other.

New MoveCursor Appium test helper (AppiumMouseActions.cs, HelperExtensions.cs)

Adds MoveCursor(string), MoveCursor(IQuery), MoveCursor(IUIElement), and MoveCursorCoordinates(float, float) to the Appium test infrastructure for simulating mouse hover actions:

  • Windows: Uses windows: hover WinAppDriver script with DPI-aware coordinate calculation via P/Invoke (GetForegroundWindow, GetDpiForWindow).
  • Other platforms: Uses Selenium PointerInputDevice with CreatePointerMove.

UITest.Appium.csproj adds <AllowUnsafeBlocks>True</AllowUnsafeBlocks> to support LibraryImport-based P/Invoke for the Windows DPI APIs.

UI Test (Issue19752.xaml, Issue19752.cs)

New UI test verifying the fixed behavior across scenarios: initial states, hover → PointerOver, press+release → PointerOver, hover+leave → Normal, enable → Normal, disable unfocused → Disabled, and the critical regression case: disable focused button → Disabled (not stuck in Focused).

Key Technical Details

Why a single VisualStateGroup fails:

  • Each group has one active state at a time
  • Putting Focused, Unfocused, Normal, PointerOver in one group means they compete
  • Normal can never apply if Unfocused fires after it (same slot, last-write wins)
  • When focus states are missing entirely, GoToState silently no-ops — states appear "stuck"

State transition order matters because VSM is last-write-wins within a group:

  • Unfocused must come before Disabled to clear stale focus visuals
  • PointerOver must come last to override focus appearance when hovering a focused element

Issues Fixed

Fixes #19752

Code Review: ⚠️ Issues Found

Code Review — PR #27477

🔴 Critical Issues

Dead code: TapCoordinates method never called

  • File: src/TestUtils/src/UITest.Appium/Actions/AppiumMouseActions.cs

  • Problem: A TapCoordinates(IDictionary<string, object> parameters) method was added that calls ClickCoordinates(Convert.ToSingle(x), Convert.ToSingle(y)), but it is:

    1. Not registered in the _commandNames list
    2. Not referenced in the Execute switch expression
    3. Never called from any test or extension method

    This is unreachable dead code that will never execute and introduces confusion (a TapCoordinates command that shadows the existing ClickCoordinates command behavior).

  • Recommendation: Remove the TapCoordinates method entirely. If a tapCoordinates command is intentionally being added, register it in _commandNames and the Execute switch, and add a corresponding HelperExtensions method.


🟡 Suggestions

1. Typo in test comment

  • File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue19752.cs
  • Location: PressingAndReleasingButtonMovesToPointerOverState test method
  • Issue: Comment reads "the pointer over state is appplied after" (triple 'p')
  • Fix: "the pointer over state is applied after"

2. Unnecessary using statements in Issue19752.xaml.cs

  • File: src/Controls/tests/TestCases.HostApp/Issues/Issue19752.xaml.cs
  • Issue: Six unused using directives are included that are not referenced by any code in the file:
    using System.Collections.Generic;
    using System.Collections.ObjectModel;
    using System.ComponentModel;
    using System.Runtime.CompilerServices;
    using System.Windows.Input;
    using System.Collections.Specialized;
    These appear to be copy-paste residue from another issue file.
  • Fix: Remove all six unused using statements. Only System is needed (for EventArgs).

3. Double blank line in HelperExtensions.cs

  • File: src/TestUtils/src/UITest.Appium/HelperExtensions.cs
  • Issue: There are two consecutive blank lines between the end of the TapTab XML doc block and the start of the new MoveCursor XML doc comment. The existing file style uses single blank lines between members.
  • Fix: Remove one of the two blank lines.

4. GetForegroundWindow() may not target the correct window during tests

  • File: src/TestUtils/src/UITest.Appium/Actions/AppiumMouseActions.cs
  • Method: GetCurrentMonitorScaleFactor
  • Issue: The P/Invoke call uses GetForegroundWindow() to retrieve the HWND for DPI calculation. During Appium-driven Windows tests, the foreground window at the moment of the MoveCursor call may not be the WinAppDriver-controlled app window (e.g., if a dialog, the IDE, or another tool has briefly taken focus). This could produce incorrect DPI scale factors and offset hover coordinates.
  • Recommendation: Use the Appium driver's window handle (obtainable via _appiumApp.Driver.CurrentWindowHandle) or a more reliable HWND source tied to the test app's process rather than relying on the OS foreground window. Alternatively, document the assumption that the app under test is always the foreground window during hover actions.

5. Missing newline at end of files

  • Files: Issue19752.xaml, AppiumMouseActions.cs
  • Issue: Both files are missing a trailing newline (diff shows \ No newline at end of file). This is a minor convention violation but causes noisy diffs if other tools add the newline later.
  • Fix: Add a trailing newline to both files.

✅ Looks Good

  • ChangeVisualState() logic is well-commented and the new ordering (Unfocused → Normal/Disabled → Focused → PointerOver) is clearly reasoned.
  • FocusStates class extraction is a clean structural improvement; CommonStates.Focused correctly forwarding to FocusStates.Focused maintains backward compatibility.
  • LibraryImport P/Invoke is the modern, recommended approach over DllImport — good choice.
  • Test coverage is comprehensive; the seven test scenarios cover the primary regressions well.
  • Windows hover workaround using windows: hover WinAppDriver script is the correct pattern for Windows hover simulation.
  • partial class declaration on AppiumMouseActions enables clean source-generated P/Invoke separation.

@kubaflo kubaflo added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-gate-failed AI could not verify tests catch the bug s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) stale Indicates a stale issue/pr and will be closed soon labels Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-xaml XAML, CSS, Triggers, Behaviors s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-gate-failed AI could not verify tests catch the bug s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) stale Indicates a stale issue/pr and will be closed soon

Projects

Status: Changes Requested

Development

Successfully merging this pull request may close these issues.

Button does not behave properly when pointer hovers over the button because it's in focused state

8 participants