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

PlayerView click handling does not match standard Android behavior #5784

Closed
jasonostrander opened this issue Apr 19, 2019 · 4 comments
Closed
Assignees

Comments

@jasonostrander
Copy link

[REQUIRED] Issue description

Hi,
This issue is related to: #5433

It appears the PlayerView onClick handling triggers on TOUCH_DOWN events, rather than on TOUCH_UP, which is the standard Android behavior.

Observed: Tapping and holding on PlayerView triggers onClickListener.
Expected: Tapping and holding on PlayerView does not trigger onClickListener. OnClickListener is triggered when the users' finger is lifted.

Note: this may be the desired behavior for hiding/showing the PlayerView controls. However, it also affects any OnClickListener attached by app developers. For example, I have a PlayerView inside a scrolling container and set a click handler with: playerView.setOnClickListener(). The click handler is being triggered while swiping on the PlayerView. This is unexpected behavior.

I believe the proper way to fix this is detailed in these docs:
https://developer.android.com/guide/topics/ui/accessibility/custom-views#custom-touch-events

[REQUIRED] Reproduction steps

This can be reproduced on the demo app without modification. Open the demo app, select the Youtube Dash -> Google Glass (1st entry) video. During playback press and hold on the PlayerView. Player controls will toggle. For an example of standard Android behavior, press and hold on the play/pause button. No action is taken until after the finger is lifted.

[REQUIRED] Link to test content

See ExoPlayer demo app.

[REQUIRED] A full bug report captured from the device

There is no relevant log.

[REQUIRED] Version of ExoPlayer being used

2.9.6

[REQUIRED] Device(s) and version(s) of Android being used

Tested on a Galaxy Note 9 running Android 9

@ojw28
Copy link
Contributor

ojw28 commented Apr 23, 2019

@tonihei - I think this problem was introduced by 0a7745b, which caused ACTION_DOWN touch events to call performClick, which then calls a registered listener. Could you take a look?

@tonihei
Copy link
Collaborator

tonihei commented Apr 24, 2019

The purpose of performClick() is to trigger all the logic associated with a click. This includes system logic (such as calling on onClickListener and playing a sound), but also custom logic like toggling the visibility of a view. The onTouchEvent handler is supposed to trigger performClick at the right moment where all this logic is supposed to happen. This ensures that both touch events and accessibility events call through to performClick() and perform the same actions.

We defined our custom click action to be triggered on TOUCH_DOWN and not on TOUCH_UP, so it makes sense to call performClick in TOUCH_DOWN and also trigger all related system actions like playing a sound and notifying listeners. So it's arguably working as intended. On the other hand, I agree that the usual expected behaviour is to trigger the click event only on TOUCH_UP.

What exactly are you trying to achieve with the distinction between TOUCH_DOWN and TOUCH_UP? If you are only interested in the logical click events, then you should leave it up to the view to send the event in the moment in which it actually handles a click action. If you are interested in some custom behaviour which relies on certain touch events, then you should probably set an onTouchListener instead.

@jasonostrander
Copy link
Author

@tonihei The issue I'm having with the current behavior is that swiping on a PlayerView triggers an onClick event. For example: if you place the PlayerView inside a ViewPager, scrolling through pages will trigger the onClick handler set on the PlayerView.

It's possible to work around this issue by using a TouchListener and GestureDetector, but this is much more effort than a simple OnClickListener. I would also argue that breaking standard android expectations around click events should be avoided unless there is a compelling reason to do otherwise.

May I ask: what is the reasoning around the current behavior of calling performClick during TOUCH_DOWN? Based on the commit message in 0a7745b, this does not seem like it was designed so much as a side effect of an effort to fix warnings.

@tonihei
Copy link
Collaborator

tonihei commented Apr 24, 2019

The warning which was fixed in 0a7745b was about not calling performClick from onTouchEvent. What got fixed by adding this call is that accessibility services performing clicks can now toggle the visibility of the playback controls which wasn't possible previously.

I am able to reproduce the problem you describe when using the PlayerView in a ViewPager. Note that this problem only occurs because the ViewPager intercepts all touch events as soon as you start a small movement. That means the ACTION_DOWN event is still forwarded to the PlayerView in such a case, but ACTION_UP isn't. That is also what you can see - the playback controls are toggled even if you swipe in the ViewPager. That's why I would also argue - given the current behaviour of PlayerView - that calling onClickListener is correct because PlayerView did indeed handle the click.

I think we should rather look into changing our default behaviour to not toggle on ACTION_DOWN. A quick look at other apps shows that all of them toggle playback controls on ACTION_UP only. So I mark this as an enhancement to change the default behaviour (and indirectly fix your issue).

@tonihei tonihei added enhancement and removed bug labels Apr 24, 2019
ojw28 pushed a commit that referenced this issue Apr 26, 2019
We currently toggle the view in onTouchEvent ACTION_DOWN which is non-standard
and causes problems when used in a ViewGroup intercepting touch events.

Switch to standard Android click handling instead which is also what most
other player apps are doing.

Issue:#5784
PiperOrigin-RevId: 245219728
@ojw28 ojw28 closed this as completed May 6, 2019
ojw28 pushed a commit that referenced this issue Jun 3, 2019
We currently toggle the view in onTouchEvent ACTION_DOWN which is non-standard
and causes problems when used in a ViewGroup intercepting touch events.

Switch to standard Android click handling instead which is also what most
other player apps are doing.

Issue:#5784
PiperOrigin-RevId: 245219728
@google google locked and limited conversation to collaborators Sep 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants