From 7674929cec1d1876aee6ab235f946dad6442a7d2 Mon Sep 17 00:00:00 2001 From: conniekxu Date: Fri, 13 May 2022 02:45:29 +0000 Subject: [PATCH] capture_mode: Make select capture region work properly This CL fixed the issue that capture region is not selectable nor draggable after start pressing on the capture bar or settings and then release the press outsiede of it. To fix it, we should let the event targeted on the capture bar or settings go through and to be handled by it instead of setting the event as handled and stopping propagation. Test: Manual, added a regression test Change-Id: I8f5fbbd30d95f0fd520a67a32fcac5c9a04f9869 Fixed: 1325028 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645696 Commit-Queue: Ahmed Fakhry Reviewed-by: Ahmed Fakhry Cr-Commit-Position: refs/heads/main@{#1002973} --- ash/capture_mode/capture_mode_session.cc | 38 +++++++--------------- ash/capture_mode/capture_mode_session.h | 8 ----- ash/capture_mode/capture_mode_unittests.cc | 25 ++++++++++++++ 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/ash/capture_mode/capture_mode_session.cc b/ash/capture_mode/capture_mode_session.cc index d826804b8de5d3..fea14c245b9b6b 100644 --- a/ash/capture_mode/capture_mode_session.cc +++ b/ash/capture_mode/capture_mode_session.cc @@ -1966,29 +1966,6 @@ void CaptureModeSession::OnLocatedEvent(ui::LocatedEvent* event, } DCHECK(is_capture_region); - // Allow events that are located on the capture mode bar or settings menu to - // pass through so we can click the buttons. - if (!is_event_on_capture_bar && - !(capture_mode_settings_widget_ && - capture_mode_settings_widget_->GetWindowBoundsInScreen().Contains( - screen_location))) { - if (capture_mode_settings_widget_ && - located_press_event_on_settings_menu_) { - capture_mode_settings_widget_->GetNativeWindow()->delegate()->OnEvent( - event); - } - event->SetHandled(); - event->StopPropagation(); - } - - // OnLocatedEventPressed() and OnLocatedEventDragged used root locations since - // CaptureModeController::user_capture_region() is stored in root - // coordinates.. - // TODO(sammiequon): Update CaptureModeController::user_capture_region() to - // store screen coordinates. - gfx::Point location_in_root = event->location(); - aura::Window::ConvertPointToTarget(event_target, current_root_, - &location_in_root); // Early return if the given event is targeted on the capture bar or settings // menu, since the switch block below is for updating the user capture region @@ -2003,11 +1980,21 @@ void CaptureModeSession::OnLocatedEvent(ui::LocatedEvent* event, return; } + event->SetHandled(); + event->StopPropagation(); + + // OnLocatedEventPressed() and OnLocatedEventDragged used root locations since + // CaptureModeController::user_capture_region() is stored in root + // coordinates.. + // TODO(sammiequon): Update CaptureModeController::user_capture_region() to + // store screen coordinates. + gfx::Point location_in_root = event->location(); + aura::Window::ConvertPointToTarget(event_target, current_root_, + &location_in_root); + switch (event->type()) { case ui::ET_MOUSE_PRESSED: case ui::ET_TOUCH_PRESSED: - if (is_event_on_settings_menu) - located_press_event_on_settings_menu_ = true; old_mouse_warp_status_ = SetMouseWarpEnabled(false); OnLocatedEventPressed(location_in_root, is_touch); break; @@ -2022,7 +2009,6 @@ void CaptureModeSession::OnLocatedEvent(ui::LocatedEvent* event, SetMouseWarpEnabled(*old_mouse_warp_status_); old_mouse_warp_status_.reset(); OnLocatedEventReleased(location_in_root); - located_press_event_on_settings_menu_ = false; break; default: break; diff --git a/ash/capture_mode/capture_mode_session.h b/ash/capture_mode/capture_mode_session.h index 5c3cfab6a28b36..8916f1e0fe8645 100644 --- a/ash/capture_mode/capture_mode_session.h +++ b/ash/capture_mode/capture_mode_session.h @@ -538,14 +538,6 @@ class ASH_EXPORT CaptureModeSession // The object which handles tab focus while in a capture session. std::unique_ptr focus_cycler_; - // This helps indicating whether located events should be handled by the - // capture mode settings menu view or the capture mode Pre-EventHandler. When - // it's true, settings menu view should handle the event. Set it to true when - // the event is a press event and is located on the settings menu view. Set it - // to false when the event is a release event and "event_on_settings_menu_" is - // true. - bool located_press_event_on_settings_menu_ = false; - // True if a located event should be passed to camera preview to be handled. bool should_pass_located_event_to_camera_preview_ = false; diff --git a/ash/capture_mode/capture_mode_unittests.cc b/ash/capture_mode/capture_mode_unittests.cc index 97152609205525..3bb16787c18df2 100644 --- a/ash/capture_mode/capture_mode_unittests.cc +++ b/ash/capture_mode/capture_mode_unittests.cc @@ -1064,6 +1064,31 @@ TEST_F(CaptureModeTest, CaptureRegionCaptureButtonDoesNotIntersectCaptureBar) { EXPECT_GT(GetCaptureModeLabelWidget()->GetWindowBoundsInScreen().x(), 20); } +// Tests that pressing on the capture bar and releasing the press outside of the +// capture bar, the capture region could still be draggable and set. Regression +// test for https://crbug.com/1325028. +TEST_F(CaptureModeTest, SetCaptureRegionAfterPressOnCaptureBar) { + UpdateDisplay("800x600"); + + auto* controller = + StartCaptureSession(CaptureModeSource::kRegion, CaptureModeType::kVideo); + auto* settings_button = GetSettingsButton(); + + // Press on the settings button without release. + auto* event_generator = GetEventGenerator(); + event_generator->MoveMouseTo( + settings_button->GetBoundsInScreen().CenterPoint()); + event_generator->PressLeftButton(); + // Move mouse to the outside of the capture bar and then release the press. + event_generator->MoveMouseTo({300, 300}); + event_generator->ReleaseLeftButton(); + + // Set the capture region, and verify it's set successfully. + const gfx::Rect region_bounds(100, 100, 200, 200); + SelectRegion(region_bounds); + EXPECT_EQ(controller->user_capture_region(), region_bounds); +} + TEST_F(CaptureModeTest, WindowCapture) { // Create 2 windows that overlap with each other. const gfx::Rect bounds1(0, 0, 200, 200);