UI tests for InputTransparent and fixes for Android/Windows #808

Merged
merged 5 commits into from Mar 23, 2017

Conversation

Projects
None yet
5 participants
@hartez
Member

hartez commented Mar 10, 2017

Description of Change

This change adds UI tests for the InputTransparent property on all of the controls. It also makes the behavior of InputTransparent on Android consistent with the other platforms (prior to this, most of the Android controls were always input transparent).

This change also incorporates the UI tests and part of the Android fixes from PR 483 (with some slight modifications). Getting those tests passing on Windows ended up incidentally fixing some other open issues.

Also, replaces ugly MSPaint test image with a picture of my brother's dog.

Bugs Fixed

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
@samhouts

samhouts approved these changes Mar 10, 2017 edited

👍 with passing tests (some are failing now)
👍 👍 for the new oasis.jpg

+#endif
+
+ [Preserve(AllMembers = true)]
+ [Issue(IssueTracker.None, 8675309, "Test InputTransparent true/false on various controls")]

This comment has been minimized.

@samhouts

samhouts Mar 10, 2017

Member

thanks for getting the song stuck in my head now, and thanks in advance for getting it stuck in my head every time i break this test.

@samhouts

samhouts Mar 10, 2017

Member

thanks for getting the song stuck in my head now, and thanks in advance for getting it stuck in my head every time i break this test.

@@ -93,7 +93,7 @@ void IOnClickListener.OnClick(AView v)
public override bool OnInterceptTouchEvent(MotionEvent ev)
{
if (Element.InputTransparent && Element.IsEnabled)
- return false;
+ return true;

This comment has been minimized.

@samhouts

samhouts Mar 10, 2017

Member

Ok. So, this is what I think you're saying. The Button is a child of the ViewGroup. It receives a touch event. That event hits this method first to see if it should flow through to the Button. Previously, if the Element was InputTransparent, it would flow through to the Button. But the Button did not handle onTouch, so it wouldn't actually respect the InputTransparent value. Now, this ViewGroup will intercept the event and use its onTouch handler, which does take the InputTransparent value into account. Cool.

@samhouts

samhouts Mar 10, 2017

Member

Ok. So, this is what I think you're saying. The Button is a child of the ViewGroup. It receives a touch event. That event hits this method first to see if it should flow through to the Button. Previously, if the Element was InputTransparent, it would flow through to the Button. But the Button did not handle onTouch, so it wouldn't actually respect the InputTransparent value. Now, this ViewGroup will intercept the event and use its onTouch handler, which does take the InputTransparent value into account. Cool.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Mar 10, 2017

Contributor

@hartez I submitted #483 a while back. Please take that into account as well. Also IsEnabled property doesn't seem to be working well on Android.

Contributor

adrianknight89 commented Mar 10, 2017

@hartez I submitted #483 a while back. Please take that into account as well. Also IsEnabled property doesn't seem to be working well on Android.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 14, 2017

Member

The InputTransparentTests are failing @hartez

Member

rmarinho commented Mar 14, 2017

The InputTransparentTests are failing @hartez

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Mar 14, 2017

Member

@adrianknight89 Apologies, I hadn't realized there was already a PR dealing with this. I'll spend some time tomorrow pulling in your UI tests and making sure my changes didn't break any of them (or, if they did, that they were supposed to).

Member

hartez commented Mar 14, 2017

@adrianknight89 Apologies, I hadn't realized there was already a PR dealing with this. I'll spend some time tomorrow pulling in your UI tests and making sure my changes didn't break any of them (or, if they did, that they were supposed to).

@hartez hartez changed the title from UI tests for InputTransparent and fixes for Android to UI tests for InputTransparent and fixes for Android [DO NOT MERGE] Mar 17, 2017

@hartez hartez changed the title from UI tests for InputTransparent and fixes for Android [DO NOT MERGE] to UI tests for InputTransparent and fixes for Android Mar 21, 2017

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Mar 21, 2017

Member

This now includes the tests from PR 483.

Member

hartez commented Mar 21, 2017

This now includes the tests from PR 483.

+ public void UpdateElement(VisualElement element)
+ {
+ _isInViewCell = false;
+ _element = element;

This comment has been minimized.

@rmarinho

rmarinho Mar 21, 2017

Member

I worry we will be keeping this reference around since we don't clear or set to null our instance of MotionEventHelper.

@rmarinho

rmarinho Mar 21, 2017

Member

I worry we will be keeping this reference around since we don't clear or set to null our instance of MotionEventHelper.

@@ -23,6 +23,8 @@ public class LabelRenderer : ViewRenderer<Label, TextView>
FormsTextView _view;
bool _wasFormatted;
+ readonly MotionEventHelper _motionEventHelper = new MotionEventHelper();

This comment has been minimized.

@rmarinho

rmarinho Mar 21, 2017

Member

Can we remove readonly and set this to null on Dispose ? Just worried we could leak by setting a element reference on MotionEventHelper

@rmarinho

rmarinho Mar 21, 2017

Member

Can we remove readonly and set this to null on Dispose ? Just worried we could leak by setting a element reference on MotionEventHelper

This comment has been minimized.

@hartez

hartez Mar 23, 2017

Member

Which object are you worried about leaking?

@hartez

hartez Mar 23, 2017

Member

Which object are you worried about leaking?

This comment has been minimized.

@rmarinho

rmarinho Mar 23, 2017

Member

The renderer not being colected because MotionEventHelper will keep a reference to it.

@rmarinho

rmarinho Mar 23, 2017

Member

The renderer not being colected because MotionEventHelper will keep a reference to it.

This comment has been minimized.

@rmarinho

rmarinho Mar 23, 2017

Member

Sorry.. it keeps a reference to the element.. not the renderer.

@rmarinho

rmarinho Mar 23, 2017

Member

Sorry.. it keeps a reference to the element.. not the renderer.

This comment has been minimized.

@hartez

hartez Mar 23, 2017

Member

The only thing which holds a reference to MotionEventHelper is the renderer; when the renderer is collected, MotionEventHelper goes away, too. Setting it to null during dispose doesn't make any difference.

@hartez

hartez Mar 23, 2017

Member

The only thing which holds a reference to MotionEventHelper is the renderer; when the renderer is collected, MotionEventHelper goes away, too. Setting it to null during dispose doesn't make any difference.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 22, 2017

Member

@hartez can you resolve this conflit, don't want to make the wrong choice :)

Member

rmarinho commented Mar 22, 2017

@hartez can you resolve this conflit, don't want to make the wrong choice :)

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Mar 23, 2017

Member

Rebased with changes i did to used shared project for tests

Member

rmarinho commented Mar 23, 2017

Rebased with changes i did to used shared project for tests

@hartez hartez changed the title from UI tests for InputTransparent and fixes for Android to UI tests for InputTransparent and fixes for Android/Windows Mar 23, 2017

@rmarinho rmarinho merged commit f27f5a3 into master Mar 23, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3754, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

@rmarinho rmarinho deleted the inputtransparent-uitests branch Apr 26, 2017

@rmarinho rmarinho referenced this pull request Sep 1, 2017

Closed

[Android] Run gesture code on OnTouchEvent #1122

0 of 4 tasks complete

@samhouts samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment