Fixed volume slider element event handling

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

 /*
  * Copyright (C) 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 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.
@@ skipping 66 matching lines @@
 
 bool isUserInteractionEvent(Event* event) {
   const AtomicString& type = event->type();
   return type == EventTypeNames::mousedown || type == EventTypeNames::mouseup ||
          type == EventTypeNames::click || type == EventTypeNames::dblclick ||
          event->isKeyboardEvent() || event->isTouchEvent();
 }
 
 // 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 .
   if (isUserInteractionEvent(event))
     return true;
 
   // Some events are only captured during a slider drag.
   LayoutSliderItem slider = LayoutSliderItem(toLayoutSlider(layoutObject));
   if (!slider.isNull() && !slider.inDragMode())
     return false;
 
   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) {
   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);
@@ skipping 621 matching lines @@
 
 void MediaControlTimelineElement::defaultEventHandler(Event* event) {
   if (event->isMouseEvent() &&
       toMouseEvent(event)->button() !=
           static_cast<short>(WebPointerProperties::Button::Left))
     return;
 
   if (!isConnected() || !document().isActive())
     return;
 
   if (event->type() == EventTypeNames::mousedown) {

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

Should `pointerdown` be added?

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

This may cause double handling. Because each "physical mouse down" fires a
pointerdown followed by a mousedown, an UI element should handle either pointer*
or mouse* events. Handling both is bad in general, e.g. consider a checkbox that
toggles with these events.

Ideally all mouse events sent to media controls should be filtered earlier so
the conditional handling in the rest of the code is for pointer events only. We
should address this separately from this bug fix. The TODO I added in
MediaControlElementTypes.h covers this. Please let me know if you want more
details there.

     Platform::current()->recordAction(
         UserMetricsAction("Media.Controls.ScrubbingBegin"));
     mediaControls().beginScrubbing();
   }
 
   if (event->type() == EventTypeNames::mouseup) {

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

ditto for `pointerup`?

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

Same as above: want to avoid double-handling.

     Platform::current()->recordAction(
         UserMetricsAction("Media.Controls.ScrubbingEnd"));
     mediaControls().endScrubbing();
   }
 
   MediaControlInputElement::defaultEventHandler(event);
 
-  if (event->type() == EventTypeNames::mouseover ||
-      event->type() == EventTypeNames::mouseout ||
-      event->type() == EventTypeNames::mousemove)
-    return;
+  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();
 
-  double time = value().toDouble();
-  if (event->type() == EventTypeNames::input) {
     // 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() {
   return isConnected() && document().isActive();
 }
 
 void MediaControlTimelineElement::setPosition(double currentTime) {
   setValue(String::number(currentTime));
 
   if (LayoutObject* layoutObject = this->layoutObject())
@@ skipping 24 matching lines @@
   slider->ensureUserAgentShadowRoot();
   slider->setType(InputTypeNames::range);
   slider->setAttribute(stepAttr, "any");
   slider->setAttribute(maxAttr, "1");
   slider->setShadowPseudoId(
       AtomicString("-webkit-media-controls-volume-slider"));
   return slider;
 }
 
 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"));
 
   if (event->type() == EventTypeNames::mouseup)
     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() {
   if (!isConnected() || !document().isActive())
     return false;
 
   return MediaControlInputElement::willRespondToMouseMoveEvents();
 }
 
 bool MediaControlVolumeSliderElement::willRespondToMouseClickEvents() {
@@ skipping 193 matching lines @@
 MediaControlCurrentTimeDisplayElement*
 MediaControlCurrentTimeDisplayElement::create(MediaControls& mediaControls) {
   MediaControlCurrentTimeDisplayElement* element =
       new MediaControlCurrentTimeDisplayElement(mediaControls);
   element->setShadowPseudoId(
       AtomicString("-webkit-media-controls-current-time-display"));
   return element;
 }
 
 }  // namespace blink