Click to play/pause video

[Philip Jägenstedt]

In Opera for Android, we've added a feature to play/pause a video simply
by clicking it, the meat of the "feature" being:

+void HTMLMediaElement::defaultEventHandler(Event* event)
+{
+ if (isVideo() && event->type() == eventNames().clickEvent &&
document()->page()
+ && document()->page()->chrome()->clickToPlayPauseVideo()) {
+ if (paused())
+ play();
+ else
+ pause();
+ return;
+ }
+ HTMLElement::defaultEventHandler(event);
+}

clickToPlayPauseVideo() returns true on Android. We believe this is an
improvement, because the controls (native of not) can often be rather
small and fiddly.

Is this a feature of any interest to other products, e.g. Chrome for
Android?

One issue I can foresee is that the page already does the same using
scripts, and that the default handler then interferes with this. The
script could preventDefault() to fix the problem, but existing content
probably does not. Is there an acceptable way around this, e.g. an easy
way to simply do nothing if there is an event handler attached to the
video element?

--
Philip Jägenstedt
Opera Software

[Anton Vayvod]

I generally like the idea. It's kind of consistent with what desktop players do: when you click on the video, it pauses/resumes.

[Adam Barth]

On Tue, Jun 18, 2013 at 2:53 AM, Philip Jägenstedt <phi...@opera.com> wrote:

In Opera for Android, we've added a feature to play/pause a video simply by clicking it, the meat of the "feature" being:

+void HTMLMediaElement::defaultEventHandler(Event* event)
+{
+    if (isVideo() && event->type() == eventNames().clickEvent && document()->page()
+        && document()->page()->chrome()->clickToPlayPauseVideo()) {

^^^ We can work this sort of thing out in a code review, but the clickToPlayPauseVideo setting should be on WebCore::Setting rather than on Chrome.

[Philip Jägenstedt]

On Tue, 18 Jun 2013 19:30:28 +0200, Adam Barth <aba...@chromium.org> wrote:

> On Tue, Jun 18, 2013 at 2:53 AM, Philip Jägenstedt
> <phi...@opera.com>wrote:
>
>> In Opera for Android, we've added a feature to play/pause a video simply
>> by clicking it, the meat of the "feature" being:
>>

>> +void HTMLMediaElement::**defaultEventHandler(Event* event)

>> +{
>> + if (isVideo() && event->type() == eventNames().clickEvent &&
>> document()->page()

>> + && document()->page()->chrome()->**clickToPlayPauseVideo()) {

>>
>
> ^^^ We can work this sort of thing out in a code review, but the
> clickToPlayPauseVideo setting should be on WebCore::Setting rather than
> on
> Chrome.

Yeah, I've noticed that this kind of use of chrome() in HTMLMediaElement
has completely gone away since I wrote the patch.

[Philip Jägenstedt]

Great, I've greated a bug and two code reviews:

http://code.google.com/p/chromium/issues/detail?id=251645
https://codereview.chromium.org/17391015
https://codereview.chromium.org/17448004

In the future, is it necessary to create a bug for each feature patch, or
is just a review OK?

Philip

On Tue, 18 Jun 2013 16:11:38 +0200, Anton Vayvod <ava...@chromium.org>
wrote:

> I generally like the idea. It's kind of consistent with what desktop
> players do: when you click on the video, it pauses/resumes.
>
>
> On Tue, Jun 18, 2013 at 10:53 AM, Philip Jägenstedt
> <phi...@opera.com>wrote:
>
>> In Opera for Android, we've added a feature to play/pause a video simply
>> by clicking it, the meat of the "feature" being:
>>

>> +void HTMLMediaElement::**defaultEventHandler(Event* event)

>> +{
>> + if (isVideo() && event->type() == eventNames().clickEvent &&
>> document()->page()

>> + && document()->page()->chrome()->**clickToPlayPauseVideo()) {

>> + if (paused())
>> + play();
>> + else
>> + pause();
>> + return;
>> + }

>> + HTMLElement::**defaultEventHandler(event);

[Dominic Mazzoni]

On Wed, Jun 19, 2013 at 2:01 AM, Philip Jägenstedt <phi...@opera.com> wrote:

In the future, is it necessary to create a bug for each feature patch, or is just a review OK?

No, it's fine to associate multiple patches with the same bug.

[Philip Jägenstedt]

Ah, that's good, but I actually meant to ask when it's required to create
a bug at all. There are commits in the history that lack a BUG= line or
have BUG=None, after all.

[Dominic Mazzoni]

On Wed, Jun 19, 2013 at 2:45 AM, Philip Jägenstedt <phi...@opera.com> wrote:

No, it's fine to associate multiple patches with the same bug.

Ah, that's good, but I actually meant to ask when it's required to create a bug at all. There are commits in the history that lack a BUG= line or have BUG=None, after all.

Please file a bug for anything nontrivial. In your specific case, the bug can describe the goal of your changes at a high level (rather than explaining the specific code details), and also the bug ties together your multiple patches in one place. Also, the bug is really helpful to keep track of the status if by any chance any of your patches is reverted, re-landed, merged, etc.

[Philip Jägenstedt]

Understood, thanks!

[Ojan Vafai]

I don't file bugs for every patch. Even the non-trivial ones. Bugs are useful for tracking large projects and their associated TODOs (e.g. implementing a new layout mode), fixing regressions or discussing things that aren't directly code related. I think it's fine if people really want to use bugs for every non-trivial patch, but I don't think it's required or expected.

In this specific case, I think it's useful to have a bug so that we can CC Chrome UI folks and get their thoughts on turning this feature on my generally, i.e. I'd like to see this feature always enabled everywhere, in which case we don't even need the setting.

[Anton Vayvod]

For reference (so we don't repeat the debate here), there was a thread about that on blink-dev sometime ago: https://groups.google.com/a/chromium.org/d/msg/blink-dev/8uFIGaO7RdI/NTJf0c6eMDoJ

[Xabier Rodriguez Calvar]

O Mér, 19-06-2013 ás 11:01 +0200, Philip Jägenstedt escribiu:
> Great, I've greated a bug and two code reviews:
>
> http://code.google.com/p/chromium/issues/detail?id=251645
> https://codereview.chromium.org/17391015
> https://codereview.chromium.org/17448004
>
> In the future, is it necessary to create a bug for each feature patch, or
> is just a review OK?

I had found a bug in WK sometime ago that I couldn't reproduce in Blink
at the moment (when clicking the volume bar in a video when inside a
MediaDocument, video was pausing because the event handler was not being
default handled when setting the volume) and I submitted the patch to
Blink too:
https://chromiumcodereview.appspot.com/14842016/

Maybe it becomes useful now.

Br.

[Philip Jägenstedt]

Thinking about this prompted me to write to the WHATWG about an old issue:

Should <video controls> generate click events?

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-June/039873.html

Input from Blink media developers on this would be useful.