[CompositorThreadedScrollbarScrolling] Scrollbar invalidation fix.

The CL fixes a bug where the scrollbar thumb and arrows were not being
invalidated when users clicked on them. The root cause here was, the
MouseEvent(s) were not being passed on deep enough into the main thread
scrollbar code (where the logic to invalidate exists).

Bug: 1040258
Change-Id: I3a1b413aea06b437f07e37822e16fcfcab11aef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2008132
Commit-Queue: Rahul Arakeri <arakeri@microsoft.com>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#735303}

cc/input/scrollbar_controller.cc

@@ -307,12 +307,21 @@ InputHandlerPointerResult ScrollbarController::HandlePointerMove(
   RecomputeAutoscrollStateIfNeeded();
   InputHandlerPointerResult scroll_result;
 
-  // If a thumb drag is not in progress or if a GSU was already produced for a
-  // thumb drag in this frame, there's no point in continuing on. Please see the
-  // header file for details.
   const ScrollbarLayerImplBase* scrollbar = ScrollbarLayer();
-  if (!scrollbar || !drag_state_.has_value() ||
-      drag_processed_for_current_frame_)
+  if (!scrollbar || !drag_state_.has_value())
+    return scroll_result;
+
+  // If the scrollbar thumb is being dragged, it qualifies as a kScrollbarScroll
+  // (although the delta might still be zero). Setting the "type" to
+  // kScrollbarScroll ensures that the correct event modifier (in
+  // InputHandlerProxy) is set which in-turn tells the main thread to invalidate
+  // the respective scrollbar parts. This needs to be done for all
+  // pointermove(s) since they are not VSync aligned.
+  scroll_result.type = PointerResultType::kScrollbarScroll;
+
+  // If a GSU was already produced for a thumb drag in this frame, there's no
+  // point in continuing on. Please see the header file for details.
+  if (drag_processed_for_current_frame_)
     return scroll_result;
 
   const ScrollNode* currently_scrolling_node =
@@ -339,7 +348,6 @@ InputHandlerPointerResult ScrollbarController::HandlePointerMove(
   // movement. Hence we use kPrecisePixel when dragging the thumb.
   scroll_result.scroll_units =
       ui::input_types::ScrollGranularity::kScrollByPrecisePixel;
-  scroll_result.type = PointerResultType::kScrollbarScroll;
   scroll_result.scroll_offset = gfx::ScrollOffset(clamped_scroll_offset);
   drag_processed_for_current_frame_ = true;
 

third_party/blink/renderer/core/input/event_handler.cc

@@ -843,12 +843,7 @@ WebInputEventResult EventHandler::HandleMousePressEvent(
     mouse_event_manager_->SetCapturesDragging(false);
   }
 
-  // If the scrollbar manipulation was already handled on the compositor thread,
-  // don't pass on the event to the scrollbar.
-  if (mouse_event.GetModifiers() &
-          WebInputEvent::Modifiers::
-              kScrollbarManipulationHandledOnCompositorThread ||
-      PassMousePressEventToScrollbar(mev))
+  if (PassMousePressEventToScrollbar(mev))
     event_result = WebInputEventResult::kHandledSystem;
 
   if (event_result == WebInputEventResult::kNotHandled) {

third_party/blink/renderer/core/scroll/scrollbar.cc

@@ -72,6 +72,7 @@ Scrollbar::Scrollbar(ScrollableArea* scrollable_area,
       track_needs_repaint_(true),
       thumb_needs_repaint_(true),
       injected_gesture_scroll_begin_(false),
+      scrollbar_manipulation_in_progress_on_cc_thread_(false),
       style_source_(style_source) {
   theme_.RegisterScrollbar(*this);
 
@@ -450,6 +451,15 @@ bool Scrollbar::HandleTapGesture() {
 
 void Scrollbar::MouseMoved(const WebMouseEvent& evt) {
   IntPoint position = FlooredIntPoint(evt.PositionInRootFrame());
+  ScrollbarPart part = GetTheme().HitTestRootFramePosition(*this, position);
+
+  // If the WebMouseEvent was already handled on the compositor thread, simply
+  // set up the ScrollbarPart for invalidation and exit.
+  if (scrollbar_manipulation_in_progress_on_cc_thread_) {
+    SetHoveredPart(part);
+    return;
+  }
+
   if (pressed_part_ == kThumbPart) {
     if (GetTheme().ShouldSnapBackToDragOrigin(*this, evt)) {
       if (scrollable_area_) {
@@ -472,7 +482,6 @@ void Scrollbar::MouseMoved(const WebMouseEvent& evt) {
                        : ConvertFromRootFrame(position).Y();
   }
 
-  ScrollbarPart part = GetTheme().HitTestRootFramePosition(*this, position);
   if (part != hovered_part_) {
     if (pressed_part_ != kNoPart) {
       if (part == pressed_part_) {
@@ -506,6 +515,11 @@ void Scrollbar::MouseExited() {
 void Scrollbar::MouseUp(const WebMouseEvent& mouse_event) {
   bool is_captured = pressed_part_ == kThumbPart;
   SetPressedPart(kNoPart, mouse_event.GetType());
+  if (scrollbar_manipulation_in_progress_on_cc_thread_) {
+    scrollbar_manipulation_in_progress_on_cc_thread_ = false;
+    return;
+  }
+
   pressed_pos_ = 0;
   dragging_document_ = false;
   StopTimerIfNeeded();
@@ -537,6 +551,19 @@ void Scrollbar::MouseDown(const WebMouseEvent& evt) {
   IntPoint position = FlooredIntPoint(evt.PositionInRootFrame());
   SetPressedPart(GetTheme().HitTestRootFramePosition(*this, position),
                  evt.GetType());
+
+  // Scrollbar manipulation (for a mouse) always begins with a MouseDown. If
+  // this is already being handled by the compositor thread, blink::Scrollbar
+  // needs to be made aware of this. It also means that, all the actions which
+  // follow (like MouseMove(s) and MouseUp) will also be handled on the cc
+  // thread. However, the scrollbar parts still need to be invalidated on the
+  // main thread.
+  scrollbar_manipulation_in_progress_on_cc_thread_ =
+      evt.GetModifiers() &
+      WebInputEvent::Modifiers::kScrollbarManipulationHandledOnCompositorThread;
+  if (scrollbar_manipulation_in_progress_on_cc_thread_)
+    return;
+
   int pressed_pos = Orientation() == kHorizontalScrollbar
                         ? ConvertFromRootFrame(position).X()
                         : ConvertFromRootFrame(position).Y();

third_party/blink/renderer/core/scroll/scrollbar.h

@@ -254,6 +254,15 @@ class CORE_EXPORT Scrollbar : public GarbageCollected<Scrollbar>,
   bool track_needs_repaint_;
   bool thumb_needs_repaint_;
   bool injected_gesture_scroll_begin_;
+
+  // This is set based on the event modifiers. In scenarios like scrolling or
+  // layout, the element that the cursor is over can change without the cursor
+  // itself moving. In these cases, a "fake" mouse move may be dispatched (see
+  // MouseEventManager::RecomputeMouseHoverState) in order to apply hover etc.
+  // Such mouse events do not have the modifier set and hence, maintaining this
+  // additional state is necessary.
+  bool scrollbar_manipulation_in_progress_on_cc_thread_;
+
   IntRect visual_rect_;
   IntRect frame_rect_;
   Member<Element> style_source_;

third_party/blink/renderer/core/scroll/scrollbar_test_suite.h

@@ -96,6 +96,14 @@ class MockScrollableArea : public GarbageCollected<MockScrollableArea>,
   bool ScrollAnimatorEnabled() const override { return false; }
   int PageStep(ScrollbarOrientation) const override { return 0; }
   void ScrollControlWasSetNeedsPaintInvalidation() override {}
+  IntPoint ConvertFromRootFrame(const IntPoint& point_in_root_frame) const {
+    return point_in_root_frame;
+  }
+  IntPoint ConvertFromContainingEmbeddedContentViewToScrollbar(
+      const Scrollbar& scrollbar,
+      const IntPoint& parent_point) const {
+    return parent_point;
+  }
 
   scoped_refptr<base::SingleThreadTaskRunner> GetTimerTaskRunner() const final {
     return blink::scheduler::GetSingleThreadTaskRunnerForTesting();

third_party/blink/renderer/core/scroll/scrollbar_theme_aura_test.cc

@@ -4,6 +4,7 @@
 
 #include "third_party/blink/renderer/core/scroll/scrollbar_theme_aura.h"
 
+#include "third_party/blink/public/common/input/web_mouse_event.h"
 #include "third_party/blink/renderer/core/scroll/scrollbar_test_suite.h"
 #include "third_party/blink/renderer/platform/testing/testing_platform_support_with_mock_scheduler.h"
 
@@ -23,6 +24,10 @@ class ScrollbarThemeAuraButtonOverride final : public ScrollbarThemeAura {
     return has_scrollbar_buttons_;
   }
 
+  int MinimumThumbLength(const Scrollbar& scrollbar) override {
+    return ThumbThickness(scrollbar);
+  }
+
  private:
   bool has_scrollbar_buttons_;
 };
@@ -31,6 +36,35 @@ class ScrollbarThemeAuraButtonOverride final : public ScrollbarThemeAura {
 
 class ScrollbarThemeAuraTest : public testing::Test {};
 
+// Note that this helper only sends mouse events that are already handled on the
+// compositor thread, to the scrollbar (i.e they will have the event modifier
+// "kScrollbarManipulationHandledOnCompositorThread" set). The point of this
+// exercise is to validate that the scrollbar parts invalidate as expected
+// (since we still rely on the main thread for invalidation).
+void SendEvent(Scrollbar* scrollbar,
+               blink::WebInputEvent::Type type,
+               gfx::PointF point) {
+  const blink::WebMouseEvent web_mouse_event(
+      type, point, point, blink::WebPointerProperties::Button::kLeft, 0,
+      blink::WebInputEvent::kScrollbarManipulationHandledOnCompositorThread,
+      base::TimeTicks::Now());
+  switch (type) {
+    case blink::WebInputEvent::kMouseDown:
+      scrollbar->MouseDown(web_mouse_event);
+      break;
+    case blink::WebInputEvent::kMouseMove:
+      scrollbar->MouseMoved(web_mouse_event);
+      break;
+    case blink::WebInputEvent::kMouseUp:
+      scrollbar->MouseUp(web_mouse_event);
+      break;
+    default:
+      // The rest are unhandled. Let the called know that this helper has not
+      // yet implemented them.
+      NOTIMPLEMENTED();
+  }
+}
+
 TEST_F(ScrollbarThemeAuraTest, ButtonSizeHorizontal) {
   ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
       platform;
@@ -97,4 +131,49 @@ TEST_F(ScrollbarThemeAuraTest, NoButtonsReturnsSize0) {
   ThreadState::Current()->CollectAllGarbageForTesting();
 }
 
+TEST_F(ScrollbarThemeAuraTest, ScrollbarPartsInvalidationTest) {
+  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
+      platform;
+
+  MockScrollableArea* mock_scrollable_area =
+      MockScrollableArea::Create(ScrollOffset(0, 1000));
+  ScrollbarThemeAuraButtonOverride theme;
+  Scrollbar* scrollbar = Scrollbar::CreateForTesting(
+      mock_scrollable_area, kVerticalScrollbar, kRegularScrollbar, &theme);
+
+  IntRect vertical_rect(1010, 0, 14, 768);
+  scrollbar->SetFrameRect(vertical_rect);
+  scrollbar->ClearThumbNeedsRepaint();
+  scrollbar->ClearTrackNeedsRepaint();
+
+  // Tests that mousedown on the thumb causes an invalidation.
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseMove, gfx::PointF(10, 20));
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseDown, gfx::PointF(10, 20));
+  EXPECT_TRUE(scrollbar->ThumbNeedsRepaint());
+
+  // Tests that mouseup on the thumb causes an invalidation.
+  scrollbar->ClearThumbNeedsRepaint();
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseUp, gfx::PointF(10, 20));
+  EXPECT_TRUE(scrollbar->ThumbNeedsRepaint());
+
+  // Tests that mousedown on the arrow causes an invalidation. Note that, to
+  // check if the arrow was invalidated, TrackNeedsRepaint is used.
+  // TrackNeedsRepaint here means "everything except the thumb needs to be
+  // repainted".
+  scrollbar->ClearTrackNeedsRepaint();
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseMove, gfx::PointF(10, 760));
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseDown, gfx::PointF(10, 760));
+  EXPECT_TRUE(scrollbar->TrackNeedsRepaint());
+
+  // Tests that mouseup on the arrow causes an invalidation.
+  scrollbar->ClearTrackNeedsRepaint();
+  SendEvent(scrollbar, blink::WebInputEvent::kMouseUp, gfx::PointF(10, 760));
+  EXPECT_TRUE(scrollbar->TrackNeedsRepaint());
+
+  ThreadState::Current()->CollectAllGarbageForTesting();
+}
+
+// TODO(arakeri): Add more comprehensive tests for scrollbar invalidation
+// (tracked in crbug.com/1045808).
+
 }  // namespace blink

ui/events/blink/input_handler_proxy.cc

@@ -576,7 +576,6 @@ InputHandlerProxy::RouteToTypeSpecificHandler(
                 original_latency_info, mouse_event.TimeStamp());
           }
 
-          // Mark this event as being handled on the compositor thread.
           if (event_with_callback) {
             event_with_callback
                 ->SetScrollbarManipulationHandledOnCompositorThread();
@@ -602,7 +601,6 @@ InputHandlerProxy::RouteToTypeSpecificHandler(
                                        pointer_result, original_latency_info,
                                        mouse_event.TimeStamp());
 
-          // Mark this event as being handled on the compositor thread.
           if (event_with_callback) {
             event_with_callback
                 ->SetScrollbarManipulationHandledOnCompositorThread();
@@ -622,13 +620,15 @@ InputHandlerProxy::RouteToTypeSpecificHandler(
               gfx::Point(mouse_event.PositionInWidget().x(),
                          mouse_event.PositionInWidget().y()));
       if (pointer_result.type == cc::PointerResultType::kScrollbarScroll) {
-        // Generate a GSU event and add it to the CompositorThreadEventQueue.
-        InjectScrollbarGestureScroll(WebInputEvent::Type::kGestureScrollUpdate,
-                                     mouse_event.PositionInWidget(),
-                                     pointer_result, original_latency_info,
-                                     mouse_event.TimeStamp());
+        // Generate a GSU event and add it to the CompositorThreadEventQueue if
+        // delta is non zero.
+        if (!pointer_result.scroll_offset.IsZero()) {
+          InjectScrollbarGestureScroll(
+              WebInputEvent::Type::kGestureScrollUpdate,
+              mouse_event.PositionInWidget(), pointer_result,
+              original_latency_info, mouse_event.TimeStamp());
+        }
 
-        // Mark this event as being handled on the compositor thread.
         if (event_with_callback) {
           event_with_callback
               ->SetScrollbarManipulationHandledOnCompositorThread();