From 9494b2d142aaf9584ee0fe00c242167ff6685404 Mon Sep 17 00:00:00 2001 From: Michelle Date: Wed, 25 Jan 2023 00:18:39 +0000 Subject: [PATCH] Use base and extent in TouchSelectionControllerImpl drag updates. Rewrite some views touch selection methods in preparation for implementing an "expand by word, shrink by character" feature (part of go/cros-touch-text-editing, for visualisation of feature see https://drive.google.com/file/d/1LdcaCgzLQCrSaqXF9gxgTPOj_l1Eetup/view?usp=sharing): 1. In TouchSelectionControllerImpl, replace SetDraggingHandle and SelectionHandleDragged by OnDragBegin, OnDragUpdate and OnDragEnd, based on the corresponding methods in TouchSelectionController. 2. In TouchEditable, replace MoveCaretTo and SelectRect by MoveCaret, MoveRangeSelectionExtent and SelectBetweenCoordinates, based on the TouchSelectionControllerClient interface. This CL doesn't change any visible behaviour. The new "expand by word, shrink by character" behaviour will be implemented in later CLs. Bug: b:244116654, b:264824692 Change-Id: I0b3e3549b277297611d3f0765e0751579898d94b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4172677 Reviewed-by: Mohsen Izadi Reviewed-by: Dana Fried Reviewed-by: Mitsuru Oshima Commit-Queue: Michelle Chen Cr-Commit-Position: refs/heads/main@{#1096516} --- ui/base/pointer/touch_editing_controller.h | 41 ++++++++++-- ui/views/controls/textfield/textfield.cc | 37 ++++++++--- ui/views/controls/textfield/textfield.h | 6 +- .../controls/textfield/textfield_unittest.cc | 58 +++++++++++++++++ .../touch_selection_controller_impl.cc | 65 +++++++++++-------- .../touchui/touch_selection_controller_impl.h | 6 +- ...ouch_selection_controller_impl_unittest.cc | 16 +++-- 7 files changed, 177 insertions(+), 52 deletions(-) diff --git a/ui/base/pointer/touch_editing_controller.h b/ui/base/pointer/touch_editing_controller.h index 8e3d761a7b809..e29ff1996feac 100644 --- a/ui/base/pointer/touch_editing_controller.h +++ b/ui/base/pointer/touch_editing_controller.h @@ -33,16 +33,43 @@ class COMPONENT_EXPORT(UI_BASE) TouchEditable kLastTouchEditableCommandId = kSelectWord, }; - // TODO(mohsen): Consider switching from local coordinates to screen + // TODO(b/266345972): Consider switching from local coordinates to screen // coordinates in this interface and see if it will simplify things. - // Select everything between start and end (points are in view's local - // coordinate system). |start| is the logical start and |end| is the logical - // end of selection. Visually, |start| may lie after |end|. - virtual void SelectRect(const gfx::Point& start, const gfx::Point& end) = 0; + // Moves the caret to |position|. |position| is in local coordinates. + virtual void MoveCaret(const gfx::Point& position) = 0; - // Move the caret to |point|. |point| is in local coordinates. - virtual void MoveCaretTo(const gfx::Point& point) = 0; + // Moves the logical end of the selection according to |extent| while keeping + // the logical start of the selection fixed. Here, |extent| corresponds to the + // position (in local coordinates) of the touch handle being dragged to update + // the selection range. + // + // Note that the resultant end of the selection depends on the behaviour of + // the TouchEditable, e.g. for "expand by word, shrink by character", the + // selection end can move to the character or word boundary nearest to + // |extent| depending on the previous extent position: + // ____________________________________ + // | textf|ield wit|h selected text | + // ------------------------------------ + // ^extent + // ^start ^end + // + // ____________________________________ + // | textf|ield with selec|ted| text | + // ------------------------------------ + // ^extent + // ^start ^end + // + virtual void MoveRangeSelectionExtent(const gfx::Point& extent) = 0; + + // Sets the logical start and end of the selection according to |base| and + // |extent|. |base| corresponds to the position of the fixed touch handle and + // determines the logical start of the selection. |extent| corresponds to the + // position of the currently dragging handle and determines the logical end of + // the selection, which may be visually before, on, or after the logical start + // of the selection. Both |base| and |start| are in local coordinates. + virtual void SelectBetweenCoordinates(const gfx::Point& base, + const gfx::Point& extent) = 0; // Gets the end points of the current selection. The end points |anchor| and // |focus| must be the cursor rect for the logical start and logical end of diff --git a/ui/views/controls/textfield/textfield.cc b/ui/views/controls/textfield/textfield.cc index 119f89e3478f3..479947174ddce 100644 --- a/ui/views/controls/textfield/textfield.cc +++ b/ui/views/controls/textfield/textfield.cc @@ -1184,23 +1184,44 @@ bool Textfield::HasTextBeingDragged() const { //////////////////////////////////////////////////////////////////////////////// // Textfield, ui::TouchEditable overrides: -void Textfield::SelectRect(const gfx::Point& start, const gfx::Point& end) { - if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE) +void Textfield::MoveCaret(const gfx::Point& position) { + SelectBetweenCoordinates(position, position); +} + +void Textfield::MoveRangeSelectionExtent(const gfx::Point& extent) { + if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE) { return; + } - gfx::SelectionModel start_caret = GetRenderText()->FindCursorPosition(start); - gfx::SelectionModel end_caret = GetRenderText()->FindCursorPosition(end); + gfx::SelectionModel base_caret = + GetRenderText()->GetSelectionModelForSelectionStart(); + gfx::SelectionModel extent_caret = + GetRenderText()->FindCursorPosition(extent); gfx::SelectionModel selection( - gfx::Range(start_caret.caret_pos(), end_caret.caret_pos()), - end_caret.caret_affinity()); + gfx::Range(base_caret.caret_pos(), extent_caret.caret_pos()), + extent_caret.caret_affinity()); OnBeforeUserAction(); SelectSelectionModel(selection); OnAfterUserAction(); } -void Textfield::MoveCaretTo(const gfx::Point& point) { - SelectRect(point, point); +void Textfield::SelectBetweenCoordinates(const gfx::Point& base, + const gfx::Point& extent) { + if (GetTextInputType() == ui::TEXT_INPUT_TYPE_NONE) { + return; + } + + gfx::SelectionModel base_caret = GetRenderText()->FindCursorPosition(base); + gfx::SelectionModel extent_caret = + GetRenderText()->FindCursorPosition(extent); + gfx::SelectionModel selection( + gfx::Range(base_caret.caret_pos(), extent_caret.caret_pos()), + extent_caret.caret_affinity()); + + OnBeforeUserAction(); + SelectSelectionModel(selection); + OnAfterUserAction(); } void Textfield::GetSelectionEndPoints(gfx::SelectionBound* anchor, diff --git a/ui/views/controls/textfield/textfield.h b/ui/views/controls/textfield/textfield.h index 5782d993657dc..043b0b40056f9 100644 --- a/ui/views/controls/textfield/textfield.h +++ b/ui/views/controls/textfield/textfield.h @@ -387,8 +387,10 @@ class VIEWS_EXPORT Textfield : public View, bool HasTextBeingDragged() const override; // ui::TouchEditable overrides: - void SelectRect(const gfx::Point& start, const gfx::Point& end) override; - void MoveCaretTo(const gfx::Point& point) override; + void MoveCaret(const gfx::Point& position) override; + void MoveRangeSelectionExtent(const gfx::Point& extent) override; + void SelectBetweenCoordinates(const gfx::Point& base, + const gfx::Point& extent) override; void GetSelectionEndPoints(gfx::SelectionBound* anchor, gfx::SelectionBound* focus) override; gfx::Rect GetBounds() override; diff --git a/ui/views/controls/textfield/textfield_unittest.cc b/ui/views/controls/textfield/textfield_unittest.cc index 27ade25954d51..be7755c48f615 100644 --- a/ui/views/controls/textfield/textfield_unittest.cc +++ b/ui/views/controls/textfield/textfield_unittest.cc @@ -3802,6 +3802,64 @@ TEST_F(TextfieldTouchSelectionTest, MAYBE_TapOnSelection) { EXPECT_EQ(tap_range, range); } +TEST_F(TextfieldTest, MoveCaret) { + InitTextfield(); + textfield_->SetText(u"hello world"); + const int cursor_y = GetCursorYForTesting(); + gfx::Range range; + + textfield_->MoveCaret(gfx::Point(GetCursorPositionX(3), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(3)); + + textfield_->MoveCaret(gfx::Point(GetCursorPositionX(0), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(0)); + + textfield_->MoveCaret(gfx::Point(GetCursorPositionX(11), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(11)); +} + +TEST_F(TextfieldTest, MoveRangeSelectionExtent) { + InitTextfield(); + textfield_->SetText(u"hello world"); + const int cursor_y = GetCursorYForTesting(); + gfx::Range range; + + textfield_->SelectBetweenCoordinates( + gfx::Point(GetCursorPositionX(2), cursor_y), + gfx::Point(GetCursorPositionX(3), cursor_y)); + textfield_->MoveRangeSelectionExtent( + gfx::Point(GetCursorPositionX(5), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(2, 5)); + + textfield_->MoveRangeSelectionExtent( + gfx::Point(GetCursorPositionX(0), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(2, 0)); +} + +TEST_F(TextfieldTest, SelectBetweenCoordinates) { + InitTextfield(); + textfield_->SetText(u"hello world"); + const int cursor_y = GetCursorYForTesting(); + gfx::Range range; + + textfield_->SelectBetweenCoordinates( + gfx::Point(GetCursorPositionX(1), cursor_y), + gfx::Point(GetCursorPositionX(2), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(1, 2)); + + textfield_->SelectBetweenCoordinates( + gfx::Point(GetCursorPositionX(0), cursor_y), + gfx::Point(GetCursorPositionX(11), cursor_y)); + textfield_->GetEditableSelectionRange(&range); + EXPECT_EQ(range, gfx::Range(0, 11)); +} + TEST_F(TextfieldTest, AccessiblePasswordTest) { InitTextfield(); textfield_->SetText(u"password"); diff --git a/ui/views/touchui/touch_selection_controller_impl.cc b/ui/views/touchui/touch_selection_controller_impl.cc index 60859c7f4e17e..7281f69a3740f 100644 --- a/ui/views/touchui/touch_selection_controller_impl.cc +++ b/ui/views/touchui/touch_selection_controller_impl.cc @@ -249,7 +249,7 @@ class TouchSelectionControllerImpl::EditingHandleView : public View { switch (event->type()) { case ui::ET_GESTURE_SCROLL_BEGIN: { widget_->SetCapture(this); - controller_->SetDraggingHandle(this); + controller_->OnDragBegin(this); // Distance from the point which is |kSelectionHandleVerticalDragOffset| // pixels above the bottom of the selection bound edge to the event // location (aka the touch-drag point). @@ -259,13 +259,13 @@ class TouchSelectionControllerImpl::EditingHandleView : public View { break; } case ui::ET_GESTURE_SCROLL_UPDATE: { - controller_->SelectionHandleDragged(event->location() + drag_offset_); + controller_->OnDragUpdate(event->location() + drag_offset_); break; } case ui::ET_GESTURE_SCROLL_END: case ui::ET_SCROLL_FLING_START: { widget_->ReleaseCapture(); - controller_->SetDraggingHandle(nullptr); + controller_->OnDragEnd(); break; } default: @@ -360,8 +360,7 @@ class TouchSelectionControllerImpl::EditingHandleView : public View { bool is_cursor_handle_; // Offset applied to the scroll events location when calling - // TouchSelectionControllerImpl::SelectionHandleDragged while dragging the - // handle. + // TouchSelectionControllerImpl::OnDragUpdate while dragging the handle. gfx::Vector2d drag_offset_; // If set to true, the handle will not draw anything, hence providing an empty @@ -508,40 +507,52 @@ void TouchSelectionControllerImpl::ShowQuickMenuImmediatelyForTesting() { } } -void TouchSelectionControllerImpl::SetDraggingHandle( - EditingHandleView* handle) { +void TouchSelectionControllerImpl::OnDragBegin(EditingHandleView* handle) { dragging_handle_ = handle; - if (dragging_handle_) - HideQuickMenu(); - else - StartQuickMenuTimer(); + HideQuickMenu(); + if (dragging_handle_ == cursor_handle_) { + return; + } + + DCHECK(dragging_handle_ == selection_handle_1_ || + dragging_handle_ == selection_handle_2_); + + // Find selection end points in client_view's coordinate system. + gfx::Point base = selection_bound_1_.edge_start_rounded(); + base.Offset(0, selection_bound_1_.GetHeight() / 2); + client_view_->ConvertPointFromScreen(&base); + + gfx::Point extent = selection_bound_2_.edge_start_rounded(); + extent.Offset(0, selection_bound_2_.GetHeight() / 2); + client_view_->ConvertPointFromScreen(&extent); + + if (dragging_handle_ == selection_handle_1_) { + std::swap(base, extent); + } + + // When moving the handle we want to move only the extent point. Before + // doing so we must make sure that the base point is set correctly. + client_view_->SelectBetweenCoordinates(base, extent); } -void TouchSelectionControllerImpl::SelectionHandleDragged( - const gfx::Point& drag_pos) { +void TouchSelectionControllerImpl::OnDragUpdate(const gfx::Point& drag_pos) { DCHECK(dragging_handle_); gfx::Point drag_pos_in_client = drag_pos; ConvertPointToClientView(dragging_handle_, &drag_pos_in_client); if (dragging_handle_ == cursor_handle_) { - client_view_->MoveCaretTo(drag_pos_in_client); + client_view_->MoveCaret(drag_pos_in_client); return; } - // Find the stationary selection handle. - gfx::SelectionBound anchor_bound = selection_handle_1_ == dragging_handle_ - ? selection_bound_2_ - : selection_bound_1_; + DCHECK(dragging_handle_ == selection_handle_1_ || + dragging_handle_ == selection_handle_2_); + client_view_->MoveRangeSelectionExtent(drag_pos_in_client); +} - // Find selection end points in client_view's coordinate system. - gfx::Point p2 = anchor_bound.edge_start_rounded(); - p2.Offset(0, anchor_bound.GetHeight() / 2); - client_view_->ConvertPointFromScreen(&p2); - - // Instruct client_view to select the region between p1 and p2. The position - // of |fixed_handle| is the start and that of |dragging_handle| is the end - // of selection. - client_view_->SelectRect(p2, drag_pos_in_client); +void TouchSelectionControllerImpl::OnDragEnd() { + dragging_handle_ = nullptr; + StartQuickMenuTimer(); } void TouchSelectionControllerImpl::ConvertPointToClientView( diff --git a/ui/views/touchui/touch_selection_controller_impl.h b/ui/views/touchui/touch_selection_controller_impl.h index 000bc4e1b1cb3..98377941d2dfd 100644 --- a/ui/views/touchui/touch_selection_controller_impl.h +++ b/ui/views/touchui/touch_selection_controller_impl.h @@ -47,13 +47,15 @@ class VIEWS_EXPORT TouchSelectionControllerImpl private: friend class TouchSelectionControllerImplTest; - void SetDraggingHandle(EditingHandleView* handle); + void OnDragBegin(EditingHandleView* handle); // Callback to inform the client view that the selection handle has been // dragged, hence selection may need to be updated. |drag_pos| is the new // position for the edge of the selection corresponding to |dragging_handle_|, // specified in handle's coordinates - void SelectionHandleDragged(const gfx::Point& drag_pos); + void OnDragUpdate(const gfx::Point& drag_pos); + + void OnDragEnd(); // Convenience method to convert a point from a selection handle's coordinate // system to that of the client view. diff --git a/ui/views/touchui/touch_selection_controller_impl_unittest.cc b/ui/views/touchui/touch_selection_controller_impl_unittest.cc index 7774d75c50e2c..77d3b787022ec 100644 --- a/ui/views/touchui/touch_selection_controller_impl_unittest.cc +++ b/ui/views/touchui/touch_selection_controller_impl_unittest.cc @@ -408,9 +408,9 @@ TEST_F(TouchSelectionControllerImplTest, SelectionInBidiTextfieldTest) { VerifyHandlePositions(false, true, FROM_HERE); } -// Tests if the SelectRect callback is called appropriately when selection -// handles are moved. -TEST_F(TouchSelectionControllerImplTest, SelectRectCallbackTest) { +// Tests if the selection update callbacks are called appropriately when +// selection handles are moved. +TEST_F(TouchSelectionControllerImplTest, SelectionUpdateCallbackTest) { CreateTextfield(); textfield_->SetText(u"textfield with selected text"); // Tap the textfield to invoke touch selection. @@ -452,7 +452,7 @@ TEST_F(TouchSelectionControllerImplTest, SelectRectCallbackTest) { VerifyHandlePositions(false, true, FROM_HERE); } -TEST_F(TouchSelectionControllerImplTest, SelectRectInBidiCallbackTest) { +TEST_F(TouchSelectionControllerImplTest, SelectionUpdateInBidiCallbackTest) { CreateTextfield(); textfield_->SetText( u"abc\x05e1\x05e2\x05e3" @@ -668,10 +668,14 @@ class TestTouchEditable : public ui::TouchEditable { private: // Overridden from ui::TouchEditable. - void SelectRect(const gfx::Point& start, const gfx::Point& end) override { + void MoveCaret(const gfx::Point& position) override { NOTREACHED(); } + void MoveRangeSelectionExtent(const gfx::Point& extent) override { + NOTREACHED(); + } + void SelectBetweenCoordinates(const gfx::Point& base, + const gfx::Point& extent) override { NOTREACHED(); } - void MoveCaretTo(const gfx::Point& point) override { NOTREACHED(); } void GetSelectionEndPoints(gfx::SelectionBound* anchor, gfx::SelectionBound* focus) override { *anchor = *focus = cursor_bound_;