From b0092c498210587441a9fcc2ff92f45acf54c632 Mon Sep 17 00:00:00 2001 From: Mitsuru Oshima Date: Mon, 23 May 2022 17:19:18 +0000 Subject: [PATCH] Use screen coordinates in ExtendeDragSource 1) OnToplevelWindowDragStarted's location is in screen coordinates 2) It alsp simplifies the new window's bounds computation Bug: 1306688 Test: covered by unittests Change-Id: Ib206eacb540452a56a16f2e6a00bcb44b1511cc3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3659257 Reviewed-by: Antonio Gomes Commit-Queue: Mitsuru Oshima Cr-Commit-Position: refs/heads/main@{#1006495} --- .../mouse_cursor_event_filter_unittest.cc | 5 +- ash/test/ash_test_base.cc | 7 ++ ash/wm/drag_window_resizer_unittest.cc | 8 +-- components/exo/extended_drag_source.cc | 35 +++++----- components/exo/extended_drag_source.h | 5 +- .../exo/extended_drag_source_unittest.cc | 67 ++++++++++++++++++- components/exo/pointer.cc | 2 - components/exo/seat.cc | 9 ++- components/exo/seat.h | 6 -- components/exo/touch.cc | 2 - 10 files changed, 99 insertions(+), 47 deletions(-) diff --git a/ash/display/mouse_cursor_event_filter_unittest.cc b/ash/display/mouse_cursor_event_filter_unittest.cc index 0d98a8be7115ca..7c4999efbfeddc 100644 --- a/ash/display/mouse_cursor_event_filter_unittest.cc +++ b/ash/display/mouse_cursor_event_filter_unittest.cc @@ -150,11 +150,10 @@ TEST_F(MouseCursorEventFilterTest, CursorDeviceScaleFactor) { display::test::CreateDisplayLayout(display_manager(), display::DisplayPlacement::RIGHT, 0)); auto* cursor_manager = Shell::Get()->cursor_manager(); - EXPECT_EQ(1.0f, cursor_manager->GetCursor().image_scale_factor()); - TestIfMouseWarpsAt(gfx::Point(399, 200)); + GetEventGenerator()->MoveMouseTo(401, 200); EXPECT_EQ(2.0f, cursor_manager->GetCursor().image_scale_factor()); - TestIfMouseWarpsAt(gfx::Point(400, 200)); + GetEventGenerator()->MoveMouseTo(399, 200); EXPECT_EQ(1.0f, cursor_manager->GetCursor().image_scale_factor()); } diff --git a/ash/test/ash_test_base.cc b/ash/test/ash_test_base.cc index 67d8a2a769b6af..a0814d8d656eca 100644 --- a/ash/test/ash_test_base.cc +++ b/ash/test/ash_test_base.cc @@ -85,8 +85,15 @@ class AshEventGeneratorDelegate ui::EventTarget* GetTargetAt(const gfx::Point& point_in_screen) override { display::Screen* screen = display::Screen::GetScreen(); display::Display display = screen->GetDisplayNearestPoint(point_in_screen); + if (current_display_id_ != display.id()) { + Shell::Get()->cursor_manager()->SetDisplay(display); + current_display_id_ = display.id(); + } return Shell::GetRootWindowForDisplayId(display.id())->GetHost()->window(); } + + private: + int64_t current_display_id_ = display::kInvalidDisplayId; }; } // namespace diff --git a/ash/wm/drag_window_resizer_unittest.cc b/ash/wm/drag_window_resizer_unittest.cc index 3412ed333f779f..6ed8735075016c 100644 --- a/ash/wm/drag_window_resizer_unittest.cc +++ b/ash/wm/drag_window_resizer_unittest.cc @@ -732,11 +732,8 @@ TEST_F(DragWindowResizerTest, CursorDeviceScaleFactor) { CreateDragWindowResizer(window_.get(), gfx::Point(), HTCAPTION)); EXPECT_EQ(1.0f, cursor_manager->GetCursor().image_scale_factor()); ASSERT_TRUE(resizer.get()); - // TODO(crbug.com/990589): Unit tests should be able to simulate mouse input - // without having to call |CursorManager::SetDisplay|. - cursor_manager->SetDisplay(display1); resizer->Drag(CalculateDragPoint(*resizer, 399, 200), 0); - TestIfMouseWarpsAt(gfx::Point(399, 200)); + TestIfMouseWarpsAt(gfx::Point(699, 200)); EXPECT_EQ(2.0f, cursor_manager->GetCursor().image_scale_factor()); resizer->CompleteDrag(); EXPECT_EQ(2.0f, cursor_manager->GetCursor().image_scale_factor()); @@ -752,9 +749,6 @@ TEST_F(DragWindowResizerTest, CursorDeviceScaleFactor) { CreateDragWindowResizer(window_.get(), gfx::Point(), HTCAPTION)); EXPECT_EQ(2.0f, cursor_manager->GetCursor().image_scale_factor()); ASSERT_TRUE(resizer.get()); - // TODO(crbug.com/990589): Unit tests should be able to simulate mouse input - // without having to call |CursorManager::SetDisplay|. - cursor_manager->SetDisplay(display0); resizer->Drag(CalculateDragPoint(*resizer, -200, 200), 0); TestIfMouseWarpsAt(gfx::Point(400, 200)); EXPECT_EQ(1.0f, cursor_manager->GetCursor().image_scale_factor()); diff --git a/components/exo/extended_drag_source.cc b/components/exo/extended_drag_source.cc index c0d9ce4a9a7bed..93e80a97b20b55 100644 --- a/components/exo/extended_drag_source.cc +++ b/components/exo/extended_drag_source.cc @@ -20,6 +20,7 @@ #include "components/exo/wm_helper.h" #include "third_party/abseil-cpp/absl/types/optional.h" #include "ui/aura/client/aura_constants.h" +#include "ui/aura/client/screen_position_client.h" #include "ui/aura/window.h" #include "ui/aura/window_delegate.h" #include "ui/aura/window_event_dispatcher.h" @@ -219,7 +220,7 @@ void ExtendedDragSource::OnToplevelWindowDragStarted( MaybeLockCursor(); if (dragged_window_holder_ && dragged_window_holder_->toplevel_window()) - StartDrag(dragged_window_holder_->toplevel_window(), start_location); + StartDrag(dragged_window_holder_->toplevel_window()); } DragOperation ExtendedDragSource::OnToplevelWindowDragDropped() { @@ -239,7 +240,9 @@ void ExtendedDragSource::OnToplevelWindowDragCancelled() { void ExtendedDragSource::OnToplevelWindowDragEvent(ui::LocatedEvent* event) { DCHECK(event); + aura::Window* target = static_cast(event->target()); pointer_location_ = event->root_location_f(); + wm::ConvertPointToScreen(target->GetRootWindow(), &pointer_location_); if (!dragged_window_holder_) return; @@ -278,13 +281,12 @@ void ExtendedDragSource::UnlockCursor() { } } -void ExtendedDragSource::StartDrag(aura::Window* toplevel, - const gfx::PointF& pointer_location) { +void ExtendedDragSource::StartDrag(aura::Window* toplevel) { // Ensure |toplevel| window does skip events while it's being dragged. event_blocker_ = std::make_unique(toplevel); - DVLOG(1) << "Starting drag. pointer_loc=" << pointer_location.ToString(); + DVLOG(1) << "Starting drag. pointer_loc=" << pointer_location_.ToString(); auto* toplevel_handler = ash::Shell::Get()->toplevel_window_event_handler(); auto move_source = drag_event_source_ == ui::mojom::DragEventSource::kTouch ? ::wm::WINDOW_MOVE_SOURCE_TOUCH @@ -305,8 +307,12 @@ void ExtendedDragSource::StartDrag(aura::Window* toplevel, // TODO(crbug.com/1167581): Experiment setting |update_gesture_target| back // to true when capture is removed from drag and drop. + + gfx::PointF pointer_location_in_parent(pointer_location_); + wm::ConvertPointFromScreen(toplevel->parent(), &pointer_location_in_parent); + toplevel_handler->AttemptToStartDrag( - toplevel, pointer_location, HTCAPTION, move_source, + toplevel, pointer_location_in_parent, HTCAPTION, move_source, std::move(end_closure), /*update_gesture_target=*/false, /*grab_capture =*/ @@ -324,7 +330,6 @@ void ExtendedDragSource::OnDraggedWindowVisibilityChanging(bool visible) { aura::Window* toplevel = dragged_window_holder_->toplevel_window(); DCHECK(toplevel); - DCHECK(drag_source_window_); toplevel->SetProperty(ash::kIsDraggingTabsKey, true); if (drag_source_window_ != toplevel) { @@ -349,11 +354,14 @@ void ExtendedDragSource::OnDraggedWindowVisibilityChanged(bool visible) { // The |toplevel| window for the dragged surface has just been created and // it's about to be mapped. Calculate and set its position based on // |drag_offset_| and |pointer_location_| before starting the actual drag. - auto screen_location = CalculateOrigin(toplevel); + auto screen_location = + gfx::ToFlooredPoint(pointer_location_ - dragged_window_holder_->offset()); auto toplevel_bounds = gfx::Rect({screen_location, toplevel->bounds().size()}); - toplevel->SetBounds(toplevel_bounds); + auto display = display::Screen::GetScreen()->GetDisplayNearestWindow( + drag_source_window_); + toplevel->SetBoundsInScreen(toplevel_bounds, display); if (WMHelper::GetInstance()->InTabletMode()) { // The bounds that is stored in ash::kRestoreBoundsOverrideKey will be used @@ -366,16 +374,7 @@ void ExtendedDragSource::OnDraggedWindowVisibilityChanged(bool visible) { DVLOG(1) << "Dragged window mapped. toplevel=" << toplevel << " origin=" << screen_location.ToString(); - gfx::PointF pointer_location(screen_location + - dragged_window_holder_->offset()); - StartDrag(toplevel, pointer_location); -} - -gfx::Point ExtendedDragSource::CalculateOrigin(aura::Window* target) const { - DCHECK(dragged_window_holder_); - gfx::Point screen_location = gfx::ToRoundedPoint(pointer_location_); - wm::ConvertPointToScreen(target->GetRootWindow(), &screen_location); - return screen_location - dragged_window_holder_->offset(); + StartDrag(toplevel); } void ExtendedDragSource::Cleanup() { diff --git a/components/exo/extended_drag_source.h b/components/exo/extended_drag_source.h index e3a19002047dba..f1fa7c1ab8407d 100644 --- a/components/exo/extended_drag_source.h +++ b/components/exo/extended_drag_source.h @@ -92,11 +92,9 @@ class ExtendedDragSource : public DataSourceObserver, void MaybeLockCursor(); void UnlockCursor(); - void StartDrag(aura::Window* toplevel, - const gfx::PointF& pointer_location_in_screen); + void StartDrag(aura::Window* toplevel); void OnDraggedWindowVisibilityChanging(bool visible); void OnDraggedWindowVisibilityChanged(bool visible); - gfx::Point CalculateOrigin(aura::Window* target) const; void Cleanup(); static ExtendedDragSource* instance_; @@ -107,6 +105,7 @@ class ExtendedDragSource : public DataSourceObserver, // tied to the zcr_extended_drag_source_v1 object it's attached to. Delegate* const delegate_; + // The pointer location in screen coordinates. gfx::PointF pointer_location_; ui::mojom::DragEventSource drag_event_source_; bool cursor_locked_ = false; diff --git a/components/exo/extended_drag_source_unittest.cc b/components/exo/extended_drag_source_unittest.cc index c0f2c11dd49abe..2c4125404d260e 100644 --- a/components/exo/extended_drag_source_unittest.cc +++ b/components/exo/extended_drag_source_unittest.cc @@ -14,6 +14,7 @@ #include "ash/public/cpp/window_properties.h" #include "ash/shell.h" #include "ash/wm/toplevel_window_event_handler.h" +#include "ash/wm/window_state.h" #include "base/strings/utf_string_conversions.h" #include "base/test/bind.h" #include "base/test/gmock_callback_support.h" @@ -596,9 +597,9 @@ TEST_F(ExtendedDragSourceTest, DragWithScreenCoordinates) { auto operation = DragDropOperation::Create( &data_exchange_delegate, data_source.get(), shell_surface->root_surface(), nullptr, location, ui::mojom::DragEventSource::kMouse); - auto* drag_drop_controller = static_cast( aura::client::GetDragDropClient(ash::Shell::GetPrimaryRootWindow())); + EXPECT_FALSE(shell_surface->IsDragged()); base::RunLoop loop; drag_drop_controller->SetLoopClosureForTesting( @@ -615,4 +616,68 @@ TEST_F(ExtendedDragSourceTest, DragWithScreenCoordinates) { EXPECT_FALSE(shell_surface->IsDragged()); } +TEST_F(ExtendedDragSourceTest, DragToAnotherDisplay) { + UpdateDisplay("800x600,800x600"); + + // Create and map a toplevel shell surface. + auto shell_surface = + exo::test::ShellSurfaceBuilder({32, 32}).BuildShellSurface(); + auto* surface = shell_surface->root_surface(); + + shell_surface->GetWidget()->SetBounds({810, 10, 32, 32}); + + auto display = display::Screen::GetScreen()->GetDisplayNearestWindow( + shell_surface->GetWidget()->GetNativeWindow()); + EXPECT_EQ(gfx::Rect(800, 0, 800, 600), display.bounds()); + + gfx::Rect expected_bounds = + shell_surface->GetWidget()->GetWindowBoundsInScreen(); + + constexpr int kDragOffset = 10; + extended_drag_source_->Drag(surface, gfx::Vector2d(kDragOffset, 0)); + + // Start the DND + extended-drag session. + // Creates a mouse-pressed event before starting the drag session. + ui::test::EventGenerator* generator = GetEventGenerator(); + generator->MoveMouseTo({810 + kDragOffset, 10}); + generator->PressLeftButton(); + + // Start a DragDropOperation. + drag_drop_controller_->set_should_block_during_drag_drop(true); + seat_->StartDrag(data_source_.get(), surface, /*icon=*/nullptr, + ui::mojom::DragEventSource::kMouse); + // Just move to the middle to avoid snapping. + int x_movement = 500; + expected_bounds.set_x(310); + + constexpr int kXDragDelta = 20; + auto* toplevel_handler = ash::Shell::Get()->toplevel_window_event_handler(); + + base::RunLoop loop; + drag_drop_controller_->SetLoopClosureForTesting( + base::BindLambdaForTesting([&]() { + if (x_movement == 500) { + auto* window_state = ash::WindowState::Get( + shell_surface->GetWidget()->GetNativeWindow()); + EXPECT_EQ(gfx::PointF(20, 10), + window_state->drag_details()->initial_location_in_parent); + } + if (x_movement > 0) { + x_movement -= kXDragDelta; + generator->MoveMouseBy(-kXDragDelta, 0); + EXPECT_TRUE(toplevel_handler->is_drag_in_progress()); + } else { + generator->ReleaseLeftButton(); + } + }), + loop.QuitClosure()); + loop.Run(); + EXPECT_FALSE(toplevel_handler->is_drag_in_progress()); + EXPECT_EQ(expected_bounds, + shell_surface->GetWidget()->GetWindowBoundsInScreen()); + display = display::Screen::GetScreen()->GetDisplayNearestWindow( + shell_surface->GetWidget()->GetNativeWindow()); + EXPECT_EQ(gfx::Rect(0, 0, 800, 600), display.bounds()); +} + } // namespace exo diff --git a/components/exo/pointer.cc b/components/exo/pointer.cc index 1163ba9ff4d63d..e0bcb66d26dcff 100644 --- a/components/exo/pointer.cc +++ b/components/exo/pointer.cc @@ -486,8 +486,6 @@ void Pointer::OnMouseEvent(ui::MouseEvent* event) { if (event->type() == ui::ET_MOUSE_CAPTURE_CHANGED) return; - seat_->SetLastPointerLocation(event->root_location_f()); - Surface* target = GetEffectiveTargetForEvent(event); gfx::PointF location_in_target = event->location_f(); if (target) { diff --git a/components/exo/seat.cc b/components/exo/seat.cc index 08354790e7c43d..156952b8d2fb62 100644 --- a/components/exo/seat.cc +++ b/components/exo/seat.cc @@ -31,6 +31,7 @@ #include "ui/base/clipboard/scoped_clipboard_writer.h" #include "ui/base/data_transfer_policy/data_transfer_endpoint.h" #include "ui/base/data_transfer_policy/data_transfer_endpoint_serializer.h" +#include "ui/display/screen.h" #include "ui/events/event_utils.h" #include "ui/events/platform/platform_event_source.h" #include "ui/gfx/geometry/point_f.h" @@ -130,13 +131,11 @@ void Seat::StartDrag(DataSource* source, Surface* icon, ui::mojom::DragEventSource event_source) { // DragDropOperation manages its own lifetime. + auto cursor_location = + gfx::PointF(display::Screen::GetScreen()->GetCursorScreenPoint()); drag_drop_operation_ = DragDropOperation::Create(data_exchange_delegate_.get(), source, origin, - icon, last_pointer_location_, event_source); -} - -void Seat::SetLastPointerLocation(const gfx::PointF& last_pointer_location) { - last_pointer_location_ = last_pointer_location; + icon, cursor_location, event_source); } void Seat::AbortPendingDragOperation() { diff --git a/components/exo/seat.h b/components/exo/seat.h index 619eda37fff306..82129c46c2a4ae 100644 --- a/components/exo/seat.h +++ b/components/exo/seat.h @@ -118,10 +118,6 @@ class Seat : public aura::client::FocusChangeObserver, Surface* icon, ui::mojom::DragEventSource event_source); - // Sets the last location in screen coordinates, irrespective of mouse or - // touch. - void SetLastPointerLocation(const gfx::PointF& last_pointer_location); - // Abort any drag operations that haven't been started yet. void AbortPendingDragOperation(); @@ -237,8 +233,6 @@ class Seat : public aura::client::FocusChangeObserver, // True while Seat is updating clipboard data to selection source. bool changing_clipboard_data_to_selection_source_; - gfx::PointF last_pointer_location_; - bool was_shutdown_ = false; #if BUILDFLAG(IS_CHROMEOS_ASH) diff --git a/components/exo/touch.cc b/components/exo/touch.cc index 6ad15885bf53fe..2991da66875023 100644 --- a/components/exo/touch.cc +++ b/components/exo/touch.cc @@ -65,8 +65,6 @@ void Touch::OnTouchEvent(ui::TouchEvent* event) { if (seat_->was_shutdown()) return; - seat_->SetLastPointerLocation(event->root_location_f()); - bool send_details = false; const int touch_pointer_id = event->pointer_details().id;