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

[iOS] Tapping on disabled context action cells should not forward touches down #510

Closed
wants to merge 3 commits into from

Conversation

@adrianknight89
Copy link
Contributor

commented Nov 7, 2016

Description of Change

On iOS, if a context action cell is disabled, a tap gesture passes through and is caught by ListView which results in a list item being selected. There doesn't seem to be any stack trace triggered when a disabled button is clicked. The first point of entry seems to be ShouldReceiveTouch for row selection.

The simplest solution I could think of was checking if the touch point is inside a disabled button so that the touch can be canceled.

Bugs Fixed

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
@@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.Collections.Specialized;
using System.ComponentModel;
using System.Linq;

This comment has been minimized.

Copy link
@samhouts

samhouts Nov 10, 2016

Member

We've found that using Linq extensions in the Core and Platform code does not always produce the most optimized path of execution on devices, so we tend to avoid it. I don't believe that we've seen performance issues with .Any or .Aggregate, but it might be best to write a foreach in IsTouchInDisabledButton instead.

This comment has been minimized.

Copy link
@adrianknight89

adrianknight89 Nov 10, 2016

Author Contributor

Done.

@@ -59,6 +60,13 @@ public bool IsOpen
get { return ScrollDelegate.IsOpen; }
}

public bool IsTouchInDisabledButton(PointF pointf)
{
double totalButtonWidth = _buttons.Aggregate(0.0, (current, uiButton) => current + uiButton.Frame.Width);

This comment has been minimized.

Copy link
@samhouts

samhouts Nov 10, 2016

Member

I haven't tested this, but I wonder if it would make sense to cache this value instead of calculating it every time...?

This comment has been minimized.

Copy link
@adrianknight89

adrianknight89 Nov 10, 2016

Author Contributor

I'm not so sure about this. I'd assume that the calculation should be quick since the number of buttons is unlikely to be greater than 3. If a button resizes on user action or programmatically, a cached value somehow needs to be updated.

This comment has been minimized.

Copy link
@samhouts

samhouts Nov 10, 2016

Member

Fair enough. 😄

@samhouts samhouts self-assigned this Nov 10, 2016
@@ -653,7 +670,7 @@ public SelectGestureRecognizer() : base(Tapped)

var cell = table.CellAt(_lastPath) as ContextActionsCell;

return cell != null;
return cell != null && (!cell.IsOpen || !cell.IsTouchInDisabledButton(pos));

This comment has been minimized.

Copy link
@jassmith

jassmith Nov 16, 2016

Contributor

Why check IsOpen here? This would seem to prevent selection of the cell when its open even if you were to tap on the left part which is still able to be selected.

This comment has been minimized.

Copy link
@adrianknight89

adrianknight89 Nov 17, 2016

Author Contributor

@jassmith I'm checking for IsOpen to make sure context actions are visible and we are not inadvertently hit testing actual cell content. In my testing, tapping on the left part of the cell closes the menu instead of selecting the cell. If you look at iOS Mail app, it seems to behave the same with each email row.

@jassmith

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2017

@adrianknight89 we are trying to run this one through testing, can you please rebase? :)

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Jan 4, 2017

Done.

@rmarinho

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

Test 31330 is failing. Can you check the problema?

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2017

@rmarinho I think what is happening is the PR ignores taps on disabled context actions whereas the UI test expects the context menu to close whenever a tap occurs. The behavior of the PR makes more sense to me. Let me know if the test needs to be updated. Alternatively, the PR can adjust to the test. Sounds like a design decision.

I'm not sure how Xamarin.iOS behaves, so perhaps it's better to check there first.

@rmarinho

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Closing as this fix is a breaking change on existing behaviour.

@rmarinho rmarinho closed this Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.