From f88df129629d6495e9e4692cafb76cba3c216769 Mon Sep 17 00:00:00 2001 From: "mohsen@chromium.org" Date: Fri, 11 Apr 2014 22:53:02 +0000 Subject: [PATCH] Clip touch selection handles to top of client view's boundaries To hide touch selection handles in fewer situations, we allow top of the handles to go out of client view's boundaries and instead clip it to top of the boundaries. For the bottom, we allow it to stick out of the client view for a few pixels. BUG=345473 Review URL: https://codereview.chromium.org/208473003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@263384 0039d316-1c4b-4281-b951-d872f2087c98 --- .../touchui/touch_selection_controller_impl.cc | 88 +++++++---- ui/views/touchui/touch_selection_controller_impl.h | 9 +- .../touch_selection_controller_impl_unittest.cc | 171 ++++++++++++++++++++- 3 files changed, 230 insertions(+), 38 deletions(-) diff --git a/ui/views/touchui/touch_selection_controller_impl.cc b/ui/views/touchui/touch_selection_controller_impl.cc index 9289f30f8ee7..e5a6b7f90970 100644 --- a/ui/views/touchui/touch_selection_controller_impl.cc +++ b/ui/views/touchui/touch_selection_controller_impl.cc @@ -60,6 +60,13 @@ const int kContextMenuTimoutMs = 200; const int kSelectionHandleQuickFadeDurationMs = 50; +// Minimum height for selection handle bar. If the bar height is going to be +// less than this value, handle will not be shown. +const int kSelectionHandleBarMinHeight = 5; +// Maximum amount that selection handle bar can stick out of client view's +// boundaries. +const int kSelectionHandleBarBottomAllowance = 3; + // Creates a widget to host SelectionHandleView. views::Widget* CreateTouchSelectionPopupWidget( gfx::NativeView context, @@ -99,8 +106,8 @@ gfx::Rect Union(const gfx::Rect& r1, const gfx::Rect& r2) { return gfx::Rect(rx, ry, rr - rx, rb - ry); } -// Convenience method to convert a |rect| from screen to the |client|'s -// coordinate system. +// Convenience methods to convert a |rect| from screen to the |client|'s +// coordinate system and vice versa. // Note that this is not quite correct because it does not take into account // transforms such as rotation and scaling. This should be in TouchEditable. // TODO(varunjain): Fix this. @@ -109,6 +116,11 @@ gfx::Rect ConvertFromScreen(ui::TouchEditable* client, const gfx::Rect& rect) { client->ConvertPointFromScreen(&origin); return gfx::Rect(origin, rect.size()); } +gfx::Rect ConvertToScreen(ui::TouchEditable* client, const gfx::Rect& rect) { + gfx::Point origin = rect.origin(); + client->ConvertPointToScreen(&origin); + return gfx::Rect(origin, rect.size()); +} } // namespace @@ -331,21 +343,26 @@ TouchSelectionControllerImpl::~TouchSelectionControllerImpl() { void TouchSelectionControllerImpl::SelectionChanged() { gfx::Rect r1, r2; client_view_->GetSelectionEndPoints(&r1, &r2); - gfx::Point screen_pos_1(r1.origin()); - client_view_->ConvertPointToScreen(&screen_pos_1); - gfx::Point screen_pos_2(r2.origin()); - client_view_->ConvertPointToScreen(&screen_pos_2); - gfx::Rect screen_rect_1(screen_pos_1, r1.size()); - gfx::Rect screen_rect_2(screen_pos_2, r2.size()); - if (screen_rect_1 == selection_end_point_1_ && - screen_rect_2 == selection_end_point_2_) + gfx::Rect screen_rect_1 = ConvertToScreen(client_view_, r1); + gfx::Rect screen_rect_2 = ConvertToScreen(client_view_, r2); + gfx::Rect client_bounds = client_view_->GetBounds(); + if (r1.y() < client_bounds.y()) + r1.Inset(0, client_bounds.y() - r1.y(), 0, 0); + if (r2.y() < client_bounds.y()) + r2.Inset(0, client_bounds.y() - r2.y(), 0, 0); + gfx::Rect screen_rect_1_clipped = ConvertToScreen(client_view_, r1); + gfx::Rect screen_rect_2_clipped = ConvertToScreen(client_view_, r2); + if (screen_rect_1_clipped == selection_end_point_1_clipped_ && + screen_rect_2_clipped == selection_end_point_2_clipped_) return; selection_end_point_1_ = screen_rect_1; selection_end_point_2_ = screen_rect_2; + selection_end_point_1_clipped_ = screen_rect_1_clipped; + selection_end_point_2_clipped_ = screen_rect_2_clipped; if (client_view_->DrawsHandles()) { - UpdateContextMenu(r1.origin(), r2.origin()); + UpdateContextMenu(); return; } if (dragging_handle_) { @@ -356,14 +373,14 @@ void TouchSelectionControllerImpl::SelectionChanged() { // If the new location of this handle is out of client view, its widget // should not get hidden, since it should still receive touch events. // Hence, we are not using |SetHandleSelectionRect()| method here. - dragging_handle_->SetSelectionRectInScreen(screen_rect_2); + dragging_handle_->SetSelectionRectInScreen(screen_rect_2_clipped); // Temporary fix for selection handle going outside a window. On a webpage, // the page should scroll if the selection handle is dragged outside the // window. That does not happen currently. So we just hide the handle for // now. // TODO(varunjain): Fix this: crbug.com/269003 - dragging_handle_->SetDrawInvisible(!client_view_->GetBounds().Contains(r2)); + dragging_handle_->SetDrawInvisible(!ShouldShowHandleFor(r2)); if (dragging_handle_ != cursor_handle_.get()) { // The non-dragging-handle might have recently become visible. @@ -374,23 +391,27 @@ void TouchSelectionControllerImpl::SelectionChanged() { // selection and the other handle to the start of selection. selection_end_point_1_ = screen_rect_2; selection_end_point_2_ = screen_rect_1; + selection_end_point_1_clipped_ = screen_rect_2_clipped; + selection_end_point_2_clipped_ = screen_rect_1_clipped; } - SetHandleSelectionRect(non_dragging_handle, r1, screen_rect_1); + SetHandleSelectionRect(non_dragging_handle, r1, screen_rect_1_clipped); } } else { - UpdateContextMenu(r1.origin(), r2.origin()); + UpdateContextMenu(); // Check if there is any selection at all. - if (screen_pos_1 == screen_pos_2) { + if (screen_rect_1.origin() == screen_rect_2.origin()) { selection_handle_1_->SetWidgetVisible(false, false); selection_handle_2_->SetWidgetVisible(false, false); - SetHandleSelectionRect(cursor_handle_.get(), r1, screen_rect_1); + SetHandleSelectionRect(cursor_handle_.get(), r1, screen_rect_1_clipped); return; } cursor_handle_->SetWidgetVisible(false, false); - SetHandleSelectionRect(selection_handle_1_.get(), r1, screen_rect_1); - SetHandleSelectionRect(selection_handle_2_.get(), r2, screen_rect_2); + SetHandleSelectionRect(selection_handle_1_.get(), r1, + screen_rect_1_clipped); + SetHandleSelectionRect(selection_handle_2_.get(), r2, + screen_rect_2_clipped); } } @@ -453,11 +474,20 @@ void TouchSelectionControllerImpl::SetHandleSelectionRect( EditingHandleView* handle, const gfx::Rect& rect, const gfx::Rect& rect_in_screen) { - handle->SetWidgetVisible(client_view_->GetBounds().Contains(rect), false); + handle->SetWidgetVisible(ShouldShowHandleFor(rect), false); if (handle->IsWidgetVisible()) handle->SetSelectionRectInScreen(rect_in_screen); } +bool TouchSelectionControllerImpl::ShouldShowHandleFor( + const gfx::Rect& rect) const { + if (rect.height() < kSelectionHandleBarMinHeight) + return false; + gfx::Rect bounds = client_view_->GetBounds(); + bounds.Inset(0, 0, 0, -kSelectionHandleBarBottomAllowance); + return bounds.Contains(rect); +} + bool TouchSelectionControllerImpl::IsCommandIdEnabled(int command_id) const { return client_view_->IsCommandIdEnabled(command_id); } @@ -499,11 +529,11 @@ void TouchSelectionControllerImpl::ContextMenuTimerFired() { gfx::Rect end_rect_1_in_screen; gfx::Rect end_rect_2_in_screen; if (cursor_handle_->IsWidgetVisible()) { - end_rect_1_in_screen = selection_end_point_1_; + end_rect_1_in_screen = selection_end_point_1_clipped_; end_rect_2_in_screen = end_rect_1_in_screen; } else { - end_rect_1_in_screen = selection_end_point_1_; - end_rect_2_in_screen = selection_end_point_2_; + end_rect_1_in_screen = selection_end_point_1_clipped_; + end_rect_2_in_screen = selection_end_point_2_clipped_; } // Convert from screen to client. @@ -514,13 +544,12 @@ void TouchSelectionControllerImpl::ContextMenuTimerFired() { // in the middle of the end points on the top. Else, we show it above the // visible handle. If no handle is visible, we do not show the menu. gfx::Rect menu_anchor; - gfx::Rect client_bounds = client_view_->GetBounds(); - if (client_bounds.Contains(end_rect_1) && - client_bounds.Contains(end_rect_2)) + if (ShouldShowHandleFor(end_rect_1) && + ShouldShowHandleFor(end_rect_2)) menu_anchor = Union(end_rect_1_in_screen,end_rect_2_in_screen); - else if (client_bounds.Contains(end_rect_1)) + else if (ShouldShowHandleFor(end_rect_1)) menu_anchor = end_rect_1_in_screen; - else if (client_bounds.Contains(end_rect_2)) + else if (ShouldShowHandleFor(end_rect_2)) menu_anchor = end_rect_2_in_screen; else return; @@ -540,8 +569,7 @@ void TouchSelectionControllerImpl::StartContextMenuTimer() { &TouchSelectionControllerImpl::ContextMenuTimerFired); } -void TouchSelectionControllerImpl::UpdateContextMenu(const gfx::Point& p1, - const gfx::Point& p2) { +void TouchSelectionControllerImpl::UpdateContextMenu() { // Hide context menu to be shown when the timer fires. HideContextMenu(); StartContextMenuTimer(); diff --git a/ui/views/touchui/touch_selection_controller_impl.h b/ui/views/touchui/touch_selection_controller_impl.h index d96868db6e08..c7694bfe0703 100644 --- a/ui/views/touchui/touch_selection_controller_impl.h +++ b/ui/views/touchui/touch_selection_controller_impl.h @@ -52,6 +52,10 @@ class VIEWS_EXPORT TouchSelectionControllerImpl void SetHandleSelectionRect(EditingHandleView* handle, const gfx::Rect& rect, const gfx::Rect& rect_in_screen); + // Checks if handle should be shown for a selection end-point at |rect|. + // |rect| should be the clipped version of the selection end-point. + bool ShouldShowHandleFor(const gfx::Rect& rect) const; + // Overridden from TouchEditingMenuController. virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE; virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE; @@ -71,7 +75,7 @@ class VIEWS_EXPORT TouchSelectionControllerImpl void StartContextMenuTimer(); // Convenience method to update the position/visibility of the context menu. - void UpdateContextMenu(const gfx::Point& p1, const gfx::Point& p2); + void UpdateContextMenu(); // Convenience method for hiding context menu. void HideContextMenu(); @@ -106,6 +110,9 @@ class VIEWS_EXPORT TouchSelectionControllerImpl // position of handles which might be invalid when handles are hidden. gfx::Rect selection_end_point_1_; gfx::Rect selection_end_point_2_; + // Selection end points, clipped to client view's boundaries. + gfx::Rect selection_end_point_1_clipped_; + gfx::Rect selection_end_point_2_clipped_; DISALLOW_COPY_AND_ASSIGN(TouchSelectionControllerImpl); }; diff --git a/ui/views/touchui/touch_selection_controller_impl_unittest.cc b/ui/views/touchui/touch_selection_controller_impl_unittest.cc index d57205245a4d..f79020a6946a 100644 --- a/ui/views/touchui/touch_selection_controller_impl_unittest.cc +++ b/ui/views/touchui/touch_selection_controller_impl_unittest.cc @@ -5,6 +5,7 @@ #include "base/command_line.h" #include "base/strings/utf_string_conversions.h" #include "grit/ui_resources.h" +#include "ui/aura/client/screen_position_client.h" #include "ui/aura/test/event_generator.h" #include "ui/aura/window.h" #include "ui/base/resource/resource_bundle.h" @@ -27,6 +28,13 @@ namespace { // Should match kSelectionHandlePadding in touch_selection_controller. const int kPadding = 10; +// Should match kSelectionHandleBarMinHeight in touch_selection_controller. +const int kBarMinHeight = 5; + +// Should match kSelectionHandleBarBottomAllowance in +// touch_selection_controller. +const int kBarBottomAllowance = 3; + gfx::Image* GetHandleImage() { static gfx::Image* handle_image = NULL; if (!handle_image) { @@ -46,7 +54,8 @@ namespace views { class TouchSelectionControllerImplTest : public ViewsTestBase { public: TouchSelectionControllerImplTest() - : widget_(NULL), + : textfield_widget_(NULL), + widget_(NULL), textfield_(NULL), views_tsc_factory_(new ViewsTouchSelectionControllerFactory) { CommandLine::ForCurrentProcess()->AppendSwitch( @@ -59,6 +68,8 @@ class TouchSelectionControllerImplTest : public ViewsTestBase { } virtual void TearDown() { + if (textfield_widget_) + textfield_widget_->Close(); if (widget_) widget_->Close(); ViewsTestBase::TearDown(); @@ -66,22 +77,36 @@ class TouchSelectionControllerImplTest : public ViewsTestBase { void CreateTextfield() { textfield_ = new Textfield(); - widget_ = new Widget; + textfield_widget_ = new Widget; Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP); params.bounds = gfx::Rect(0, 0, 200, 200); - widget_->Init(params); + textfield_widget_->Init(params); View* container = new View(); - widget_->SetContentsView(container); + textfield_widget_->SetContentsView(container); container->AddChildView(textfield_); textfield_->SetBoundsRect(params.bounds); textfield_->set_id(1); - widget_->Show(); + textfield_widget_->Show(); textfield_->RequestFocus(); } + void CreateWidget() { + widget_ = new Widget; + Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP); + params.bounds = gfx::Rect(0, 0, 200, 200); + widget_->Init(params); + widget_->Show(); + } + protected: + static bool IsCursorHandleVisibleFor( + ui::TouchSelectionController* controller) { + return static_cast( + controller)->IsCursorHandleVisible(); + } + gfx::Point GetCursorPosition(const gfx::SelectionModel& sel) { gfx::RenderText* render_text = textfield_->GetRenderText(); gfx::Rect cursor_bounds = render_text->GetCursorBounds(sel, true); @@ -138,6 +163,7 @@ class TouchSelectionControllerImplTest : public ViewsTestBase { return textfield_->GetRenderText(); } + Widget* textfield_widget_; Widget* widget_; Textfield* textfield_; @@ -207,11 +233,11 @@ TEST_F(TouchSelectionControllerImplTest, SelectionInTextfieldTest) { VERIFY_HANDLE_POSITIONS(false); // Test with lost focus. - widget_->GetFocusManager()->ClearFocus(); + textfield_widget_->GetFocusManager()->ClearFocus(); EXPECT_FALSE(GetSelectionController()); // Test with focus re-gained. - widget_->GetFocusManager()->SetFocusedView(textfield_); + textfield_widget_->GetFocusManager()->SetFocusedView(textfield_); EXPECT_FALSE(GetSelectionController()); textfield_->OnGestureEvent(&tap); VERIFY_HANDLE_POSITIONS(false); @@ -487,4 +513,135 @@ TEST_F(TouchSelectionControllerImplTest, VERIFY_HANDLE_POSITIONS(false); } +// A simple implementation of TouchEditable that allows faking cursor position +// inside its boundaries. +class TestTouchEditable : public ui::TouchEditable { + public: + explicit TestTouchEditable(aura::Window* window) + : window_(window) { + DCHECK(window); + } + + void set_bounds(const gfx::Rect& bounds) { + bounds_ = bounds; + } + + void set_cursor_rect(const gfx::Rect& cursor_rect) { + cursor_rect_ = cursor_rect; + } + + virtual ~TestTouchEditable() {} + + private: + // Overridden from ui::TouchEditable. + virtual void SelectRect( + const gfx::Point& start, const gfx::Point& end) OVERRIDE { + NOTREACHED(); + } + virtual void MoveCaretTo(const gfx::Point& point) OVERRIDE { + NOTREACHED(); + } + virtual void GetSelectionEndPoints(gfx::Rect* p1, gfx::Rect* p2) OVERRIDE { + *p1 = *p2 = cursor_rect_; + } + virtual gfx::Rect GetBounds() OVERRIDE { + return gfx::Rect(bounds_.size()); + } + virtual gfx::NativeView GetNativeView() const OVERRIDE { + return window_; + } + virtual void ConvertPointToScreen(gfx::Point* point) OVERRIDE { + aura::client::ScreenPositionClient* screen_position_client = + aura::client::GetScreenPositionClient(window_->GetRootWindow()); + if (screen_position_client) + screen_position_client->ConvertPointToScreen(window_, point); + } + virtual void ConvertPointFromScreen(gfx::Point* point) OVERRIDE { + aura::client::ScreenPositionClient* screen_position_client = + aura::client::GetScreenPositionClient(window_->GetRootWindow()); + if (screen_position_client) + screen_position_client->ConvertPointFromScreen(window_, point); + } + virtual bool DrawsHandles() OVERRIDE { + return false; + } + virtual void OpenContextMenu(const gfx::Point& anchor) OVERRIDE { + NOTREACHED(); + } + + // Overridden from ui::SimpleMenuModel::Delegate. + virtual bool IsCommandIdChecked(int command_id) const OVERRIDE { + NOTREACHED(); + return false; + } + virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE { + NOTREACHED(); + return false; + } + virtual bool GetAcceleratorForCommandId( + int command_id, + ui::Accelerator* accelerator) OVERRIDE { + NOTREACHED(); + return false; + } + virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE { + NOTREACHED(); + } + + aura::Window* window_; + + // Boundaries of the client view. + gfx::Rect bounds_; + + // Cursor position inside the client view. + gfx::Rect cursor_rect_; + + DISALLOW_COPY_AND_ASSIGN(TestTouchEditable); +}; + +// Tests if the touch editing handle is shown or hidden properly according to +// the cursor position relative to the client boundaries. +TEST_F(TouchSelectionControllerImplTest, + VisibilityOfHandleRegardingClientBounds) { + CreateWidget(); + + TestTouchEditable touch_editable(widget_->GetNativeView()); + scoped_ptr touch_selection_controller( + ui::TouchSelectionController::create(&touch_editable)); + + touch_editable.set_bounds(gfx::Rect(0, 0, 100, 20)); + + // Put the cursor completely inside the client bounds. Handle should be + // visible. + touch_editable.set_cursor_rect(gfx::Rect(2, 0, 1, 20)); + touch_selection_controller->SelectionChanged(); + EXPECT_TRUE(IsCursorHandleVisibleFor(touch_selection_controller.get())); + + // Move the cursor up such that |kBarMinHeight| pixels are still in the client + // bounds. Handle should still be visible. + touch_editable.set_cursor_rect(gfx::Rect(2, kBarMinHeight - 20, 1, 20)); + touch_selection_controller->SelectionChanged(); + EXPECT_TRUE(IsCursorHandleVisibleFor(touch_selection_controller.get())); + + // Move the cursor up such that less than |kBarMinHeight| pixels are in the + // client bounds. Handle should be hidden. + touch_editable.set_cursor_rect(gfx::Rect(2, kBarMinHeight - 20 - 1, 1, 20)); + touch_selection_controller->SelectionChanged(); + EXPECT_FALSE(IsCursorHandleVisibleFor(touch_selection_controller.get())); + + // Move the Cursor down such that |kBarBottomAllowance| pixels are out of the + // client bounds. Handle should be visible. + touch_editable.set_cursor_rect(gfx::Rect(2, kBarBottomAllowance, 1, 20)); + touch_selection_controller->SelectionChanged(); + EXPECT_TRUE(IsCursorHandleVisibleFor(touch_selection_controller.get())); + + // Move the cursor down such that more than |kBarBottomAllowance| pixels are + // out of the client bounds. Handle should be hidden. + touch_editable.set_cursor_rect(gfx::Rect(2, kBarBottomAllowance + 1, 1, 20)); + touch_selection_controller->SelectionChanged(); + EXPECT_FALSE(IsCursorHandleVisibleFor(touch_selection_controller.get())); + + touch_selection_controller.reset(); +} + } // namespace views -- 2.11.4.GIT