Add support for clicking to play/pause <video>

Add support for clicking to play/pause <video>

It's helpful to be able to pause a video simply by tapping it, if the
pause button of the native or scripted controls is too small for
comfort.

BUG=251645

[philipj_slow, 2013-06-19 08:59:17]

Click to play/pause, as initially discussed in
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vlHmWhdSSDQ/ak_pwMp0...

The absence of tests is known, I'll add those if the feature itself is deemed
acceptable for inclusion.

[qinmin, 2013-06-19 16:01:02]

On 2013/06/19 08:59:17, philipj wrote:
> Click to play/pause, as initially discussed in
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vlHmWhdSSDQ/ak_pwMp0...
> 
> The absence of tests is known, I'll add those if the feature itself is deemed
> acceptable for inclusion.

I made a similar patch to WebKit before, so I don't think we should submit this.
see https://bugs.webkit.org/show_bug.cgi?id=90498
My previous patch was rejected by Eric Carlson. That's why we introduced the
Overlay play button.
The problem of adding a handler to HTMLMediaElement is that is often messes up
with other touch handlers and cause many pages to break.

For example, lots of websites have click listeners on the div element
encompassing the video. So when clicking the video rect, it will call
video.play(). And your change will make the click being handled twice, thus
nothing happens.

[philipj_slow, 2013-06-20 07:06:36]

On 2013/06/19 16:01:02, qinmin wrote:
> On 2013/06/19 08:59:17, philipj wrote:
> > Click to play/pause, as initially discussed in
> >
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/vlHmWhdSSDQ/ak_pwMp0...
> > 
> > The absence of tests is known, I'll add those if the feature itself is
deemed
> > acceptable for inclusion.
> 
> I made a similar patch to WebKit before, so I don't think we should submit
this.
> see https://bugs.webkit.org/show_bug.cgi?id=90498
> My previous patch was rejected by Eric Carlson. That's why we introduced the
> Overlay play button.
> The problem of adding a handler to HTMLMediaElement is that is often messes up
> with other touch handlers and cause many pages to break.
> 
> For example, lots of websites have click listeners on the div element
> encompassing the video. So when clicking the video rect, it will call
> video.play(). And your change will make the click being handled twice, thus
> nothing happens.

Yeah, I assumed this would come up. However, we haven't run into any pages that
break yet, do you remember any that broke in your trial of this?

Would it be accpetable to check !hasEventListeners(eventNames().clickEvent), or
is that too simplistic? Another approach is to not toggle the play state if it's
already been toggled in this event handling "thread", by setting a
m_toggledPlayState flag in updatePlayState and resetting it after awaiting a
stable state. If the m_toggledPlayState flag is set in the default event
handler, then do nothing.

One difference between your patch and mine is that mine also pauses video.
Assuming none of the above suggestions pan out, would it be OK to tweak <video
controls> to play/pause when clicking any part of the video, or are there known
sites which both use <video controls> and implement play/pause in their own
click handler?

[philipj_slow, 2013-06-20 07:49:26]

I wrote http://people.opera.com/~philipj/click.html to test the problem a bit.
It is indeed the case that the script and default event handler cancel each
other out. However, I noticed that in Chrome for Android the same is true when
using the play/pause button in the lower left of the native controls (not the
overlay). Is it a known bug that click events are dispatched even when the
controls are clicked?

[qinmin, 2013-06-20 13:46:20]

On 2013/06/20 07:49:26, philipj wrote:
> I wrote http://people.opera.com/%7Ephilipj/click.html to test the problem a
bit.
> It is indeed the case that the script and default event handler cancel each
> other out. However, I noticed that in Chrome for Android the same is true when
> using the play/pause button in the lower left of the native controls (not the
> overlay). Is it a known bug that click events are dispatched even when the
> controls are clicked?

Somewhere the js has to call preventdefault on the event, or otherwise the
default behavior will be called.
However, not too many people uses preventdefault. and the clicklistener can be
on some other elements, so HtmlMediaElement does not know whether there is a
event listener.

[philipj_slow, 2013-06-26 06:29:27]

I thought a bit about this over the weekend, and what it comes down to is that
it's currently too difficult to pause a video. How about adding an overlay pause
button the same size as the overlay play button, that is visible for 1 second
(or so) after interacting with the controls or touching the video? Essentially
the same as what the Android YouTube app does, that is.

[qinmin, 2013-06-26 14:12:45]

On 2013/06/26 06:29:27, philipj wrote:
> I thought a bit about this over the weekend, and what it comes down to is that
> it's currently too difficult to pause a video. How about adding an overlay
pause
> button the same size as the overlay play button, that is visible for 1 second
> (or so) after interacting with the controls or touching the video? Essentially
> the same as what the Android YouTube app does, that is.

hmm... So doing a seek will also bring the pause button up?  
or another choice is that after overlay play button is gone, the first time
touching the overlay enclosure will show the pause button for a sec, and
clicking it will pause the video

[philipj_slow, 2013-06-27 06:40:40]

I can think of two ways that would make sense, after adding a pause state to the
overlay button:

1. Auto-hide the entire controls after x seconds of playback. Touching anywhere
will show the controls.

2. Just auto-hide the overlay after x seconds of playback. Touching anywhere
except the visible controls will show the overlay.

Option 1 is what I was intending in the previous comment, and more like what
YouTube does.

[qinmin, 2013-06-27 13:46:22]

On 2013/06/27 06:40:40, philipj wrote:
> I can think of two ways that would make sense, after adding a pause state to
the
> overlay button:
> 
> 1. Auto-hide the entire controls after x seconds of playback. Touching
anywhere
> will show the controls.
> 
> 2. Just auto-hide the overlay after x seconds of playback. Touching anywhere
> except the visible controls will show the overlay.
> 
> Option 1 is what I was intending in the previous comment, and more like what
> YouTube does.

Currently desktop use mouse hover to detect whether it should hide controls. For
android, hover does not make too much sense. 
You can probably use focus, or do something simiIar to
hideFullscreenControlsTimerFired in your own MediaControl files.

[philipj_slow, 2013-06-27 14:27:27]

On 2013/06/27 13:46:22, qinmin wrote:
> On 2013/06/27 06:40:40, philipj wrote:
> > I can think of two ways that would make sense, after adding a pause state to
> the
> > overlay button:
> > 
> > 1. Auto-hide the entire controls after x seconds of playback. Touching
> anywhere
> > will show the controls.
> > 
> > 2. Just auto-hide the overlay after x seconds of playback. Touching anywhere
> > except the visible controls will show the overlay.
> > 
> > Option 1 is what I was intending in the previous comment, and more like what
> > YouTube does.
> 
> Currently desktop use mouse hover to detect whether it should hide controls.
For
> android, hover does not make too much sense. 
> You can probably use focus, or do something simiIar to
> hideFullscreenControlsTimerFired in your own MediaControl files.

Do you think that showing the controls when touching anywhere on the video is
too obscure? If so, I'm fine with only hiding the overlay pause button and
letting people use fullscreen when they want a completely unobscured view.

[qinmin, 2013-06-27 14:50:42]

On 2013/06/27 14:27:27, philipj wrote:
> On 2013/06/27 13:46:22, qinmin wrote:
> > On 2013/06/27 06:40:40, philipj wrote:
> > > I can think of two ways that would make sense, after adding a pause state
to
> > the
> > > overlay button:
> > > 
> > > 1. Auto-hide the entire controls after x seconds of playback. Touching
> > anywhere
> > > will show the controls.
> > > 
> > > 2. Just auto-hide the overlay after x seconds of playback. Touching
anywhere
> > > except the visible controls will show the overlay.
> > > 
> > > Option 1 is what I was intending in the previous comment, and more like
what
> > > YouTube does.
> > 
> > Currently desktop use mouse hover to detect whether it should hide controls.
> For
> > android, hover does not make too much sense. 
> > You can probably use focus, or do something simiIar to
> > hideFullscreenControlsTimerFired in your own MediaControl files.
> 
> Do you think that showing the controls when touching anywhere on the video is
> too obscure? If so, I'm fine with only hiding the overlay pause button and
> letting people use fullscreen when they want a completely unobscured view.

For android, having a timer to hide all the controls is a reasonable solution.
And when user hit the video, i think it makes sense to show the controls again,
unless the controls attribute is not set.

[philipj_slow, 2013-06-28 06:58:36]

On 2013/06/27 14:50:42, qinmin wrote:
> 
> For android, having a timer to hide all the controls is a reasonable solution.
> And when user hit the video, i think it makes sense to show the controls
again,
> unless the controls attribute is not set.

That sounds good to me. To verify, I will prepare two patches:

1. Auto-hide the controls after a timeout when playing, and show them again when
touching any part of the video.

2. Add a pause state to the big overlay button.

Both assuming the controls attribute is present, of course. Sound good?

[qinmin, 2013-06-28 13:44:28]

On 2013/06/28 06:58:36, philipj wrote:
> On 2013/06/27 14:50:42, qinmin wrote:
> > 
> > For android, having a timer to hide all the controls is a reasonable
solution.
> > And when user hit the video, i think it makes sense to show the controls
> again,
> > unless the controls attribute is not set.
> 
> That sounds good to me. To verify, I will prepare two patches:
> 
> 1. Auto-hide the controls after a timeout when playing, and show them again
when
> touching any part of the video.
> 
> 2. Add a pause state to the big overlay button.
> 
> Both assuming the controls attribute is present, of course. Sound good?

Yes, sounds good to me

[philipj_slow, 2013-07-03 13:57:03]

@qinmin, I've created https://code.google.com/p/chromium/issues/detail?id=257030
and will drop this review. Do you think that issue 251645 should be closed as
well, or can that be reused for the overlay pause button?

[qinmin, 2013-07-03 16:43:30]

On 2013/07/03 13:57:03, philipj wrote:
> @qinmin, I've created
https://code.google.com/p/chromium/issues/detail?id=257030
> and will drop this review. Do you think that issue 251645 should be closed as
> well, or can that be reused for the overlay pause button?

you can set 251645 to be clocked by 257030. So you can submit CLs only fixing
the latter but not the former.

[philipj_slow, 2013-07-04 08:47:28]

On 2013/07/03 16:43:30, qinmin wrote:
> On 2013/07/03 13:57:03, philipj wrote:
> > @qinmin, I've created
> https://code.google.com/p/chromium/issues/detail?id=257030
> > and will drop this review. Do you think that issue 251645 should be closed
as
> > well, or can that be reused for the overlay pause button?
> 
> you can set 251645 to be clocked by 257030. So you can submit CLs only fixing
> the latter but not the former.

I'm afraid I don't have very high privileges in the bug tracker, could you help
me link them? Do you know who to ask in order to get editing rights?