-
Notifications
You must be signed in to change notification settings - Fork 2k
[Android] Fix CollectionView selection not triggering when PointerGestureRecognizer is used in ItemTemplate #34627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
6f1aa02
5cf444b
4c36477
8ff694e
2c1cad8
e9e893f
fc1393c
b1b3c71
0b0b96a
f1b0656
ccce26c
9b46f92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 34491, "CollectionView item selection not triggered when PointerGestureRecognizer is added inside ItemTemplate", PlatformAffected.Android)] | ||
| public class Issue34491 : ContentPage | ||
| { | ||
| public Issue34491() | ||
| { | ||
| var statusLabel = new Label | ||
| { | ||
| Text = "No Selection", | ||
| AutomationId = "StatusLabel", | ||
| Padding = new Thickness(10), | ||
| FontSize = 18 | ||
| }; | ||
|
|
||
| var pointerStatusLabel = new Label | ||
| { | ||
| Text = "No Pointer Events", | ||
| AutomationId = "PointerStatusLabel", | ||
| Padding = new Thickness(10), | ||
| FontSize = 16 | ||
| }; | ||
|
|
||
| var collectionView = new CollectionView | ||
| { | ||
| AutomationId = "TestCollectionView", | ||
| SelectionMode = SelectionMode.Single, | ||
| ItemsSource = new List<string> { "Item 1", "Item 2", "Item 3" }, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| var label = new Label | ||
| { | ||
| Padding = new Thickness(10), | ||
| FontSize = 16 | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); | ||
| label.SetBinding(Label.AutomationIdProperty, "."); | ||
|
|
||
| var grid = new Grid | ||
| { | ||
| BackgroundColor = Colors.LightGray, | ||
| Padding = new Thickness(10), | ||
| HeightRequest = 50, | ||
| Children = { label } | ||
| }; | ||
|
|
||
| var pointerGesture = new PointerGestureRecognizer(); | ||
|
|
||
| pointerGesture.PointerPressed += (s, e) => | ||
| pointerStatusLabel.Text = $"Pointer Pressed: {grid.BindingContext}"; | ||
|
|
||
| pointerGesture.PointerReleased += (s, e) => | ||
| pointerStatusLabel.Text = $"Pointer Released: {grid.BindingContext}"; | ||
|
|
||
| grid.GestureRecognizers.Add(pointerGesture); | ||
|
|
||
| return grid; | ||
| }) | ||
| }; | ||
|
|
||
| collectionView.SelectionChanged += (s, e) => | ||
| { | ||
| if (e.CurrentSelection.Count > 0) | ||
| { | ||
| statusLabel.Text = $"Selected: {e.CurrentSelection[0]}"; | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| var mixedSelectionStatusLabel = new Label | ||
| { | ||
| Text = "No Mixed Selection", | ||
| AutomationId = "MixedSelectionStatusLabel", | ||
| Padding = new Thickness(10), | ||
| FontSize = 18 | ||
| }; | ||
|
|
||
| var mixedTapStatusLabel = new Label | ||
| { | ||
| Text = "No Mixed Tap", | ||
| AutomationId = "MixedTapStatusLabel", | ||
| Padding = new Thickness(10), | ||
| FontSize = 16 | ||
| }; | ||
|
|
||
| var mixedCollectionView = new CollectionView | ||
| { | ||
| AutomationId = "MixedTestCollectionView", | ||
| SelectionMode = SelectionMode.Single, | ||
| ItemsSource = new List<string> { "Mixed Item 1", "Mixed Item 2", "Mixed Item 3" }, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| var label = new Label | ||
| { | ||
| Padding = new Thickness(10), | ||
| FontSize = 16 | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); | ||
| label.SetBinding(Label.AutomationIdProperty, "."); | ||
|
|
||
| var grid = new Grid | ||
| { | ||
| BackgroundColor = Colors.LightBlue, | ||
| Padding = new Thickness(10), | ||
| HeightRequest = 50, | ||
| Children = { label } | ||
| }; | ||
|
|
||
| var pointerGesture = new PointerGestureRecognizer(); | ||
|
|
||
| pointerGesture.PointerPressed += (s, e) => | ||
| pointerStatusLabel.Text = $"Pointer Pressed: {grid.BindingContext}"; | ||
|
|
||
| pointerGesture.PointerReleased += (s, e) => | ||
| pointerStatusLabel.Text = $"Pointer Released: {grid.BindingContext}"; | ||
|
|
||
| grid.GestureRecognizers.Add(pointerGesture); | ||
|
|
||
| var tapGesture = new TapGestureRecognizer(); | ||
| tapGesture.Tapped += (s, e) => | ||
| mixedTapStatusLabel.Text = $"Tapped: {grid.BindingContext}"; | ||
|
|
||
| grid.GestureRecognizers.Add(tapGesture); | ||
|
|
||
| return grid; | ||
| }) | ||
| }; | ||
|
|
||
| mixedCollectionView.SelectionChanged += (s, e) => | ||
| { | ||
| if (e.CurrentSelection.Count > 0) | ||
| { | ||
| mixedSelectionStatusLabel.Text = $"Mixed Selected: {e.CurrentSelection[0]}"; | ||
| } | ||
| }; | ||
|
|
||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = new Thickness(10), | ||
| Spacing = 10, | ||
| Children = | ||
| { | ||
| statusLabel, | ||
| pointerStatusLabel, | ||
| collectionView, | ||
|
|
||
| mixedSelectionStatusLabel, | ||
| mixedTapStatusLabel, | ||
| mixedCollectionView | ||
| } | ||
| }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| #if ANDROID | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue34491 : _IssuesUITest | ||
| { | ||
| public Issue34491(TestDevice device) : base(device) { } | ||
|
|
||
| public override string Issue => "CollectionView item selection not triggered when PointerGestureRecognizer is added inside ItemTemplate"; | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CollectionView)] | ||
| public void CollectionViewSelectionWorksWithPointerGestureRecognizer() | ||
| { | ||
| App.WaitForElement("TestCollectionView"); | ||
| App.WaitForElement("StatusLabel"); | ||
|
|
||
| var initialText = App.FindElement("StatusLabel").GetText() ?? string.Empty; | ||
| Assert.That(initialText, Is.EqualTo("No Selection")); | ||
|
|
||
| App.WaitForElement("Item 1"); | ||
| App.Tap("Item 1"); | ||
|
|
||
| App.WaitForTextToBePresentInElement("StatusLabel", "Selected: Item 1"); | ||
|
|
||
| var finalText = App.FindElement("StatusLabel").GetText() ?? string.Empty; | ||
|
|
||
| Assert.That(finalText, Is.EqualTo("Selected: Item 1"), | ||
| "SelectionChanged should fire when tapping a CollectionView item that has a PointerGestureRecognizer"); | ||
| } | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CollectionView)] | ||
| public void PointerPressedAndReleasedStillFire() | ||
| { | ||
| App.WaitForElement("TestCollectionView"); | ||
| App.WaitForElement("PointerStatusLabel"); | ||
|
|
||
| var initialText = App.FindElement("PointerStatusLabel").GetText() ?? string.Empty; | ||
| Assert.That(initialText, Is.EqualTo("No Pointer Events")); | ||
|
|
||
| App.WaitForElement("Item 1"); | ||
| App.Tap("Item 1"); | ||
|
|
||
| App.WaitForTextToBePresentInElement("PointerStatusLabel", "Pointer Released: Item 1"); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Regression test coverage — This test name says |
||
|
|
||
| var finalText = App.FindElement("PointerStatusLabel").GetText() ?? string.Empty; | ||
|
|
||
| Assert.That(finalText, Is.EqualTo("Pointer Released: Item 1"), | ||
| "PointerPressed/PointerReleased should still fire after fix"); | ||
| } | ||
|
|
||
| [Test] | ||
| [Category(UITestCategories.CollectionView)] | ||
| public void MixedTapAndPointerGesturesStillAllowSelectionAndTap() | ||
| { | ||
| App.WaitForElement("MixedTestCollectionView"); | ||
| App.WaitForElement("MixedSelectionStatusLabel"); | ||
| App.WaitForElement("MixedTapStatusLabel"); | ||
|
|
||
| App.WaitForElement("Mixed Item 1"); | ||
| App.Tap("Mixed Item 1"); | ||
|
|
||
| App.WaitForTextToBePresentInElement("MixedSelectionStatusLabel", "Mixed Selected: Mixed Item 1"); | ||
| App.WaitForTextToBePresentInElement("MixedTapStatusLabel", "Tapped: Mixed Item 1"); | ||
|
|
||
| var selectionText = App.FindElement("MixedSelectionStatusLabel").GetText() ?? string.Empty; | ||
| var tapText = App.FindElement("MixedTapStatusLabel").GetText() ?? string.Empty; | ||
|
|
||
| Assert.That(selectionText, Is.EqualTo("Mixed Selected: Mixed Item 1"), | ||
| "SelectionChanged should still fire when tap and pointer gestures coexist"); | ||
|
|
||
| Assert.That(tapText, Is.EqualTo("Tapped: Mixed Item 1"), | ||
| "TapGestureRecognizer should still fire when pointer gestures are present"); | ||
| } | ||
| } | ||
| #endif | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] Android gesture handler correctness — Skipping the touch/RecyclerView listener when the effective recognizers are pointer-only removes the only path that delivers Down/Move/Up/Cancel to
PointerGestureHandler.OnTouch.SetupHandlerForPointer()only installs the hover listener, whilePointerPressed/PointerReleasedare raised fromTapAndPanGestureDetector.OnTouchEvent; withoutplatformView.Touch/GestureItemTouchListener, pointer-only views will not receive pressed/released touch events. This breaks the pointer semantics the PR is trying to preserve.