New issue
Advanced search Search tips

Issue 630365 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug


Sign in to add a comment

Label: support text selection

Project Member Reported by ellyjo...@chromium.org, Jul 21 2016

Issue description

Label should support being optionally selectable, which means:

1) Supporting multi-line text selection in gfx::RenderText ( https://crbug.com/650120 )
2) Adding selection support in Label, probably by factoring out the selection logic in Textfield::OnMousePressed/OnMouseDragged into a controller class Textfield and Label can both share.
 
Blocking: 617439

Comment 2 by msw@chromium.org, Aug 11 2016

Cc: msw@chromium.org
Description: Show this description
Blockedon: 650120
Hey Elly, have you started this yet? If not, I can probably take a stab at this.
Cc: ellyjo...@chromium.org
Owner: karandeepb@chromium.org
All yours :)
Blocking: 437993
Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bc2b7bbb5062c62fb98fb7d925765f64be80552

commit 9bc2b7bbb5062c62fb98fb7d925765f64be80552
Author: karandeepb <karandeepb@chromium.org>
Date: Wed Oct 26 06:04:30 2016

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/1JTjY8RLE84hxubCAouHuD6xVtvuMnzbqSDZyf_4N6tY/edit?usp=sharing
BUG= 630365 ,  437993 ,  657691 

Review-Url: https://codereview.chromium.org/2408623002
Cr-Commit-Position: refs/heads/master@{#427604}

[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/gfx/render_text.cc
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/gfx/render_text.h
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/BUILD.gn
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/controls/textfield/textfield_model.cc
[modify] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/controls/textfield/textfield_model.h
[add] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/selection_controller.cc
[add] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/selection_controller.h
[add] https://crrev.com/9bc2b7bbb5062c62fb98fb7d925765f64be80552/ui/views/selection_controller_delegate.h

Blockedon: 661379
Blockedon: -661379
Blockedon: 661394
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e225043e0f7aa5b951289e15b78c83aee7fda9d

commit 8e225043e0f7aa5b951289e15b78c83aee7fda9d
Author: karandeepb <karandeepb@chromium.org>
Date: Tue Nov 08 01:20:10 2016

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/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.

Review-Url: https://codereview.chromium.org/2413223003
Cr-Commit-Position: refs/heads/master@{#430461}

[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/chrome/browser/ui/libgtkui/native_theme_gtk.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/gfx/render_text.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/gfx/render_text_mac.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/gfx/render_text_mac.mm
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/native_theme/common_theme.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/native_theme/native_theme.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/native_theme/native_theme_dark_aura.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/native_theme/native_theme_mac.mm
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/native_theme/native_theme_win.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/label.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/label.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/label_unittest.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/link.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/link.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/textfield/textfield.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/examples/label_example.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/selection_controller.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/selection_controller_delegate.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/style/platform_style.cc
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/style/platform_style.h
[modify] https://crrev.com/8e225043e0f7aa5b951289e15b78c83aee7fda9d/ui/views/style/platform_style_mac.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d52fcdcee551a2f4e77c1f7223eb73f6d85c210b

commit d52fcdcee551a2f4e77c1f7223eb73f6d85c210b
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Nov 10 00:41:51 2016

views::Label: Enable text selection related tests on Mac.

Currently text selection related tests for Labels are not run on Mac since it
uses RenderTextMac by default. Enable these tests on Mac, by explicitly using
RenderTextHarfBuzz which supports text selection. To do this, create a new test
fixture class called LabelSelectionTest and append the kEnableHarfBuzzRenderText
switch to the list of command line switches.

BUG= 630365 

Review-Url: https://codereview.chromium.org/2486963003
Cr-Commit-Position: refs/heads/master@{#431107}

[modify] https://crrev.com/d52fcdcee551a2f4e77c1f7223eb73f6d85c210b/ui/views/controls/label.h
[modify] https://crrev.com/d52fcdcee551a2f4e77c1f7223eb73f6d85c210b/ui/views/controls/label_unittest.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb049755748ea449878a7bef42e6d4fabfc80b5a

commit fb049755748ea449878a7bef42e6d4fabfc80b5a
Author: karandeepb <karandeepb@chromium.org>
Date: Thu Nov 17 03:15:44 2016

views::Label: Implement context menu, keyboard shortcuts for copy/select all.

This CL is a follow-up to r430461 which implemented text selection for Labels.
This CL implements the following for Labels-
- Copy selected text on [Ctrl+C] or [Ctrl+Insert].
- Select all text on [Ctrl+A].
- Context Menu with the options Copy and Select All.
- Copy selected text using the Browser menu.

BridgedContentView's validateUserInterfaceItem method is also modified.
Currently it returns YES only when there is an active TextInputClient which can
perform the given command. But since views::Label does not implement the
TextInputClient interface, the method is modified to return YES in case the
focused view is a Label for the Copy and Select All commands. This is necessary
for Copy and Select All keyboard shortcuts and browser menu commands to work on
MacViews for labels.

Some other changes-
- The stored selection range is now correctly invalidated in Label::SetText and
  Label::SetSelectable.
- Text selection is disabled for obscured labels.

BUG= 630365 ,  437993 

Review-Url: https://codereview.chromium.org/2422993002
Cr-Commit-Position: refs/heads/master@{#432720}

[modify] https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a/ui/views/cocoa/bridged_content_view.mm
[modify] https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a/ui/views/controls/label.cc
[modify] https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a/ui/views/controls/label.h
[modify] https://crrev.com/fb049755748ea449878a7bef42e6d4fabfc80b5a/ui/views/controls/label_unittest.cc

Blocking: 603386
Project Member

Comment 17 by bugdroid1@chromium.org, Jan 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e4ca175171eb6c46603799904a624e6797773e4

commit 4e4ca175171eb6c46603799904a624e6797773e4
Author: karandeepb <karandeepb@chromium.org>
Date: Wed Jan 04 02:12:24 2017

RenderTextHarfBuzz: Add support for multi line text selection.

This CL-
  - Adds support for multi-line text selection to RenderTextHarfBuzz. To do this
    RenderTextHarfBuzz::FindCursorPosition and
    RenderTextHarfBuzz::GetSubstringBounds are reimplemented.
  - RenderTextHarfBuzz::FindCursorPosition now returns valid grapheme boundaries
    hence fixing  issue 673986 .
  - RenderTextHarfBuzz::GetSubstringBounds did already support multiline but the
    implementation was flawed. It relied on the text space bounds intersection
    computed from LineSegment's x_range paramater and from TextRunHarfBuzz.
    However these were not in sync if some newline segments were popped or if
    some text was truncated.
  - Enables multi-line text selection for views::Labels.
  - Corrects the behavior for RTL when mouse is dragged above/below the text
    bounds on MacViews.
  - Adds lots of tests.

BUG= 650120 ,  630365 ,  600166 ,  673986 
TEST= Open views_examples_with_content_exe. Select Labels from the dropdown.
Enter a large string in the "Content" textfield. Enable the Multiline and Text
Selection checkboxes. Ensure text selection works correctly on the Label.

Review-Url: https://codereview.chromium.org/2541313002
Cr-Commit-Position: refs/heads/master@{#441290}

[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/gfx/render_text.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/gfx/render_text.h
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/gfx/render_text_harfbuzz.h
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/gfx/render_text_unittest.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/controls/label.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/controls/label.h
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/controls/label_unittest.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/controls/textfield/textfield_unittest.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/examples/label_example.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/examples/label_example.h
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/selection_controller.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/style/platform_style.cc
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/style/platform_style.h
[modify] https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4/ui/views/style/platform_style_mac.mm

Status: Fixed (was: Started)

Sign in to add a comment