Support different gestures in landscape mode #411

mykola-mokhnach · 2016-12-11T16:36:36Z

Fixes #362

…on (fixes #362)

…ntly. Fixed now

marekcirkos · 2016-12-15T09:24:13Z

WebDriverAgentLib/Commands/FBElementCommands.m

@@ -58,6 +59,8 @@ + (NSArray *)routes
    [[FBRoute POST:@"/uiaElement/:uuid/twoFingerTap"] respondWithTarget:self action:@selector(handleTwoFingerTap:)],
    [[FBRoute POST:@"/uiaElement/:uuid/touchAndHold"] respondWithTarget:self action:@selector(handleTouchAndHold:)],
    [[FBRoute POST:@"/uiaElement/:uuid/scroll"] respondWithTarget:self action:@selector(handleScroll:)],
+    [[FBRoute POST:@"/dragfromtoforduration"] respondWithTarget:self action:@selector(handleDrag:)],


Can you not use different endpoint something like /wda/dragfromtoforduration

I have just created it based on existing API methods examples, like "/touchAndHold" and "/doubleTap". Shouldn't we follow the same template there?

Those are legacy stuff, that should die.

ok then I will use wda/ prefix as you suggested

marekcirkos · 2016-12-15T09:27:14Z

WebDriverAgentLib/Commands/FBElementCommands.m

@@ -58,6 +59,8 @@ + (NSArray *)routes
    [[FBRoute POST:@"/uiaElement/:uuid/twoFingerTap"] respondWithTarget:self action:@selector(handleTwoFingerTap:)],
    [[FBRoute POST:@"/uiaElement/:uuid/touchAndHold"] respondWithTarget:self action:@selector(handleTouchAndHold:)],
    [[FBRoute POST:@"/uiaElement/:uuid/scroll"] respondWithTarget:self action:@selector(handleScroll:)],
+    [[FBRoute POST:@"/dragfromtoforduration"] respondWithTarget:self action:@selector(handleDrag:)],
+    // TODO: remove obsolete endpoint
    [[FBRoute POST:@"/uiaTarget/:uuid/dragfromtoforduration"] respondWithTarget:self action:@selector(handleDrag:)],


We should split those handler. I don't like that we try to handle both cases in one handle and being smart about element presence.

no pb - I will implement both methods

marekcirkos · 2016-12-15T09:28:38Z

WebDriverAgentLib/Utilities/FBMathUtils.m

+CGSize FBAdjustDimensionsForApplication(CGSize actualSize, UIInterfaceOrientation orientation)
+{
+  if (orientation == UIInterfaceOrientationLandscapeLeft || orientation == UIInterfaceOrientationLandscapeRight) {
+    if (actualSize.height > actualSize.width) {


XCTest bug is, that it returns exchanged dimensions for landscape mode. This verification is just to make sure the bug is still there (since height is never greater than width in landscape) and to make it still working properly after XCTest itself starts to respect landscape.

Can you and comment on that?

marekcirkos · 2016-12-15T09:31:45Z

WebDriverAgentLib/Commands/FBElementCommands.m

-  XCUICoordinate *gestureCoordinate = [[XCUICoordinate alloc] initWithCoordinate:appCoordinate pointsOffset:point];
-  return gestureCoordinate;
+  // TODO: handle this properly after XCTest starts to respect landscape interface orientation
+  CGPoint point = FBInvertPointForApplication(coordinate, application.frame.size, application.interfaceOrientation);


You should check for iOS version here as well.

Instead of forcing those checks everywhere, you might want to consider adding FBUICoordinate class and patch coordinates on the go?

You should check for iOS version here as well.

This might be a bad idea, because you never know whether given coordinates were calculated based on some element position or just based on the actual screen size. I would prefer to keep it like this, because this is generic method and only have such verification for methods, where we explicitly calculate offset based on element location (like tap)

Instead of forcing those checks everywhere, you might want to consider adding FBUICoordinate class and patch coordinates on the go?

I think this might be too complicated. There are many places where you can read these coordinates (wdRect, wdFrame). Plus these a widely used for internal calculations and we need to cover the case, that iOS9.3 properly calculates them

Yes, but all versions checks etc, would happen in that class. So final product would be "transparent" for all clients. All of them should expect correct values anyway. Only problem would be argument you made in first point.

Let's make it simple. Most of users don't even use 20% of features we have here. I could think about improving this if there are enough complains in Appium forum. But for now it might take a lot of time to just create a workaround for the stuff, which is not widely used. And time is money 💯

I've also noticed one more thing, that is not going to be resolved if we just transform element coordinates. All the methods of XCUIElement, like doubleTap, pinch, etc will still fail in landscape. We anyway need to replace them with something, that does the same action by coordinates (actually, this is what you've already done for click) :-(

marekcirkos

Nice work, however I think we can do much better.

marekcirkos · 2016-12-16T14:15:52Z

I am sorry @mykola-mokhnach, I will not be able to merge this before Christmas. There are few things I would like do differently, but I wouldn't have time to sit to properly.

codeskyblue · 2017-01-10T07:09:54Z

@marekcirkos Any progress on this pr?

marekcirkos

Ok, Let's get done with it :).

marekcirkos · 2017-01-10T12:13:06Z

WebDriverAgentLib/Categories/XCUIElement+FBTap.h

+ @param relativeCoordinate hit point coordinate relative to the current element position
+ @return YES if the operation succeeds, otherwise NO.
+ */
+- (BOOL)fb_tapCoordinateWithError:(NSError **)error relativeCoordinate:(CGPoint)relativeCoordinate;


Error parameter should be last.

- (BOOL)fb_tapAtRelativeCoordinate:(CGPoint)relativeCoordinate error:(NSError **)error;

marekcirkos · 2017-01-10T12:14:52Z

WebDriverAgentLib/Categories/XCUIElement+FBTap.m

-    CGPoint hitPoint = hitpointValue ? hitpointValue.CGPointValue : [self coordinateWithNormalizedOffset:CGVectorMake(0.5, 0.5)].screenPoint;
-    hitPoint = FBInvertPointForApplication(hitPoint, self.application.frame.size, self.application.interfaceOrientation);
+    CGPoint hitPoint = CGPointMake(self.frame.origin.x + relativeCoordinate.x, self.frame.origin.y + relativeCoordinate.y);
+    // TODO: Change that after XCTest starts to respect landscape orientation


Instead of todo I would add comment why this is needed.

added detailed comment

marekcirkos · 2017-01-10T12:23:22Z

WebDriverAgentLib/Commands/FBElementCommands.m

@@ -206,7 +208,8 @@ + (NSArray *)routes

 + (id<FBResponsePayload>)handleDoubleTapCoordinate:(FBRouteRequest *)request
 {
-  XCUICoordinate *doubleTapCoordinate = [self.class getGestureCoordinate:request];
+  CGPoint doubleTapPoint = CGPointMake((CGFloat)[request.arguments[@"x"] doubleValue], (CGFloat)[request.arguments[@"y"] doubleValue]);
+  XCUICoordinate *doubleTapCoordinate = [self.class gestureCoordinateWithCoordinate:doubleTapPoint application:request.session.application shouldApplyOrientationWorkaround:YES];


Why you always apply changes here without iOS check? Would it be better to make iOS check inside that method?

Sorry, please ignore my recent comment. This is done, because in iOS 9.3 drag coordinates are also inverted. So the current implementation works properly if one provides absolute coordinates based on the actual screen width/height, but will not work if relative coordinates based on some element position are provided. I think the first case is more relevant for such method call. If one wanted to have a thing that performs drag on element by its relative coordinates, then he would create some other method (something like tap for clicking). But the method above only accepts absolute coordinates

@imurchie what is your opinion on it?

Just to summarise what we know:

On iOS 9.x coordinates are not inverted

On iOS 10.x coordinates are inverted

We can assume that coordinates passed by user are correct (meaning they are in reference to current orientation). This is mainly because we give them the coordinates or they manually calculate them.
Is that right?

On iOS 9.x coordinates are not inverted

Yes, but only coordinates we get when we want to know XCUIElement frame. All the other XCUIElement methods have the same bug as in 10.0+

On iOS 10.x coordinates are inverted

Yes, but this might be fixed in any of upcoming iOS releases.

We can assume that coordinates passed by user are correct (meaning they are in reference to current orientation). This is mainly because we give them the coordinates or they manually calculate them.

I want to discuss this point with @imurchie, because most of WDA calls where we do operations with absolute screen coordinates cannot be invoked directly from Appium, but rather via TouchAction interface. And we need to be in sync about our expectations here.

marekcirkos · 2017-01-18T11:18:33Z

@mykola-mokhnach is it ready for review or we are waiting for @imurchie?

mykola-mokhnach · 2017-01-18T12:19:04Z

Yes, this is ready from my side. I just want @imurchie to take a final look at this PR, because he was working on TouchAction interface in Appium, which, actually, is the main consumer of fixed WDA API methods.

imurchie · 2017-01-18T13:14:36Z

Looks good to me! This will be awesome to get in!

mykola-mokhnach · 2017-01-24T19:59:20Z

Hi @marekcirkos
Can you please initiate marge procedure for this PR? I'd like to create another PR for visibility fi, but it uses some stuff from this one.

marekcirkos · 2017-01-25T11:24:03Z

WebDriverAgentLib/Commands/FBElementCommands.m

-  XCUICoordinate *appCoordinate = [[XCUICoordinate alloc] initWithElement:session.application normalizedOffset:CGVectorMake(0, 0)];
-  XCUICoordinate *endCoordinate = [[XCUICoordinate alloc] initWithCoordinate:appCoordinate pointsOffset:endPoint];
-  XCUICoordinate *startCoordinate = [[XCUICoordinate alloc] initWithCoordinate:appCoordinate pointsOffset:startPoint];
+  BOOL shouldApplyOrientationWorkaround = SYSTEM_VERSION_GREATER_THAN_OR_EQUAL_TO(@"10.0");


Why we care about iOS version if we are handling coordinates there that are always buggy?

here we calculate coordinates based on element's position, which is calculated properly for iOS 9.3 and is broken for iOS10.0+
In other cases coordinates are passed as absolute values.

Ok, let's do this then!

facebook-github-bot · 2017-01-25T11:51:44Z

@marekcirkos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…ng values (#411) * Fix for appium/appium#7055 (comment) * Fix test data

Summary: Fixes facebookarchive/WebDriverAgent#362 Closes facebookarchive/WebDriverAgent#411 Differential Revision: D4461734 Pulled By: marekcirkos fbshipit-source-id: b388682e70d11a498e6a5fa8eb33172514371dab

Mykola Mokhnach and others added 17 commits September 28, 2016 17:41

Remove unnecessary udid parameter from window/size API call

f2b95f6

Address review comment: add TODO and keep an old interface for now

1d6e910

Update from master from remote

68e4680

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

ea1b23e

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

3b0e601

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

e6a4486

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

bb2ad41

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

694ccee

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

3ac3cbe

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

77e5b25

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

e370ccf

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

c32c2cb

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

c8d016a

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

c9b77c7

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

8e3776f

Merge branch 'master' of https://github.com/facebook/WebDriverAgent

30f1897

Make different types of gestures work properly in landscape orientati…

3ae18e9

…on (fixes #362)

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 11, 2016

Avoid lint errors

1526779

mykola-mokhnach mentioned this pull request Dec 12, 2016

tap coordinate still wrong on landscape mode, iOS 10 #362

Closed

Mykola Mokhnach added 3 commits December 12, 2016 12:12

Make things a bit smarter. Also, iOS 9.3 handles clicks a bit differe…

76fa93d

…ntly. Fixed now

Cache tap point

4d9448e

More testing

a009539

mykola-mokhnach mentioned this pull request Dec 12, 2016

Tap by x,y does not work any more appium/appium#7436

Closed

uuid parameter is not used in ‘handleDrag’ handler

af42a93

mykola-mokhnach mentioned this pull request Dec 14, 2016

Error copying attributes #372

Closed

marekcirkos reviewed Dec 15, 2016

View reviewed changes

marekcirkos suggested changes Dec 15, 2016

View reviewed changes

mykola-mokhnach mentioned this pull request Dec 15, 2016

Clicks on elements don't work properly on iPad and iPhone 10.0+ in landscape mode appium/appium#6994

Closed

Mykola Mokhnach added 2 commits December 15, 2016 12:22

Address review comments

abdb7f3

Fix typos

ff0f197

dpgraham mentioned this pull request Dec 18, 2016

Appium TouchAction issues [1.6.1, 1.6.2 and 1.6.3] appium/appium#7564

Closed

mykola-mokhnach mentioned this pull request Dec 19, 2016

Swipe gesture works incorrect on landscape mode using xcuitest driver. appium/appium#7566

Closed

dpgraham mentioned this pull request Dec 23, 2016

click and tap do not work on NATIVE_APP - iOS 10.2 - XCode 8.2.1 - Real Device appium/appium#7602

Closed

mykola-mokhnach mentioned this pull request Dec 28, 2016

Selenium Click/Sendkeys not working in Appium v1.6 beta3 in iOS Native application appium/appium#6941

Closed

mykola-mokhnach mentioned this pull request Jan 9, 2017

Width and height values are not exchanged if screen orientation is set to landscape #427

Closed

marekcirkos suggested changes Jan 10, 2017

View reviewed changes

Rename a method

6e8f3a5

mykola-mokhnach mentioned this pull request Jan 17, 2017

"Continue" button does not get clicked with iOS-9+ version. appium/appium#7692

Closed

mykola-mokhnach mentioned this pull request Jan 18, 2017

tap is not working #438

Closed

marekcirkos reviewed Jan 25, 2017

View reviewed changes

facebook-github-bot added GH Review: accepted Import Started and removed GH Review: review-needed labels Jan 25, 2017

facebook-github-bot closed this in 04d1b34 Jan 25, 2017

mykola-mokhnach deleted the landscape_tap branch August 14, 2017 12:01

imurchie referenced this pull request in appium/WebDriverAgent Nov 16, 2017

Fix for getting action coordinates in case they are presented as stri…

244b570

…ng values (#411) * Fix for appium/appium#7055 (comment) * Fix test data

Support different gestures in landscape mode #411

Support different gestures in landscape mode #411

Conversation

mykola-mokhnach commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

mykola-mokhnach Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekcirkos left a comment

Choose a reason for hiding this comment

marekcirkos commented Dec 16, 2016

codeskyblue commented Jan 10, 2017

marekcirkos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mykola-mokhnach Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marekcirkos commented Jan 18, 2017

mykola-mokhnach commented Jan 18, 2017

imurchie commented Jan 18, 2017

mykola-mokhnach commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 25, 2017

mykola-mokhnach Dec 15, 2016 •

edited

Loading

mykola-mokhnach Dec 15, 2016 •

edited

Loading

mykola-mokhnach Jan 10, 2017 •

edited

Loading