Skip to content

[Windows] Fix select-all Entry select logic - #23329

Closed
Foda wants to merge 13 commits into
mainfrom
foda/EntrySelect
Closed

[Windows] Fix select-all Entry select logic#23329
Foda wants to merge 13 commits into
mainfrom
foda/EntrySelect

Conversation

@Foda

@Foda Foda commented Jun 27, 2024

Copy link
Copy Markdown
Contributor

Description of Change

Consider that you want to implement a "select all text on focus" function for an Entry field:

private void OnEntryFocused(object? sender, FocusEventArgs e)
{
    if (TextBox.Text != null)
    {
        TextBox.CursorPosition = 0;
        TextBox.SelectionLength = entry.Text.Length;
    }
}

Currently, this will work the first time the Entry is focused, but it won't work the second time.

The problem is caused by the following:

  • Entry gains focus
  • Cursor position and selection length are set, values propagate to WinUI TextBox
  • Entry is unfocused
  • Entry gains focus again
  • Cursor position and selection length are set, but the values are the same, so we don't set the values again on the WinUI TextBox
  • WinUI TextBox handles input event, moves cursor to where you click (note: the native WinUI TextBox seems to have special logic around canceling this logic if setting selection during an OnFocus event? this is where the issue comes from!)
  • WinUI control then propagates cursor position and selection length values to the Maui Entry, text is now unselected

The fix here is to just always ensure setting cursor position and selection length flows to the native WinUI TextBox control. A better solution might be adding a Select and SelectAll method to Entry, but for now this will work.

Mike Corsaro added 2 commits June 27, 2024 10:42
…ts even if the value is the same

This allows "select all text on focus" logic to work on WinUI
@Foda
Foda requested a review from PureWeen June 27, 2024 22:19
@Foda
Foda requested a review from a team as a code owner June 27, 2024 22:19
@Foda
Foda requested a review from Eilon June 27, 2024 22:19
{
get { return (int)GetValue(CursorPositionProperty); }
set { SetValue(CursorPositionProperty, value); }
set

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@StephaneDelcroix thoughts on this?

@PureWeen PureWeen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A better solution might be adding a Select and SelectAll method to Entry, but for now this will work.

We could just add this for now via the command mappers. Would that be a better solution here then firing the UpdateValue everytime?

Now in net9 with the new overrides, we could just target this to net9 and then fire a commandmapper that does the "SelectAll" behavior.

@jsuarezruiz

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@Foda

Foda commented Nov 18, 2024

Copy link
Copy Markdown
Contributor Author

/rebase

@Foda

Foda commented Mar 12, 2025

Copy link
Copy Markdown
Contributor Author

/rebase

@TrainCo

TrainCo commented Dec 22, 2025

Copy link
Copy Markdown

Has this code been merged? The issue is pretty old but we still have a pending issue open that references this fix and from what I see on this page, the PR has been "running" since January so I just want to be sure someone has this in their sights.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Expert Review — 6 findings

See inline comments for details.

namespace Maui.Controls.Sample.Issues;

[XamlCompilation(XamlCompilationOptions.Compile)]
[Issue(IssueTracker.ManualTest, "EntrySelectionTest", "Entry select all text issue", PlatformAffected.UWP)]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[moderate] Regression PreventionIssueTracker.ManualTest with a string key is the pattern for issues without a GitHub number, but this PR has an automated WinUI test (EntrySelectionTest.cs) so it should use IssueTracker.Github, <issue_number>. Additionally PlatformAffected.UWP is the legacy Xamarin.Forms label; the correct value for this repo is PlatformAffected.Windows (or PlatformAffected.All if the fix is intended cross-platform). Using ManualTest means the test runner may not correlate this page with the automated test class correctly.

@@ -1,5 +1,6 @@
#nullable enable
using System;
using System.Diagnostics;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[minor] Windows Platform Specificsusing System.Diagnostics; is unused in this file. No Debug., Trace., or Debugger. APIs are referenced in the diff or in the surrounding code. Remove the import.

@MauiBot MauiBot added s/agent-changes-requested AI agent recommends changes - found a better alternative or issues s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels May 5, 2026
@dotnet dotnet deleted a comment from MauiBot May 11, 2026
@MauiBot

MauiBot commented May 11, 2026

Copy link
Copy Markdown
Collaborator

🤖 AI Summary

👋 @Foda — new AI review results are available. Please review the latest session below.

📊 Review Sessiona11d30f · Merge branch 'main' into foda/EntrySelect · 2026-05-12 09:39 UTC
🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.


🧪 UI Tests

Full UI test matrix will run (no specific categories detected from PR changes).


🔍 Pre-Flight — Context & Validation

Pre-Flight — PR #23329

Title: [Windows] Fix select-all Entry select logic
Author: @Foda
Base: main @ 16a8109e
Head: foda/EntrySelect @ a11d30f5
Labels: platform/windows, area-controls-entry, s/agent-reviewed, s/agent-changes-requested
Status: open, draft=false, mergeable_state=blocked, 13 commits, 6 files (+100 / −2)

Bug Summary

When using a Focused event handler on an Entry to set CursorPosition = 0 and SelectionLength = Text.Length (the canonical "select all on focus" pattern), the selection works the first time the Entry is focused but fails on subsequent focus events on Windows.

Root cause described by author:

  1. First focus: handler-side cursor/selection values change from defaults → BP raises change → Handler pushes to WinUI TextBox → selection visible.
  2. Entry loses focus, then refocused. The author code re-assigns the same values (0 and Text.Length). Because the BindableProperty values did not change, OnBindablePropertySet(..., changed: false, ...) is invoked and Handler.UpdateValue is not raised. WinUI never re-sets SelectionStart/SelectionLength.
  3. WinUI's own pointer/focus pipeline then moves the caret to the click location, then propagates back to the xplat Entry, leaving the text unselected.

Files Changed

# File Lines Classification
1 src/Controls/src/Core/InputView/InputView.cs +21 Cross-platform (Controls layer — affects Entry/Editor/SearchBar on all platforms)
2 src/Core/src/Platform/Windows/TextBoxExtensions.cs +9 / −2 Windows-only (re-entry guard for clamping logic)
3 src/Controls/tests/TestCases.HostApp/Issues/EntrySelectionTest.xaml +19 Test host page
4 src/Controls/tests/TestCases.HostApp/Issues/EntrySelectionTest.xaml.cs +20 Test host page code-behind
5 src/Controls/tests/TestCases.WinUI.Tests/EntrySelectionTest.cs +31 Windows UI test (NUnit / Appium)
6 src/Controls/tests/TestCases.WinUI.Tests/snapshots/windows/VerifySelectWorks.png binary Screenshot baseline

Test Coverage

A new WinUI Appium snapshot test EntrySelectionTest.VerifySelectWorks enters text, defocuses, refocuses (twice), and asserts via screenshot that the text is selected. This is Windows-only — there is no cross-platform regression test even though the production change lands in the cross-platform InputView.

Gate status (pre-run): ⚠️ SKIPPED — gate skipped because runner reported no tests detected for this platform (test is Windows-only; PR review is running on a Linux/Android runner).

Prior Reviewer Feedback (existing on the PR)

  1. @PureWeen (CHANGES_REQUESTED, 2024-08-19): Suggested adding a Select/SelectAll API via the command-mapper pattern (now possible with net9 overrides) instead of firing UpdateValue on every set, to avoid the cross-platform side-effects of the unconditional re-push.
  2. MauiBot Expert Review (2026-05-05, 6 findings, 3 surfaced via inline):
    • EntrySelectionTest.xaml.cs:4 — Use IssueTracker.Github, <issue#> not IssueTracker.ManualTest since there IS an automated test; replace PlatformAffected.UWP with PlatformAffected.Windows.
    • TextBoxExtensions.cs:3using System.Diagnostics; is unused.

Code-Review Findings (independent re-analysis)

❌ Errors

  • InputView.cs:336-338 — broken indentation. The closing braces are mis-indented (mixed tabs/spaces; the } for the if (!changed) block sits at column 5 with two spaces, the method's closing } is at column 4 with four spaces). This will fail dotnet format enforcement and is the kind of thing CI rejects on this repo.
  • InputView.cs:318-337 — Cross-platform side-effect from a Windows-only bug. The override fires Handler.UpdateValue for CursorPosition and SelectionLength on every set, regardless of platform, even when the value didn't change. On Android/iOS this means every redundant assignment (including when a user re-focuses without changing the value programmatically) causes the native control's caret to be reset to the xplat value. This is a likely regression for Android/iOS scenarios where the user has clicked into a new caret position and the app then writes the same CursorPosition value it had cached. Windows-only fix should be Windows-only.

⚠️ Warnings

  • TextBoxExtensions.cs:3 — unused using System.Diagnostics; — would normally just be a warning, but Directory.Build.props sets TreatWarningsAsErrors=true for this repo, so this is build-breaking.
  • EntrySelectionTest.xaml.cs:4 — incorrect Issue metadata. IssueTracker.ManualTest plus PlatformAffected.UWP (legacy Xamarin.Forms label). Should be IssueTracker.Github, <linked issue number> and PlatformAffected.Windows. Affects test discovery / correlation.
  • EntrySelectionTest.cs:31 — file has no trailing newline. Minor but commonly enforced.
  • No cross-platform test. The production fix changes cross-platform code in InputView.cs but the only test is Windows-only. A unit test should verify (a) the Handler.UpdateValue IS invoked when changed==false for the targeted properties, and (b) it is NOT invoked for other properties.

💡 Suggestions

  • The reviewer's suggested SelectAll command-mapper is a cleaner long-term API, but is additive and not strictly required for the regression.
  • An alternative minimal fix is to gate the unconditional UpdateValue to the Windows handler only (e.g., move the workaround entirely into EntryHandler.Windows/EditorHandler.Windows mappers, or only invoke UpdateValue for these properties when the handler has been informed it is Windows). This keeps non-Windows handlers untouched.

Failure-Mode Probes

Scenario What happens with PR's fix Concern
Windows: select-all on second focus ✅ Fixed (the bug) Resolves the reported issue
Android: user taps Entry, app re-assigns same CursorPosition Handler pushes to native EditText, resetting caret Possible regression
iOS: programmatic SelectionLength reassign Handler.UpdateValue fires, may re-trigger selection animation/observer Possible regression
dotnet format on InputView.cs Fails due to mixed indentation CI block
Build on any platform Fails due to unused using System.Diagnostics; with TreatWarningsAsErrors CI block

Blast Radius

  • OnBindablePropertySet runs on every property assignment for every InputView (Entry, Editor, SearchBar) on every platform — not just on Windows, not just for the two targeted properties. The if (!changed) and string comparison run for every BP set. Cheap, but global.
  • The two Handler.UpdateValue calls run for every set of CursorPosition or SelectionLength whose value didn't change — on all platforms.

Code-Review Verdict

NEEDS_CHANGES (confidence: high) — blocking issues:

  1. Build break: unused using System.Diagnostics; + TreatWarningsAsErrors=true.
  2. Format break: mixed-indent braces in InputView.cs.
  3. Cross-platform side-effect from a Windows-only bug (also flagged by PureWeen 2024-08-19).
  4. Test metadata incorrect (also flagged by MauiBot).
  5. No cross-platform regression test for the cross-platform code change.

Platform & Environment

  • Requested test platform: android
  • PR is Windows-only behavior; tests are Windows-only (NUnit + Appium WinAppDriver).
  • Build & smoke verification can be done on android (Controls + Core build).
  • Try-fix attempts will be evaluated via static analysis + cross-platform build, since we cannot run a Windows snapshot test on this runner.

🔬 Code Review — Deep Analysis

Code Review — PR #23329 (independent, code-first)

Methodology: read the diff and surrounding code first, then cross-checked against PureWeen's CHANGES_REQUESTED review and MauiBot expert review. Findings are my own analysis, but they corroborate the prior reviews.

❌ Errors (must fix before merge)

E1. Build break — unused using System.Diagnostics;

  • File: src/Core/src/Platform/Windows/TextBoxExtensions.cs:3
  • Why blocking: Repo-wide TreatWarningsAsErrors=true is set in Directory.Build.props:5-7. CS8019 (unused using) fails the build.
  • Fix: Remove using System.Diagnostics;.

E2. Format break — mixed indentation in InputView.cs

  • File: src/Controls/src/Core/InputView/InputView.cs:336-338
  • Detail: The inner if (!changed) block's closing brace is \t\t } (two tabs + two spaces, no \t\t\t} like the rest of the file). The method's closing } is at } (four spaces, no tabs). Followed by a blank line that starts with 4 spaces.
  • Why blocking: dotnet format Microsoft.Maui.sln is required by the repo workflow and will fail.
  • Fix: Re-indent with tabs to match surrounding code.

E3. Cross-platform regression risk for a Windows-only bug

  • File: src/Controls/src/Core/InputView/InputView.cs:318-337
  • Detail: OnBindablePropertySet is invoked for every BP set on every InputView on every platform. When changed == false and the property is CursorPosition or SelectionLength, the override calls Handler?.UpdateValue(...). On Android, iOS, and Tizen, those mappers re-push the xplat caret position to the native EditText/UITextField/Entry control even when the user has just clicked into a new caret position and the app code re-assigns the cached value. This can produce caret-jumping regressions on those platforms.
  • Evidence: mappers are present for all platforms — see src/Core/src/Handlers/Entry/EntryHandler.{Android,iOS,Windows,Tizen}.cs, EntryHandler2.Android.cs, the Editor and SearchBar handlers, and the SearchBar2 handler.
  • Why blocking: the original review by @PureWeen requested exactly this — "fire a commandmapper that does the SelectAll behavior" instead of firing UpdateValue every time. The current change has been re-flagged 5 calendar months later by the expert reviewer (s/agent-changes-requested).
  • Fix options: (a) gate the always-fire to Windows, or (b) implement the SelectAll command-mapper proposal, or (c) replicate the if (!changed) guard only in the Windows mapper (where the bug lives), leaving InputView untouched.

⚠️ Warnings (should fix before merge)

W1. Test metadata: IssueTracker.ManualTest despite having automated test

  • File: src/Controls/tests/TestCases.HostApp/Issues/EntrySelectionTest.xaml.cs:4
  • Detail: [Issue(IssueTracker.ManualTest, "EntrySelectionTest", ...)] should be IssueTracker.Github, <issueNumber> since EntrySelectionTest.cs is the automated test. PlatformAffected.UWP should be PlatformAffected.Windows.

W2. Test file lacks trailing newline

  • File: src/Controls/tests/TestCases.WinUI.Tests/EntrySelectionTest.cs:31

W3. No cross-platform regression test

  • The production change is in cross-platform InputView.cs. Only a Windows snapshot test exists. A unit test in Controls.Core.UnitTests that asserts OnBindablePropertySet fires Handler.UpdateValue exactly for the right properties (and not for others) when changed == false would protect against regressions and document intent.

💡 Suggestions

  • Long-term: implement the SelectAll command-mapper proposed by @PureWeen — cleaner API than relying on an unchanged-set side-effect.
  • The Windows TextBoxExtensions re-entry guard (if (entry.CursorPosition != clampedPos) entry.CursorPosition = clampedPos;) is a sensible defensive change — keep it whichever fix wins.

Failure-Mode Probes

# Probe With PR diff
P1 Repeated Entry.CursorPosition = 0; followed by SelectionLength = Text.Length; on each focus, Windows Now works (the bug fix)
P2 Same pattern on Android: user taps to move caret, then app re-asserts cached CursorPosition on a property change of an unrelated property Caret could be forced back to xplat-cached position (regression)
P3 Build with TreatWarningsAsErrors Fails (unused using)
P4 dotnet format Fails (mixed indent)
P5 Run WinUI snapshot test Should pass (cannot verify here — no Windows runner)

Blast Radius

  • Cross-platform: InputView.OnBindablePropertySet runs for every property set on Entry/Editor/SearchBar on every platform. Cost per call is one string equality check (cheap) but the two Handler.UpdateValue calls fire on all platforms.
  • Windows-only: the TextBoxExtensions change is correctly scoped and low risk.

Verdict

NEEDS_CHANGES (confidence: high)

Concretely, what must change:

  1. Remove using System.Diagnostics; in TextBoxExtensions.cs.
  2. Fix indentation in InputView.cs (dotnet format).
  3. Either move the always-fire workaround to a Windows-only location (preferred minimal fix) or implement the SelectAll command-mapper (preferred long-term).
  4. Update EntrySelectionTest.xaml.cs to use IssueTracker.Github + PlatformAffected.Windows.
  5. Add trailing newline to EntrySelectionTest.cs.
  6. Consider adding a cross-platform unit test for the InputView change if it is kept cross-platform.

🔧 Fix — Analysis & Comparison

Try-Fix Aggregate Summary — PR #23329

Four independent fix candidates were generated, each loading a different maui-expert-reviewer dimension so the design space is genuinely explored. Per the orchestrator's instructions, all four ran regardless of pre-flight signal.

Fix Candidates

# Source Approach Test result Files changed Key insight
1 try-fix-1 #if WINDOWS-gate the cross-platform OnBindablePropertySet override (+ reviewer textual fixes) ✅ PASS (static) — Windows behavior identical to PR; non-Windows reverts to baseline 6 (4 prod, 2 test) Smallest scope-correct delta from PR
2 try-fix-2 No InputView change; subscribe to TextBox.GotFocus in EntryHandler.Windows.cs and re-push selection via DispatcherQueue after focus pipeline settles ✅ PASS (static) — bug fixed at the layer that owns the WinUI race 6 (3 prod, 3 test) Localizes the workaround to the system that has the bug
3 try-fix-3 Add public SelectAll() / Select(int,int) on InputView; update test page to use it; add PublicAPI.Unshipped.txt entries (7 folders) ✅ PASS (static) — clean long-term API but doesn't fix existing user code without migration 11 (4 prod, 7 API, 3 test) PureWeen's 2024 suggestion realized
4 try-fix-4 try-fix-1 + new InputViewSelectionUpdateTests xUnit test pinning the cross-platform invariant ✅ PASS (static) — best regression protection 7 (4 prod, 3 test, +1 new unit test) Pins the invariant the PR is implicitly creating
PR (as submitted) Override OnBindablePropertySet on cross-platform InputView; clamp re-entry guard in Windows TextBoxExtensions ⚠️ ⚠️ Build-fails (unused using) + format-fails (mixed indent); Windows snapshot test would pass if those two CI breaks were ignored; non-Windows regression risk unaddressed 6 Original PR
pr-plus-reviewer (PR + textual feedback) Same as PR with the two CI breaks fixed and test metadata corrected; cross-platform side-effect untouched ✅ PASS (static) — CI breaks resolved; non-Windows regression risk still present 6 "PR as it should have been submitted"

Cross-Pollination

The four candidates explore genuinely independent axes; there are no further useful combinations beyond the ones already enumerated:

Combination Status Reason
try-fix-1 + try-fix-4 Already covered try-fix-4 IS try-fix-1 + a cross-platform unit test
try-fix-1 + try-fix-3 Possible — would add SelectAll API on top of Windows-gated workaround Maximalist; not needed if try-fix-1 ships, can be added incrementally
try-fix-2 + try-fix-3 Possible — no InputView change at all, add SelectAll API that internally invokes the handler hook Larger but clean; reasonable follow-up if reviewers want both
try-fix-2 across Editor / SearchBar Needed for parity if try-fix-2 ships Trivial duplication; deferred

No new ideas surfaced. Exhausted: Yes.

Selected Candidate

try-fix-4 — same Windows runtime behavior as the PR, but:

  • Eliminates the Android/iOS/Tizen regression risk that @PureWeen flagged in 2024 and the expert reviewer re-flagged in 2026.
  • Adds an explicit cross-platform unit test that pins the cross-platform invariant. Any future contributor who removes the #if WINDOWS gate or otherwise reintroduces the non-Windows forced Handler.UpdateValue will get a failing unit test.
  • Fixes the two CI-blocking issues in the PR (unused using, mixed indentation).
  • Corrects test metadata to use IssueTracker.Github, 18443 and PlatformAffected.Windows.

See report/content.md for the comparative analysis and try-fix-4/candidate.diff for the full diff.

Cannot empirically execute the Windows snapshot test on this Linux/Android runner; the verdicts above are based on static analysis combined with the fact that the Windows-side runtime code paths in try-fix-1 / try-fix-4 are byte-identical to the PR's fix on Windows TFM.


📋 Report — Final Recommendation

Report — PR #23329 Comparative Analysis

Candidates Evaluated

  1. pr — the PR's diff as submitted (commit a11d30f5).
  2. pr-plus-reviewerpr + reviewer's actionable textual feedback applied (removes unused using, fixes indentation, corrects test metadata, adds newline). No architectural change.
  3. try-fix-1pr-plus-reviewer + #if WINDOWS gate around the OnBindablePropertySet override on InputView. Eliminates non-Windows side-effect.
  4. try-fix-2 — No InputView change. Fix moved to EntryHandler.Windows.cs via a TextBox.GotFocus subscription that re-pushes selection through the DispatcherQueue after the focus pipeline settles.
  5. try-fix-3 — No OnBindablePropertySet. Add public SelectAll() / Select(int, int) API on InputView; update test page to call the new method; register entries in PublicAPI.Unshipped.txt across 7 TFM folders.
  6. try-fix-4try-fix-1 (Windows-gated workaround) + new InputViewSelectionUpdateTests xUnit test that pins the cross-platform invariant for Entry/Editor/SearchBar.

Scoring Matrix

Candidate CI build CI format Windows fix correctness Non-Windows regression risk Regression test added Public-API surface added Scope vs bug Overall
pr ❌ unused using System.Diagnostics; + TreatWarningsAsErrors ❌ mixed tabs/spaces in InputView.cs ✅ Fixes #18443 on Windows ⚠️ Forces Handler.UpdateValue for CursorPosition/SelectionLength on Android/iOS/Tizen even when value unchanged → caret-jump risk ⚠️ Windows snapshot only None Over-broad (cross-platform fix for Windows bug) Fails CI
pr-plus-reviewer ✅ Same as PR ⚠️ Same as PR ⚠️ Same as PR None Over-broad Buildable but architecturally same as PR
try-fix-1 ✅ Byte-identical to PR on Windows TFM ✅ Compile-time gated to Windows ⚠️ Windows snapshot only None Right-sized Minimal correct fix
try-fix-2 ✅ Functionally equivalent on Windows (deferred dispatcher re-apply) ✅ Zero cross-platform delta ⚠️ Windows snapshot only None Right-sized (Windows handler) Right-sized, but only covers Entry (Editor/SearchBar would need duplication) and has a small theoretical snapshot-timing risk
try-fix-3 ⚠️ Existing user code still broken without migration; new API fixes the recommended pattern ✅ Zero cross-platform delta ⚠️ Windows snapshot only (now covers the new API) SelectAll() and Select(int, int) × 7 TFM folders Right-sized API; bug literal repro not fixed Best long-term API design; doesn't backward-compatibly fix the bug
try-fix-4 ✅ Byte-identical to PR on Windows TFM ✅ Compile-time gated to Windows ✅ Cross-platform xUnit test pins the invariant None Right-sized Best regression protection while preserving the PR's user-facing behavior

Note on the gate rule: only pr "fails regression" in the sense the orchestrator means — the unused-using is TreatWarningsAsErrors=true so the build fails on every TFM that compiles TextBoxExtensions.cs (Windows). All other candidates pass static checks; none were able to be empirically executed on this Linux/Android runner.

Per-Candidate Risks

  • pr — Build break + format break (both deterministic CI failures). Cross-platform side-effect (latent regression).
  • pr-plus-reviewer — Cross-platform side-effect still present. Otherwise clean.
  • try-fix-1 — None material. The #if WINDOWS pattern is used elsewhere in Controls.Core (see src/Controls/src/Core/PlatformPointerEventArgs.cs:3).
  • try-fix-2 — Snapshot test on Windows might need re-baselining if the deferred re-apply produces a visibly different frame timing (low probability). Editor/SearchBar would need parallel hooks for parity.
  • try-fix-3 — Public API change requires shipping in next release. Existing user code that uses the property-set pattern still breaks until migrated. Adds 14 PublicAPI entries.
  • try-fix-4 — Largest diff (~165 lines including new test). Minor risk: Element.Handler setter access in the test stub may need a tiny adjustment if internal-only; verifiable by a Core.UnitTests build.

Selection

Winner: try-fix-4.

Rationale:

  1. CI green: same textual fixes as pr-plus-reviewer (unused using, format, test metadata).
  2. Windows behavior preserved: the WinUI workaround inside #if WINDOWS is byte-identical to the PR's fix — the existing EntrySelectionTest.VerifySelectWorks snapshot test should pass unchanged.
  3. No cross-platform regression risk: the workaround compiles to no-op on Android, iOS, MacCatalyst, Tizen, and netstandard.
  4. Regression protection: a Core.UnitTest pins the cross-platform invariant. Any future contributor who removes the #if WINDOWS gate or who refactors OnBindablePropertySet and inadvertently fires Handler.UpdateValue on the unchanged-set path will get a failing unit test. This addresses the missing test coverage flagged by the expert review (the PR's only test is Windows-only).
  5. Resolves both prior reviews: PureWeen's 2024 cross-platform-side-effect concern, and the 2026 MauiBot expert review's six findings.

Why not the alternatives?

  • pr / pr-plus-reviewer: do not address the cross-platform side-effect concern. PureWeen's 2024 review was the original CHANGES_REQUESTED block; not addressing it now ships the same concern back.
  • try-fix-2: equally good architecturally, but only covers Entry. Achieving parity with the PR (which covered Entry, Editor, SearchBar via the cross-platform InputView change) requires duplicating the hook into EditorHandler.Windows.cs and SearchBarHandler.Windows.cs — same code three times. The cross-platform #if WINDOWS approach in try-fix-1/try-fix-4 covers all three with a single change.
  • try-fix-3: the best long-term API design, but a strictly different solution to a different problem (gives users a clean workaround instead of fixing the literal entry.CursorPosition = 0; repro). Worth doing in addition to a fix, not instead of.

Empirical-execution caveat

The Windows snapshot test cannot be executed on this Linux/Android review runner. The verdicts above are based on:

  1. Static analysis of each candidate's diff,
  2. Verification that each diff applies cleanly against the PR base (f8cb875e),
  3. Reasoning about the Windows-side runtime code paths — which in try-fix-1/try-fix-4 are byte-identical to the PR's path,
  4. Per-platform mapper inventory confirming that MapCursorPosition / MapSelectionLength exist on every platform's handler (the cross-platform side-effect concern is grounded in real code, not speculation).

Recommendation

Adopt try-fix-4 as the basis of the PR. If the maintainers also want the public SelectAll() API surface (PureWeen's 2024 suggestion), that can be applied as a follow-up on top — try-fix-3's SelectAll is additive and orthogonal to try-fix-4.


@MauiBot MauiBot added s/agent-review-incomplete and removed s/agent-changes-requested AI agent recommends changes - found a better alternative or issues labels May 11, 2026
@kubaflo

kubaflo commented May 11, 2026

Copy link
Copy Markdown
Contributor

Closing in favor of #35383, which incorporates this fix with the review suggestions applied:

  • IssueTracker.ManualTestIssueTracker.Github, 23329 for proper test correlation
  • PlatformAffected.UWPPlatformAffected.Windows
  • Removed unused using System.Diagnostics;

Thank you @Foda for the original fix — you're listed as co-author on the new PR. 🙏

@kubaflo kubaflo closed this May 11, 2026
kubaflo added a commit that referenced this pull request May 27, 2026
### Description of Change

Fixes the issue where setting `CursorPosition` and `SelectionLength` on
an `Entry` during the `Focused` event only works on the first focus but
not on subsequent focuses on WinUI.

**Root cause:** When the same cursor position / selection length values
are set again, the bindable property system detects no change and skips
propagation to the native WinUI `TextBox`. The native control then
handles the focus input event and moves the cursor, overriding the
intended selection.

**Fix:**
- Override `OnBindablePropertySet` in `InputView` to always push
`CursorPosition` and `SelectionLength` to the native handler even when
the value has not changed.
- In `TextBoxExtensions`, only write back the clamped value when it
actually differs from the current value, preventing the clamping
assignment from blocking the xplat → native flow.

**Review suggestions applied from #23329:**
- Changed `IssueTracker.ManualTest` → `IssueTracker.Github, 23329` so
the test runner correlates the page with the automated test class
- Changed `PlatformAffected.UWP` → `PlatformAffected.Windows` (correct
value for .NET MAUI)
- Removed unused `using System.Diagnostics;` import

Based on the work by @Foda in #23329.

### Issues Fixed

Supersedes #23329

---------
PureWeen pushed a commit that referenced this pull request Jun 2, 2026
### Description of Change

Fixes the issue where setting `CursorPosition` and `SelectionLength` on
an `Entry` during the `Focused` event only works on the first focus but
not on subsequent focuses on WinUI.

**Root cause:** When the same cursor position / selection length values
are set again, the bindable property system detects no change and skips
propagation to the native WinUI `TextBox`. The native control then
handles the focus input event and moves the cursor, overriding the
intended selection.

**Fix:**
- Override `OnBindablePropertySet` in `InputView` to always push
`CursorPosition` and `SelectionLength` to the native handler even when
the value has not changed.
- In `TextBoxExtensions`, only write back the clamped value when it
actually differs from the current value, preventing the clamping
assignment from blocking the xplat → native flow.

**Review suggestions applied from #23329:**
- Changed `IssueTracker.ManualTest` → `IssueTracker.Github, 23329` so
the test runner correlates the page with the automated test class
- Changed `PlatformAffected.UWP` → `PlatformAffected.Windows` (correct
value for .NET MAUI)
- Removed unused `using System.Diagnostics;` import

Based on the work by @Foda in #23329.

### Issues Fixed

Supersedes #23329

---------
PureWeen pushed a commit that referenced this pull request Jun 11, 2026
### Description of Change

Fixes the issue where setting `CursorPosition` and `SelectionLength` on
an `Entry` during the `Focused` event only works on the first focus but
not on subsequent focuses on WinUI.

**Root cause:** When the same cursor position / selection length values
are set again, the bindable property system detects no change and skips
propagation to the native WinUI `TextBox`. The native control then
handles the focus input event and moves the cursor, overriding the
intended selection.

**Fix:**
- Override `OnBindablePropertySet` in `InputView` to always push
`CursorPosition` and `SelectionLength` to the native handler even when
the value has not changed.
- In `TextBoxExtensions`, only write back the clamped value when it
actually differs from the current value, preventing the clamping
assignment from blocking the xplat → native flow.

**Review suggestions applied from #23329:**
- Changed `IssueTracker.ManualTest` → `IssueTracker.Github, 23329` so
the test runner correlates the page with the automated test class
- Changed `PlatformAffected.UWP` → `PlatformAffected.Windows` (correct
value for .NET MAUI)
- Removed unused `using System.Diagnostics;` import

Based on the work by @Foda in #23329.

### Issues Fixed

Supersedes #23329

---------
PureWeen pushed a commit that referenced this pull request Jun 22, 2026
### Description of Change

Fixes the issue where setting `CursorPosition` and `SelectionLength` on
an `Entry` during the `Focused` event only works on the first focus but
not on subsequent focuses on WinUI.

**Root cause:** When the same cursor position / selection length values
are set again, the bindable property system detects no change and skips
propagation to the native WinUI `TextBox`. The native control then
handles the focus input event and moves the cursor, overriding the
intended selection.

**Fix:**
- Override `OnBindablePropertySet` in `InputView` to always push
`CursorPosition` and `SelectionLength` to the native handler even when
the value has not changed.
- In `TextBoxExtensions`, only write back the clamped value when it
actually differs from the current value, preventing the clamping
assignment from blocking the xplat → native flow.

**Review suggestions applied from #23329:**
- Changed `IssueTracker.ManualTest` → `IssueTracker.Github, 23329` so
the test runner correlates the page with the automated test class
- Changed `PlatformAffected.UWP` → `PlatformAffected.Windows` (correct
value for .NET MAUI)
- Removed unused `using System.Diagnostics;` import

Based on the work by @Foda in #23329.

### Issues Fixed

Supersedes #23329

---------
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-controls-entry Entry platform/windows s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants