[Android] GraphicsView: prevent TapGesture crash when previous touch points are empty#34348
[Android] GraphicsView: prevent TapGesture crash when previous touch points are empty#34348michalpobuta wants to merge 1 commit intodotnet:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34348Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34348" |
|
Hey there @@michalpobuta! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Pull request overview
Fixes an Android crash in PlatformTouchGraphicsView.TouchesMoved that can occur when prior touch points are empty, and adds an Issue34296 UI test to validate tapping a GraphicsView does not crash.
Changes:
- Android: Adjusted the
TouchesMoveddrag-start guard to avoid indexing into an empty “previous points” array. - Tests (HostApp): Added an Issue34296 page with a
GraphicsView+TapGestureRecognizerto exercise the crash path. - Tests (Shared): Added an Issue34296 NUnit/Appium test that taps the
GraphicsViewtwice and verifiesTapCount:2.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Core/src/Platform/Android/PlatformTouchGraphicsView.cs | Updates the move/drag guard logic intended to prevent the crash when previous touch points are empty. |
| src/Controls/tests/TestCases.HostApp/Issues/Issue34296.cs | Adds a repro page with a tappable GraphicsView and status label used by the UI test. |
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34296.cs | Adds an Appium-based UITest that taps the GraphicsView twice and asserts the tap count text. |
Comments suppressed due to low confidence (1)
src/Core/src/Platform/Android/PlatformTouchGraphicsView.cs:96
- The new guard prevents
_lastMovedViewPoints[0]from throwing when the previous points were empty, but this block still readspoints[0]unconditionally and also changes the original single-touch behavior (points.Length == 1). Consider guarding on both arrays (e.g., requirepoints.Length > 0and preserve the original single-touch check) so multi-touch moves aren’t accidentally treated as a small-delta tap and emptypointscan’t crash this method (it’s public and can be called directly).
if (_lastMovedViewPoints.Length > 0)
{
float deltaX = _lastMovedViewPoints[0].X - points[0].X;
float deltaY = _lastMovedViewPoints[0].Y - points[0].Y;
You can also share your feedback on Copilot code review. Take the survey.
| App.WaitForElement("Issue34296StatusLabel"); | ||
|
|
||
| App.Tap("Issue34296GraphicsView"); | ||
|
|
There was a problem hiding this comment.
To reduce UI test flakiness, consider waiting for the label to reach the expected value after the second tap (e.g., WaitForTextToBePresentInElement on Issue34296StatusLabel with TapCount:2) before calling GetText() and asserting. Reading immediately after App.Tap can race the UI update on slower devices.
| App.WaitForTextToBePresentInElement("Issue34296StatusLabel", "TapCount:2"); |
| } | ||
| #endif |
There was a problem hiding this comment.
This page is intended to exercise the specific Android crash path by calling TouchesBegan(Array.Empty<PointF>()) followed by TouchesMoved(...). Right now, if graphicsView.Handler/PlatformView isn’t the expected PlatformTouchGraphicsView for any reason, the test will still pass and won’t actually validate the fix. Consider making this explicit (e.g., update the status label to an error state or throw in DEBUG when the platform view isn’t found) so the UITest can fail if the reproduction path wasn’t executed.
| } | |
| #endif | |
| } | |
| else | |
| { | |
| #if DEBUG | |
| throw new InvalidOperationException("Issue34296: Expected PlatformTouchGraphicsView as PlatformView but it was not found."); | |
| #else | |
| _statusLabel.Text = "Error: PlatformTouchGraphicsView not found"; | |
| return; | |
| #endif | |
| } | |
| #endif |
|
Duplicate of #34301 |
Summary
Fixes an Android crash path in
PlatformTouchGraphicsView.TouchesMovedwhen previous touch points can be empty.src/Core/src/Platform/Android/PlatformTouchGraphicsView.cspoints.Length == 1to_lastMovedViewPoints.Length > 0before reading_lastMovedViewPoints[0].Tests
Added/updated Issue34296 UI test coverage:
src/Controls/tests/TestCases.HostApp/Issues/Issue34296.cssrc/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue34296.csTest verifies tapping the
GraphicsViewtwice updates the status label toTapCount:2and does not crash.Local Validation
./.dotnet/dotnet tool run pwsh .github/scripts/BuildAndRunHostApp.ps1 -Platform android -TestFilter "Issue34296"Result: ✅ All tests passed (SUCCESS) on Android
Issues fixed
Fixes #34296
Platforms Affected