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 b5a95e3f43509d32fd5744917377a56a92e13d7c..0691e01799cf5c8372edcc5ffd0c592c9277dd5a 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp
@@ -84,6 +84,10 @@ bool isUserInteractionEvent(Event* event) {
 
 // Sliders (the volume control and timeline) need to capture some additional
 // events used when dragging the thumb.
+//
+// TODO(mustaq): Only used through EventTarget::keepEventInNode(), and
+// MediaControlElements are the only descendants of EventTarget that return
+// true for that method. Can we nuke the method?

[mlamouri (slow - plz ping), 2017/01/24 02:06:51]

I think it was created for that purpose. Any reason why you think it should be
removed?

[mustaq, 2017/01/24 20:51:12]

Removed the TODO. I wasn't sure if the special handling for media elements (vs
all event targets) is intentional.

 bool isUserInteractionEventForSlider(Event* event, LayoutObject* layoutObject) {
   // It is unclear if this can be converted to isUserInteractionEvent(), since
   // mouse* events seem to be eaten during a drag anyway.  crbug.com/516416 .
@@ -97,7 +101,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 ||
+         type == EventTypeNames::mousemove ||
+         type == EventTypeNames::pointerover ||
+         type == EventTypeNames::pointerout ||
+         type == EventTypeNames::pointermove;
 }
 
 Element* elementFromCenter(Element& element) {
@@ -753,22 +761,18 @@ void MediaControlTimelineElement::defaultEventHandler(Event* event) {
 
   MediaControlInputElement::defaultEventHandler(event);
 
-  if (event->type() == EventTypeNames::mouseover ||
-      event->type() == EventTypeNames::mouseout ||
-      event->type() == EventTypeNames::mousemove)
-    return;
-
-  double time = value().toDouble();
   if (event->type() == EventTypeNames::input) {

[mlamouri (slow - plz ping), 2017/01/24 02:06:51]

maybe an early return?

[mustaq, 2017/01/24 20:51:12]

Done.

+    double time = value().toDouble();
+
     // FIXME: This will need to take the timeline offset into consideration
     // once that concept is supported, see https://crbug.com/312699
     if (mediaElement().seekable()->contain(time))
       mediaElement().setCurrentTime(time);
-  }
 
-  LayoutSliderItem slider = LayoutSliderItem(toLayoutSlider(layoutObject()));
-  if (!slider.isNull() && slider.inDragMode())
-    mediaControls().updateCurrentTimeDisplay();
+    LayoutSliderItem slider = LayoutSliderItem(toLayoutSlider(layoutObject()));
+    if (!slider.isNull() && slider.inDragMode())
+      mediaControls().updateCurrentTimeDisplay();
+  }
 }
 
 bool MediaControlTimelineElement::willRespondToMouseClickEvents() {
@@ -813,21 +817,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)

[mlamouri (slow - plz ping), 2017/01/24 02:06:51]

`pointerdown`/`pointerup`?

     Platform::current()->recordAction(
         UserMetricsAction("Media.Controls.VolumeChangeBegin"));
@@ -836,9 +830,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() {