Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2413223003:
Views:: Make Labels support text selection. (Closed)
|
Created:
4 years, 2 months ago by karandeepb Modified:
4 years, 1 month ago CC:
chromium-reviews, yusukes+watch_chromium.org, derat+watch_chromium.org, tfarina, shuchen+watch_chromium.org, nona+watch_chromium.org, mac-reviews_chromium.org, James Su, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionViews:: Make Labels support text selection.
This CL is a follow-up to r427604 which extracted the text selection logic from
views::Textfield and introduced the SelectionController class. This CL makes
views::Label implement the SelectionControllerDelegate interface. As a result,
single-line views::Label now support text selection.
Some of the member functions in the Label class which do not change the external
state of the Label are made const. This is made possible by making the render
text instances that are used for drawing, mutable.
A new test fixture class is also added for testing Labels. Existing tests are
modified to use it, and new tests are added to test the text selection behavior.
Support for copying and dragging selected text and text selection in multi-line
views::Label will be added subsequently.
Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6tY/edit?usp=sharing
BUG=630365, 437993
TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown.
Verify that text selection in the label with the text "A label supporting text
selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext
flag.
Committed: https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d
Cr-Commit-Position: refs/heads/master@{#430461}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase. #Patch Set 3 : Rebase. #Patch Set 4 : -- #Patch Set 5 : Make HasSelection const. #Patch Set 6 : Rebase. #Patch Set 7 : -- #
Total comments: 18
Patch Set 8 : Address comments by msw@. #Patch Set 9 : Tests. #Patch Set 10 : Rebase. #Patch Set 11 : Cleanup. More tests. #
Total comments: 40
Patch Set 12 : Address comments. #
Total comments: 8
Patch Set 13 : Address comments. #
Total comments: 6
Patch Set 14 : Fix text selection background color on Windows. #
Total comments: 1
Patch Set 15 : Rebase #Patch Set 16 : Rebase #Dependent Patchsets: Messages
Total messages: 122 (103 generated)
Description was changed from ========== Make Views::Label support text selection. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to http://crrev.com/2408623002/ which extracted the text selection logic from Views::Textfield and introduced the SelectionController class. This CL makes Views::Label implement the SelectionController::Delegate interface. Hence, single-line Views::Labels now support text selection. Support for copying selected text and text selection in multi-line Views::Labels will be added subsequently. BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A selectable Label with custom colors for text." works properly. ==========
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:: Make Labels support text selection. This CL is a follow-up to http://crrev.com/2408623002/ which extracted the text selection logic from Views::Textfield and introduced the SelectionController class. This CL makes Views::Label implement the SelectionController::Delegate interface. Hence, single-line Views::Labels now support text selection. Support for copying selected text and text selection in multi-line Views::Labels will be added subsequently. BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A selectable Label with custom colors for text." works properly. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to http://crrev.com/2408623002/ which extracted the text selection logic from Views::Textfield and introduced the SelectionController class. This CL makes Views::Label implement the SelectionController::Delegate interface. Hence, single-line Views::Labels now support text selection. Support for copying selected text and text selection in multi-line Views::Labels will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A selectable Label with custom colors for text." works properly. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
karandeepb@chromium.org changed reviewers: + msw@chromium.org, sky@chromium.org, tapted@chromium.org
PTAL. This still needs some work. Will be adding tests subsequently. https://codereview.chromium.org/2413223003/diff/1/ui/views/controls/label.cc File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/1/ui/views/controls/label.cc#... ui/views/controls/label.cc:500: // the currently focused view (due to selection) was to lose focus, focus See the linked doc for some possible approaches to solve this. https://codereview.chromium.org/2413223003/diff/1/ui/views/controls/label.cc#... ui/views/controls/label.cc:822: ui::NativeTheme::kColorId_TextfieldSelectionColor); Would it be ok to use the same colors for Textfields and Labels. If yes, is just renaming it to kColorId_TextSelectionColor fine or should a separate constant be created for Labels?
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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 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 #6 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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: 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_...)
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 #9 (id:180001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #6 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_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 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 #8 (id:240001) has been deleted
Patchset #8 (id:260001) 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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) 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...
Patchset #10 (id:320001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #7 (id:220001) has been deleted
Description was changed from ========== Views:: Make Labels support text selection. This CL is a follow-up to http://crrev.com/2408623002/ which extracted the text selection logic from Views::Textfield and introduced the SelectionController class. This CL makes Views::Label implement the SelectionController::Delegate interface. Hence, single-line Views::Labels now support text selection. Support for copying selected text and text selection in multi-line Views::Labels will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A selectable Label with custom colors for text." works properly. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. ==========
PTAL msw@. Still need to add tests. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:521: // TODO(karandeepb): If a widget with a label having FocusBehavior::NEVER as Still need to find a workaround for this. Also, not sure if it's ok to set focus to a view with FocusBehavior::NEVER. Have suggested a few options here - https://docs.google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice work! I have mostly minor comments and questions. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:493: return GetRenderTextForSelectionController() ? GetNativeIBeamCursor() nit: should this just check IsSelectable? https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:516: if(!GetRenderTextForSelectionController()) nit: space after if; ditto elsewhere below, run 'git cl format'. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:521: // TODO(karandeepb): If a widget with a label having FocusBehavior::NEVER as On 2016/10/27 07:31:54, karandeepb wrote: > Still need to find a workaround for this. Also, not sure if it's ok to set focus > to a view with FocusBehavior::NEVER. Have suggested a few options here - > https://docs.google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6... I think this is probably fine as a temporary solution (I'm not aware of any real problems forcibly focusing a FocusBehavior::NEVER view, but if we encounter any, your other doc ideas are good leads!). Nice comment, just cite a tracking bug. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:538: bool Label::OnMouseDragged(const ui::MouseEvent& event) { aside: Making SelectionController a pre-target event handler (as Scott suggested in the prior CL), or seeking some other refinement would be nice, but not a blocker to land this. Ultimately, it'd be great to have one RenderText impl that supports multi-line, and then we could maybe even make Textfield a Label subclass, or share more code. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:626: if (!obscured()) { nit: do this before getting |render_text| https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.h:193: // If a subclass stops supporting text selection, it should call Since Label [subclass] users can just call SetSelectable(true), shouldn't subclasses also override this function to return false when they stop supporting text selection (like how Label itself treats multi-line)? I guess that might be implied here, but maybe say so explicitly? https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.h:271: mutable base::Optional<gfx::Range> last_selection_range_; Nice explanatory comment. Some questions/comments here: 1) How bad is it to just clear the selection on layout and other ClearRenderTextLines() callers? Is it really common/annoying? 2) Can we just use the selection range from the render_text_ instance? 3) Can we avoid optional and set the range invalid instead? 4) And maybe name this |stored_selection_range_|? https://codereview.chromium.org/2413223003/diff/340001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:35: if (delegate_->SupportsDrag()) Is this just disabling something that otherwise works or is it entirely not supported by Labels (and would require non-trivial work to support)? If it's the former, let's remove the flag and support dragging selected text from labels to be dropped elsewhere; if it's the latter, then this is fine for now, but we might want to investigate adding support later (maybe file/cite a bug?).
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_...)
Patchset #10 (id:400001) 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: 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 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:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. ==========
Patchset #11 (id:440001) has been deleted
Patchset #11 (id:460001) has been deleted
Patchset #11 (id:480001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ==========
Patchset #11 (id:500001) has been deleted
Patchset #11 (id:520001) has been deleted
PTAL msw@. Thanks. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:493: return GetRenderTextForSelectionController() ? GetNativeIBeamCursor() On 2016/10/28 06:30:26, msw wrote: > nit: should this just check IsSelectable? So GetRenderTextForSelectionController() may still return nullptr even if IsSelectable returns true. This can happen when say the content bounds of the view are empty. Think this is more appropriate. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:516: if(!GetRenderTextForSelectionController()) On 2016/10/28 06:30:26, msw wrote: > nit: space after if; ditto elsewhere below, run 'git cl format'. Done. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:521: // TODO(karandeepb): If a widget with a label having FocusBehavior::NEVER as On 2016/10/28 06:30:26, msw wrote: > On 2016/10/27 07:31:54, karandeepb wrote: > > Still need to find a workaround for this. Also, not sure if it's ok to set > focus > > to a view with FocusBehavior::NEVER. Have suggested a few options here - > > > https://docs.google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6... > > I think this is probably fine as a temporary solution (I'm not aware of any real > problems forcibly focusing a FocusBehavior::NEVER view, but if we encounter any, > your other doc ideas are good leads!). Nice comment, just cite a tracking bug. Done. Will use the label text selection bug to track this. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:538: bool Label::OnMouseDragged(const ui::MouseEvent& event) { > aside: Making SelectionController a pre-target event handler (as Scott suggested > in the prior CL), or seeking some other refinement would be nice, but not a > blocker to land this. As pointed out in the prior CL, making SelectionController a pre-target handler may not be feasible since we need specific ordering. Eg classes like OmniboxViewViews and Link override the OnMouse* handlers for Textfield and Label respectively. Then there is also the TextfieldController class to which we pass OnMousePressed events in Textfield. > Ultimately, it'd be great to have one RenderText impl that > supports multi-line, and then we could maybe even make Textfield a Label > subclass, or share more code. Yeah I think it'd be great, but we should try to clean up the Label implementation first somehow. The implementation is currently a bit confusing with multiple render text instances (due to supporting RenderTextMac) used for drawing which are cleared and created on each layout. Also, there's the |render_text_| instance which is used for preferred sizing and persisting some data. Some things which may probably be looked at- 1) If we are able to move away from using RenderTextMac on Mac, can we use a single persistent RenderText instance (like we do in Textfields). 2) Is it necessary to use a separate |render_text_| instance for preferred sizing. Can we use a temporary render text instance when necessary and maybe have a cache layer on top in the Label class? 3) Also, there's the StyledLabel class which is used for creating a text view with multiple styles. Haven't looked into it, but it may be possible for Label or a Label subclass to support the StyledLabel class API, since RenderText supports different styles for a given piece of text. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:626: if (!obscured()) { On 2016/10/28 06:30:26, msw wrote: > nit: do this before getting |render_text| Obsolete. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.h:193: // If a subclass stops supporting text selection, it should call On 2016/10/28 06:30:26, msw wrote: > Since Label [subclass] users can just call SetSelectable(true), shouldn't > subclasses also override this function to return false when they stop supporting > text selection (like how Label itself treats multi-line)? I guess that might be > implied here, but maybe say so explicitly? Yeah that is why this is made virtual and is overriden in views::Link. Have clarified this. https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.h:271: mutable base::Optional<gfx::Range> last_selection_range_; >>1) How bad is it to just clear the selection on layout and other >>ClearRenderTextLines() callers? Is it really common/annoying? That would cause the selection to erase every time we resize the Label. Don't think we'd want that. >>2) Can we just use the selection range from the render_text_ instance? This seems clearer to me. It would be confusing to have render_text_->selection() not always be in sync with the actual selection (like last_selection_range_ is currently). To keep them in sync, would require some more code. >>3) Can we avoid optional and set the range invalid instead? Done. >>4) And maybe name this |stored_selection_range_|? Done. https://codereview.chromium.org/2413223003/diff/340001/ui/views/selection_con... File ui/views/selection_controller.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/selection_con... ui/views/selection_controller.cc:35: if (delegate_->SupportsDrag()) On 2016/10/28 06:30:26, msw wrote: > Is this just disabling something that otherwise works or is it entirely not > supported by Labels (and would require non-trivial work to support)? If it's the > former, let's remove the flag and support dragging selected text from labels to > be dropped elsewhere; if it's the latter, then this is fine for now, but we > might want to investigate adding support later (maybe file/cite a bug?). To support dragging selected text, Labels would need to implement DragController. Have added a TODO to Label::SupportsDrag. The implementation would be almost similar to the Textfield one. It seems that as you suggested, Textfield and Label can share more code by being in the same class hierarchy. But again, probably the Label implementation can be simplified first. Will put up a CL subsequently duplicating some of the code in Textfield for dragging selected text.
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.
https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/340001/ui/views/controls/labe... ui/views/controls/label.cc:538: bool Label::OnMouseDragged(const ui::MouseEvent& event) { On 2016/11/01 11:06:25, karandeepb wrote: > > aside: Making SelectionController a pre-target event handler (as Scott > suggested > > in the prior CL), or seeking some other refinement would be nice, but not a > > blocker to land this. > As pointed out in the prior CL, making SelectionController a pre-target handler > may not be feasible since we need specific ordering. Eg classes like > OmniboxViewViews and Link override the OnMouse* handlers for Textfield and Label > respectively. Then there is also the TextfieldController class to which we pass > OnMousePressed events in Textfield. > > > Ultimately, it'd be great to have one RenderText impl that > > supports multi-line, and then we could maybe even make Textfield a Label > > subclass, or share more code. > > Yeah I think it'd be great, but we should try to clean up the Label > implementation first somehow. The implementation is currently a bit confusing > with multiple render text instances (due to supporting RenderTextMac) used for > drawing which are cleared and created on each layout. Also, there's the > |render_text_| instance which is used for preferred sizing and persisting some > data. > > Some things which may probably be looked at- > 1) If we are able to move away from using RenderTextMac on Mac, can we use a > single persistent RenderText instance (like we do in Textfields). > 2) Is it necessary to use a separate |render_text_| instance for preferred > sizing. > Can we use a temporary render text instance when necessary and maybe have a > cache layer on top in the Label class? > 3) Also, there's the StyledLabel class which is used for creating a text view > with multiple styles. Haven't looked into it, but it may be possible for Label > or a Label subclass to support the StyledLabel class API, since RenderText > supports different styles for a given piece of text. Yeah, these would be good tasks to pursue, I'm in favor of them. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.cc:581: // Todo(karandeepb): Labels should support dragging selected text. Tracked in optional nit: all-caps TODO is more common, you can optionally do: TODO(crbug.com/630365): Labels should support dragging selected text. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.cc:874: if (HasSelection()) nit: curlies https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.h:165: base::string16 GetSelectedText() const; nit: inline this; it's only called once by Label, and the tests can have their own local helper that uses GetRenderTextForSelectionController. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:35: // ::testing::Test: nit: cite the more direct base class (ViewsTestBase) instead. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:39: widget_ = new Widget; Avoid new and a raw pointer; use a plain member |Widget widget_;| or std::unique_ptr and base::MakeUnique if really needed. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:42: params.bounds = gfx::Rect(200, 200, 200, 200); q: are the bounds important or should they just be non-empty? (maybe just use gfx::Rect(200, 200); for a size with 0,0 position?) https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:54: if (widget_) I don't see when widget_ would be null here; remove this check? https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:55: widget_->Close(); nit: Use WIDGET_OWNS_NATIVE_WIDGET and skip this? Destruction should suffice. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:66: return widget()->GetFocusManager()->GetFocusedView(); optional nit: inline this. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:69: void PerformMousePress(const gfx::Point& point, int extra_flags = 0) { I wonder if it makes sense to use EventGenerator here? (don't bother if that doesn't work right for unit tests) https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:88: void DragMouseTo(const gfx::Point& point) { nit: name PerformMouseDragTo or similar for consistency. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:103: Label* label_; nit: |= nullptr| here to avoid initializer list. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:774: // Todo(karandeepb): Re-enable these once Mac uses RenderTextHarfBuzz for ditto nit: TODO https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:775: // Labels. Tracked in https://crbug.com/630365. aside: careful overloading a single issue with many defects; sometimes it makes more sense to file separate issues for different aspects of some larger meta-task. Really, it'd be a shame to mark 630365 fixed once it works on win/linux/cros, and let this dangle with a reference to a closed bug while there's no ongoing work or tracking bug to support mac. In this sepcific case, maybe we should cite 454835? https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:778: // By default labels don't support text selection. grammar nit: add a comma after "By default" (matches your use below). https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:784: // Verify making a label multiline, causes the label to not support text grammar nit: remove the comma and optionally say "Verify that" https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:790: // Verify labels supporting text selection, get focus on clicks. ditto grammar nit: remove the comma and optionally say "Verify that" https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:797: EXPECT_NE(label(), GetFocusedView()); Be careful checking focus in a unit test; multiple unit tests running in parallel can cause conflicts with global state like focus. These might need to be made into interactive_ui_tests, but I'm not 100% sure that's necessary... sadrul or sky might know offhand. If you do need to make interactive_ui_tests; consider reverting the LabelTest changes here. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:870: DragMouseTo(gfx::Point()); It seems odd to send a drag event after release (part of click). Should we do mouse press, drag, *then* release here and elsewhere below? https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:913: // Verify text selection using the mouse, updates the selection clipboard. grammar nit: remove comma
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:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying and dragging selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ==========
Patchset #12 (id:560001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL msw@. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.cc:581: // Todo(karandeepb): Labels should support dragging selected text. Tracked in On 2016/11/01 23:42:21, msw wrote: > optional nit: all-caps TODO is more common, you can optionally do: > TODO(crbug.com/630365): Labels should support dragging selected text. Done. Also, filed a new bug report for this. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.cc:874: if (HasSelection()) On 2016/11/01 23:42:21, msw wrote: > nit: curlies Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label.h:165: base::string16 GetSelectedText() const; On 2016/11/01 23:42:22, msw wrote: > nit: inline this; it's only called once by Label, and the tests can have their > own local helper that uses GetRenderTextForSelectionController. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:35: // ::testing::Test: On 2016/11/01 23:42:22, msw wrote: > nit: cite the more direct base class (ViewsTestBase) instead. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:39: widget_ = new Widget; On 2016/11/01 23:42:22, msw wrote: > Avoid new and a raw pointer; use a plain member |Widget widget_;| or > std::unique_ptr and base::MakeUnique if really needed. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:42: params.bounds = gfx::Rect(200, 200, 200, 200); On 2016/11/01 23:42:22, msw wrote: > q: are the bounds important or should they just be non-empty? > (maybe just use gfx::Rect(200, 200); for a size with 0,0 position?) Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:54: if (widget_) On 2016/11/01 23:42:22, msw wrote: > I don't see when widget_ would be null here; remove this check? Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:55: widget_->Close(); On 2016/11/01 23:42:22, msw wrote: > nit: Use WIDGET_OWNS_NATIVE_WIDGET and skip this? Destruction should suffice. Using WIDGET_OWNS_NATIVE_WIDGET since we are managing the Widget lifetime now. Close is still necessary since there are checks to ensure that all windows are closed during TearDown. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:66: return widget()->GetFocusManager()->GetFocusedView(); On 2016/11/01 23:42:22, msw wrote: > optional nit: inline this. Keeping this. This is used multiple times. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:69: void PerformMousePress(const gfx::Point& point, int extra_flags = 0) { On 2016/11/01 23:42:22, msw wrote: > I wonder if it makes sense to use EventGenerator here? > (don't bother if that doesn't work right for unit tests) This is easier to set up (Don't have to think about coordinates etc.) and since we are just unit-testing the Label class and not the event propagation, this should suffice. Tests for textfield/combobox also don't use EventGenerator for mouse events. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:88: void DragMouseTo(const gfx::Point& point) { On 2016/11/01 23:42:22, msw wrote: > nit: name PerformMouseDragTo or similar for consistency. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:103: Label* label_; On 2016/11/01 23:42:22, msw wrote: > nit: |= nullptr| here to avoid initializer list. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:774: // Todo(karandeepb): Re-enable these once Mac uses RenderTextHarfBuzz for On 2016/11/01 23:42:22, msw wrote: > ditto nit: TODO Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:775: // Labels. Tracked in https://crbug.com/630365. On 2016/11/01 23:42:22, msw wrote: > aside: careful overloading a single issue with many defects; sometimes it makes > more sense to file separate issues for different aspects of some larger > meta-task. Really, it'd be a shame to mark 630365 fixed once it works on > win/linux/cros, and let this dangle with a reference to a closed bug while > there's no ongoing work or tracking bug to support mac. > > In this sepcific case, maybe we should cite 454835? Yeah you are correct. Have filed a new issue blocking the larger Label selection issue. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:778: // By default labels don't support text selection. On 2016/11/01 23:42:22, msw wrote: > grammar nit: add a comma after "By default" (matches your use below). Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:784: // Verify making a label multiline, causes the label to not support text On 2016/11/01 23:42:22, msw wrote: > grammar nit: remove the comma and optionally say "Verify that" Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:790: // Verify labels supporting text selection, get focus on clicks. On 2016/11/01 23:42:22, msw wrote: > ditto grammar nit: remove the comma and optionally say "Verify that" Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:797: EXPECT_NE(label(), GetFocusedView()); On 2016/11/01 23:42:22, msw wrote: > Be careful checking focus in a unit test; multiple unit tests running in > parallel can cause conflicts with global state like focus. These might need to > be made into interactive_ui_tests, but I'm not 100% sure that's necessary... > sadrul or sky might know offhand. If you do need to make interactive_ui_tests; > consider reverting the LabelTest changes here. There are similar unit tests at many other places. I think there are already checks in place to ensure key window state can be "faked" in unit tests. For example on Mac, we have this - https://cs.chromium.org/chromium/src/ui/base/test/scoped_fake_nswindow_focus..... Not really sure how other platforms handle this though. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:870: DragMouseTo(gfx::Point()); On 2016/11/01 23:42:22, msw wrote: > It seems odd to send a drag event after release (part of click). > Should we do mouse press, drag, *then* release here and elsewhere below? Yeah you are correct. Done. https://codereview.chromium.org/2413223003/diff/540001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:913: // Verify text selection using the mouse, updates the selection clipboard. On 2016/11/01 23:42:22, msw wrote: > grammar nit: remove comma Done.
lgtm
https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.cc:262: selection_controller_.reset(new SelectionController(this)); MakeUnique. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.cc:264: // On Linux, update the selection clipboard on a text selection. It seems like this should be the default, meaning you shouldn't have to explicitly call it. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.h:65: void SetSelectionTextColor(SkColor color); Is setting the selection colors really needed? AFAICT no one actually uses it. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.h:149: bool SetSelectable(bool selectable); Couple of questions: Is the plan to make selection always supported? If not, how come? What do you expect callers to do with a return value of false? Is there a reason not to make all labels selectable?
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
PTAL sky@. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.cc:262: selection_controller_.reset(new SelectionController(this)); On 2016/11/03 03:23:20, sky wrote: > MakeUnique. Done. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.cc:264: // On Linux, update the selection clipboard on a text selection. On 2016/11/03 03:23:20, sky wrote: > It seems like this should be the default, meaning you shouldn't have to > explicitly call it. Moved to the SelectionController constructor. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.h:65: void SetSelectionTextColor(SkColor color); On 2016/11/03 03:23:20, sky wrote: > Is setting the selection colors really needed? AFAICT no one actually uses it. Yeah for textfields there are no usages of SetSelectionTextColor. However since Labels are generally rendered with a custom background/text color, I think this will be useful. For example, no. of usages of SetTextColor/SetBackgroundColor for Labels and Textfields are 86/29 vs 4/7 respectively. Of course Labels are used more often though. https://codereview.chromium.org/2413223003/diff/580001/ui/views/controls/labe... ui/views/controls/label.h:149: bool SetSelectable(bool selectable); >> Is the plan to make selection always supported? If not, how come? Cases where selection is not supported- 1) MacViews currently uses RenderTextMac which does not support text selection. Have created a bug to track the migration of Views::Labels to HarfBuzz. 2) Multiline labels do not currently support text selection. There are changes required in RenderTextHarfBuzz which I'll be working on subsequently. 3) Links do not support text selection. Don't think they should, since a click event inside a link should click the link. On Webcontents, a click inside a Link, clicks the link, while a drag drags it. >>What do you expect callers to do with a return value of false? Yeah it's not useful probably. Can remove it. >>Is there a reason not to make all labels selectable? For example, we don't want Labels inside buttons to intercept mouse events for text selection. Also, looking at the native Cocoa UI (where text selection is supported), some text views support selection while others do not. I think we'll only want to make labels selectable where we want the user to say copy the text (like crbug.com/437993 demands). Also, quoting one of your comments from an earlier attempt at this (https://codereview.chromium.org/1272363002/)- "I haven't looked at your patch, but make sure you don't change the default context-menu/mouse/focus handling at all. Sites should have to opt into the new behavior where they know it won't cause problems. For example, the tabstrip uses labels and if labels started consuming touch events you would break touch handling in the tabstrip."
https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.cc:820: void Label::ApplyTextColors() const { Why are you making this const? It's modifying internal state, shouldn't it be non-const? https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.cc:864: void Label::ClearRenderTextLines() const { Same comment about constness here. https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.h:270: mutable std::vector<std::unique_ptr<gfx::RenderText>> lines_; Why are you making lines_ mutable? It seems like an intrinsic part of label and something that should not be made mutable.
PTAL sky@. https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... File ui/views/controls/label.cc (right): https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.cc:820: void Label::ApplyTextColors() const { On 2016/11/03 17:16:25, sky wrote: > Why are you making this const? It's modifying internal state, shouldn't it be > non-const? See comment for |lines_| https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.cc:864: void Label::ClearRenderTextLines() const { On 2016/11/03 17:16:25, sky wrote: > Same comment about constness here. See comment for |lines_|. https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2413223003/diff/600001/ui/views/controls/labe... ui/views/controls/label.h:270: mutable std::vector<std::unique_ptr<gfx::RenderText>> lines_; On 2016/11/03 17:16:25, sky wrote: > Why are you making lines_ mutable? It seems like an intrinsic part of label and > something that should not be made mutable. I may be wrong here but my reasoning was- The |lines_| vector is used to maintain the render text instances actually used for drawing. And they are cleared and re-created in a way that is oblivious to the public Label API. Hence I thought that it was reasonable to make |lines_| mutable. This is needed to make the HasSelection, GetFocusBounds and GetRenderTextForSelectionController methods const. Private methods like ApplyTextColors, ClearRenderTextLines, MaybeCreateRenderTextLines are just re-applying the state that was changed earlier through some non-const public API methods (SetEnabledColor, SetMultiline etc.) Since these methods are not changing the logical state of the Label class, I thought they can be made const.
LGTM
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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 sky@. Had to make a small change for Windows. https://codereview.chromium.org/2413223003/diff/620001/ui/native_theme/native... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2413223003/diff/620001/ui/native_theme/native... ui/native_theme/native_theme_win.cc:557: return kLabelTextSelectionBackgroundFocusedColor; Tested the patch on Windows as well. Had to change this. Auto color readability in Labels (which is enabled by default) was changing the text selection color from white to black on Windows. See Label::RecalculateColors. This background color causes white text selection color on Windows, which is what we want.
SLGTM
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/2413223003/#ps640001 (title: "Rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #16 (id:660001) has been deleted
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/2413223003/#ps680001 (title: "Rebase")
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:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying and dragging selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying and dragging selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ==========
Message was sent while issue was closed.
Committed patchset #16 (id:680001)
Message was sent while issue was closed.
Description was changed from ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying and dragging selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. ========== to ========== Views:: Make Labels support text selection. This CL is a follow-up to r427604 which extracted the text selection logic from views::Textfield and introduced the SelectionController class. This CL makes views::Label implement the SelectionControllerDelegate interface. As a result, single-line views::Label now support text selection. Some of the member functions in the Label class which do not change the external state of the Label are made const. This is made possible by making the render text instances that are used for drawing, mutable. A new test fixture class is also added for testing Labels. Existing tests are modified to use it, and new tests are added to test the text selection behavior. Support for copying and dragging selected text and text selection in multi-line views::Label will be added subsequently. Doc= https://docs.google.com/a/google.com/document/d/1JTjY8RLE84hxubCAouHuD6xVtvuM... BUG=630365, 437993 TEST= Launch views_examples_with_content_exe. Select Labels from the dropdown. Verify that text selection in the label with the text "A label supporting text selection." works properly. On Mac, also pass the --enable-harfbuzz-rendertext flag. Committed: https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d Cr-Commit-Position: refs/heads/master@{#430461} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d Cr-Commit-Position: refs/heads/master@{#430461} |
