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

Source/core/html/shadow/MediaControls.cpp

 /*
  * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 173 matching lines @@
     makeOpaque();
 }
 
 void MediaControls::show()
 {
     makeOpaque();
     m_panel->setIsDisplayed(true);
     m_panel->show();
 }
 
+void MediaControls::mediaElementFocused()
+{
+    show();
+    stopHideMediaControlsTimer();
+}
+
 void MediaControls::hide()
 {
     m_panel->setIsDisplayed(false);
     m_panel->hide();
 }
 
 void MediaControls::makeOpaque()
 {
     m_panel->makeOpaque();
 }
 
 void MediaControls::makeTransparent()
 {
     m_panel->makeTransparent();
 }
 
-bool MediaControls::shouldHideMediaControls()
+bool MediaControls::shouldHideMediaControls(HideBehavior behavior) const
 {
-    return !m_panel->hovered();
+    // Never hide for <audio>.

[philipj_slow, 2014/05/23 13:22:43]

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.

[fs, 2014/05/23 14:39:30]

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

+    if (!mediaElement().hasVideo())
+        return false;
+    // Don't hide if hovered or the mouse is over the controls.
+    if (m_panel->hovered() || m_isMouseOverControls)

[philipj_slow, 2014/05/23 13:22:43]

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

[fs, 2014/05/23 14:39:30]

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

+        return false;
+    // Don't hide if focus is on the HTMLMediaElement or within the
+    // controls/shadow tree. (Perform the checks separately to avoid going

[philipj_slow, 2014/05/23 13:22:43]

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:39:30]

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.

[philipj_slow, 2014/05/23 20:08:34]

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 :)

+    // through all the potential ancestor hosts for the focused element.)
+    if (behavior != IgnoreFocus && (mediaElement().focused() || contains(document().focusedElement())))
+        return false;
+    return true;
 }
 
 void MediaControls::playbackStarted()
 {
     m_currentTimeDisplay->show();
     m_durationDisplay->hide();
 
     updatePlayState();
     m_timeline->setPosition(mediaElement().currentTime());
     updateCurrentTimeDisplay();
 
     startHideMediaControlsTimer();
 }
 
 void MediaControls::playbackProgressed()
 {
     m_timeline->setPosition(mediaElement().currentTime());
     updateCurrentTimeDisplay();
 
-    if (!m_isMouseOverControls && mediaElement().hasVideo())
+    if (shouldHideMediaControls())
         makeTransparent();
 }
 
 void MediaControls::playbackStopped()
 {
     updatePlayState();
     m_timeline->setPosition(mediaElement().currentTime());
     updateCurrentTimeDisplay();
     makeOpaque();
 
@@ skipping 119 matching lines @@
             startHideMediaControlsTimer();
         return;
     }
 }
 
 void MediaControls::hideMediaControlsTimerFired(Timer<MediaControls>*)
 {
     if (mediaElement().togglePlayStateWillPlay())
         return;
 
-    if (!shouldHideMediaControls())
+    if (!shouldHideMediaControls(IgnoreFocus))

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

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]

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).
To make them re-appear again in that situation, you'd need to re-focus the
<video>.

         return;
 
     makeTransparent();
 }
 
 void MediaControls::startHideMediaControlsTimer()
 {
     m_hideMediaControlsTimer.startOneShot(timeWithoutMouseMovementBeforeHidingMediaControls, FROM_HERE);
 }
 
@@ skipping 68 matching lines @@
     visitor->trace(m_muteButton);
     visitor->trace(m_volumeSlider);
     visitor->trace(m_toggleClosedCaptionsButton);
     visitor->trace(m_fullScreenButton);
     visitor->trace(m_durationDisplay);
     visitor->trace(m_enclosure);
     HTMLDivElement::trace(visitor);
 }
 
 }