Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2408623002:
Views: Extract text selection code from Textfield. (Closed)
|
Created:
4 years, 2 months ago by karandeepb Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews: Extract text selection code from Textfield.
This CL is first in part of CLs to implement text selection for Views::Labels.
This CL extracts the text selection functionality from Views::Textfield into a
new controller class called SelectionController. This class works in tandem with
its delegate. Views::Textfield implements the SelectionController::Delegate
interface.
This also fixes a couple of bugs related to middle clicks on Linux-
1) Middle clicking inside the selection region of an unfocused textfield should
now give it focus.
2) This fixes the appearance of multiple cursors in textfields when middle
clicking with an empty selection clipboard.
Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag
selection timer is stopped and the selection clipboard is updated when mouse
capture is released.
A subsequent CL will modify views::Label to implement the
SelectionController::Delegate interface so that it also supports text selection.
Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6tY/edit?usp=sharing
BUG=630365, 437993, 657691
Committed: https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552
Cr-Commit-Position: refs/heads/master@{#427604}
Patch Set 1 #Patch Set 2 : - #Patch Set 3 : Reorder funcs. #Patch Set 4 : Rename [Starting/Ending]MouseAction. #
Total comments: 11
Patch Set 5 : Rebase. #Patch Set 6 : SelectionControllerDelegate now an inner class. #Patch Set 7 : Pull more functionality into SelectionController #Patch Set 8 #
Total comments: 31
Patch Set 9 : Changes for msw@. #Patch Set 10 : SelectionController::Delegate => SelectionControllerDelegate. #Patch Set 11 : Rebase. #
Total comments: 19
Patch Set 12 : Removed On[Before|After]SelectionUpdated. #
Total comments: 2
Patch Set 13 : Changes to PasteSelectionClipboard #
Total comments: 9
Patch Set 14 #
Total comments: 12
Patch Set 15 : Rebase. #Patch Set 16 : Address nits. #
Total comments: 16
Patch Set 17 : Address comments by sky@. #
Total comments: 2
Patch Set 18 : Address nit. #Patch Set 19 : Check for |handles_selection_clipboard_| #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 113 (80 generated)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== Extract text selection functionlity. ========== to ========== Views: Extract text selection code from Views::Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new concrete class called SelectionController. This class works in tandem with a new interface class called SelectionControllerHost. Views::Textfield implements the SelectionControllerHost interface. A subsequent CL will modify Views::Label to implement the SelectionControllerHost interface so that it also supports text selection. BUG=630365 TEST=No observed changes in text selection in Views::Textfield. ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
Trent, can you take a quick look at this and comment about the design. Subsequently Views::Label will implement the SelectionControllerHost interface. See https://codereview.chromium.org/2379993005 (still a WIP). https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:245: void SelectAll(bool reversed) override; I think it's a bit problematic that these overridden functions are also part of the public Textfield interface. Since these can be called by external callers, we can't for example DCHECK(performing_mouse_action_) in them. Do you think it's a good idea to have a nested class implement SelectionControllerHost instead. This way these overridden functions which are merely an implementation detail of supporting selection, wouldn't be exposed as part of the public Textfield interface. However, it will lead to some more boilerplate code.
there does seem to be an opportunity to encapsulate more logic into the SelectionController that we miss out on because it doesn't have access to the TextfieldModel - see what you think about the suggestion below (i.e. split TextfieldModel into selection/editing capabilities) https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1006: UpdateSelectionClipboard(); this feels like something the SelectionController should be responsible for managing https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1752: UpdateAfterChange(false, true); yeah if these can move into TextfieldModel (or something it inherits from), we can probably have the SelectionControllerHost interface narrowed down a lot more -- perhaps just to UpdateAfterChange.. https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:245: void SelectAll(bool reversed) override; On 2016/10/11 03:43:56, karandeepb wrote: > I think it's a bit problematic that these overridden functions are also part of > the public Textfield interface. Since these can be called by external callers, > we can't for example DCHECK(performing_mouse_action_) in them. > > Do you think it's a good idea to have a nested class implement > SelectionControllerHost instead. This way these overridden functions which are > merely an implementation detail of supporting selection, wouldn't be exposed as > part of the public Textfield interface. However, it will lead to some more > boilerplate code. I think it's reasonable for classes outside to be able to consider a TextField as a thing-that-can-have-a-selection, and for them to manipulate that. But I also like the idea of encapsulating more things into a nested class. E.g. something that just wraps a RenderText, plus helpers to manipulate its selection. Maybe this should be owned by TextfieldModel though. E.g. split out the selection/non-editing parts from TextfieldModel, or have TextfieldModel inherit from a `TextSelectionModel` and then extend it with editing capabilities. https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... ui/views/selection_controller.h:15: }; no ; https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... File ui/views/selection_controller_host.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... ui/views/selection_controller_host.h:20: class SelectionControllerHost { in views parlance, this would probably be a SelectionFooDelegate (not a "Host")
https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.h:245: void SelectAll(bool reversed) override; On 2016/10/11 04:42:09, tapted wrote: > On 2016/10/11 03:43:56, karandeepb wrote: > > I think it's a bit problematic that these overridden functions are also part > of > > the public Textfield interface. Since these can be called by external callers, > > we can't for example DCHECK(performing_mouse_action_) in them. > > > > Do you think it's a good idea to have a nested class implement > > SelectionControllerHost instead. This way these overridden functions which are > > merely an implementation detail of supporting selection, wouldn't be exposed > as > > part of the public Textfield interface. However, it will lead to some more > > boilerplate code. > > I think it's reasonable for classes outside to be able to consider a TextField > as a thing-that-can-have-a-selection, and for them to manipulate that. Yeah we definitely need these on the public interface. I meant having a SelectionControllerHostImpl inner class. Then Textfield can have a SelectionControllerHostImpl as a member. This has the advantage that Textfield::SelectRange will be decoupled from SelectionControllerHost::SelectRange, and the methods on the SelectionControllerHost interface wouldn't be exposed through Textfield. But it'll probably add unnecessary boiler plate code.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #13 (id:260001) has been deleted
Patchset #13 (id:280001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #8 (id:320001) has been deleted
Description was changed from ========== Views: Extract text selection code from Views::Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new concrete class called SelectionController. This class works in tandem with a new interface class called SelectionControllerHost. Views::Textfield implements the SelectionControllerHost interface. A subsequent CL will modify Views::Label to implement the SelectionControllerHost interface so that it also supports text selection. BUG=630365 TEST=No observed changes in text selection in Views::Textfield. ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc=https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6tY/edit?usp=sharing BUG=630365, 437993 TEST=No observed changes in text selection in views::Textfield. ==========
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc=https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6tY/edit?usp=sharing BUG=630365, 437993 TEST=No observed changes in text selection in views::Textfield. ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST=No observed changes in text selection in views::Textfield. ==========
PTAL Trent. Have removed the Select* methods from the SelectionController::Delegate interface and implemented these within the SelectionController. This will allow us to share more code between Textfields and Labels. Have kept these private, since it would be confusing to have multiple ways to modify the selection of the RenderText instance in Textfield. There is bound to be some code duplication between TextfieldModel and SelectionController, but I think it's ok since the SelectionController has a well defined purpose. https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1006: UpdateSelectionClipboard(); On 2016/10/11 04:42:09, tapted wrote: > this feels like something the SelectionController should be responsible for > managing Not sure if this is still relevant. Also, it seems to me that this is redundant anyway, https://codereview.chromium.org/2408623002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1752: UpdateAfterChange(false, true); On 2016/10/11 04:42:09, tapted wrote: > yeah if these can move into TextfieldModel (or something it inherits from), we > can probably have the SelectionControllerHost interface narrowed down a lot more > -- perhaps just to UpdateAfterChange.. Have updated SelectionController and narrowed down the SelectionController::Delegate interface. https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... ui/views/selection_controller.h:15: }; On 2016/10/11 04:42:09, tapted wrote: > no ; Done. https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... File ui/views/selection_controller_host.h (right): https://codereview.chromium.org/2408623002/diff/60001/ui/views/selection_cont... ui/views/selection_controller_host.h:20: class SelectionControllerHost { On 2016/10/11 04:42:09, tapted wrote: > in views parlance, this would probably be a SelectionFooDelegate (not a "Host") Done. Also made it an inner class of SelectionController to show that they are tightly coupled.
karandeepb@chromium.org changed reviewers: + msw@chromium.org, sky@chromium.org
PTAL msw@, sky@. WDYT of the approach?
I like the general approach, but have some comments. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:325: // Todo is this redundant? nit: address todo https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1044: return initiating_drag_; Should this live in SelectionController? https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1732: return false; Textfields can be read-only. This should be: return read_only(); https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1784: UpdateAfterChange(false, true); It seems like we'll have some redundant calls to UpdateAfterChange, when On[Before|After]SelectionUpdated calls are made nested within On[Before|After]MouseAction calls. Is there a way to combine or otherwise simplify these pairs of calls? https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:103: std::min(std::max(0, x), delegate_->GetViewWidth())); optional nit: cache delegate_->GetViewWidth() https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:106: // Todo what happens when this object is destroyed before timer is invoked The timer dtor should suffice, right? Remove/Address comment. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:206: // Todo(karandeepb): See if this can be handled at the RenderText level. nit: use TODO, unless this capitalization is also a common chromium style for TODO comments. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:35: // mouse event is just used to update the internal state without making any nit: It's not entirely obvious what this means... can you clarify what other changes or just clarify the meaning of |handled| (ie. should it be true iff the event was already handled by the delegate, or what?). Can we re-use the existing Event::handled bool flag or similar? Otherwise, what does it mean if the event's flag doesn't match this flag? It seems like we could expose TrackMouseClicks to call unconditionally from Textfield, etc. and then only call OnMousePressed (without a handled flag) from Textfield if the event wasn't handled by its controller, etc. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:55: // Todo(karandeepb): Remove once multi-line text selection is supported. nit: TODO https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:89: virtual gfx::RenderText* GetRenderTextForSelection() = 0; Is the ForSelection postfix just there to avoid a naming collision? Maybe do ForSelectionController? (for selection makes me think that the returned value might depend on the current selection, which doesn't make sense) https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:109: // Methods called to notify a mouse action which may change the associated Should these be generalized to support keyboard-based selection modification? (blink supports SHIFT+CTRL+RIGHT to extend the selection right by word, etc.) You could rename them On[Before|After]SelectionAction or similar. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:138: virtual void PasteSelectionClipboard(const ui::MouseEvent& event) = 0; It seems odd for this to live here, on an interface that will also be implemented by read-only views (ie. Label), instead of remaining on Textfield. That said, I see how the logic to handle this is pretty closely tied to updating the selection clipboard, and textfields themselves can be read-only, so it's not unreasonable...
I'm assuming Mike is reviewing this. I'll look at ui/views/selection_controller. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:86: class VIEWS_EXPORT SelectionController::Delegate { The chromium style guide encourages files like this in their own header. I also wouldn't use an inner class, but SelectionControllerDelegate.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL msw@. Thanks. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:325: // Todo is this redundant? On 2016/10/14 18:54:20, msw wrote: > nit: address todo Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1044: return initiating_drag_; On 2016/10/14 18:54:20, msw wrote: > Should this live in SelectionController? initiaiting_drag_ is just a hint that a drag and drop may be in progress. Currently, within textfield it is modified either during a mouse event, or a gesture event or in OnDragDone. Keeping it on the Delegate interface makes the handling of drag operations for the subclasses easier. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1732: return false; On 2016/10/14 18:54:20, msw wrote: > Textfields can be read-only. This should be: return read_only(); Oh yeah, thanks for catching this. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1784: UpdateAfterChange(false, true); On 2016/10/14 18:54:20, msw wrote: > It seems like we'll have some redundant calls to UpdateAfterChange, when > On[Before|After]SelectionUpdated calls are made nested within > On[Before|After]MouseAction calls. Is there a way to combine or otherwise > simplify these pairs of calls? During a mouse action, UpdateAfterChange will be a NO-OP due to the if (performing_mouse_action_) return; check. So this line is actually a NO-OP and can be removed. I kept it for consistency in case the UpdateAfterChange function was modified later. Any changes will be made only on the call to OnAfterMouseAction. My initial approach did not have On[Before|After]MouseAction, and I was calling On[Before|After]UserAction from within Textfield::OnMouse* functions, but it is needed since SelectionController::SelectThroughLastDragLocation can be called through a timer. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:103: std::min(std::max(0, x), delegate_->GetViewWidth())); On 2016/10/14 18:54:20, msw wrote: > optional nit: cache delegate_->GetViewWidth() Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:106: // Todo what happens when this object is destroyed before timer is invoked On 2016/10/14 18:54:20, msw wrote: > The timer dtor should suffice, right? Remove/Address comment. Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:206: // Todo(karandeepb): See if this can be handled at the RenderText level. On 2016/10/14 18:54:20, msw wrote: > nit: use TODO, unless this capitalization is also a common chromium style for > TODO comments. Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:35: // mouse event is just used to update the internal state without making any >>nit: It's not entirely obvious what this means... can you clarify what other >>changes or just clarify the meaning of |handled| (ie. should it be true iff the >>event was already handled by the delegate, or what?) Yeah I meant already handled by the delegate. Have clarified the comment. >>Can we re-use the existing >>Event::handled bool flag or similar? Otherwise, what does it mean if the event's >>flag doesn't match this flag? Reading the Event::handled() documentation, it's not clear to me if this is the right use case for Event::SetHandled/handled. >>It seems like we could expose TrackMouseClicks to call unconditionally from >>Textfield, etc. and then only call OnMousePressed (without a handled flag) from >>Textfield if the event wasn't handled by its controller, etc. Yeah can do that. Although, I prefer this slightly more, since it doesn't seem right to make the delegate call both TrackMouseClicks and OnMousePressed one after the other. This seems less error-prone, since the delegate implementation may for example, fail to call TrackMouseClicks. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:55: // Todo(karandeepb): Remove once multi-line text selection is supported. On 2016/10/14 18:54:21, msw wrote: > nit: TODO Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:86: class VIEWS_EXPORT SelectionController::Delegate { On 2016/10/14 22:29:48, sky wrote: > The chromium style guide encourages files like this in their own header. I also > wouldn't use an inner class, but SelectionControllerDelegate. Done. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:89: virtual gfx::RenderText* GetRenderTextForSelection() = 0; On 2016/10/14 18:54:20, msw wrote: > Is the ForSelection postfix just there to avoid a naming collision? Maybe do > ForSelectionController? (for selection makes me think that the returned value > might depend on the current selection, which doesn't make sense) Changed to ForSelectionController. Didn't want to use just GetRenderText since a view may manage multiple render text instances like in the case of Labels. And also, to avoid name collision. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:109: // Methods called to notify a mouse action which may change the associated On 2016/10/14 18:54:20, msw wrote: > Should these be generalized to support keyboard-based selection modification? > (blink supports SHIFT+CTRL+RIGHT to extend the selection right by word, etc.) > You could rename them On[Before|After]SelectionAction or similar. But SelectionController is only managing selection modifications made through the mouse. I can rename the class to MouseSelectionController, if it makes the intent more clear. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:138: virtual void PasteSelectionClipboard(const ui::MouseEvent& event) = 0; On 2016/10/14 18:54:21, msw wrote: > It seems odd for this to live here, on an interface that will also be > implemented by read-only views (ie. Label), instead of remaining on Textfield. > That said, I see how the logic to handle this is pretty closely tied to updating > the selection clipboard, and textfields themselves can be read-only, so it's not > unreasonable... Agreed, it's a bit odd. Alternatively, we can handle selection clipboard management in the subclasses, but it is closely tied to other code in the SelectionController. Also, we are sharing more logic this way.
ping msw@.
Sorry for the delay, I was trying to wrap my head around the On[Before|After]* stuff. It's already complex, and this CL seems to complicate them a bit more, hopefully we can find some room for simplification. https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1784: UpdateAfterChange(false, true); On 2016/10/17 07:08:15, karandeepb wrote: > On 2016/10/14 18:54:20, msw wrote: > > It seems like we'll have some redundant calls to UpdateAfterChange, when > > On[Before|After]SelectionUpdated calls are made nested within > > On[Before|After]MouseAction calls. Is there a way to combine or otherwise > > simplify these pairs of calls? > > During a mouse action, UpdateAfterChange will be a NO-OP due to the > if (performing_mouse_action_) > return; > check. > So this line is actually a NO-OP and can be removed. I kept it for consistency > in case the UpdateAfterChange function was modified later. > > Any changes will be made only on the call to OnAfterMouseAction. > > My initial approach did not have On[Before|After]MouseAction, and I was calling > On[Before|After]UserAction from within Textfield::OnMouse* functions, but it is > needed since SelectionController::SelectThroughLastDragLocation can be called > through a timer. Could we avoid adding On[Before|After]MouseAction and do something else for SelectThroughLastDragLocation or just expose On[Before|After]UserAction and UpdateAfterChange? I would like to avoid adding two new pairs of On[Before|After]* calls (MouseAction and SelectionUpdated), since we already have On[Before|After]UserAction. It's not immediately obvious exactly how these should be used/nested as-is; see my other comments about some of the apparent redundancy I see here. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:35: // mouse event is just used to update the internal state without making any On 2016/10/17 07:08:15, karandeepb wrote: > >>nit: It's not entirely obvious what this means... can you clarify what other > >>changes or just clarify the meaning of |handled| (ie. should it be true iff > the > >>event was already handled by the delegate, or what?) > > Yeah I meant already handled by the delegate. Have clarified the comment. > > >>Can we re-use the existing > >>Event::handled bool flag or similar? Otherwise, what does it mean if the > event's > >>flag doesn't match this flag? > > Reading the Event::handled() documentation, it's not clear to me if this is the > right use case for Event::SetHandled/handled. > > >>It seems like we could expose TrackMouseClicks to call unconditionally from > >>Textfield, etc. and then only call OnMousePressed (without a handled flag) > from > >>Textfield if the event wasn't handled by its controller, etc. > > Yeah can do that. Although, I prefer this slightly more, since it doesn't seem > right to make the delegate call both TrackMouseClicks and OnMousePressed one > after the other. This seems less error-prone, since the delegate implementation > may for example, fail to call TrackMouseClicks. Acknowledged. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:109: // Methods called to notify a mouse action which may change the associated On 2016/10/17 07:08:15, karandeepb wrote: > On 2016/10/14 18:54:20, msw wrote: > > Should these be generalized to support keyboard-based selection modification? > > (blink supports SHIFT+CTRL+RIGHT to extend the selection right by word, etc.) > > You could rename them On[Before|After]SelectionAction or similar. > > But SelectionController is only managing selection modifications made through > the mouse. I can rename the class to MouseSelectionController, if it makes the > intent more clear. Yeah, but it seems reasonable to add keyboard-based selection changes soon. It's fine as-is, at least don't rename the class to be mouse-specific. https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1793: void Textfield::OnBeforeSelectionUpdated() { nit: rename these to OnBeforeSelectionUpdate and OnAfterSelectionUpdate to match the present tense of the other similar function pairs. https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:376: // Does necessary updates when the text and/or cursor position changes. NO-OP nit: put NO-OP on the next line. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:27: if (!GetRenderText() || handled) When would there not be a valid RenderText instance if the delegate provided in the ctor is valid? Consider making that a DCHECK instead? https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:59: delegate_->OnAfterMouseAction(false, selection_changed); MoveCursorTo, SelectWordAt, and SelectAll all call OnAfterSelectionUpdated, which already calls UpdateAfterChange(false, true), so this doesn't seem to be needed; inlining these functions (or just calling the RenderText counterparts within a single set of On[Before|After]* function calls right here) should make the redundancy more obvious. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:76: delegate_->OnAfterMouseAction(text_changed, selection_changed); PasteSelectionClipboard already calls UpdateAfterChange(true, true) and ClearSelection calls OnAfterSelectionUpdated (which calls UpdateAfterChange(false, true)), so this doesn't seem to be needed. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:119: delegate_->OnBeforeMouseAction(); Should these On[Before|After]MouseAction calls only be made in the delegate_->HasTextBeingDragged() conditional block that calls MoveCursorTo? It's also a bit odd that MoveCursorTo is nesting calls to On[Before|After]SelectionUpdated within this pair of On[Before|After]MouseAction; I presume that's okay, but I'm obviously failing to understand the nuances of these functions. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:134: void SelectionController::SelectAll(bool reversed) { Most of these helper functions are only called once, and simply wrap the RenderText equivalent with calls to delegate->On[Before|After]SelectionUpdated. I think it makes sense to inline the functions that are only have one caller. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:226: delegate_->OnAfterMouseAction(false, true); SelectTillEdge, MoveCursorTo, SelectWord, and SelectRange all already call OnAfterSelectionUpdated, which calls UpdateAfterChange(false, true), so this doesn't seem to be needed. If we keep this around, it seems like we might want to determine if the selection actually changed, instead of unconditionally sending true. It seems possible that the selection didn't change if the last_drag_location_ hasn't changed amid visible text, or if all text toward a last_drag_location_ beyond the view bounds is already selected) https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller_delegate.h:57: // Called before the selection is updated. Not called in succession and always nit: normalize phrases here and below with those above: 'Should not be called' vs 'Not called' 'must always be followed by' vs 'always followed by'
On 2016/10/18 23:15:05, msw wrote: > Sorry for the delay, I was trying to wrap my head around the On[Before|After]* > stuff. It's already complex, and this CL seems to complicate them a bit more, > hopefully we can find some room for simplification. I had an idea earlier which I didn't articulate very well. We might not be able to pull it off though. It went like: On 2016/10/11 04:42:09, tapted wrote: > But I also like the idea of encapsulating more things into a nested class. E.g. > something that just wraps a RenderText, plus helpers to manipulate its > selection. Maybe this should be owned by TextfieldModel though. E.g. split out > the selection/non-editing parts from TextfieldModel, or have TextfieldModel > inherit from a `TextSelectionModel` and then extend it with editing > capabilities. The idea would be wrap "the" RenderText in views::Label in a similar way to how the RenderText for Textfield is wrapped in a TextfieldModel. E.g. * views::TextSelectionModel holds a gfx::RenderText; accepts Events from a views::View and translates them into selection operations on the RenderText * views::TextfieldModel now inherits from TextSelectionModel; extends it to add editing capabilities. The hope would be that the delegate interface can be narrowed down to something like just `UpdateAfterChange()`. Some of the existing delegate methods instead become properties on the model. But Karan pointed out added complications when we chatted about this. E.g. - views::Label actually has a collection of RenderText instances that it recreates often - some of the delegate methods allow access to properties that change during layout (layout may be more complicated if we need to update a bunch of properties on the TextFooModels. So it's possible this approach could come out a lot worse once everything is nutted out. I hesitate to raise it because I don't want to derail the review, but does this sound like something worth exploring further?
On 2016/10/18 23:34:06, tapted wrote: > On 2016/10/18 23:15:05, msw wrote: > > Sorry for the delay, I was trying to wrap my head around the On[Before|After]* > > stuff. It's already complex, and this CL seems to complicate them a bit more, > > hopefully we can find some room for simplification. > > I had an idea earlier which I didn't articulate very well. We might not be able > to pull it off though. It went like: > > On 2016/10/11 04:42:09, tapted wrote: > > But I also like the idea of encapsulating more things into a nested class. > E.g. > > something that just wraps a RenderText, plus helpers to manipulate its > > selection. Maybe this should be owned by TextfieldModel though. E.g. split out > > the selection/non-editing parts from TextfieldModel, or have TextfieldModel > > inherit from a `TextSelectionModel` and then extend it with editing > > capabilities. > > The idea would be wrap "the" RenderText in views::Label in a similar way to how > the RenderText for Textfield is wrapped in a TextfieldModel. > > E.g. > * views::TextSelectionModel holds a gfx::RenderText; accepts Events from a > views::View and translates them into selection operations on the RenderText > * views::TextfieldModel now inherits from TextSelectionModel; extends it to add > editing capabilities. > > The hope would be that the delegate interface can be narrowed down to something > like just `UpdateAfterChange()`. Some of the existing delegate methods instead > become properties on the model. > > But Karan pointed out added complications when we chatted about this. E.g. > - views::Label actually has a collection of RenderText instances that it > recreates often > - some of the delegate methods allow access to properties that change during > layout (layout may be more complicated if we need to update a bunch of > properties on the TextFooModels. > > So it's possible this approach could come out a lot worse once everything is > nutted out. > > I hesitate to raise it because I don't want to derail the review, but does this > sound like something worth exploring further? It's possible that approach might work and yield something nicer, but the complications that Karan pointed out do indeed seem to make that difficult for views:::Label. IIRC, views::Label is still using a RenderText instance-per-line because (1) RenderTextMac doesn't support multi-line (see RenderTextMac::MultilineSupported), and (2) multi-line RenderTextHarfBuzz instances only support a subset of the eliding types (see RenderTextHarfBuzz::GetDisplayText)... It would be nice to fix those and pursue your suggested approach, but that might be a more difficult undertaking than refining this potentially-less-optimal approach. It's also possible that we could make your approach work with the existing Label implementation, but it's not immediately obvious how to make that work.
>>Sorry for the delay, I was trying to wrap my head around the On[Before|After]* >>stuff. It's already complex, and this CL seems to complicate them a bit more, >>hopefully we can find some room for simplification. Yeah it seems to be the case, since it seems the intent of the design isn't that clear. I'll take a stab at explaining the rationale for the current design and try to come up with something better in the meanwhile- On[Before/After]MouseAction are needed since SelectThroughLastDragLocation is async. With the current design, OnAfterMouseAction also calls UpdateAfterChange which calls SchedulePaint etc. I think it may be possible to remove these somehow, will work on it. OnBeforeSelectionUpdated is needed since we need to confirm the composition text before making any selection related changes. Alternatives are- 1) It would appear that using something like a TextSelectionModel which Trent suggested may help here, but then I'd assume ConfirmCompositionText is a part of the text editing capability and will lie within TextfieldModel. 3) Make all Select* functions part of the delegate interface. This will bloat the delegate interface and lead to duplication of code within the subclasses. OnAfterSelectionUpdated is not actually needed. The UpdateAfterChange call within it is a NO-OP. Added it to accompany the corresponding OnBeforeSelectionUpdated method. Regarding Trent's suggested approach, I think one benefit that it would have is that all selection related changes will go through a single codepath. Textfield->TextfieldSelectionModel. With the current design, Textfield is making selection related changes through both SelectionController (while within a mouse action) and TextfieldModel, although all these selection related methods are private to the SelectionController, and so it is not a big issue probably. Also, I am not sure how it will help to reduce the Delegate interface. Can give this a go, if there's consensus.
On 2016/10/19 01:15:50, karandeepb wrote: > On[Before/After]MouseAction are needed since SelectThroughLastDragLocation is > async. With the current design, OnAfterMouseAction also calls UpdateAfterChange > which calls SchedulePaint etc. I think it may be possible to remove these > somehow, will work on it. Can you elaborate on why the async nature precludes combining these with On[Before|After]UserAction (by making those [and UpdateAfterChange] part of the SelectionControllerDelegate interface)? Also, why can't OnBeforeSelectionUpdated be combined with OnBeforeUserAction or OnBeforeMouseAction? > Regarding Trent's suggested approach... Can give this a go, if there's consensus. I'd agree that Trent's suggestion is nice, but I wouldn't go down that path if you find that it makes Label integration much harder, since that's the whole point of this change. It might make sense to stick with refining the current approach, but I'll leave it up to you.
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:440001) has been deleted
Patchset #13 (id:460001) has been deleted
Patchset #13 (id:480001) has been deleted
Patchset #14 (id:520001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST=No observed changes in text selection in views::Textfield. ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 ==========
Patchset #13 (id:500001) has been deleted
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1784: UpdateAfterChange(false, true); On 2016/10/18 23:15:05, msw wrote: > On 2016/10/17 07:08:15, karandeepb wrote: > > On 2016/10/14 18:54:20, msw wrote: > > > It seems like we'll have some redundant calls to UpdateAfterChange, when > > > On[Before|After]SelectionUpdated calls are made nested within > > > On[Before|After]MouseAction calls. Is there a way to combine or otherwise > > > simplify these pairs of calls? > > > > During a mouse action, UpdateAfterChange will be a NO-OP due to the > > if (performing_mouse_action_) > > return; > > check. > > So this line is actually a NO-OP and can be removed. I kept it for consistency > > in case the UpdateAfterChange function was modified later. > > > > Any changes will be made only on the call to OnAfterMouseAction. > > > > My initial approach did not have On[Before|After]MouseAction, and I was > calling > > On[Before|After]UserAction from within Textfield::OnMouse* functions, but it > is > > needed since SelectionController::SelectThroughLastDragLocation can be called > > through a timer. > > Could we avoid adding On[Before|After]MouseAction and do something else for > SelectThroughLastDragLocation or just expose On[Before|After]UserAction and > UpdateAfterChange? I would like to avoid adding two new pairs of > On[Before|After]* calls (MouseAction and SelectionUpdated), since we already > have On[Before|After]UserAction. It's not immediately obvious exactly how these > should be used/nested as-is; see my other comments about some of the apparent > redundancy I see here. Removed On[Before|After]SelectionUpdated. https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/340001/ui/views/selection_con... ui/views/selection_controller.h:109: // Methods called to notify a mouse action which may change the associated On 2016/10/18 23:15:05, msw wrote: > On 2016/10/17 07:08:15, karandeepb wrote: > > On 2016/10/14 18:54:20, msw wrote: > > > Should these be generalized to support keyboard-based selection > modification? > > > (blink supports SHIFT+CTRL+RIGHT to extend the selection right by word, > etc.) > > > You could rename them On[Before|After]SelectionAction or similar. > > > > But SelectionController is only managing selection modifications made through > > the mouse. I can rename the class to MouseSelectionController, if it makes the > > intent more clear. > > Yeah, but it seems reasonable to add keyboard-based selection changes soon. It's > fine as-is, at least don't rename the class to be mouse-specific. Oh ok. I didn't realize that text selection could be modified using the keyboard for labels. Keeping it as it is for the time being, since it makes the intent more clear. This can be modified once keyboard selection support is added. https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1793: void Textfield::OnBeforeSelectionUpdated() { On 2016/10/18 23:15:05, msw wrote: > nit: rename these to OnBeforeSelectionUpdate and OnAfterSelectionUpdate to match > the present tense of the other similar function pairs. Done. (Obsolete) https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:376: // Does necessary updates when the text and/or cursor position changes. NO-OP On 2016/10/18 23:15:05, msw wrote: > nit: put NO-OP on the next line. Obsolete. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:27: if (!GetRenderText() || handled) On 2016/10/18 23:15:05, msw wrote: > When would there not be a valid RenderText instance if the delegate provided in > the ctor is valid? Consider making that a DCHECK instead? Done. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:59: delegate_->OnAfterMouseAction(false, selection_changed); On 2016/10/18 23:15:05, msw wrote: > MoveCursorTo, SelectWordAt, and SelectAll all call OnAfterSelectionUpdated, > which already calls UpdateAfterChange(false, true), so this doesn't seem to be > needed; inlining these functions (or just calling the RenderText counterparts > within a single set of On[Before|After]* function calls right here) should make > the redundancy more obvious. Done. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:76: delegate_->OnAfterMouseAction(text_changed, selection_changed); On 2016/10/18 23:15:05, msw wrote: > PasteSelectionClipboard already calls UpdateAfterChange(true, true) and > ClearSelection calls OnAfterSelectionUpdated (which calls > UpdateAfterChange(false, true)), so this doesn't seem to be needed. Done. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:119: delegate_->OnBeforeMouseAction(); On 2016/10/18 23:15:05, msw wrote: > Should these On[Before|After]MouseAction calls only be made in the > delegate_->HasTextBeingDragged() conditional block that calls MoveCursorTo? > It's also a bit odd that MoveCursorTo is nesting calls to > On[Before|After]SelectionUpdated within this pair of > On[Before|After]MouseAction; I presume that's okay, but I'm obviously failing to > understand the nuances of these functions. Done. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:134: void SelectionController::SelectAll(bool reversed) { On 2016/10/18 23:15:05, msw wrote: > Most of these helper functions are only called once, and simply wrap the > RenderText equivalent with calls to delegate->On[Before|After]SelectionUpdated. > I think it makes sense to inline the functions that are only have one caller. Done. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:226: delegate_->OnAfterMouseAction(false, true); On 2016/10/18 23:15:05, msw wrote: > SelectTillEdge, MoveCursorTo, SelectWord, and SelectRange all already call > OnAfterSelectionUpdated, which calls > UpdateAfterChange(false, true), so this doesn't seem to be needed. Done. > If we keep this around, it seems like we might want to determine if the > selection actually changed, instead of unconditionally sending true. It seems > possible that the selection didn't change if the last_drag_location_ hasn't > changed amid visible text, or if all text toward a last_drag_location_ beyond > the view bounds is already selected) This is in line with what we have currently. Not all RenderText Select* functions return a bool to indicate whether an actual change was made. I think this can be handled subsequently. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller_delegate.h:57: // Called before the selection is updated. Not called in succession and always On 2016/10/18 23:15:05, msw wrote: > nit: normalize phrases here and below with those above: > 'Should not be called' vs 'Not called' > 'must always be followed by' vs 'always followed by' Obsolete. https://codereview.chromium.org/2408623002/diff/420001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/420001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:493: // textfield; i.e. OnBeforeUserAction() has been called, but This is in line with the comments regarding On[Before|After]UserAction in TextfieldController. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (left): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:2075: model_->MoveCursorTo(mouse); Do you know if using TextfieldModel::MoveCursorTo(SelectionModel) rather than TextfieldModel::MoveCursorTo(Point, bool) here deliberate? MovecursorTo(SelectionModel) seems to have specific behavior for an active composition, which won't be triggered with the way things are structured now. Although I'd think it's not significant, happening only on Linux on a middle click with active composition. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:583: if (!handled && !HasFocus() && event.IsOnlyMiddleMouseButton()) This causes a behavior change (for the better IMO). On Linux, if you have some selection in a textfield, and you press tab to remove focus from it. If you middle click inside the (not visible) selection region of the (unfocused) textfield, the current behavior is that the selection will be cleared internally, but the cursor won't be drawn since the textfield won't request focus. This changes the behavior to first requesting focus and then clearing the selection, so that middle clicking inside the selection region of an unfocused textfield, moves the cursor to the end of the selection. There are some other bugs I found related to not clearing selection on blur- 1) If you hover inside the (not visible) selection region of an unfocused textfield, we get an "arrow" as the cursor and not the text cursor. 2) If you left click inside the (not visible) selection region of an unfocused textfield, the selection region flashes and reduces to a caret later. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1803: model_->InsertText(selection_clipboard_text); Also, removed the UpdateAfterChange here. https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (left): https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:2075: model_->MoveCursorTo(mouse); We were not calling UpdateAfterChange when only the selection changed here. This caused https://bugs.chromium.org/p/chromium/issues/detail?id=657691.
PTAL msw@. On 2016/10/19 01:38:57, msw wrote: > On 2016/10/19 01:15:50, karandeepb wrote: > > On[Before/After]MouseAction are needed since SelectThroughLastDragLocation is > > async. With the current design, OnAfterMouseAction also calls > UpdateAfterChange > > which calls SchedulePaint etc. I think it may be possible to remove these > > somehow, will work on it. > > Can you elaborate on why the async nature precludes combining these with > On[Before|After]UserAction (by making those [and UpdateAfterChange] part of the > SelectionControllerDelegate interface)? Also, why can't OnBeforeSelectionUpdated > be combined with OnBeforeUserAction or OnBeforeMouseAction? Have combined On[Before|After]SelectionUpdated with On[Before|After]MouseAction. > > Regarding Trent's suggested approach... Can give this a go, if there's > consensus. > > I'd agree that Trent's suggestion is nice, but I wouldn't go down that path if > you find that it makes Label integration much harder, since that's the whole > point of this change. It might make sense to stick with refining the current > approach, but I'll leave it up to you. Yeah I'll continue with the current approach for the time being.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits. Thanks for all your work here, and suffering my review. https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/400001/ui/views/selection_con... ui/views/selection_controller.cc:226: delegate_->OnAfterMouseAction(false, true); On 2016/10/20 04:12:57, karandeepb wrote: > On 2016/10/18 23:15:05, msw wrote: > > If we keep this around, it seems like we might want to determine if the > > selection actually changed, instead of unconditionally sending true. It seems > > possible that the selection didn't change if the last_drag_location_ hasn't > > changed amid visible text, or if all text toward a last_drag_location_ beyond > > the view bounds is already selected) > > This is in line with what we have currently. Not all RenderText Select* > functions return a bool to indicate whether an actual change was made. I think > this can be handled subsequently. Acknowledged; you're right. https://codereview.chromium.org/2408623002/diff/420001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/420001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:493: // textfield; i.e. OnBeforeUserAction() has been called, but On 2016/10/20 04:12:57, karandeepb wrote: > This is in line with the comments regarding On[Before|After]UserAction in > TextfieldController. Acknowledged. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (left): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:2075: model_->MoveCursorTo(mouse); On 2016/10/20 04:12:58, karandeepb wrote: > Do you know if using TextfieldModel::MoveCursorTo(SelectionModel) rather than > TextfieldModel::MoveCursorTo(Point, bool) here deliberate? > MovecursorTo(SelectionModel) seems to have specific behavior for an active > composition, which won't be triggered with the way things are structured now. > Although I'd think it's not significant, happening only on Linux on a middle > click with active composition. I also doubt that it's deliberate. It seems like we should just call the <point,bool> version; feel free to make that change. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:583: if (!handled && !HasFocus() && event.IsOnlyMiddleMouseButton()) On 2016/10/20 04:12:57, karandeepb wrote: > This causes a behavior change (for the better IMO). On Linux, if you have some > selection in a textfield, and you press tab to remove focus from it. If you > middle click inside the (not visible) selection region of the (unfocused) > textfield, the current behavior is that the selection will be cleared > internally, but the cursor won't be drawn since the textfield won't request > focus. > > This changes the behavior to first requesting focus and then clearing the > selection, so that middle clicking inside the selection region of an unfocused > textfield, moves the cursor to the end of the selection. > > There are some other bugs I found related to not clearing selection on blur- > 1) If you hover inside the (not visible) selection region of an unfocused > textfield, we get an "arrow" as the cursor and not the text cursor. > 2) If you left click inside the (not visible) selection region of an unfocused > textfield, the selection region flashes and reduces to a caret later. I hugely appreciate your attention to detail and explanations here! The behavior change might be acceptable, but I'm not sure that it makes sense to move the cursor to the end of the invisible selection, that might be confusing, but it does match the focused behavior, afaik. I'd try to follow the example of what some Linux GTK textfields do in this case, but it's such a minor corner case that the behavior change you describe is probably okay. We sure do have some weirdness that emerges from retaining, but not painting the selection while unfocused. I originally drew the unfocused selection, but we decided to change that a long time ago, and I don't recall the reasons for retaining (rather than saving and restoring) the selection while blurred. That said, saving and restoring selections has its own set of complications... Not to make excuses, but middle-click selection clipboard behavior was also tacked-on recently, partly by myself with little familiarity with the native behavior. The last two points are indeed from retaining the selection. It seems like we only want to retain the selection for tab-focus events, not mouse-focus events. If that's the case, then you're right these are bugs, otherwise, (1) seems to match the expected behavior (similar to your new behavior for middle-click, right?), but there's no reason for (2) afaict, we should try to update the cursor position before painting and avoid the flash. Feel free to file a bug or two, thanks! https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1803: model_->InsertText(selection_clipboard_text); On 2016/10/20 04:12:57, karandeepb wrote: > Also, removed the UpdateAfterChange here. That might be worth a comment, ie. "Callers should call UpdateAfterChange after this." Another option is moving more of the code to SelectionController, but I don't think we'd want to add or call SelectionControllerDelegate::InsertText. https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (left): https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:2075: model_->MoveCursorTo(mouse); On 2016/10/20 04:12:58, karandeepb wrote: > We were not calling UpdateAfterChange when only the selection changed here. This > caused https://bugs.chromium.org/p/chromium/issues/detail?id=657691. Thanks for noticing, filing a bug, and fixing this! https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:513: std::unique_ptr<SelectionController> selection_controller_; nit: make this a member object without using unique_ptr, if possible. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller.cc:41: delegate_->OnBeforeMouseAction(); optional nit: you may wrap the whole switch in On[Before|After]MouseAction, with a shared |selection_changed| flag set by each case, if you choose. The potential drag-and-drop case should be okay calling those functions too. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:41: // Methods called to notify a mouse action which may change the associated optional nit: It'd be nice to integrate this comment into one or both of the actual function comments, but I'll leave that up to you. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:53: // Selection clipboard related methods. nit: probably not necessary. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:62: // the render text instance, it may not be called within a mouse action. nit: "it may be called outside of a mouse action"
Patchset #15 (id:580001) has been deleted
https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (left): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:2075: model_->MoveCursorTo(mouse); On 2016/10/21 02:14:15, msw wrote: > On 2016/10/20 04:12:58, karandeepb wrote: > > Do you know if using TextfieldModel::MoveCursorTo(SelectionModel) rather than > > TextfieldModel::MoveCursorTo(Point, bool) here deliberate? > > MovecursorTo(SelectionModel) seems to have specific behavior for an active > > composition, which won't be triggered with the way things are structured now. > > Although I'd think it's not significant, happening only on Linux on a middle > > click with active composition. > > I also doubt that it's deliberate. It seems like we should just call the > <point,bool> version; feel free to make that change. Already doing it. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:583: if (!handled && !HasFocus() && event.IsOnlyMiddleMouseButton()) >>The behavior change might be acceptable, but I'm not sure that it makes sense to >>move the cursor to the end of the invisible selection, that might be confusing, >>but it does match the focused behavior, afaik. I'd try to follow the example of >>what some Linux GTK textfields do in this case, but it's such a minor corner >>case that the behavior change you describe is probably okay. Yeah I think it's still better than the current behavior of a middle click inside the selection region of an unfocused textfield causing no user visible change. The behavior can be further corrected while fixing bugs related to retaining the selection on losing focus. >>We sure do have some weirdness that emerges from retaining, but not painting the >>selection while unfocused. I originally drew the unfocused selection, but we >>decided to change that a long time ago, and I don't recall the reasons for >>retaining (rather than saving and restoring) the selection while blurred. That >>said, saving and restoring selections has its own set of complications... Not to >>make excuses, but middle-click selection clipboard behavior was also tacked-on >>recently, partly by myself with little familiarity with the native behavior. Interestingly, there's work ongoing to draw the unfocused selection - https://codereview.chromium.org/2345183002/. >>The last two points are indeed from retaining the selection. It seems like we >>only want to retain the selection for tab-focus events, not mouse-focus events. >>If that's the case, then you're right these are bugs, otherwise, (1) seems to >>match the expected behavior (similar to your new behavior for middle-click, >>right?), but there's no reason for (2) afaict, we should try to update the >>cursor position before painting and avoid the flash. >>Feel free to file a bug or two, thanks! Will file bugs for these. These can probably be tackled all at once. https://codereview.chromium.org/2408623002/diff/540001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1803: model_->InsertText(selection_clipboard_text); On 2016/10/21 02:14:16, msw wrote: > On 2016/10/20 04:12:57, karandeepb wrote: > > Also, removed the UpdateAfterChange here. > > That might be worth a comment, ie. "Callers should call UpdateAfterChange after > this." Another option is moving more of the code to SelectionController, but I > don't think we'd want to add or call SelectionControllerDelegate::InsertText. There are currently no callers within Textfield. Added a comment. https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... File ui/views/controls/textfield/textfield.h (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/controls/text... ui/views/controls/textfield/textfield.h:513: std::unique_ptr<SelectionController> selection_controller_; On 2016/10/21 02:14:16, msw wrote: > nit: make this a member object without using unique_ptr, if possible. Done. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller.cc:41: delegate_->OnBeforeMouseAction(); On 2016/10/21 02:14:16, msw wrote: > optional nit: you may wrap the whole switch in On[Before|After]MouseAction, with > a shared |selection_changed| flag set by each case, if you choose. The potential > drag-and-drop case should be okay calling those functions too. Will keep it as it is. Some of these functions don't return a bool to specify whether the selection changed, which'll lead to redundant paints. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:41: // Methods called to notify a mouse action which may change the associated On 2016/10/21 02:14:16, msw wrote: > optional nit: It'd be nice to integrate this comment into one or both of the > actual function comments, but I'll leave that up to you. Done. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:53: // Selection clipboard related methods. On 2016/10/21 02:14:16, msw wrote: > nit: probably not necessary. Done. https://codereview.chromium.org/2408623002/diff/560001/ui/views/selection_con... ui/views/selection_controller_delegate.h:62: // the render text instance, it may not be called within a mouse action. On 2016/10/21 02:14:16, msw wrote: > nit: "it may be called outside of a mouse action" Done.
PTAL sky@ for owner's review for ui/views/selection_controller* ui/views/BUILD.gn
https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.cc:67: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) I would inject whether this behavior should be enabled so that you can test it everywhere. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:37: bool OnMousePressed(const ui::MouseEvent& event, bool handled); As commented over IM I think touch/gesture should be forwarded as well so that this responds to touch too. Please add a TODO. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:37: bool OnMousePressed(const ui::MouseEvent& event, bool handled); I'm wondering if you can make this class an EventHandler and add it as a pre-target handler for the textfield/label. That way you wouldn't need to change textfield/label to explicitly call into this class. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:39: void OnMouseReleased(const ui::MouseEvent& event); What happens if capture is lost? Doesn't that need to be forwarded too? https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:42: gfx::Point last_click_location() const { return last_click_location_; } const gfx::Point& https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:62: size_t aggregated_clicks_; Please add a comment as to what this means. It isn't readily obvious to me. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller_delegate.h:44: virtual void OnBeforeMouseAction() = 0; You are going to need to handle touch/gesture at some point. I'm ok postponing that for now, but I think you should at least get the api right. Don't use Mouse here as it's going to be used for touch/gesture in the future. We've used Pointer to mean touch or mouse. So, I recommend using Pointer here too, e.g. OnBeforePointerAction.
Patchset #17 (id:640001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped when capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped when capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped and the selection clipboard is updated when mouse capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL sky@. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.cc:67: #if defined(OS_LINUX) && !defined(OS_CHROMEOS) On 2016/10/21 15:03:40, sky wrote: > I would inject whether this behavior should be enabled so that you can test it > everywhere. Yeah this seems better. Done. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:37: bool OnMousePressed(const ui::MouseEvent& event, bool handled); On 2016/10/21 15:03:40, sky wrote: > As commented over IM I think touch/gesture should be forwarded as well so that > this responds to touch too. Please add a TODO. Done. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:37: bool OnMousePressed(const ui::MouseEvent& event, bool handled); On 2016/10/21 15:03:40, sky wrote: > I'm wondering if you can make this class an EventHandler and add it as a > pre-target handler for the textfield/label. That way you wouldn't need to change > textfield/label to explicitly call into this class. Currently Textfield::OnMousePressed first forwards the event to the TextfieldController, then if the event is not handled, we request focus etc. if needed and only then do we pass the event to the selection controller. Also, classes like OmniboxViewViews override OnMouse* handlers for Textfield (similarly views::Link will override OnMouse* handlers for Labels) So I am not sure if adding SelectionController as a pre-target handler is feasible with the way things are currently implemented. Also, I am not sure whether it is the correct thing to do, meaning when should we handle an event in the target phase vs the pre/post target phase? https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:39: void OnMouseReleased(const ui::MouseEvent& event); On 2016/10/21 15:03:40, sky wrote: > What happens if capture is lost? Doesn't that need to be forwarded too? Thanks for the catch. Textfield wasn't handling OnMouseCaptureLost till now. So on Windows, if during an active mouse drag doing a text selection, we were to focus to a different window, the drag_selection_timer_ would not have stopped. Interestingly, on Mac and Linux, focusing to a different window during an active selection, doesn't release capture, but it seems in accordance with the native behavior. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:42: gfx::Point last_click_location() const { return last_click_location_; } On 2016/10/21 15:03:40, sky wrote: > const gfx::Point& Done. Is it preferred that we should be returning a const reference for simple getters? Didn't find anything in the style guides. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:62: size_t aggregated_clicks_; On 2016/10/21 15:03:40, sky wrote: > Please add a comment as to what this means. It isn't readily obvious to me. Done. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller_delegate.h:44: virtual void OnBeforeMouseAction() = 0; On 2016/10/21 15:03:40, sky wrote: > You are going to need to handle touch/gesture at some point. I'm ok postponing > that for now, but I think you should at least get the api right. Don't use Mouse > here as it's going to be used for touch/gesture in the future. We've used > Pointer to mean touch or mouse. So, I recommend using Pointer here too, e.g. > OnBeforePointerAction. Done.
LGTM - thanks https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... File ui/views/selection_controller.h (right): https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:37: bool OnMousePressed(const ui::MouseEvent& event, bool handled); On 2016/10/25 05:30:42, karandeepb wrote: > On 2016/10/21 15:03:40, sky wrote: > > I'm wondering if you can make this class an EventHandler and add it as a > > pre-target handler for the textfield/label. That way you wouldn't need to > change > > textfield/label to explicitly call into this class. > > Currently Textfield::OnMousePressed first forwards the event to the > TextfieldController, then if the event is not handled, we request focus etc. if > needed and only then do we pass the event to the selection controller. > > Also, classes like OmniboxViewViews override OnMouse* handlers for Textfield > (similarly views::Link will override OnMouse* handlers for Labels) So I am not > sure if adding SelectionController as a pre-target handler is feasible with the > way things are currently implemented. > > Also, I am not sure whether it is the correct thing to do, meaning when should > we handle an event in the target phase vs the pre/post target phase? Fair enough. If you need certain ordering than I agree EventHandler isn't the right thing. https://codereview.chromium.org/2408623002/diff/620001/ui/views/selection_con... ui/views/selection_controller.h:42: gfx::Point last_click_location() const { return last_click_location_; } On 2016/10/25 05:30:42, karandeepb wrote: > On 2016/10/21 15:03:40, sky wrote: > > const gfx::Point& > > Done. Is it preferred that we should be returning a const reference for simple > getters? Didn't find anything in the style guides. Generally in chromium code you'll see const& return types. https://codereview.chromium.org/2408623002/diff/660001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/660001/ui/views/selection_con... ui/views/selection_controller_delegate.h:41: // Called before a mouse action which may change the associated view's mouse->pointer (same below).
https://codereview.chromium.org/2408623002/diff/660001/ui/views/selection_con... File ui/views/selection_controller_delegate.h (right): https://codereview.chromium.org/2408623002/diff/660001/ui/views/selection_con... ui/views/selection_controller_delegate.h:41: // Called before a mouse action which may change the associated view's On 2016/10/25 16:46:03, sky wrote: > mouse->pointer (same below). Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2408623002/#ps680001 (title: "Address nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by karandeepb@chromium.org
https://codereview.chromium.org/2408623002/diff/700001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2408623002/diff/700001/ui/views/selection_con... ui/views/selection_controller.cc:132: if (handles_selection_clipboard_ && !render_text->selection().is_empty()) Also added a couple of these checks.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2408623002/#ps700001 (title: "Check for |handles_selection_clipboard_|")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped and the selection clipboard is updated when mouse capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped and the selection clipboard is updated when mouse capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:700001)
Message was sent while issue was closed.
Description was changed from ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped and the selection clipboard is updated when mouse capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 ========== to ========== Views: Extract text selection code from Textfield. This CL is first in part of CLs to implement text selection for Views::Labels. This CL extracts the text selection functionality from Views::Textfield into a new controller class called SelectionController. This class works in tandem with its delegate. Views::Textfield implements the SelectionController::Delegate interface. This also fixes a couple of bugs related to middle clicks on Linux- 1) Middle clicking inside the selection region of an unfocused textfield should now give it focus. 2) This fixes the appearance of multiple cursors in textfields when middle clicking with an empty selection clipboard. Also, Textfield now handles OnMouseCaptureLost. This ensures that the drag selection timer is stopped and the selection clipboard is updated when mouse capture is released. A subsequent CL will modify views::Label to implement the SelectionController::Delegate interface so that it also supports text selection. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993, 657691 Committed: https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552 Cr-Commit-Position: refs/heads/master@{#427604} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552 Cr-Commit-Position: refs/heads/master@{#427604} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
