Implement heuristic for showing media controls during playback w/o a mouse

Implement heuristic for showing media controls during playback w/o a mouse

This implements a heuristic that allows the media controls to re-appear
when playback has started and the mouse is unavailable.
The behavior is triggered by a 'focusin' event on the HTMLMediaElement,
which shows the controls. The controls then remain visible as long as
the media element itself or any part of the control retains focus, or
until the hide timer fires (which it will on a pause-play "cycle" for
instance).

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

[acolwell GONE FROM CHROMIUM, 2014-05-22 00:31:16]

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:196: show();
Should we have a stopHideMediaControlsTimer() call here?

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:247: if (mediaElement().hasVideo() &&
!shouldShowMediaControls())
!shouldShowMediaControls() conceptually seems like it should be equivalent to
shouldHideMediaControls(). Can we combine these into 1 function somehow so
different code paths don't end up disagreeing on whether the controls should be
visible or not? 

Also FYI https://codereview.chromium.org/296743007/ adds the hasVideo()
condition to shouldHideMediaControls() so that is one less check you'll have to
do here.

[fs, 2014-05-22 15:48:44]

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:196: show();
On 2014/05/22 00:31:17, acolwell wrote:
> Should we have a stopHideMediaControlsTimer() call here?

Probably makes sense. Added.

https://codereview.chromium.org/297783004/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControls.cpp:247: if (mediaElement().hasVideo() &&
!shouldShowMediaControls())
On 2014/05/22 00:31:17, acolwell wrote:
> !shouldShowMediaControls() conceptually seems like it should be equivalent to
> shouldHideMediaControls(). Can we combine these into 1 function somehow so
> different code paths don't end up disagreeing on whether the controls should
be
> visible or not? 

I integrated should_Show_MediaControls in should_Hide_MediaControls (dropping
the hasVideo() check here per below).
 
> Also FYI https://codereview.chromium.org/296743007/ adds the hasVideo()
> condition to shouldHideMediaControls() so that is one less check you'll have
to
> do here.

I added that bit here, and will rebase and fix up as needed.

[acolwell GONE FROM CHROMIUM, 2014-05-22 18:44:09]

looks good. Just have a few questions.

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:389: if
(!shouldHideMediaControls(IgnoreFocus))
What is the situation where we would want the controls to hide yet we still have
focus? What would the user have to do to make them appear again?

[fs, 2014-05-23 07:05:40]

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:389: if
(!shouldHideMediaControls(IgnoreFocus))
On 2014/05/22 18:44:09, acolwell wrote:
> What is the situation where we would want the controls to hide yet we still
have
> focus?

The case where I found this a useful behavior was when navigating to the <video>
by keyboard (<tab>) and then selecting the play button and activating it
(starting playback). Without this tweak, the controls would then stay visible.
It felt like a reasonable behavior to me (as opposed to having to blur to get
the controls to disappear), but that's of course just an opinion. I'd be happy
to remove this tweak if it's felt that non-tweaked behavior is better (or just
leave that undecided for later).

> What would the user have to do to make them appear again?

To make them re-appear again in that situation, you'd need to re-focus the
<video>.

[philipj_slow, 2014-05-23 11:28:41]

On 2014/05/23 07:05:40, fs wrote:
>
https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
> File Source/core/html/shadow/MediaControls.cpp (right):
> 
>
https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
> Source/core/html/shadow/MediaControls.cpp:389: if
> (!shouldHideMediaControls(IgnoreFocus))
> On 2014/05/22 18:44:09, acolwell wrote:
> > What is the situation where we would want the controls to hide yet we still
> have
> > focus?
> 
> The case where I found this a useful behavior was when navigating to the
<video>
> by keyboard (<tab>) and then selecting the play button and activating it
> (starting playback). Without this tweak, the controls would then stay visible.
> It felt like a reasonable behavior to me (as opposed to having to blur to get
> the controls to disappear), but that's of course just an opinion. I'd be happy
> to remove this tweak if it's felt that non-tweaked behavior is better (or just
> leave that undecided for later).

I think this seems useful as well. I'm not sure what makes sense if you start
playback and then tab your way to (say) the volume controls.

> > What would the user have to do to make them appear again?
> 
> To make them re-appear again in that situation, you'd need to re-focus the
> <video>.

Would pressing tab once do the trick, or will that also move the tab position
forward?

[fs, 2014-05-23 11:58:49]

On 2014/05/23 11:28:41, philipj wrote:
> On 2014/05/23 07:05:40, fs wrote:
> >
>
https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
> > File Source/core/html/shadow/MediaControls.cpp (right):
> > 
> >
>
https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
> > Source/core/html/shadow/MediaControls.cpp:389: if
> > (!shouldHideMediaControls(IgnoreFocus))
> > On 2014/05/22 18:44:09, acolwell wrote:
> > > What is the situation where we would want the controls to hide yet we
still
> > have
> > > focus?
> > 
> > The case where I found this a useful behavior was when navigating to the
> <video>
> > by keyboard (<tab>) and then selecting the play button and activating it
> > (starting playback). Without this tweak, the controls would then stay
visible.
> > It felt like a reasonable behavior to me (as opposed to having to blur to
get
> > the controls to disappear), but that's of course just an opinion. I'd be
happy
> > to remove this tweak if it's felt that non-tweaked behavior is better (or
just
> > leave that undecided for later).
> 
> I think this seems useful as well. I'm not sure what makes sense if you start
> playback and then tab your way to (say) the volume controls.

I suspect that "sense" is a (wildly) variable term in these cases =). Anyway,
what should happen in that case is that HTMLMediaElement ends up calling
mediaElementFocused() which cancels the timeout and the controls remain visible.
 
> > > What would the user have to do to make them appear again?
> > 
> > To make them re-appear again in that situation, you'd need to re-focus the
> > <video>.
> 
> Would pressing tab once do the trick, or will that also move the tab position
> forward?

I don't think so (don't have a build with this readily available ATM to test).
It depends a bit on where focus transfers to when the controls (which will have
focus) are hidden. (If focus does end up on <video>, you'll still be unable to
get to the controls since they are not visible. If focus is on the document,
then you'll need to re-navigate to <video>. Etc.) Anyway, "stealing" tab would
be bad in any case.

[philipj_slow, 2014-05-23 13:05:34]

Maybe a test for where the tab focus ends up when the controls fade out would be
in order, so that in the unlikely event that it currently does the right thing,
it continues to do so.

[philipj_slow, 2014-05-23 13:22:42]

LGTM % comments and maybe that test I mentioned

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-controls-show-on-focus.html (right):

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-show-on-focus.html:15:
document.querySelector("video").addEventListener("timeupdate", function(event) {
The { should go on a new line after function in tests that follow WebKit style,
acolwell has told me.

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-show-on-focus.html:27:
testExpected("waitingForFade", true);
This looks a bit on the paranoid side :)

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:218: // Never hide for <audio>.
The comment doesn't quite match the code. isHTMLAudioElement(mediaElement())
would match the comment, but I suppose that treating video-less <video> like
<audio> is OK too.

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:222: if (m_panel->hovered() ||
m_isMouseOverControls)
Almost completely off topic: When I've seen m_isMouseOverControls before I
haven't understood why using hovered() instead wouldn't work. Until now I
assumed I didn't understand how hovered work with nested elements, but this demo
makes it seem like it should work:
http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3036

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:225: // controls/shadow tree. (Perform
the checks separately to avoid going
I feel like this comment should make me understand, but it doesn't. Using just
contains(document().focusedElement()) looks like it should be just as cheap
looking at how Document::focusedElement() and Node::contains() is implemented.
What are these ancestor hosts the comment refers to?

[fs, 2014-05-23 14:17:21]

On 2014/05/23 13:05:34, philipj wrote:
> Maybe a test for where the tab focus ends up when the controls fade out would
be
> in order, so that in the unlikely event that it currently does the right
thing,
> it continues to do so.

Added (and the answer is "no element" == the document -> activeElement reports
the body.)

[fs, 2014-05-23 14:39:29]

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-controls-show-on-focus.html (right):

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-show-on-focus.html:15:
document.querySelector("video").addEventListener("timeupdate", function(event) {
On 2014/05/23 13:22:43, philipj wrote:
> The { should go on a new line after function in tests that follow WebKit
style,
> acolwell has told me.

I thought these were off the hook since they're "anonymous" ;-). Changed.

https://codereview.chromium.org/297783004/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-controls-show-on-focus.html:27:
testExpected("waitingForFade", true);
On 2014/05/23 13:22:43, philipj wrote:
> This looks a bit on the paranoid side :)

Dropped.

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:218: // Never hide for <audio>.
On 2014/05/23 13:22:43, philipj wrote:
> The comment doesn't quite match the code. isHTMLAudioElement(mediaElement())
> would match the comment, but I suppose that treating video-less <video> like
> <audio> is OK too.

Will change to something more long-winded about "visual representation" or
somesuch.

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:222: if (m_panel->hovered() ||
m_isMouseOverControls)
On 2014/05/23 13:22:43, philipj wrote:
> Almost completely off topic: When I've seen m_isMouseOverControls before I
> haven't understood why using hovered() instead wouldn't work. Until now I
> assumed I didn't understand how hovered work with nested elements, but this
demo
> makes it seem like it should work:
> http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3036

Yes... Personally I'm more worried that this is too broad for some cases. (Since
the predicate used in playbackProgressed() was wildly different before.)

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:225: // controls/shadow tree. (Perform
the checks separately to avoid going
On 2014/05/23 13:22:43, philipj wrote:
> I feel like this comment should make me understand, but it doesn't. Using just
> contains(document().focusedElement()) looks like it should be just as cheap
> looking at how Document::focusedElement() and Node::contains() is implemented.
> What are these ancestor hosts the comment refers to?

contains(...) will not cross into the shadow tree. There's a
containsIncludingShadowDOM(...) which does the same as contains(...) but does
traverse the ancestor (shadow) host chain.

[fs, 2014-05-23 15:36:37]

On 2014/05/23 14:39:29, fs wrote:
>
https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
> Source/core/html/shadow/MediaControls.cpp:222: if (m_panel->hovered() ||
> m_isMouseOverControls)
> On 2014/05/23 13:22:43, philipj wrote:
> > Almost completely off topic: When I've seen m_isMouseOverControls before I
> > haven't understood why using hovered() instead wouldn't work. Until now I
> > assumed I didn't understand how hovered work with nested elements, but this
> demo
> > makes it seem like it should work:
> > http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3036
> 
> Yes... Personally I'm more worried that this is too broad for some cases.
(Since
> the predicate used in playbackProgressed() was wildly different before.)

Got the feeling that something might've regressed around here - which it had.
Fixed and added a test.

[acolwell GONE FROM CHROMIUM, 2014-05-23 18:26:28]

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-controls-focus-movement-on-hide.html (right):

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-controls-focus-movement-on-hide.html:28: if
(video.currentTime < 4)
nit: I think we should define a constant in media-controls.js to indicate that
this is enough time for the controls to auto-hide. I think that will make it
easier to fix broken tests if this value ever changes.

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-controls-hide-on-move-outside-controls.html
(right):

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-controls-hide-on-move-outside-controls.html:20: if
(video.currentTime < 4)
ditto. Actually, I wonder if we should put this functionality in some sort of
helper function and make it setTimeout() based since technically the fade out
isn't dependent on elapsed playback time. We just have to make sure that we are
playing. 

I realize now that the test I added set a bad example to follow. :)

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
File LayoutTests/media/video-controls-show-on-focus.html (right):

https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
LayoutTests/media/video-controls-show-on-focus.html:13: var fadeinTime = 300;
I think we should make this a constant in media-controls.js too.

[philipj_slow, 2014-05-23 20:08:33]

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/MediaControls.cpp (right):

https://codereview.chromium.org/297783004/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/MediaControls.cpp:225: // controls/shadow tree. (Perform
the checks separately to avoid going
On 2014/05/23 14:39:30, fs wrote:
> On 2014/05/23 13:22:43, philipj wrote:
> > I feel like this comment should make me understand, but it doesn't. Using
just
> > contains(document().focusedElement()) looks like it should be just as cheap
> > looking at how Document::focusedElement() and Node::contains() is
implemented.
> > What are these ancestor hosts the comment refers to?
> 
> contains(...) will not cross into the shadow tree. There's a
> containsIncludingShadowDOM(...) which does the same as contains(...) but does
> traverse the ancestor (shadow) host chain.

Ah, that's what I overlooked, thanks! The comment made me think it had something
to do with avoiding useless traversal, which I couldn't find :)

[philipj_slow, 2014-05-23 20:31:03]

PS7 LGTM, leaving the final say to acolwell.

[acolwell GONE FROM CHROMIUM, 2014-05-24 01:12:42]

On 2014/05/23 20:31:03, philipj wrote:
> PS7 LGTM, leaving the final say to acolwell.

I'm out on vacation next week so I won't be able to do another pass on this. The
code changes LGTM. If you could address the test comments that would be great,
but I'm fine with doing it in a followup CL when I get back if necessary.

[fs, 2014-05-26 07:34:17]

On 2014/05/24 01:12:42, acolwell_OOO_5-24_to_5-30 wrote:
> On 2014/05/23 20:31:03, philipj wrote:
> > PS7 LGTM, leaving the final say to acolwell.
> 
> I'm out on vacation next week so I won't be able to do another pass on this.
The
> code changes LGTM. If you could address the test comments that would be great,
> but I'm fine with doing it in a followup CL when I get back if necessary.

Thanks (both). I think I'll do that as a followup CL.

[fs, 2014-05-26 07:34:35]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-05-26 07:34:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/297783004/120001

[commit-bot: I haz the power, 2014-05-26 09:03:59]

Change committed as 174794

[fs, 2014-05-26 13:22:03]

On 2014/05/23 18:26:28, acolwell_OOO_5-24_to_5-30 wrote:
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> File LayoutTests/media/video-controls-focus-movement-on-hide.html (right):
> 
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> LayoutTests/media/video-controls-focus-movement-on-hide.html:28: if
> (video.currentTime < 4)
> nit: I think we should define a constant in media-controls.js to indicate that
> this is enough time for the controls to auto-hide. I think that will make it
> easier to fix broken tests if this value ever changes.
> 
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> File LayoutTests/media/video-controls-hide-on-move-outside-controls.html
> (right):
> 
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> LayoutTests/media/video-controls-hide-on-move-outside-controls.html:20: if
> (video.currentTime < 4)
> ditto. Actually, I wonder if we should put this functionality in some sort of
> helper function and make it setTimeout() based since technically the fade out
> isn't dependent on elapsed playback time. We just have to make sure that we
are
> playing. 
> 
> I realize now that the test I added set a bad example to follow. :)
> 
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> File LayoutTests/media/video-controls-show-on-focus.html (right):
> 
>
https://codereview.chromium.org/297783004/diff/120001/LayoutTests/media/video...
> LayoutTests/media/video-controls-show-on-focus.html:13: var fadeinTime = 300;
> I think we should make this a constant in media-controls.js too.

https://codereview.chromium.org/302603003/