fix: Cancel click on touch long-press menu - #23596
Conversation
github-actions
Bot
added
the
platform/wasm 🌐
|
Claude finished @MartinZikmund's task in 6m 51s —— View job PR Review
Overall: ✅ Correct fix, 2 minor style nits and 1 defensive robustness suggestion. The approach correctly mirrors WinUI's behavior: when a context popup steals OS pointer capture, WinUI gets a Key correctness verifications:
Findings (3 inline comments posted):
PR template note: "## PR Type:" and "## What changed? 🚀" sections are left empty. The issue is correctly referenced via |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23596/wasm-skia-net9/index.html |
37f8b82 to
8a550f8
Compare
|
Claude finished @MartinZikmund's task in 3m 51s —— View job PR Review
Overall: ✅ Correct approach — 3 prior nits still open, 2 new style nits postedThe new WASM commit ( Carryover findings from the previous review (still unaddressed): The 3 inline comments posted in the first review pass are still outstanding on the C# changes:
New findings (2 inline comments posted):
All findings are style/robustness nits; none affect correctness. The core fix and the runtime tests are sound. |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23596/wasm-skia-net9/index.html |
|
|
|
Claude finished @MartinZikmund's task in 4m 36s —— View job PR Review #23596
Overall: ✅ All previous nits addressed — ready to shipAll 5 findings from prior review passes have been resolved:
Correctness checks (all pass):
CI: Triggered runs from the |
Review comments addressed —
|
| file:line | reviewer point | what changed |
|---|---|---|
ContextMenuProcessor.cs:122 |
_contextMenuOnHoldingPointerId is 0 when SetContextMenuOnHoldingPointer was never called; add an explicit early-return to make the invariant clear |
Fixed, but not with the suggested == 0 sentinel — see the note below. The field is now uint? and the method early-returns on is not { } holdingPointerId. Same clarity, no sentinel hazard. |
ContextMenuProcessor.cs:116 |
drop (issue #22229) from the doc comment |
Removed. |
UIElement.Pointers.cs:1656 |
drop (issue #22229) from the doc comment |
Removed. |
Given_ContextRequested.Injection.cs:423 |
drop (issue #22229) from the assertion message |
Removed. |
Given_ContextRequested.Injection.cs:479 |
drop (issue #22229) from the assertion message |
Removed. |
Also swept one unflagged occurrence of the same nit in the prose comment at Given_ContextRequested.Injection.cs:449. The [GitHubWorkItem] attributes remain as the traceability link, per the reviewer's own reasoning.
Note on the == 0 guard — suggestion amended, not taken as written
The suggested guard assumed "no real pointer uses ID 0". That does not hold on Android, where PointerHelpers.GetPointerId composes the id as:
return ((uint)nativeEvent.GetPointerId(pointerIndex) & _pointerIdsCount) << _pointerIdsShift | (uint)nativeEvent.DeviceId;The first finger has GetPointerId(0) == 0, so a device reporting DeviceId == 0 produces a legitimate pointer id of 0. Treating 0 as "unset" would silently skip the capture release there and reintroduce the very bug this PR fixes — on the exact platform (touch) it targets.
So the reviewer's intent (make "no holding pointer recorded" explicit rather than relying on a scan that happens to find nothing) is implemented, with a nullable field that carries the distinction unambiguously:
// Nullable as 0 is a valid pointer id (e.g. first finger on an Android device with deviceId 0).
private uint? _contextMenuOnHoldingPointerId;
private void ReleaseContextMenuHoldingPointerCapture()
{
// No holding pointer was recorded (e.g. ContextRequested raised without a holding gesture).
if (_contextMenuOnHoldingPointerId is not { } holdingPointerId)
{
return;
}
...
}This path is reachable: RaiseContextRequestedEvent(..., isTouchInput: true) can be called without a preceding SetContextMenuOnHoldingPointer (the setter has exactly one caller, the HoldingState.Started branch in UIElement.Pointers.cs), so the guard is worth having.
No change needed (0) / Needs @MartinZikmund input (0)
Nothing escalated; no threads left open.
baec683 to
888c920
Compare
|
Claude finished @MartinZikmund's task in 2m 56s —— View job PR Review #23596
Overall: ✅ All previous findings resolved — ready to shipAll 5 findings from the prior review passes have been addressed in
Correctness checks (all pass):
No new findings. The PR is clean. |
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23596/wasm-skia-net9/index.html |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var capture in captures) | ||
| { | ||
| if (capture.Pointer.PointerId == holdingPointerId | ||
| && capture.ExplicitTarget is { } captureTarget) | ||
| { | ||
| captureTarget.ReleasePointerCaptureForContextMenu(capture.Pointer); | ||
| } | ||
| } |
| foreach (var target in capture.GetTargets(PointerCaptureKind.Explicit)) | ||
| { | ||
| if (target.LastDispatched is { } lastDispatched) | ||
| { | ||
| lastDispatched.Handled = false; | ||
| } | ||
| } |
On Skia targets, long-pressing a control (e.g. a Button) with a ContextFlyout both opened the menu AND raised Click, instead of behaving like a right-tap. WinUI suppresses the click because opening the menu's popup steals the OS pointer capture (PointerCaptureLost); Uno's in-canvas popups don't, so the pressed state was never cleared. When a context menu is shown on a touch press-and-hold, release the pressing element's pointer capture and raise an unhandled PointerCaptureLost so the control clears its pressed state and does not click on release. fixes #22229 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQEkhvePgnJMns2KhVNUxr
On touch devices the browser starts a text-selection gesture on long-press, firing a haptic buzz and selection/callout UI before the cancelable 'contextmenu' event - so preventing the context menu does not stop the buzz, and it conflicts with the app's own press-and-hold gestures (e.g. opening a ContextFlyout). Uno Skia draws all text and selection with Skia, so the browser selection is never useful. Disable it (and the long-press callout) on the app root via user-select/-webkit-touch-callout, and re-enable selection on the hidden IME input so text editing keeps working. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQEkhvePgnJMns2KhVNUxr
Guard the holding-pointer capture release with an explicit "not recorded" check (nullable id, as 0 is a valid pointer id) and drop issue-number cross-references from doc comments and assertion messages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKBgQoAmRmaFoXXZr93iLB
888c920 to
437bdac
Compare
|
Claude finished @MartinZikmund's task in 3m 42s —— View job PR Review #23596
Overall: ✅ Ready to ship — no new findingsAll 5 findings from prior review passes remain resolved in the current HEAD ( Correctness checks (all pass):
Optional (non-blocking) observations:
|
|
🤖 Your WebAssembly Skia Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-23596/wasm-skia-net9/index.html |
|
The build 229489 found UI Test snapshots differences: Details
|
3 participants
On Skia targets, long-pressing a control (e.g. a Button) with a
ContextFlyout both opened the menu AND raised Click, instead of
behaving like a right-tap. WinUI suppresses the click because opening
the menu's popup steals the OS pointer capture (PointerCaptureLost);
Uno's in-canvas popups don't, so the pressed state was never cleared.
When a context menu is shown on a touch press-and-hold, release the
pressing element's pointer capture and raise an unhandled
PointerCaptureLost so the control clears its pressed state and does not
click on release.
fixes #22229
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01WQEkhvePgnJMns2KhVNUxr**GitHub Issue:** closes #
PR Type:
What changed? 🚀
PR Checklist ✅
Screenshots Compare Test Runresults.