Fix noisy event modifiers when UI events are converted to Blink events

Certain mouse-input devices are known to send mouse events whose
modifiers don't always match the state implied by the changed
(pressed/released) buttons.  While the bad modifiers are not
exposed to JS event's button/buttons fields, internal Blink
plumbing may behave erratically with this state.

This CL:
- adds a fix where Blink WebInputEvents are first created from UI
events (for events seen in production), and
- completes an existing fix "in the middle" of Blink event path
(for events injected in testing).

Fixed: 378451943
Bug: 40851596
Change-Id: I0d7a496d0ab76089d68b653daaebd41dfae11fc9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6049401
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Auto-Submit: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1391250}

third_party/blink/common/input/web_mouse_event.cc

@@ -93,4 +93,46 @@ void WebMouseEvent::SetMenuSourceType(WebInputEvent::Type type) {
   }
 }
 
+void WebMouseEvent::UpdateEventModifiersToMatchButton() {
+  unsigned button_modifier_bit = WebInputEvent::kNoModifiers;
+
+  switch (button) {
+    case blink::WebPointerProperties::Button::kNoButton:
+      button_modifier_bit = WebInputEvent::kNoModifiers;
+      break;
+
+    case blink::WebPointerProperties::Button::kLeft:
+      button_modifier_bit = WebInputEvent::kLeftButtonDown;
+      break;
+
+    case blink::WebPointerProperties::Button::kMiddle:
+      button_modifier_bit = WebInputEvent::kMiddleButtonDown;
+      break;
+
+    case blink::WebPointerProperties::Button::kRight:
+      button_modifier_bit = WebInputEvent::kRightButtonDown;
+      break;
+
+    case blink::WebPointerProperties::Button::kBack:
+      button_modifier_bit = WebInputEvent::kBackButtonDown;
+      break;
+
+    case blink::WebPointerProperties::Button::kForward:
+      button_modifier_bit = WebInputEvent::kForwardButtonDown;
+      break;
+
+    case blink::WebPointerProperties::Button::kEraser:
+      // TODO(mustaq): WebInputEvent modifier needs to support stylus eraser
+      // buttons.
+      button_modifier_bit = WebInputEvent::kNoModifiers;
+      break;
+  }
+
+  if (GetType() == WebInputEvent::Type::kMouseDown) {
+    SetModifiers(GetModifiers() | button_modifier_bit);
+  } else if (GetType() == WebInputEvent::Type::kMouseUp) {
+    SetModifiers(GetModifiers() & ~button_modifier_bit);
+  }
+}
+
 }  // namespace blink

third_party/blink/public/common/input/web_mouse_event.h

@@ -82,6 +82,16 @@ class BLINK_COMMON_EXPORT WebMouseEvent : public WebInputEvent,
   // back to 1 and |frame_translate_| X and Y coordinates back to 0.
   WebMouseEvent FlattenTransform() const;
 
+  // Makes the event modifier bit corresponding to the `button` field match the
+  // implied button state. More precisely, at mousedown it sets the modifier bit
+  // and at mouseup it resets the bit.  Low-level events from the system may not
+  // set/reset the bit correctly as per the spec:
+  // https://www.w3.org/TR/uievents/#dom-mouseevent-buttons
+  //
+  // Other modifier bits remain unchanged for these two events, and no change is
+  // made for other events.
+  void UpdateEventModifiersToMatchButton();
+
  protected:
   WebMouseEvent(PointerId id_param) : WebPointerProperties(id_param) {}
 

third_party/blink/renderer/core/events/web_input_event_conversion.cc

@@ -32,6 +32,8 @@
 
 #include <array>
 
+#include "third_party/blink/public/common/features.h"
+#include "third_party/blink/public/common/input/web_pointer_properties.h"
 #include "third_party/blink/public/platform/platform.h"
 #include "third_party/blink/renderer/core/events/gesture_event.h"
 #include "third_party/blink/renderer/core/events/keyboard_event.h"
@@ -89,18 +91,6 @@ void UpdateWebMouseEventFromCoreMouseEvent(const MouseEvent& event,
   web_event.SetPositionInWidget(local_point);
 }
 
-unsigned ToWebInputEventModifierFrom(WebMouseEvent::Button button) {
-  if (button == WebMouseEvent::Button::kNoButton)
-    return 0;
-
-  constexpr auto web_mouse_button_to_platform_modifier = std::to_array<unsigned>(
-      {WebInputEvent::kLeftButtonDown, WebInputEvent::kMiddleButtonDown,
-       WebInputEvent::kRightButtonDown, WebInputEvent::kBackButtonDown,
-       WebInputEvent::kForwardButtonDown});
-
-  return web_mouse_button_to_platform_modifier[static_cast<int>(button)];
-}
-
 WebPointerEvent TransformWebPointerEvent(float frame_scale,
                                          gfx::Vector2dF frame_translate,
                                          const WebPointerEvent& event) {
@@ -120,12 +110,17 @@ WebMouseEvent TransformWebMouseEvent(LocalFrameView* frame_view,
                                      const WebMouseEvent& event) {
   WebMouseEvent result = event;
 
-  // TODO(dtapuska): Perhaps the event should be constructed correctly?
-  // crbug.com/686200
-  if (event.GetType() == WebInputEvent::Type::kMouseUp) {
-    result.SetModifiers(event.GetModifiers() &
-                        ~ToWebInputEventModifierFrom(event.button));
+  // The code in production fixes the modifiers field further upstream at
+  // `MakeWebMouseEventFromUiEvent`.  But thousands of our tests (both WPTs and
+  // binary tests) bypasses the upstream fix while injecting mouse clicks
+  // without proper event modifiers!  And then mouse events on DevTools overlay
+  // bypasses the fix below!!
+  if (event.GetType() == WebInputEvent::Type::kMouseUp ||
+      (RuntimeEnabledFeatures::ClickToCapturedPointerEnabled() &&
+       event.GetType() == WebInputEvent::Type::kMouseDown)) {
+    result.UpdateEventModifiersToMatchButton();
   }
+
   result.SetFrameScale(FrameScale(frame_view));
   result.SetFrameTranslate(FrameTranslation(frame_view));
   return result;
@@ -181,19 +176,19 @@ WebMouseEventBuilder::WebMouseEventBuilder(const LayoutObject* layout_object,
 
   switch (event.button()) {
     case int16_t(WebPointerProperties::Button::kLeft):
-      button = WebMouseEvent::Button::kLeft;
+      button = WebPointerProperties::Button::kLeft;
       break;
     case int16_t(WebPointerProperties::Button::kMiddle):
-      button = WebMouseEvent::Button::kMiddle;
+      button = WebPointerProperties::Button::kMiddle;
       break;
     case int16_t(WebPointerProperties::Button::kRight):
-      button = WebMouseEvent::Button::kRight;
+      button = WebPointerProperties::Button::kRight;
       break;
     case int16_t(WebPointerProperties::Button::kBack):
-      button = WebMouseEvent::Button::kBack;
+      button = WebPointerProperties::Button::kBack;
       break;
     case int16_t(WebPointerProperties::Button::kForward):
-      button = WebMouseEvent::Button::kForward;
+      button = WebPointerProperties::Button::kForward;
       break;
   }
   if (event.ButtonDown()) {
@@ -215,7 +210,7 @@ WebMouseEventBuilder::WebMouseEventBuilder(const LayoutObject* layout_object,
         break;
     }
   } else {
-    button = WebMouseEvent::Button::kNoButton;
+    button = WebPointerProperties::Button::kNoButton;
   }
   movement_x = event.movementX();
   movement_y = event.movementY();
@@ -262,7 +257,8 @@ WebMouseEventBuilder::WebMouseEventBuilder(const LayoutObject* layout_object,
   gfx::PointF screen_point = touch->ScreenLocation();
   SetPositionInScreen(screen_point.x(), screen_point.y());
 
-  button = WebMouseEvent::Button::kLeft;
+  button = WebPointerProperties::Button::kLeft;
+  // TODO(mustaq@chromium.org): Shouldn't we reset the bit for kMouseUp?
   modifiers_ |= WebInputEvent::kLeftButtonDown;
   click_count = (type_ == WebInputEvent::Type::kMouseDown ||
                  type_ == WebInputEvent::Type::kMouseUp);

third_party/blink/renderer/platform/runtime_enabled_features.json5

@@ -769,6 +769,8 @@
       name: "CheckVisibilityExtraProperties",
       status: "stable",
     },
+    // crbug.com/40851596: Send click to the capture pointer target instead of
+    // the common ancestor of pointerdown and pointerup targets.
     {
       name: "ClickToCapturedPointer",
       status: "experimental",

ui/events/blink/web_input_event.cc

@@ -4,8 +4,10 @@
 
 #include "ui/events/blink/web_input_event.h"
 
+#include "base/feature_list.h"
 #include "base/types/cxx23_to_underlying.h"
 #include "build/build_config.h"
+#include "third_party/blink/public/common/features.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/events/blink/blink_features.h"
@@ -104,7 +106,7 @@ blink::WebMouseWheelEvent MakeWebMouseWheelEventFromUiEvent(
       WebInputEvent::Type::kMouseWheel,
       EventFlagsToWebEventModifiers(event.flags()), event.time_stamp());
 
-  webkit_event.button = blink::WebMouseEvent::Button::kNoButton;
+  webkit_event.button = blink::WebPointerProperties::Button::kNoButton;
   webkit_event.delta_units = ui::ScrollGranularity::kScrollByPrecisePixel;
 
   float offset_ordinal_x = event.x_offset_ordinal();
@@ -236,6 +238,11 @@ blink::WebMouseEvent MakeWebMouseEvent(const MouseEvent& event) {
 #else
       MakeWebMouseEventFromUiEvent(event);
 #endif
+
+  if (base::FeatureList::IsEnabled(blink::features::kClickToCapturedPointer)) {
+    webkit_event.UpdateEventModifiersToMatchButton();
+  }
+
   // Replace the event's coordinate fields with translated position data from
   // |event|.
   webkit_event.SetPositionInWidget(event.x(), event.y());
@@ -402,7 +409,7 @@ blink::WebMouseEvent MakeWebMouseEventFromUiEvent(const MouseEvent& event) {
   blink::WebMouseEvent webkit_event(
       type, EventFlagsToWebEventModifiers(event.flags()), event.time_stamp(),
       event.pointer_details().id);
-  webkit_event.button = blink::WebMouseEvent::Button::kNoButton;
+  webkit_event.button = blink::WebPointerProperties::Button::kNoButton;
   int button_flags = event.flags();
   if (event.type() == EventType::kMousePressed ||
       event.type() == EventType::kMouseReleased) {
@@ -416,16 +423,21 @@ blink::WebMouseEvent MakeWebMouseEventFromUiEvent(const MouseEvent& event) {
   // TODO(mustaq): This |if| ordering look suspicious. Replacing with if-else &
   // changing the order to L/R/M/B/F breaks
   // pointerevent_pointermove_on_chorded_mouse_button-manual.html! Investigate.
-  if (button_flags & EF_BACK_MOUSE_BUTTON)
-    webkit_event.button = blink::WebMouseEvent::Button::kBack;
-  if (button_flags & EF_FORWARD_MOUSE_BUTTON)
-    webkit_event.button = blink::WebMouseEvent::Button::kForward;
-  if (button_flags & EF_LEFT_MOUSE_BUTTON)
-    webkit_event.button = blink::WebMouseEvent::Button::kLeft;
-  if (button_flags & EF_MIDDLE_MOUSE_BUTTON)
-    webkit_event.button = blink::WebMouseEvent::Button::kMiddle;
-  if (button_flags & EF_RIGHT_MOUSE_BUTTON)
-    webkit_event.button = blink::WebMouseEvent::Button::kRight;
+  if (button_flags & EF_BACK_MOUSE_BUTTON) {
+    webkit_event.button = blink::WebPointerProperties::Button::kBack;
+  }
+  if (button_flags & EF_FORWARD_MOUSE_BUTTON) {
+    webkit_event.button = blink::WebPointerProperties::Button::kForward;
+  }
+  if (button_flags & EF_LEFT_MOUSE_BUTTON) {
+    webkit_event.button = blink::WebPointerProperties::Button::kLeft;
+  }
+  if (button_flags & EF_MIDDLE_MOUSE_BUTTON) {
+    webkit_event.button = blink::WebPointerProperties::Button::kMiddle;
+  }
+  if (button_flags & EF_RIGHT_MOUSE_BUTTON) {
+    webkit_event.button = blink::WebPointerProperties::Button::kRight;
+  }
 
   webkit_event.click_count = click_count;
   webkit_event.tilt_x = event.pointer_details().tilt_x;
@@ -447,7 +459,7 @@ blink::WebMouseWheelEvent MakeWebMouseWheelEventFromUiEvent(
       WebInputEvent::Type::kMouseWheel,
       EventFlagsToWebEventModifiers(event.flags()), event.time_stamp());
 
-  webkit_event.button = blink::WebMouseEvent::Button::kNoButton;
+  webkit_event.button = blink::WebPointerProperties::Button::kNoButton;
 
   webkit_event.delta_x = event.x_offset();
   webkit_event.delta_y = event.y_offset();

ui/events/blink/web_input_event_unittest.cc

@@ -12,8 +12,10 @@
 #include <cstddef>
 #include <cstdint>
 
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/features.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/events/event.h"
@@ -423,6 +425,96 @@ TEST(WebInputEventTest, TestMakeWebMouseEventWithMultiButtons) {
   }
 }
 
+TEST(WebInputEventTest, TestMakeWebMouseEventNoisyButtonState) {
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      blink::features::kClickToCapturedPointer);
+
+  {
+    // Left pressed but the modifier doesn't reflect it.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(EventType::kMousePressed, gfx::Point(1, 1),
+                        gfx::Point(1, 1), timestamp, 0, EF_LEFT_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(blink::WebInputEvent::kLeftButtonDown,
+              webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kLeft, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseDown, webkit_event.GetType());
+  }
+  {
+    // Left released but the modifier doesn't reflect it.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(EventType::kMouseReleased, gfx::Point(1, 1),
+                        gfx::Point(1, 1), timestamp, EF_LEFT_MOUSE_BUTTON,
+                        EF_LEFT_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(0, webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kLeft, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseUp, webkit_event.GetType());
+  }
+  {
+    // Middle pressed while left and right depressed but the modifier doesn't
+    // reflect middle press.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(
+        EventType::kMousePressed, gfx::Point(1, 1), gfx::Point(1, 1), timestamp,
+        EF_LEFT_MOUSE_BUTTON | EF_RIGHT_MOUSE_BUTTON, EF_MIDDLE_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(blink::WebInputEvent::kLeftButtonDown +
+                  blink::WebInputEvent::kMiddleButtonDown +
+                  blink::WebInputEvent::kRightButtonDown,
+              webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kMiddle, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseDown, webkit_event.GetType());
+  }
+  {
+    // Middle released while left and right depressed but the modifier doesn't
+    // reflect middle release.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(
+        EventType::kMouseReleased, gfx::Point(1, 1), gfx::Point(1, 1),
+        timestamp,
+        EF_LEFT_MOUSE_BUTTON | EF_MIDDLE_MOUSE_BUTTON | EF_RIGHT_MOUSE_BUTTON,
+        EF_MIDDLE_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(blink::WebInputEvent::kLeftButtonDown +
+                  blink::WebInputEvent::kRightButtonDown,
+              webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kMiddle, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseUp, webkit_event.GetType());
+  }
+  {
+    // Right pressed while shift and control keys are depressed but the modifier
+    // doesn't reflect it.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(EventType::kMousePressed, gfx::Point(1, 1),
+                        gfx::Point(1, 1), timestamp,
+                        EF_SHIFT_DOWN | EF_CONTROL_DOWN, EF_RIGHT_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(blink::WebInputEvent::kRightButtonDown +
+                  blink::WebInputEvent::kShiftKey +
+                  blink::WebInputEvent::kControlKey,
+              webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kRight, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseDown, webkit_event.GetType());
+  }
+  {
+    // Right released while shift and control keys are depressed but the
+    // modifier doesn't reflect it.
+    base::TimeTicks timestamp = EventTimeForNow();
+    MouseEvent ui_event(EventType::kMouseReleased, gfx::Point(1, 1),
+                        gfx::Point(1, 1), timestamp,
+                        EF_RIGHT_MOUSE_BUTTON | EF_SHIFT_DOWN | EF_CONTROL_DOWN,
+                        EF_RIGHT_MOUSE_BUTTON);
+    blink::WebMouseEvent webkit_event = MakeWebMouseEvent(ui_event);
+    EXPECT_EQ(
+        blink::WebInputEvent::kShiftKey + blink::WebInputEvent::kControlKey,
+        webkit_event.GetModifiers());
+    EXPECT_EQ(blink::WebMouseEvent::Button::kRight, webkit_event.button);
+    EXPECT_EQ(blink::WebInputEvent::Type::kMouseUp, webkit_event.GetType());
+  }
+}
+
 TEST(WebInputEventTest, TestMakeWebMouseWheelEvent) {
   {
     // Mouse wheel.
@@ -504,10 +596,10 @@ TEST(WebInputEventTest, MousePointerEvent) {
     gfx::Point screen_location;
   } tests[] = {
       {ui::EventType::kMousePressed, blink::WebInputEvent::Type::kMouseDown,
-       0x0, 0x0, gfx::Point(3, 5), gfx::Point(113, 125)},
-      {ui::EventType::kMouseReleased, blink::WebInputEvent::Type::kMouseUp,
        ui::EF_LEFT_MOUSE_BUTTON, blink::WebInputEvent::kLeftButtonDown,
-       gfx::Point(100, 1), gfx::Point(50, 1)},
+       gfx::Point(3, 5), gfx::Point(113, 125)},
+      {ui::EventType::kMouseReleased, blink::WebInputEvent::Type::kMouseUp, 0,
+       0, gfx::Point(100, 1), gfx::Point(50, 1)},
       {ui::EventType::kMouseMoved, blink::WebInputEvent::Type::kMouseMove,
        ui::EF_MIDDLE_MOUSE_BUTTON | ui::EF_RIGHT_MOUSE_BUTTON,
        blink::WebInputEvent::kMiddleButtonDown |