Fixed volume slider element event handling

third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp

Index: third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp b/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
index dbf7802053de4e74a2d62a16a1ecee66c8cfe946..4a1615e58d5c56605964ae5d06fcd2e1c900224f 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
@@ -97,7 +97,11 @@ bool isUserInteractionEventForSlider(Event* event, LayoutObject* layoutObject) {
 
   const AtomicString& type = event->type();
   return type == EventTypeNames::mouseover ||
-         type == EventTypeNames::mouseout || type == EventTypeNames::mousemove;
+         type == EventTypeNames::mouseout ||

[chcunningham, 2017/01/12 21:48:46]

What is this method's purpose? I see its used to decide whether to
keepEventInNode... but I'm confused about what that means. Do we prevent these
events from bubbling beyond the current node? If so, why?

[mustaq, 2017/01/17 19:32:42]

I too found it weird: EventTarget::keepEventInNode returns true for only
MediaControlElements, among all descendants of EventTarget. I have no clue
what's so special about the MediaControlElements.

Added a TODO here for a later cleanup, to avoid breaking some assumption
somewhere in media ui code which I am not familiar with.

[chcunningham, 2017/01/20 00:01:38]

Thinking more, it may be that we want to keep these events internal because, for
better or worse, we don't expose the HTML behind the media player UI... its
simply the <video> tag. So perhaps we don't want to then expose events for
hidden DOM elements. 

Just my guess though

+         type == EventTypeNames::mousemove ||
+         type == EventTypeNames::pointerover ||
+         type == EventTypeNames::pointerout ||
+         type == EventTypeNames::pointermove;
 }
 
 Element* elementFromCenter(Element& element) {
@@ -755,7 +759,10 @@ void MediaControlTimelineElement::defaultEventHandler(Event* event) {
 
   if (event->type() == EventTypeNames::mouseover ||
       event->type() == EventTypeNames::mouseout ||
-      event->type() == EventTypeNames::mousemove)
+      event->type() == EventTypeNames::mousemove ||
+      event->type() == EventTypeNames::pointerover ||

[chcunningham, 2017/01/12 21:48:46]

If we can move l781 - 783 into the if (type == input) block, then this early
return stuff can all be deleted. 

[mustaq, 2017/01/17 19:32:42]

Done with the change you suggested. I wanted to do the same thing too but I
wasn't sure if the MediaControlTimelineElement can possibly change w/o firing
any slider "input" event.

+      event->type() == EventTypeNames::pointerout ||
+      event->type() == EventTypeNames::pointermove)
     return;
 
   double time = value().toDouble();
@@ -766,6 +773,11 @@ void MediaControlTimelineElement::defaultEventHandler(Event* event) {
       mediaElement().setCurrentTime(time);
   }
 
+  // TODO(mustaq): On a single click, this block is executed 5 or 6 times

[chcunningham, 2017/01/12 21:48:46]

Why not go ahead and move this into the if ( type == input ) block above? 

[mustaq, 2017/01/17 19:32:42]

Done. Again, I wasn't sure about correct the set of events that should trigger
this.

[chcunningham, 2017/01/20 00:01:38]

Yeah, I'm also not sure. But this seems right to me. Look forward to owner
feedback ;)

+  // depending on whether the slider position has been changed or not, once for
+  // each of these events: pointerdown, mousedown, (input?), pointerup, mouseup,
+  // DOMActive and click. Perhaps we only care about the "input" event (as in
+  // MediaControlVolumeSliderElement::defaultEventHandler)?
   LayoutSliderItem slider = LayoutSliderItem(toLayoutSlider(layoutObject()));
   if (!slider.isNull() && slider.inDragMode())
     mediaControls().updateCurrentTimeDisplay();
@@ -813,21 +825,11 @@ MediaControlVolumeSliderElement* MediaControlVolumeSliderElement::create(
 }
 
 void MediaControlVolumeSliderElement::defaultEventHandler(Event* event) {
-  if (event->isMouseEvent() &&
-      toMouseEvent(event)->button() !=
-          static_cast<short>(WebPointerProperties::Button::Left))
-    return;
-
   if (!isConnected() || !document().isActive())
     return;
 
   MediaControlInputElement::defaultEventHandler(event);
 
-  if (event->type() == EventTypeNames::mouseover ||
-      event->type() == EventTypeNames::mouseout ||
-      event->type() == EventTypeNames::mousemove)
-    return;
-
   if (event->type() == EventTypeNames::mousedown)
     Platform::current()->recordAction(
         UserMetricsAction("Media.Controls.VolumeChangeBegin"));
@@ -836,9 +838,11 @@ void MediaControlVolumeSliderElement::defaultEventHandler(Event* event) {
     Platform::current()->recordAction(
         UserMetricsAction("Media.Controls.VolumeChangeEnd"));
 
-  double volume = value().toDouble();
-  mediaElement().setVolume(volume);
-  mediaElement().setMuted(false);
+  if (event->type() == EventTypeNames::input) {
+    double volume = value().toDouble();
+    mediaElement().setVolume(volume);
+    mediaElement().setMuted(false);
+  }
 }
 
 bool MediaControlVolumeSliderElement::willRespondToMouseMoveEvents() {