Skip to content
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

[Android] Fix single taps #16561

Merged
merged 4 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using Microsoft.Maui.Controls;

namespace Maui.Controls.Sample
{
public class DoubleTapGallery : Microsoft.Maui.Controls.ContentView
{
public DoubleTapGallery()
{
AutomationId = "DoubleTapGallery";

var layout = new VerticalStackLayout() { Margin = 10, Spacing = 10 };

var result = new Label() { AutomationId = "DoubleTapResults" };

var tapSurface = new Grid()
{
HeightRequest = 200,
WidthRequest = 200,
BackgroundColor = Microsoft.Maui.Graphics.Colors.AliceBlue,
AutomationId = "DoubleTapSurface"
};

var doubleTapRecognizer = new TapGestureRecognizer() { NumberOfTapsRequired = 2 };
doubleTapRecognizer.Tapped += (sender, args) => { result.Text = "Success"; };

tapSurface.GestureRecognizers.Add(doubleTapRecognizer);

layout.Add(tapSurface);
layout.Add(result);

Content = layout;
}
}
}

Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using Microsoft.Maui.Controls.Internals;
using System.Drawing;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Internals;

namespace Maui.Controls.Sample
{
Expand All @@ -8,6 +10,7 @@ public class GestureRecognizerGallery : ContentViewGalleryPage
public GestureRecognizerGallery()
{
Add(new PointerGestureRecognizerEvents());
Add(new DoubleTapGallery());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?xml version="1.0" encoding="utf-8" ?>
<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
x:Class="Maui.Controls.Sample.Issues.Issue16561">
<StackLayout>

<Grid x:Name="TapArea"
AutomationId="TapArea"
BackgroundColor="LightBlue"
Margin="10"
HeightRequest="300">
<Label HorizontalOptions="Center"
VerticalOptions="Center"
TextColor="Black"
Text="Tap In This Grid" />
</Grid>

<Label HeightRequest="100"
AutomationId="Tap1Label"
Background="green"
x:Name="Tap1Label" />

<Label HeightRequest="100"
AutomationId="Tap2Label"
Background="green"
x:Name="Tap2Label" />

</StackLayout>
</ContentPage>
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Xaml;
using Microsoft.Maui.Platform;

namespace Maui.Controls.Sample.Issues
{
[XamlCompilation(XamlCompilationOptions.Compile)]
[Issue(IssueTracker.Github, 16561, "Quick single taps on Android have wrong second tap location", PlatformAffected.Android)]
public partial class Issue16561 : ContentPage
{
int _taps;

public Issue16561()
{
InitializeComponent();

var tapGesture = new TapGestureRecognizer();
tapGesture.Tapped += TapHandler;

TapArea.GestureRecognizers.Add(tapGesture);
}

void TapHandler(object sender, TappedEventArgs e)
{
var pos = e.GetPosition(TapArea);

if (pos == null)
{
Tap1Label.Text = $"Error, could not get tap position";
return;
}

#if ANDROID
// Adjust the results for display density, so they make sense to the
// Appium test consuming this.
var x = this.Handler.MauiContext.Context.ToPixels(pos.Value.X);
var y = this.Handler.MauiContext.Context.ToPixels(pos.Value.Y);
pos = new(x, y);
#endif
Comment on lines +33 to +39
Copy link
Member

Choose a reason for hiding this comment

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

This is scary. Is Android not scaling the coordinates like the other platforms? Do we have an issue to track this or is it by design?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Appium layer doesn't know anything about our SDK's point->pixel conversions (why would it?). So when we're asking it to retrieve the Rect for a particular element and then to tap at specific points on that element, it's doing all of that work in native points. The cross-platform tap events are in device-independent units. So we have to do that conversion somewhere. It's easier to do that on the app side, because the app already knows the display density and how to convert the values.

At the moment, we don't have any other UI tests which require this sort of point-to-point comparison. If we start to need more tests like that in the future, we could introduce an automatic conversion mechanism for the Appium Rect structure which determines the remote device's display density at startup. But that's not top of my list right now for the one test which has this issue.


if (_taps % 2 == 0)
{
Tap1Label.Text = $"{pos.Value.X}, {pos.Value.Y}";
}
else
{
Tap2Label.Text = $"{pos.Value.X}, {pos.Value.Y}";
}

_taps += 1;
}
}
}
16 changes: 10 additions & 6 deletions src/Controls/src/Core/Platform/Android/InnerGestureListener.cs
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,26 @@ bool GestureDetector.IOnDoubleTapListener.OnDoubleTap(MotionEvent e)
return _tapDelegate(2, e);
}

if (HasSingleTapHandler())
// If we're getting here and don't have a double-tap handler, we might be looking at multiple
// single taps; that'll be handled in OnDoubleTapEvent

return false;
}

bool GestureDetector.IOnDoubleTapListener.OnDoubleTapEvent(MotionEvent e)
{
if (!HasDoubleTapHandler() && HasSingleTapHandler() && e.Action == MotionEventActions.Up)
{
// If we're registering double taps and we don't actually have a double-tap handler,
// but we _do_ have a single-tap handler, then we're really just seeing two singles in a row

// Fire off the delegate for the second single-tap (OnSingleTapUp already did the first one)
return _tapDelegate(1, e);
}

return false;
}

bool GestureDetector.IOnDoubleTapListener.OnDoubleTapEvent(MotionEvent e)
{
return false;
}

bool GestureDetector.IOnGestureListener.OnDown(MotionEvent e)
{
SetStartingPosition(e);
Expand Down
14 changes: 14 additions & 0 deletions src/Controls/tests/UITests/Tests/GestureRecognizerUITests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,20 @@ public void PointerGestureTest()
var secondaryLabelText = App.Query("secondaryLabel").First().Text;
Assert.IsNotEmpty(secondaryLabelText);
}

[Test]
public void DoubleTap()
{
App.WaitForElement("TargetView");
App.EnterText("TargetView", "DoubleTapGallery");
App.Tap("GoButton");

App.WaitForElement("DoubleTapSurface");
App.DoubleTap("DoubleTapSurface");

var result = App.Query("DoubleTapResults").First().Text;
Assert.AreEqual("Success", result);
}
}
}

3 changes: 2 additions & 1 deletion src/Controls/tests/UITests/Tests/Issues/Issue15357.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Microsoft.Maui.Appium;
using System.Reflection;
using Microsoft.Maui.Appium;
using NUnit.Framework;

namespace Microsoft.Maui.AppiumTests.Issues
Expand Down
78 changes: 78 additions & 0 deletions src/Controls/tests/UITests/Tests/Issues/Issue16561.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
using System.Drawing;
using Microsoft.Maui.Appium;
using NUnit.Framework;
using OpenQA.Selenium.Appium.MultiTouch;
using TestUtils.Appium.UITests;

namespace Microsoft.Maui.AppiumTests.Issues
{
public class Issue16561 : _IssuesUITest
{
private string _tapAreaId = "TapArea";

public Issue16561(TestDevice device) : base(device)
{
}

public override string Issue => "Quick single taps on Android have wrong second tap location";

[Test]
public void TapTwoPlacesQuickly()
{
if (App is not IApp2 app2 || app2 is null || app2.Driver is null)
{
throw new InvalidOperationException("Cannot run test. Missing driver to run quick tap actions.");
}

var tapAreaResult = App.WaitForElement(_tapAreaId);
var tapArea = tapAreaResult[0].Rect;

// The test harness coordinates are absolute
var xOffset = 50;
var harnessCenterX = tapArea.CenterX;
var harnessCenterY = tapArea.CenterY;

var point1 = new PointF(harnessCenterX - xOffset, harnessCenterY);
var point2 = new PointF(harnessCenterX + xOffset, harnessCenterY);

// The TapGesture coordinates are relative to the container, so we need to adjust
// for the container position
var expectedY = harnessCenterY - tapArea.Y;
var expectedX1 = point1.X - tapArea.X;
var expectedX2 = point2.X - tapArea.X;

// Just calling Tap twice will be too slow; we need to queue up the actions so they happen quickly
var actionsList = new TouchAction(app2.Driver);

// Tap the first point, then the second point
actionsList.Tap(point1.X, point1.Y).Tap(point2.X, point2.Y);
app2.Driver.PerformTouchAction(actionsList);

// The results for each tap should show up in the labels on the screen; find the text
// of each tap result and check to see that it meets the expected values
var result = App.WaitForElement("Tap1Label");
AssertCorrectTapLocation(result[0].Text, expectedX1, expectedY, "First");

result = App.WaitForElement("Tap2Label");
AssertCorrectTapLocation(result[0].Text, expectedX2, expectedY, "Second");
}

static void AssertCorrectTapLocation(string tapData, float expectedX, float expectedY, string which)
{
// Turn the text values into numbers so we can compare with a tolerance
(var tapX, var tapY) = ParseCoordinates(tapData);

Assert.AreEqual((double)expectedX, tapX, 1, $"{which} tap has unexpected X value");
Assert.AreEqual((double)expectedY, tapY, 1, $"{which} tap has unexpected Y value");
}

static (double, double) ParseCoordinates(string text)
{
var values = text.Split(',', StringSplitOptions.TrimEntries)
.Select(double.Parse)
.ToArray();

return (values[0], values[1]);
}
}
}
33 changes: 22 additions & 11 deletions src/TestUtils/src/TestUtils.Appium.UITests/AppiumUITestApp.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ public class AppiumUITestApp : IApp2

public static TimeSpan DefaultTimeout = TimeSpan.FromSeconds(15);

// Using the default value from https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/view/ViewConfiguration.java#129
// and shaving off 50ms so we come in under the threshold
// iOS and Mac use a different way of simulating double taps, so no need for a variation of this constant for those platforms
// The Windows default is 500 ms (https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-setdoubleclicktime?redirectedfrom=MSDN#parameters)
// so this delay should be short enough to simulate a double-click on that platform.
const int DOUBLE_TAP_DELAY_MS = 250;

readonly Dictionary<string, string> _controlNameToTag = new Dictionary<string, string>
{
{ "button", "ControlType.Button" }
Expand Down Expand Up @@ -115,6 +122,8 @@ public ApplicationState AppState
}
}

public AppiumDriver? Driver => _driver;

public void ResetApp()
{
_driver?.ResetApp();
Expand Down Expand Up @@ -243,11 +252,7 @@ private void DoubleTap(AppiumElement element)
PointerInputDevice touchDevice = new PointerInputDevice(PointerType);
ActionSequence sequence = new ActionSequence(touchDevice, 0);
sequence.AddAction(touchDevice.CreatePointerMove(element, 0, 0, TimeSpan.FromMilliseconds(5)));
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePause(TimeSpan.FromMilliseconds(600)));
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
AddDoubleTap(touchDevice, sequence);
_driver?.PerformActions(new List<ActionSequence> { sequence });
}
}
Expand Down Expand Up @@ -275,16 +280,22 @@ public void DoubleTapCoordinates(float x, float y)
{
PointerInputDevice touchDevice = new PointerInputDevice(PointerType);
ActionSequence sequence = new ActionSequence(touchDevice, 0);
sequence.AddAction(touchDevice.CreatePointerMove(CoordinateOrigin.Viewport, (int)x, (int)y, TimeSpan.FromMilliseconds(5)));
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePause(TimeSpan.FromMilliseconds(600)));
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
AddDoubleTap(touchDevice, sequence);
_driver?.PerformActions(new List<ActionSequence> { sequence });
}
}

static ActionSequence AddDoubleTap(PointerInputDevice touchDevice, ActionSequence sequence)
{
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePause(TimeSpan.FromMilliseconds(DOUBLE_TAP_DELAY_MS)));
sequence.AddAction(touchDevice.CreatePointerDown(PointerButton.TouchContact));
sequence.AddAction(touchDevice.CreatePointerUp(PointerButton.TouchContact));

return sequence;
}

public void DragAndDrop(Func<AppQuery, AppQuery> from, Func<AppQuery, AppQuery> to)
{
DragAndDrop(
Expand Down
4 changes: 3 additions & 1 deletion src/TestUtils/src/TestUtils.Appium.UITests/IApp2.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Xamarin.UITest;
using OpenQA.Selenium.Appium;
using Xamarin.UITest;

namespace TestUtils.Appium.UITests
{
Expand All @@ -11,5 +12,6 @@ public interface IApp2 : IApp, IDisposable
ApplicationState AppState { get; }
bool WaitForTextToBePresentInElement(string automationId, string text);
public byte[] Screenshot();
public AppiumDriver? Driver { get; }
}
}