From 1213893d3681ba27f61e61301740ec118398433d Mon Sep 17 00:00:00 2001 From: Mustaq Ahmed Date: Tue, 3 Dec 2024 21:35:15 +0000 Subject: [PATCH] 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 Commit-Queue: Dave Tapuska Auto-Submit: Mustaq Ahmed Reviewed-by: Dave Tapuska Cr-Commit-Position: refs/heads/main@{#1391250} --- .../blink/common/input/web_mouse_event.cc | 42 ++++++++ .../public/common/input/web_mouse_event.h | 10 ++ .../core/events/web_input_event_conversion.cc | 44 ++++----- .../platform/runtime_enabled_features.json5 | 2 + ui/events/blink/web_input_event.cc | 38 ++++--- ui/events/blink/web_input_event_unittest.cc | 98 ++++++++++++++++++- 6 files changed, 194 insertions(+), 40 deletions(-) diff --git a/third_party/blink/common/input/web_mouse_event.cc b/third_party/blink/common/input/web_mouse_event.cc index 4af22a5985beff..e6069d4da36491 100644 --- a/third_party/blink/common/input/web_mouse_event.cc +++ b/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 diff --git a/third_party/blink/public/common/input/web_mouse_event.h b/third_party/blink/public/common/input/web_mouse_event.h index 2647adf84dff35..d0ee963cd58b01 100644 --- a/third_party/blink/public/common/input/web_mouse_event.h +++ b/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) {} diff --git a/third_party/blink/renderer/core/events/web_input_event_conversion.cc b/third_party/blink/renderer/core/events/web_input_event_conversion.cc index 57225489504a83..148454397b9018 100644 --- a/third_party/blink/renderer/core/events/web_input_event_conversion.cc +++ b/third_party/blink/renderer/core/events/web_input_event_conversion.cc @@ -32,6 +32,8 @@ #include +#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( - {WebInputEvent::kLeftButtonDown, WebInputEvent::kMiddleButtonDown, - WebInputEvent::kRightButtonDown, WebInputEvent::kBackButtonDown, - WebInputEvent::kForwardButtonDown}); - - return web_mouse_button_to_platform_modifier[static_cast(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); diff --git a/third_party/blink/renderer/platform/runtime_enabled_features.json5 b/third_party/blink/renderer/platform/runtime_enabled_features.json5 index 579448c63b1b0c..1588a26db7cf50 100644 --- a/third_party/blink/renderer/platform/runtime_enabled_features.json5 +++ b/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", diff --git a/ui/events/blink/web_input_event.cc b/ui/events/blink/web_input_event.cc index bae25da7d189a4..386027cc0802b1 100644 --- a/ui/events/blink/web_input_event.cc +++ b/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(); diff --git a/ui/events/blink/web_input_event_unittest.cc b/ui/events/blink/web_input_event_unittest.cc index 9866ffef8cab35..b643e9cf72e7fe 100644 --- a/ui/events/blink/web_input_event_unittest.cc +++ b/ui/events/blink/web_input_event_unittest.cc @@ -12,8 +12,10 @@ #include #include +#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 |