Tap on the video element toggles controls visibility

Source/core/html/shadow/MediaControls.cpp

Index: Source/core/html/shadow/MediaControls.cpp
diff --git a/Source/core/html/shadow/MediaControls.cpp b/Source/core/html/shadow/MediaControls.cpp
index 2f827bd203aee4f1c7007dc251071ed56f3c3b56..d107aacd99ae3690ac50f86513ebe1513ca1990b 100644
--- a/Source/core/html/shadow/MediaControls.cpp
+++ b/Source/core/html/shadow/MediaControls.cpp
@@ -360,6 +360,11 @@ void MediaControls::textTracksChanged()
     refreshClosedCaptionsButtonVisibility();
 }
 
+void MediaControls::timelineKnobDragged()
+{
+    resetHideMediaControlsTimer();
+}
+
 static Element* elementFromCenter(Element& element)
 {
     RefPtrWillBeRawPtr<ClientRect> clientRect = element.getBoundingClientRect();
@@ -435,6 +440,17 @@ void MediaControls::stoppedCasting()
     m_overlayCastButton->setIsPlayingRemotely(false);
 }
 
+bool MediaControls::isInactiveElement(Node* node) const

[philipj_slow, 2015/02/16 10:45:19]

If isn't mate moot by other changes, would willRespondToMouseClickEvents() serve
the same purpose?

+{
+    if (node == m_mediaElement
+        || node == m_textDisplayContainer
+        || node == m_overlayEnclosure
+        || node == m_enclosure)
+        return true;
+
+    return false;
+}
+
 void MediaControls::defaultEventHandler(Event* event)
 {
     HTMLDivElement::defaultEventHandler(event);
@@ -443,11 +459,40 @@ void MediaControls::defaultEventHandler(Event* event)
     // to allow the hide-timer to do the right thing when it fires.
     // FIXME: Preferably we would only do this when we're actually handling the event
     // here ourselves.
+    bool mouseIsSynthetic = event->isMouseEvent() && toMouseEvent(event)->fromTouch();

[philipj_slow, 2015/02/16 10:45:19]

Perhaps isMouseEventFromTouch will make sound less like there's a virtual mouse
or something involved.

     bool wasLastEventTouch = event->isTouchEvent() || event->isGestureEvent()

[philipj_slow, 2015/02/16 10:45:18]

I just renamed this, but actually isTouchEvent would be more accurate now.

-        || (event->isMouseEvent() && toMouseEvent(event)->fromTouch());
+        || mouseIsSynthetic;
+
     m_hideTimerBehaviorFlags |= wasLastEventTouch ? IgnoreControlsHover : IgnoreNone;
 
-    if (event->type() == EventTypeNames::mouseover) {
+    // Separate tap gestures from mouse clicks unless the tap
+    // is intended for an input element like a button.
+    if (event->type() == EventTypeNames::gesturetap) {

[philipj_slow, 2015/02/16 10:45:19]

Did you first try using the touchstart and click events for showing and hiding
respectively? GestureEvent was added to WebKit in 2012
<https://trac.webkit.org/changeset/123799> with the comment "This change does
not expose the gesture events to JavaScript, since there is no spec for that at
the moment."

There's still no spec and AFAICT the only other place where this is used
internally is HTMLSelectElement::listBoxDefaultEventHandler, so if there's any
way at all of avoiding GestureEvent, that seems worthwhile.

[Tima Vaisburd, 2015/02/16 20:32:11]

No, I must admit I haven't tried the touchstart event at all. Moreover, only
recently did I realize that touchstart, touchend, touchmove and touchcancel
events do not propagate to the renderer process right now. We need to add a
TouchEvent handler to the EventHandlerRegistry for that.
On the other hand processing these events seems necessary for the implementation
of YouTube-like behavior that you mentioned earlier. I guess this consideration
invalidates the current approach completely.

[philipj_slow, 2015/02/17 02:37:24]

Overriding Node::willRespondToTouchEvents() ought to do it, but I'm not sure. I
just found that MediaControlOverlayEnclosureElement::preDispatchEventHandler is
doing something very similar to this CL, but it doesn't override
willRespondToTouchEvents() so either it doesn't work or I don't understand why
it works.

Using preDispatchEventHandler is probably a good idea, though.

[Tima Vaisburd, 2015/02/19 19:32:28]

I was wrong: whether touch events propagate or not depends on the TouchEvent
registration
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
HTMLInputEvents does this already, and this explains why touches come to the
derived elements (timeline etc.), but not to
MediaControlOverlayEnclosureElement. Adding the registration where we need it
seems easy.

+        Node * targetNode = event->target() ? event->target()->toNode() : nullptr;
+        if (!targetNode || isInactiveElement(targetNode)) {
+            // Do not generate mouse events from this tap,
+            // otherwise a tap to show controls might also
+            // trigger some unexpected action like stop or seek.
+            event->preventDefault();

[philipj_slow, 2015/02/16 10:45:19]

This will be observable to Web content. An alternative could be to ignore clicks
where the target has changed: Keep a (weak?) reference to the event target in
the (touchstart?) event that caused the controls to be shown. In from-touch
click events, if the event target is not the same, do nothing. If it's a weak
ref there should be no need to worry about when to clear it.

+
+            if (!m_panel->isOpaque()) {
+                makeOpaque();
+                startHideMediaControlsTimer();
+                // Prevent immediate hiding by playbackProgressed()
+                m_isMouseOverControls = true;

[philipj_slow, 2015/02/16 10:45:19]

This seems like an abuse of this state variable, as the mouse may not be over
the controls at this point.

+            } else {
+                stopHideMediaControlsTimer();
+                makeTransparent();
+            }
+        } else {
+            // Prevent the controls from disappearing while we are working with them
+            if (m_panel->isOpaque())
+                resetHideMediaControlsTimer();
+        }
+        return;
+    }
+
+    if (event->type() == EventTypeNames::mouseover && !mouseIsSynthetic) {
         if (!containsRelatedTarget(event)) {
             m_isMouseOverControls = true;
             if (!mediaElement().togglePlayStateWillPlay()) {
@@ -459,7 +504,7 @@ void MediaControls::defaultEventHandler(Event* event)
         return;
     }
 
-    if (event->type() == EventTypeNames::mouseout) {
+    if (event->type() == EventTypeNames::mouseout && !mouseIsSynthetic) {
         if (!containsRelatedTarget(event)) {
             m_isMouseOverControls = false;
             stopHideMediaControlsTimer();
@@ -467,7 +512,7 @@ void MediaControls::defaultEventHandler(Event* event)
         return;
     }
 
-    if (event->type() == EventTypeNames::mousemove) {
+    if (event->type() == EventTypeNames::mousemove && !mouseIsSynthetic) {
         // When we get a mouse move, show the media controls, and start a timer
         // that will hide the media controls after a 3 seconds without a mouse move.
         makeOpaque();