Implement the activation behavior of media elements (click to play/pause)

Implement the activation behavior of media elements (click to play/pause)

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#user-interface

Added to the spec in http://html5.org/r/8315 after a long discussion:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2013-November/041662.html

Firefox already implements click to play/pause when controls are
visible, but that implementation predates the spec so accesskey does not
work.

HTMLMediaElement::togglePlayState() is made public so that it can be
used to implement play/pause in MediaControls also.

The video-click-dblckick-standalone.html test has been expected to fail
since April 2013, r148246, so remove it now since the the results would
change. The new tests provide sufficient coverage.

BUG=354746
TEST=LayoutTests/media/activation-behavior.html
     LayoutTests/media/activation-behavior-accesskey.html
     LayoutTests/media/activation-behavior-shadow.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169812

[philipj_slow, 2014-03-21 16:40:29]

PTAL. It was a pleasant surprise to find that Hixie had specified how to best
toggle a MediaController, so a follow-up to this is going to be using this logic
internally in MediaControls as well instead of what we have.

I've manually tested that the overlay play button still works, since it would be
easy to break with this kind of change.

[philipj_slow, 2014-03-21 16:46:47]

Also, it'll be possible to let the overlay play button just be an image, since
clicking anywhere is already going to play/pause. I just didnt' want to mix too
much controls changes into this CL.

[acolwell GONE FROM CHROMIUM, 2014-03-21 23:30:12]

lgtm

https://codereview.chromium.org/208483002/diff/1/LayoutTests/media/activation...
File LayoutTests/media/activation-behavior.html (right):

https://codereview.chromium.org/208483002/diff/1/LayoutTests/media/activation...
LayoutTests/media/activation-behavior.html:52: assert_false(e.paused, 'element
paused state after click');
This is really weird. I'm surprised that calling play() will call play() on all
the elements, but calling pause() doesn't seem to do anything to actually pause
the elements.

https://codereview.chromium.org/208483002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/208483002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3460: if (event->type() ==
EventTypeNames::click && controls()) {
nit: Should we use willRespondToMouseClickEvents() here so that this code and
the result of that method can't get out of sync?

https://codereview.chromium.org/208483002/diff/1/Source/core/html/MediaContro...
File Source/core/html/MediaController.cpp (right):

https://codereview.chromium.org/208483002/diff/1/Source/core/html/MediaContro...
Source/core/html/MediaController.cpp:511: allPaused = false;
I think you can just return false here and return true at the end of the
function.

[philipj_slow, 2014-03-23 03:42:48]

https://codereview.chromium.org/208483002/diff/1/LayoutTests/media/activation...
File LayoutTests/media/activation-behavior.html (right):

https://codereview.chromium.org/208483002/diff/1/LayoutTests/media/activation...
LayoutTests/media/activation-behavior.html:52: assert_false(e.paused, 'element
paused state after click');
On 2014/03/21 23:30:12, acolwell wrote:
> This is really weird. I'm surprised that calling play() will call play() on
all
> the elements, but calling pause() doesn't seem to do anything to actually
pause
> the elements.

MediaController is non-trivial, I was surprised as well!

MediaController paused state only overrides the elements:
MediaController::pause() -> MediaController::reportControllerState() ->
MediaController::updatePlaybackState() ->MediaController::updateMediaElements()
-> HTMLMediaElement::updatePlayState() -> HTMLMediaElement::potentiallyPlaying()
-> HTMLMediaElement::isBlockedOnMediaController() ->
MediaController::isBlocked(), which now returns true because it's paused.

In other words, pause() and unpause() go together, play() is the special one.

https://codereview.chromium.org/208483002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/208483002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.cpp:3460: if (event->type() ==
EventTypeNames::click && controls()) {
On 2014/03/21 23:30:12, acolwell wrote:
> nit: Should we use willRespondToMouseClickEvents() here so that this code and
> the result of that method can't get out of sync?

Done.

https://codereview.chromium.org/208483002/diff/1/Source/core/html/MediaContro...
File Source/core/html/MediaController.cpp (right):

https://codereview.chromium.org/208483002/diff/1/Source/core/html/MediaContro...
Source/core/html/MediaController.cpp:511: allPaused = false;
On 2014/03/21 23:30:12, acolwell wrote:
> I think you can just return false here and return true at the end of the
> function.

I had difficulties parsing the spec definition and even filed a bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25105

I concluded that the condition should be "playing && noneBlocked &&
(anyAutoplayingAndPaused || allPaused)" but actually still failed to implement
that correctly, and failed to catch it with my tests.

I've fix this and added tests until I couldn't change any part of the function
without causing a failure.

[philipj_slow, 2014-03-23 10:59:14]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-03-23 10:59:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/208483002/30001

[commit-bot: I haz the power, 2014-03-23 11:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-23 11:00:35]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[philipj_slow, 2014-03-23 13:05:32]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-03-23 13:05:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/208483002/30001

[commit-bot: I haz the power, 2014-03-23 13:32:31]

Change committed as 169812

[qinmin, 2014-04-02 19:02:46]

On 2014/03/23 13:32:31, I haz the power (commit-bot) wrote:
> Change committed as 169812

This default handler thing will interfere with any javascript that put an
onclick listener on the video element.

[philipj_slow, 2014-04-02 21:55:39]

On 2014/04/02 19:02:46, qinmin wrote:
> On 2014/03/23 13:32:31, I haz the power (commit-bot) wrote:
> > Change committed as 169812
> 
> This default handler thing will interfere with any javascript that put an
> onclick listener on the video element.

Per spec the activation behavior only applies when showing controls, so it's
probably not so bad. It would be a lot better without the bug I introduced
though :) https://code.google.com/p/chromium/issues/detail?id=358675