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

Align iOS, Android, Windows handling of tap gesture event bubbling #842

Merged
merged 4 commits into from Mar 29, 2017

Conversation

Projects
None yet
6 participants
@hartez
Member

hartez commented Mar 24, 2017

Description of Change

This change brings iOS, Android, and WinRT/UWP all into alignment on how they treat taps/clicks on a control within a container.

Previously, if a container (Grid, ContentView, Frame, etc.) had a tap gesture assigned to it and contained controls (Label, Box, Entry, etc.) which do not have gestures assigned, the platforms all behaved differently when those controls were tapped. On each platform, some of the controls would bubble the tap gesture up to the container and some would not, and those lists were different on each platform.

With this change, controls which become interactive on tap/click (Entry, Editor, Button, etc.) will not bubble the tap gesture to the container. Controls which do not (Label, Image, Box, Frame) will allow the gesture to bubble up to their container. The two lists are now consistent across the platforms.

Bugs Fixed

  • This change partially addresses the problems described in 45453 (in that the behavior of the controls on all of the platforms should now be consistent when the controls are enabled; the behavior inconsistencies Adrian points out in the Bugzilla discussion are still present for controls with IsEnabled set to false).

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

hartez added some commits Mar 22, 2017

Add test for gesture bubbling behavior on all controls;
Make Windows behavior consistent with other platforms;
@kingces95

This comment has been minimized.

Show comment
Hide comment
@kingces95

kingces95 Mar 27, 2017

Member

Just to be explicit about which controls bubble and which do not, the UITest tests the following matrix:

When no gesture handler is assigned, those that bubble are:

  • Image,
  • Label,
  • BoxView,
  • Frame,

And those that do not bubble are:

  • Entry,
  • Editor,
  • Button,
  • SearchBar,
  • DatePicker,
  • TimePicker,
  • Slider,
  • Switch,
  • Stepper.
Member

kingces95 commented Mar 27, 2017

Just to be explicit about which controls bubble and which do not, the UITest tests the following matrix:

When no gesture handler is assigned, those that bubble are:

  • Image,
  • Label,
  • BoxView,
  • Frame,

And those that do not bubble are:

  • Entry,
  • Editor,
  • Button,
  • SearchBar,
  • DatePicker,
  • TimePicker,
  • Slider,
  • Switch,
  • Stepper.
@kingces95

Why not fix the cases where the control is disabled instead of just the cases where the control is enabled? Do we not yet agree on the behavior?

How about this for a straw man:

A control that becomes interactive when touched (listed below) has an implicit handler. Disabling a control behaves like an enabled control with a handler that does nothing; disabled controls have an implicit handler that does nothing. A control without an explicit or implicit handler will "bubble" the event to its parent except if InputTransparent is true in which case the event is routed to whatever is behind the control.

Show outdated Hide outdated ...rols.Issues/Xamarin.Forms.Controls.Issues.Shared/GestureBubblingTests.cs
}
// These controls should allow the tap gesture to bubble up to their container; everything else should absorb the gesture
readonly List<string> _controlsWhichShouldAllowTheTapToBubbleUp = new List<string> { nameof(Image), nameof(Label), nameof(BoxView), nameof(Frame) };

This comment has been minimized.

@kingces95

kingces95 Mar 27, 2017

Member

Bummer to have lines render off screen on github.

@kingces95

kingces95 Mar 27, 2017

Member

Bummer to have lines render off screen on github.

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

Sounds like you need a bigger monitor :)

@hartez

hartez Mar 27, 2017

Member

Sounds like you need a bigger monitor :)

This comment has been minimized.

@kingces95

kingces95 Mar 27, 2017

Member

Eh, git hub is not using the full width. Is the a switch somewhere?

@kingces95

kingces95 Mar 27, 2017

Member

Eh, git hub is not using the full width. Is the a switch somewhere?

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

I reformatted it to be a more friendly width. AFAIK there's no way to get GH to use the full screen width unless you use a custom stylesheet or something.

@hartez

hartez Mar 27, 2017

Member

I reformatted it to be a more friendly width. AFAIK there's no way to get GH to use the full screen width unless you use a custom stylesheet or something.

Show outdated Hide outdated ...rols.Issues/Xamarin.Forms.Controls.Issues.Shared/GestureBubblingTests.cs
_controlsWhichShouldAllowTheTapToBubbleUp.Contains((view as Button).Text)
}));
}
}

This comment has been minimized.

@kingces95

kingces95 Mar 27, 2017

Member

LINQ?

var layout = (Layout)BuildMenu().Content;
var result =
	from Layout element in layout.InternalChildren
	from Button button in element.InternalChildren
	let text = button.Text
	select new object[]
	{
		text,
		_controlsWhichShouldAllowTheTapToBubbleUp.Contains(text)
	};
return result;
@kingces95

kingces95 Mar 27, 2017

Member

LINQ?

var layout = (Layout)BuildMenu().Content;
var result =
	from Layout element in layout.InternalChildren
	from Button button in element.InternalChildren
	let text = button.Text
	select new object[]
	{
		text,
		_controlsWhichShouldAllowTheTapToBubbleUp.Contains(text)
	};
return result;

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

Updated to query syntax.

@hartez

hartez Mar 27, 2017

Member

Updated to query syntax.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Mar 27, 2017

Member

Why not fix the cases where the control is disabled instead of just the cases where the control is enabled? Do we not yet agree on the behavior?

Because it's a very difficult problem and I don't want to let perfect be the enemy of good.

It's a difficult problem because Windows considers controls with IsEnabled == false to basically be black holes for the purpose of input, and so far I haven't found a good way around that. iOS seems to have the opposite opinion; anything not enabled just ignores input and lets it bubble up. Android is flexible about this. The correct answer is probably to choose the Windows or iOS behavior and bend the other platforms to that, but I haven't found an easy/elegant way to do that on iOS or Windows.

Until someone does figure out an answer to that question, I figured getting things in order for enabled controls was better than the total chaos we currently have.

Member

hartez commented Mar 27, 2017

Why not fix the cases where the control is disabled instead of just the cases where the control is enabled? Do we not yet agree on the behavior?

Because it's a very difficult problem and I don't want to let perfect be the enemy of good.

It's a difficult problem because Windows considers controls with IsEnabled == false to basically be black holes for the purpose of input, and so far I haven't found a good way around that. iOS seems to have the opposite opinion; anything not enabled just ignores input and lets it bubble up. Android is flexible about this. The correct answer is probably to choose the Windows or iOS behavior and bend the other platforms to that, but I haven't found an easy/elegant way to do that on iOS or Windows.

Until someone does figure out an answer to that question, I figured getting things in order for enabled controls was better than the total chaos we currently have.

#if UITEST
[Test, TestCaseSource(nameof(TestCases))]
public void VerifyTapBubbling(string menuItem, bool frameShouldRegisterTap)

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

We have a test like this for InputTransparent: #808

@hartez

hartez Mar 27, 2017

Member

We have a test like this for InputTransparent: #808

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

And any test for disabled controls wouldn't pass on all the platforms at this point, because they are not consistent for disabled controls. I'd rather leave that test for whoever fixes that bug.

@hartez

hartez Mar 27, 2017

Member

And any test for disabled controls wouldn't pass on all the platforms at this point, because they are not consistent for disabled controls. I'd rather leave that test for whoever fixes that bug.

This comment has been minimized.

@kingces95

kingces95 Mar 27, 2017

Member

And it could break people... So, just to sure, when InputTransparent is set the events, assuming they are not swallowed/handled, are routed to what's behind the control as opposed to its parent?

@kingces95

kingces95 Mar 27, 2017

Member

And it could break people... So, just to sure, when InputTransparent is set the events, assuming they are not swallowed/handled, are routed to what's behind the control as opposed to its parent?

This comment has been minimized.

@hartez

hartez Mar 27, 2017

Member

When InputTransparent is true, the events are not swallowed or handled by the control at all; they're routed to whatever is behind/below the control and if they're still not handled, they bubble up to the container/parent.

@hartez

hartez Mar 27, 2017

Member

When InputTransparent is true, the events are not swallowed or handled by the control at all; they're routed to whatever is behind/below the control and if they're still not handled, they bubble up to the container/parent.

This comment has been minimized.

@kingces95

kingces95 Mar 27, 2017

Member

Ah. Even when the control is disabled? You mentioned windows controls swallow events when disabled. That's not the case if the control is InputTransparent?

@kingces95

kingces95 Mar 27, 2017

Member

Ah. Even when the control is disabled? You mentioned windows controls swallow events when disabled. That's not the case if the control is InputTransparent?

@rmarinho rmarinho merged commit e55efa2 into master Mar 29, 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
@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 May 10, 2017

Contributor

@hartez

Controls which do not (Label, Image, Box, Frame) will allow the gesture to bubble up to their container.

I just tested 2.3.5-pre2. While the above statement is true on iOS, it isn't on Android. I have the following setup:

<StackLayout Orientation="Vertical" Spacing="10" VerticalOptions="Center">
        <Grid WidthRequest="250" HeightRequest="50" HorizontalOptions="Center" VerticalOptions="Center" BackgroundColor="Red">
            <Grid.GestureRecognizers>
                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped"></TapGestureRecognizer>
            </Grid.GestureRecognizers>
            <Label x:Name="Label1" Text="InputTransparent=True" InputTransparent="True" />
        </Grid>

        <Grid WidthRequest="250" HeightRequest="50" HorizontalOptions="Center" VerticalOptions="Center" BackgroundColor="Yellow">
            <Grid.GestureRecognizers>
                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped2"></TapGestureRecognizer>
            </Grid.GestureRecognizers>
            <Label x:Name="Label2" Text="InputTransparent=False"/>
        </Grid>
</StackLayout>

The only way for Label to bubble up a tap event is to set InputTransparent to true. I believe the intention was to let labels bubble up regardless of InputTransparent values.

Contributor

adrianknight89 commented May 10, 2017

@hartez

Controls which do not (Label, Image, Box, Frame) will allow the gesture to bubble up to their container.

I just tested 2.3.5-pre2. While the above statement is true on iOS, it isn't on Android. I have the following setup:

<StackLayout Orientation="Vertical" Spacing="10" VerticalOptions="Center">
        <Grid WidthRequest="250" HeightRequest="50" HorizontalOptions="Center" VerticalOptions="Center" BackgroundColor="Red">
            <Grid.GestureRecognizers>
                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped"></TapGestureRecognizer>
            </Grid.GestureRecognizers>
            <Label x:Name="Label1" Text="InputTransparent=True" InputTransparent="True" />
        </Grid>

        <Grid WidthRequest="250" HeightRequest="50" HorizontalOptions="Center" VerticalOptions="Center" BackgroundColor="Yellow">
            <Grid.GestureRecognizers>
                <TapGestureRecognizer Tapped="TapGestureRecognizer_Tapped2"></TapGestureRecognizer>
            </Grid.GestureRecognizers>
            <Label x:Name="Label2" Text="InputTransparent=False"/>
        </Grid>
</StackLayout>

The only way for Label to bubble up a tap event is to set InputTransparent to true. I believe the intention was to let labels bubble up regardless of InputTransparent values.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 May 10, 2017

Contributor

I believe #902 might have fixed the issue, so I'll wait for -pre3.

Contributor

adrianknight89 commented May 10, 2017

I believe #902 might have fixed the issue, so I'll wait for -pre3.

@hartez hartez deleted the gesture-bubbling branch May 16, 2017

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

@samhouts samhouts added this to the 2.5.0 milestone May 5, 2018

@samhouts samhouts modified the milestones: 2.5.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