Tap on the video element toggles controls visibility

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 342 matching lines @@
         m_toggleClosedCaptionsButton->show();
     else
         m_toggleClosedCaptionsButton->hide();
 }
 
 void MediaControls::textTracksChanged()
 {
     refreshClosedCaptionsButtonVisibility();
 }
 
+void MediaControls::timelineKnobDragged()
+{
+    resetHideMediaControlsTimer();
+}
+
 static Element* elementFromCenter(Element& element)
 {
     RefPtrWillBeRawPtr<ClientRect> clientRect = element.getBoundingClientRect();
     int centerX = static_cast<int>((clientRect->left() + clientRect->right()) / 2);
     int centerY = static_cast<int>((clientRect->top() + clientRect->bottom()) / 2);
 
     return element.document().elementFromPoint(centerX , centerY);
 }
 
 void MediaControls::tryShowOverlayCastButton()
@@ skipping 55 matching lines @@
     m_castButton->setIsPlayingRemotely(true);
     m_overlayCastButton->setIsPlayingRemotely(true);
 }
 
 void MediaControls::stoppedCasting()
 {
     m_castButton->setIsPlayingRemotely(false);
     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);
 
     // Add IgnoreControlsHover to m_hideTimerBehaviorFlags when we see a touch 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()) {
                 makeOpaque();
                 if (shouldHideMediaControls())
                     startHideMediaControlsTimer();
             }
         }
         return;
     }
 
-    if (event->type() == EventTypeNames::mouseout) {
+    if (event->type() == EventTypeNames::mouseout && !mouseIsSynthetic) {
         if (!containsRelatedTarget(event)) {
             m_isMouseOverControls = false;
             stopHideMediaControlsTimer();
         }
         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();
         refreshCastButtonVisibility();
         if (shouldHideMediaControls(IgnoreVideoHover))
             startHideMediaControlsTimer();
         return;
     }
 }
 
@@ skipping 89 matching lines @@
     visitor->trace(m_toggleClosedCaptionsButton);
     visitor->trace(m_fullScreenButton);
     visitor->trace(m_durationDisplay);
     visitor->trace(m_enclosure);
     visitor->trace(m_castButton);
     visitor->trace(m_overlayCastButton);
     HTMLDivElement::trace(visitor);
 }
 
 }