Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2541313002:
RenderTextHarfBuzz: Add support for multi line text selection. (Closed)
|
Created:
4 years ago by karandeepb Modified:
3 years, 11 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, derat+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRenderTextHarfBuzz: 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.
Committed: https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4
Cr-Commit-Position: refs/heads/master@{#441290}
Patch Set 1 : -- #
Total comments: 20
Patch Set 2 : -- #
Total comments: 1
Patch Set 3 : --- #
Total comments: 46
Patch Set 4 : Address review. #Patch Set 5 : Tests #
Total comments: 16
Patch Set 6 : GetSubstringBounds now handles mid glyph selections correctly. #Patch Set 7 : Nits. #Patch Set 8 : FindCursorPosition now returns valid grapheme boundaries. crbug.com/673986 #
Total comments: 3
Patch Set 9 : Compile error. #Patch Set 10 : Nits. #
Total comments: 11
Patch Set 11 : Address comments. #
Total comments: 2
Patch Set 12 : Nit. #Patch Set 13 : Rebase. #Patch Set 14 : Fix test. #
Dependent Patchsets: Messages
Total messages: 75 (60 generated)
Patchset #3 (id:40001) 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...
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 #4 (id:180001) has been deleted
Patchset #4 (id:200001) 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
==========
rthb multiline new approach.
==========
to
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
BUG= 650120, 630365
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.
==========
Description was changed from
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
BUG= 650120, 630365
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.
==========
to
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
BUG= 650120, 630365, 600166
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.
==========
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@. Can you do an initial review before I add tests? Also, if possible, please try it out and let me know if you find some incorrect behavior. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1253: const Range intersection = segment->x_range.Intersect(x).Ceil(); As pointed elsewhere, the logic here was incorrect, since we are intersecting text space bounds computed using TextRunHarfBuzz and LineSegment. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1013: EnsureLayout(); Since this is a public function, we should call EnsureLayout() since lines() is accessed below. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1133: WDYT of this logic? It seems to work fine, however since BiDi text is confusing!, I am not really sure. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:162: RangeF x_range; I'd like to point out some issues I found with the way the code behaves. There is this concept of text space bounds [which are the coordinates along the x axis for some block of text, computed assuming the text was rendered on a single line]. These text space bounds can be computed using either- - LineSegment's x_range parameter. This is used for example in RenderText::TextBoundsToViewBounds for the multiline case (this case is not hit currently) - TextRunHarfBuzz's preceding_run_widths and positions[]. This is used directly/indirectly for example in RenderTextHarfBuzz::GetGraphemeBounds, RenderTextHarfBuzz::GetSubstringBounds, RenderTextHarfBuzz::GetGlyphBounds, RenderText::GetCursorBounds and an omnibox function). The way GetSubstringBounds is currently implemented, it computes the intersection of text space bounds computed using LineSegment and TextRunHarfBuzz. However, the text space bounds computed using TextRunHarfBuzz may not be correct if there is newline in text (we don't render newline - https://cs.chromium.org/chromium/src/ui/gfx/render_text_harfbuzz.cc?sq=packag...) or if we truncate the text. Hence in these cases, any code using TextRunHarfBuzz to compute the text space bounds would be incorrect. This is not an issue currently, since only Textfields use GetCursorBounds etc. and they don't have newlines and they don't truncate text. I'll add some comments in the code pointing this out, if this makes sense to you. Also, I have removed the only usage of LineSegment's x_range parameter. I think it can be removed in a subsequent CL. The way it is computed currently is quite complex. See for example UpdateRTLSegmentRanges(). Even if it's needed, I think it may be easier to compute it in a post processing step. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:301: word_segments.pop_back(); Why do we remove newline segments? The change here would cause the \n glyph to be rendered as well when it's the only character in the line. Is it ok? If you think it's necessary I can add code to not render the newline segments. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1073: if (!intersection.is_empty()) { The earlier code used GetGraphemeBounds so as to consider the whole grapheme. However I am not sure if it's necessary. I guess only the graphemes at the end points would matter, but then the |range| passed would have accounted for it. Selection seems to be behaving correctly for multi character graphemes. Let me know if you think this needs a closer look. https://codereview.chromium.org/2541313002/diff/160001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/160001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1260: // If |point| is outside the text space, clip it to the end of the last line. This got hit through GetCursorBounds, when I enabled the cursor for labels. Just ensuring |line| is within the correct bounds.
Patchset #2 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks pretty good; I'll try to test it manually soon. You can also ping some test/QA folks to give it a shot once it lands. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:824: return GetDisplayTextDirection() == base::i18n::RIGHT_TO_LEFT ? CURSOR_RIGHT optional nit: make formatting consistent with GetVisualDirectionOfLogicalEnd; git cl format https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1119: gfx::VisualCursorDirection direction) { nit: no gfx:: https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1121: // Only the last line can be empty. nit: can you DCHECK_EQ(line, lines().back()); or similar, instead? https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:214: // TODO(benrg): GTK uses the first of U+25CF, U+2022, U+2731, U+273A, '*' nit: you can remove this TODO https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:482: // Note: This function does not support multiline currently. nit: rephrase as a TODO; cite a bug if appropriate https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:601: // Get the SelectionModels corresponding to visual text ends. optional nit: "Get the selection model corresponding to visual text ends." https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:605: // Get the selection model corresponding to visual text ends for the given optional nit: "Get the selection model corresponding to visual text ends of |line|. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:295: // Since the line should atleast contain some information regarding the nit: 'at least' https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:827: DCHECK(!lines().empty()); Is lines() empty after layout for non-multiline instances? (I forget) https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:836: line_index++; nit: line_index = 0; https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:838: if (line_index >= (int)lines().size()) { nit: use static_cast, or checked_cast from base/numerics/safe_conversions.h https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:842: line_index--; nit: line_index = lines().size() - 1; (with cast like above) https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:852: if (segment_index >= (int)line.segments.size()) ditto nit: casting https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:857: const Range segment_glyph_range = optional nit: just cache the min value instead https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1065: DCHECK_EQ(lines().size() - 1, line_index); ditto nit: check for lines.back() instead, if possible https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1066: continue; nit: continue isn't necessary, given the segments iteration below https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1077: float x; nit: explicitly init to 0 (or line_x and nix that term in conditional blocks) https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1293: // segment.x_range is not used because it is text space. q: Isn't line_x also in text space? https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:517: int GetCursorYForTesting(int line_num = 0) { nit: use the line_num param in new tests, or remove it https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:521: line.size.height() / 2; optional nit: just add 1 instead of half the line height, like textfield_unittest https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... File ui/views/examples/label_example.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... ui/views/examples/label_example.cc:200: text_selection_ = new Checkbox(base::ASCIIToUTF16("Text selection")); nit: rename the identifier |selectable_| and change this string to "Selectable" https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... ui/views/examples/label_example.cc:218: text_selection_->SetChecked(true); Don't set selectable and check by default, just disable the checkbox if !custom_label_->IsSelectionSupported() instead
Patchset #5 (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...
Patchset #5 (id:280001) has been deleted
PTAL msw@. Have added some tests as well. Also, I think you missed my comments/questions in the previous patch set. Can you please look at them once. Thanks. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:824: return GetDisplayTextDirection() == base::i18n::RIGHT_TO_LEFT ? CURSOR_RIGHT On 2016/12/07 20:20:34, msw wrote: > optional nit: make formatting consistent with GetVisualDirectionOfLogicalEnd; > git cl format Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1119: gfx::VisualCursorDirection direction) { On 2016/12/07 20:20:34, msw wrote: > nit: no gfx:: Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1121: // Only the last line can be empty. On 2016/12/07 20:20:34, msw wrote: > nit: can you DCHECK_EQ(line, lines().back()); or similar, instead? Wouldn't that require overloading the == operator. Passing line_index now instead. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:214: // TODO(benrg): GTK uses the first of U+25CF, U+2022, U+2731, U+273A, '*' On 2016/12/07 20:20:35, msw wrote: > nit: you can remove this TODO Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:482: // Note: This function does not support multiline currently. On 2016/12/07 20:20:35, msw wrote: > nit: rephrase as a TODO; cite a bug if appropriate Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:601: // Get the SelectionModels corresponding to visual text ends. On 2016/12/07 20:20:34, msw wrote: > optional nit: "Get the selection model corresponding to visual text ends." Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:605: // Get the selection model corresponding to visual text ends for the given On 2016/12/07 20:20:35, msw wrote: > optional nit: "Get the selection model corresponding to visual text ends of > |line|. Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:295: // Since the line should atleast contain some information regarding the On 2016/12/07 20:20:35, msw wrote: > nit: 'at least' Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:827: DCHECK(!lines().empty()); On 2016/12/07 20:20:35, msw wrote: > Is lines() empty after layout for non-multiline instances? (I forget) No it isn't. HarfBuzzLineBreaker constructor always initiates a new line. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:836: line_index++; On 2016/12/07 20:20:35, msw wrote: > nit: line_index = 0; Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:838: if (line_index >= (int)lines().size()) { On 2016/12/07 20:20:35, msw wrote: > nit: use static_cast, or checked_cast from base/numerics/safe_conversions.h Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:842: line_index--; On 2016/12/07 20:20:35, msw wrote: > nit: line_index = lines().size() - 1; (with cast like above) Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:852: if (segment_index >= (int)line.segments.size()) On 2016/12/07 20:20:35, msw wrote: > ditto nit: casting Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:857: const Range segment_glyph_range = On 2016/12/07 20:20:35, msw wrote: > optional nit: just cache the min value instead Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1065: DCHECK_EQ(lines().size() - 1, line_index); On 2016/12/07 20:20:35, msw wrote: > ditto nit: check for lines.back() instead, if possible Would require overloading ==. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1066: continue; On 2016/12/07 20:20:35, msw wrote: > nit: continue isn't necessary, given the segments iteration below Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1077: float x; On 2016/12/07 20:20:35, msw wrote: > nit: explicitly init to 0 (or line_x and nix that term in conditional blocks) Done. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1293: // segment.x_range is not used because it is text space. On 2016/12/07 20:20:35, msw wrote: > q: Isn't line_x also in text space? Not really. line_x is relative to the beginning of text in current line. Text space refers to bounds when the whole text is rendered on a single line. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:517: int GetCursorYForTesting(int line_num = 0) { On 2016/12/07 20:20:35, msw wrote: > nit: use the line_num param in new tests, or remove it Added tests using this. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:521: line.size.height() / 2; On 2016/12/07 20:20:35, msw wrote: > optional nit: just add 1 instead of half the line height, like > textfield_unittest Done. https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... File ui/views/examples/label_example.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... ui/views/examples/label_example.cc:200: text_selection_ = new Checkbox(base::ASCIIToUTF16("Text selection")); On 2016/12/07 20:20:35, msw wrote: > nit: rename the identifier |selectable_| and change this string to "Selectable" Done. https://codereview.chromium.org/2541313002/diff/220001/ui/views/examples/labe... ui/views/examples/label_example.cc:218: text_selection_->SetChecked(true); On 2016/12/07 20:20:35, msw wrote: > Don't set selectable and check by default, just disable the checkbox if > !custom_label_->IsSelectionSupported() instead Done. This required making Label::IsSelectionSupported public, which sort of makes sense.
Sorry I missed your earlier comments/questions, and sorry that I have forgotten some details about the inner workings here, but I can help you investigate if any problems arise. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (left): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1253: const Range intersection = segment->x_range.Intersect(x).Ceil(); On 2016/12/07 03:57:47, karandeepb wrote: > As pointed elsewhere, the logic here was incorrect, since we are intersecting > text space bounds computed using TextRunHarfBuzz and LineSegment. Acknowledged. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1013: EnsureLayout(); On 2016/12/07 03:57:47, karandeepb wrote: > Since this is a public function, we should call EnsureLayout() since lines() is > accessed below. Acknowledged. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1133: On 2016/12/07 03:57:47, karandeepb wrote: > WDYT of this logic? It seems to work fine, however since BiDi text is > confusing!, I am not really sure. It seems right to me. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:162: RangeF x_range; On 2016/12/07 03:57:47, karandeepb wrote: > I'd like to point out some issues I found with the way the code behaves. There > is this concept of text space bounds [which are the coordinates along the x axis > for some block of text, computed assuming the text was rendered on a single > line]. These text space bounds can be computed using either- > > - LineSegment's x_range parameter. This is used for example in > RenderText::TextBoundsToViewBounds for the multiline case (this case is not hit > currently) > - TextRunHarfBuzz's preceding_run_widths and positions[]. This is used > directly/indirectly for example in RenderTextHarfBuzz::GetGraphemeBounds, > RenderTextHarfBuzz::GetSubstringBounds, RenderTextHarfBuzz::GetGlyphBounds, > RenderText::GetCursorBounds and an omnibox function). > > The way GetSubstringBounds is currently implemented, it computes the > intersection of text space bounds computed using LineSegment and > TextRunHarfBuzz. However, the text space bounds computed using TextRunHarfBuzz > may not be correct if there is newline in text (we don't render newline - > https://cs.chromium.org/chromium/src/ui/gfx/render_text_harfbuzz.cc?sq=packag...) > or if we truncate the text. Hence in these cases, any code using TextRunHarfBuzz > to compute the text space bounds would be incorrect. This is not an issue > currently, since only Textfields use GetCursorBounds etc. and they don't have > newlines and they don't truncate text. > > I'll add some comments in the code pointing this out, if this makes sense to > you. > > Also, I have removed the only usage of LineSegment's x_range parameter. I think > it can be removed in a subsequent CL. The way it is computed currently is quite > complex. See for example UpdateRTLSegmentRanges(). Even if it's needed, I think > it may be easier to compute it in a post processing step. I'm afraid I can't offer much useful insight here. Perhaps we can actually include the newlines in the display text, so long as common fonts don't actually render a glyph for it, or add significant width. Removing x_range sgtm if we can actually rely on the rthb's preceding_run_widths and positions[] instead, and it sounds like those are more accurate anyway. (lines and segments were added after I really worked on RenderText, and though I did review them, I can't claim to know them as well as some of my own earlier work) https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:301: word_segments.pop_back(); On 2016/12/07 03:57:47, karandeepb wrote: > Why do we remove newline segments? > > The change here would cause the \n glyph to be rendered as well when it's the > only character in the line. Is it ok? If you think it's necessary I can add code > to not render the newline segments. I don't know if/why this was necessary; perhaps we should just always include the newline character in the line segments? Hopefully most fonts don't render a glyph or add width for newline characters. Maybe ping xdai@? https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:851: return LineSelectionModel(line, CURSOR_LEFT); Perhaps we need to replace CURSOR_LEFT with GetVisualDirectionOfLogicalBeginning() and CURSOR_RIGHT below with GetVisualDirectionOfLogicalEnd()? That said, I didn't notice an issue here with RTL text in manual testing. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1052: Range layout_range(TextIndexToDisplayIndex(range.start()), optional nit: rename this |display_range| https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1073: if (!intersection.is_empty()) { On 2016/12/07 03:57:47, karandeepb wrote: > The earlier code used GetGraphemeBounds so as to consider the whole grapheme. > However I am not sure if it's necessary. I guess only the graphemes at the end > points would matter, but then the |range| passed would have accounted for it. > Selection seems to be behaving correctly for multi character graphemes. Let me > know if you think this needs a closer look. Perhaps that was done to handle invalid ranges (that start/end amid multi-char graphemes), I'm not sure... What you have seems correct, but I didn't get to test multi-char graphemes manually today. Maybe look over the tests for this function and add some extra test cases if needed? https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text.cc#... ui/gfx/render_text.cc:1121: // Only the last line can be empty. On 2016/12/12 10:36:18, karandeepb wrote: > On 2016/12/07 20:20:34, msw wrote: > > nit: can you DCHECK_EQ(line, lines().back()); or similar, instead? > > Wouldn't that require overloading the == operator. Passing line_index now > instead. Acknowledged. https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/220001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1293: // segment.x_range is not used because it is text space. On 2016/12/12 10:36:18, karandeepb wrote: > On 2016/12/07 20:20:35, msw wrote: > > q: Isn't line_x also in text space? > > Not really. line_x is relative to the beginning of text in current line. Text > space refers to bounds when the whole text is rendered on a single line. Acknowledged. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:43: namespace views { Hmm, gfx shouldn't reference views, and I'm not sure if friend tests are exempted from dependency violations. Can you make it a RenderText test or expose the needed private functions (or add *ForTest intermediary helper functions)? https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1065: DCHECK_EQ(lines().size() - 1, line_index); nit: I think nesting a DCHECK in an if statement works (produces an empty statement in non-dcheck builds), but this might be nicer as DHCECK(!line.segments.empty() || (line_index == lines().size() - 1)); Relatedly, maybe we can just include newline characters everywhere? https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1813: TEST_P(RenderTextHarfBuzzTest, FindCursorPositionMultiline) { q: should we have any multi-char grapheme test cases for FindCursorPosition? https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1816: bool is_ltr; optional nit: check RenderText::GetDisplayTextDirection() below instead? https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1836: const Point query(cases[i].is_ltr ? bounds.x() + 1 : bounds.right() - 1, nit: name this cursor_position? https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1839: const SelectionModel model_for_query = nit: name this model? https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... ui/views/controls/label.h:155: virtual bool IsSelectionSupported() const; nit: reorder function definition order to match this new decl order https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:183: gfx::Point GetCursorPoint(int index, bool rtl = false) { nit: can we check if the text is rtl instead of passing in a flag?
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.
Description was changed from
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
BUG= 650120, 630365, 600166
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.
==========
to
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
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.
==========
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:340001) has been deleted
Patchset #6 (id:360001) has been deleted
Patchset #8 (id:420001) 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.
PTAL msw@. I have corrected some behavior: - Particularly, behavior of GetSubstringBounds when the queries range endpoints lie within a glyph (For eg ligatures) - Behavior of GetSubstringBounds when the queried range end points are not on valid grapheme boundaries. - FindCursorPosition now returns valid grapheme boundaries. (Not the case currently). Hence fixing crbug.com/673986. Also, added a bunch of tests for the same. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:162: RangeF x_range; On 2016/12/13 03:04:55, msw wrote: > On 2016/12/07 03:57:47, karandeepb wrote: > > I'd like to point out some issues I found with the way the code behaves. There > > is this concept of text space bounds [which are the coordinates along the x > axis > > for some block of text, computed assuming the text was rendered on a single > > line]. These text space bounds can be computed using either- > > > > - LineSegment's x_range parameter. This is used for example in > > RenderText::TextBoundsToViewBounds for the multiline case (this case is not > hit > > currently) > > - TextRunHarfBuzz's preceding_run_widths and positions[]. This is used > > directly/indirectly for example in RenderTextHarfBuzz::GetGraphemeBounds, > > RenderTextHarfBuzz::GetSubstringBounds, RenderTextHarfBuzz::GetGlyphBounds, > > RenderText::GetCursorBounds and an omnibox function). > > > > The way GetSubstringBounds is currently implemented, it computes the > > intersection of text space bounds computed using LineSegment and > > TextRunHarfBuzz. However, the text space bounds computed using TextRunHarfBuzz > > may not be correct if there is newline in text (we don't render newline - > > > https://cs.chromium.org/chromium/src/ui/gfx/render_text_harfbuzz.cc?sq=packag...) > > or if we truncate the text. Hence in these cases, any code using > TextRunHarfBuzz > > to compute the text space bounds would be incorrect. This is not an issue > > currently, since only Textfields use GetCursorBounds etc. and they don't have > > newlines and they don't truncate text. > > > > I'll add some comments in the code pointing this out, if this makes sense to > > you. > > > > Also, I have removed the only usage of LineSegment's x_range parameter. I > think > > it can be removed in a subsequent CL. The way it is computed currently is > quite > > complex. See for example UpdateRTLSegmentRanges(). Even if it's needed, I > think > > it may be easier to compute it in a post processing step. > > I'm afraid I can't offer much useful insight here. Perhaps we can actually > include the newlines in the display text, so long as common fonts don't actually > render a glyph for it, or add significant width. Removing x_range sgtm if we can > actually rely on the rthb's preceding_run_widths and positions[] instead, and it > sounds like those are more accurate anyway. (lines and segments were added after > I really worked on RenderText, and though I did review them, I can't claim to > know them as well as some of my own earlier work) Yeah I think if this goes through, xrange can be removed and replaced with width since it is not being used anymore. That should simplify the code somewhat. I pinged xdai@ regarding not rendering the newline glyph, but she couldn't remember why it was so. The newline glyph does take up some space. Since we won't like to mess with the layout of already existing multiline strings, this may be fine (only rendering newline when it's the only character in its' line). https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:301: word_segments.pop_back(); On 2016/12/13 03:04:55, msw wrote: > On 2016/12/07 03:57:47, karandeepb wrote: > > Why do we remove newline segments? > > > > The change here would cause the \n glyph to be rendered as well when it's the > > only character in the line. Is it ok? If you think it's necessary I can add > code > > to not render the newline segments. > > I don't know if/why this was necessary; perhaps we should just always include > the newline character in the line segments? Hopefully most fonts don't render a > glyph or add width for newline characters. Maybe ping xdai@? See my comment at https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text.h#n.... https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:851: return LineSelectionModel(line, CURSOR_LEFT); On 2016/12/13 03:04:55, msw wrote: > Perhaps we need to replace CURSOR_LEFT with > GetVisualDirectionOfLogicalBeginning() and CURSOR_RIGHT below with > GetVisualDirectionOfLogicalEnd()? That said, I didn't notice an issue here with > RTL text in manual testing. Think this is correct, segment_index < 0, indicates, the point was before all the segments and similarly for the other case, it is after all the segments. The segments for a line are stored in display order (left to right). See HarfBuzzLineBreaker::AdvanceLine which sorts the segments in display order and DrawVisualText which draws the segments. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1052: Range layout_range(TextIndexToDisplayIndex(range.start()), On 2016/12/13 03:04:55, msw wrote: > optional nit: rename this |display_range| Done. https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1073: if (!intersection.is_empty()) { On 2016/12/13 03:04:55, msw wrote: > On 2016/12/07 03:57:47, karandeepb wrote: > > The earlier code used GetGraphemeBounds so as to consider the whole grapheme. > > However I am not sure if it's necessary. I guess only the graphemes at the end > > points would matter, but then the |range| passed would have accounted for it. > > Selection seems to be behaving correctly for multi character graphemes. Let me > > know if you think this needs a closer look. > > Perhaps that was done to handle invalid ranges (that start/end amid multi-char > graphemes), I'm not sure... What you have seems correct, but I didn't get to > test multi-char graphemes manually today. Maybe look over the tests for this > function and add some extra test cases if needed? The code was indeed incorrect for the cases when the range end points were not valid grapheme boundaries or when the grapheme boundary was between a glyph. I did not understand the concept of multi grapheme glyphs earlier. Have modified the code, verified it behaves correctly with ligatures and when selection is not at a valid grapheme boundary. Also have added tests for the 2 cases. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:43: namespace views { On 2016/12/13 03:04:55, msw wrote: > Hmm, gfx shouldn't reference views, and I'm not sure if friend tests are > exempted from dependency violations. Can you make it a RenderText test or expose > the needed private functions (or add *ForTest intermediary helper functions)? Would like to have a test testing multiline selection at the Label level as well. Have added a ForTest* intermediary helper function. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:1065: DCHECK_EQ(lines().size() - 1, line_index); On 2016/12/13 03:04:55, msw wrote: > nit: I think nesting a DCHECK in an if statement works (produces an empty > statement in non-dcheck builds), but this might be nicer as > DHCECK(!line.segments.empty() || (line_index == lines().size() - 1)); > Relatedly, maybe we can just include newline characters everywhere? Done. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1813: TEST_P(RenderTextHarfBuzzTest, FindCursorPositionMultiline) { On 2016/12/13 03:04:56, msw wrote: > q: should we have any multi-char grapheme test cases for FindCursorPosition? Thanks for pointing this out. The existing implementation does not always return valid grapheme boundaries. As a result, bugs like http://crbug.com/673986 occur. I have modified the code so that a valid grapheme boundary is returned. Also, added a test. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1816: bool is_ltr; On 2016/12/13 03:04:56, msw wrote: > optional nit: check RenderText::GetDisplayTextDirection() below instead? Done. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1836: const Point query(cases[i].is_ltr ? bounds.x() + 1 : bounds.right() - 1, On 2016/12/13 03:04:56, msw wrote: > nit: name this cursor_position? Done. https://codereview.chromium.org/2541313002/diff/300001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1839: const SelectionModel model_for_query = On 2016/12/13 03:04:56, msw wrote: > nit: name this model? Done. https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... ui/views/controls/label.h:155: virtual bool IsSelectionSupported() const; On 2016/12/13 03:04:56, msw wrote: > nit: reorder function definition order to match this new decl order It is already matched, the order was probably incorrect earlier. https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/300001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:183: gfx::Point GetCursorPoint(int index, bool rtl = false) { On 2016/12/13 03:04:56, msw wrote: > nit: can we check if the text is rtl instead of passing in a flag? Done. https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:889: : SelectionModel(index, CURSOR_FORWARD); The behavior of FindCursorPosition still isn't fully correct for cases where grapheme boundaries lie within a glyph. Eg ffi ligature using Meiryo UI font. Ideally, we should use the offset within the glyph bound to find the exact grapheme boundary within the glyph. However, this is not a regression. Hence, this should be ok for now.
Description was changed from
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
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.
==========
to
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
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.
==========
Mostly lg, thanks for digging in to the details here! https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/120001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:851: return LineSelectionModel(line, CURSOR_LEFT); On 2016/12/16 02:58:38, karandeepb wrote: > On 2016/12/13 03:04:55, msw wrote: > > Perhaps we need to replace CURSOR_LEFT with > > GetVisualDirectionOfLogicalBeginning() and CURSOR_RIGHT below with > > GetVisualDirectionOfLogicalEnd()? That said, I didn't notice an issue here > with > > RTL text in manual testing. > > Think this is correct, segment_index < 0, indicates, the point was before all > the segments and similarly for the other case, it is after all the segments. The > segments for a line are stored in display order (left to right). See > HarfBuzzLineBreaker::AdvanceLine which sorts the segments in display order and > DrawVisualText which draws the segments. Acknowledged. https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:889: : SelectionModel(index, CURSOR_FORWARD); On 2016/12/16 02:58:39, karandeepb wrote: > The behavior of FindCursorPosition still isn't fully correct for cases where > grapheme boundaries lie within a glyph. Eg ffi ligature using Meiryo UI font. > Ideally, we should use the offset within the glyph bound to find the exact > grapheme boundary within the glyph. > > However, this is not a regression. Hence, this should be ok for now. Acknowledged. Please file a bug with enough info (and ideally some examples with pictures) so that someone might tackle this later. https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:714: size_t left_index = is_rtl ? char_range.end() - 1 : char_range.start(); Is checking the first/last char always sufficient? Consider an example where the last two characters are an 'a' and then an accent '`' or similar applied over the 'a' glyph, like 'å'; it seems like the accent's rightmost extent could be to the left of the base glyph's rightmost extent. If the cluster logic in GetGraphemeBounds is sufficient for that case, maybe add a comment here to that effect? https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.h:62: // Returns the width of the given |char_range| handling grapheme boundaries nit: it'd be nice if this were adjacent to GetGraphemeBounds, since the two are fairly related. It'd be even better if we could combine the two (users of GetGraphemeBounds would just construct a Range(text_index), and GetGraphemeWidthForCharRange callers could just get the RangeF::length() of the returned value), but I'm really not sure that makes sense... don't even bother if it's nontrivial work to combine them. https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1793: // Verify that the whole grapheme is selected even if the selection is set Hmm, maybe clarify this comment "Verify that the selection bounds extend over the entire grapheme, even if the selection is set amid the grapheme." If we actually want different behavior (ie. selecting an accent character should actually just draw a selection rectangle around the accent glyph's bounds, not the whole grapheme), then file/cite a bug? https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:183: void SimulatePaint() { nit: add a comment, explain that this is used to ensure the layout https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:192: ->GetRenderTextForSelectionController() optional nit: cache the RenderText pointer
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@. https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/440001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:889: : SelectionModel(index, CURSOR_FORWARD); On 2016/12/19 22:27:43, msw wrote: > On 2016/12/16 02:58:39, karandeepb wrote: > > The behavior of FindCursorPosition still isn't fully correct for cases where > > grapheme boundaries lie within a glyph. Eg ffi ligature using Meiryo UI font. > > Ideally, we should use the offset within the glyph bound to find the exact > > grapheme boundary within the glyph. > > > > However, this is not a regression. Hence, this should be ok for now. > > Acknowledged. Please file a bug with enough info (and ideally some examples with > pictures) so that someone might tackle this later. Done. Filed crbug.com/676287 and added a TODO. https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:714: size_t left_index = is_rtl ? char_range.end() - 1 : char_range.start(); On 2016/12/19 22:27:43, msw wrote: > Is checking the first/last char always sufficient? Consider an example where the > last two characters are an 'a' and then an accent '`' or similar applied over > the 'a' glyph, like 'å'; it seems like the accent's rightmost extent could be to > the left of the base glyph's rightmost extent. If the cluster logic in > GetGraphemeBounds is sufficient for that case, maybe add a comment here to that > effect? This should be sufficient. In the example you cite, GetGraphemeBounds(index for accent) == GetGraphemeBounds(index for a), since GetGraphemeBounds returns the bounds between the grapheme positions corresponding to text_index. For å, grapheme positions are before and after å. Hence bounds |å| will be returned for both a and the accent character. https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.h (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.h:62: // Returns the width of the given |char_range| handling grapheme boundaries On 2016/12/19 22:27:43, msw wrote: > nit: it'd be nice if this were adjacent to GetGraphemeBounds, since the two are > fairly related. It'd be even better if we could combine the two (users of > GetGraphemeBounds would just construct a Range(text_index), and > GetGraphemeWidthForCharRange callers could just get the RangeF::length() of the > returned value), but I'm really not sure that makes sense... don't even bother > if it's nontrivial work to combine them. Now they are adjacent. Removed HasMissingGlyphs which did not have any definition and wasn't being used anywhere. I think we should have a GetGraphemeBounds method which takes a single index, since it makes sense from an API point of view. This can be supplemented by a GetGraphemeBoundsForCharRange or GetGraphemeWidthForCharRange (as done currently). https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1793: // Verify that the whole grapheme is selected even if the selection is set On 2016/12/19 22:27:43, msw wrote: > Hmm, maybe clarify this comment "Verify that the selection bounds extend over > the entire grapheme, even if the selection is set amid the grapheme." > > If we actually want different behavior (ie. selecting an accent character should > actually just draw a selection rectangle around the accent glyph's bounds, not > the whole grapheme), then file/cite a bug? Done. Think selecting the whole grapheme is the correct behavior. https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:183: void SimulatePaint() { On 2016/12/19 22:27:43, msw wrote: > nit: add a comment, explain that this is used to ensure the layout Done. https://codereview.chromium.org/2541313002/diff/480001/ui/views/controls/labe... ui/views/controls/label_unittest.cc:192: ->GetRenderTextForSelectionController() On 2016/12/19 22:27:43, msw wrote: > optional nit: cache the RenderText pointer Done. https://codereview.chromium.org/2541313002/diff/500001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:880: // grapheme position within a multi-grapheme glyph. Just to confirm the terminology I am using is correct. Here is my understanding of the various terms- Grapheme positions - Anything which is a valid cursor position. Grapheme => Anything between consecutive grapheme positions. Glyph => Fonts have mappings from characters to actual images drawn called glyphs. Some fonts may draw á as two separate glyphs, one for a and one for the accent. In this way, a single grapheme can correspond to multiple glyphs as well. Reverse is also possible (see below). Ligature => Multi grapheme glyphs. Eg ffi in Meriryo UI font is drawn as a single glyph, but has 3 graphemes.
lgtm https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/480001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:714: size_t left_index = is_rtl ? char_range.end() - 1 : char_range.start(); On 2016/12/21 14:23:43, karandeepb wrote: > On 2016/12/19 22:27:43, msw wrote: > > Is checking the first/last char always sufficient? Consider an example where > the > > last two characters are an 'a' and then an accent '`' or similar applied over > > the 'a' glyph, like 'å'; it seems like the accent's rightmost extent could be > to > > the left of the base glyph's rightmost extent. If the cluster logic in > > GetGraphemeBounds is sufficient for that case, maybe add a comment here to > that > > effect? > > This should be sufficient. In the example you cite, GetGraphemeBounds(index for > accent) == GetGraphemeBounds(index for a), since GetGraphemeBounds returns the > bounds between the grapheme positions corresponding to text_index. For å, > grapheme positions are before and after å. Hence bounds |å| will be returned for > both a and the accent character. Acknowledged. https://codereview.chromium.org/2541313002/diff/500001/ui/gfx/render_text_har... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2541313002/diff/500001/ui/gfx/render_text_har... ui/gfx/render_text_harfbuzz.cc:880: // grapheme position within a multi-grapheme glyph. On 2016/12/21 14:23:44, karandeepb wrote: > Just to confirm the terminology I am using is correct. Here is my understanding > of the various terms- > > Grapheme positions - Anything which is a valid cursor position. > > Grapheme => Anything between consecutive grapheme positions. > > Glyph => Fonts have mappings from characters to actual images drawn called > glyphs. Some fonts may draw á as two separate glyphs, one for a and one for the > accent. In this way, a single grapheme can correspond to multiple glyphs as > well. Reverse is also possible (see below). > > Ligature => Multi grapheme glyphs. Eg ffi in Meriryo UI font is drawn as a > single glyph, but has 3 graphemes. That's all accurate as far as I know; I was actually just reading http://scripts.sil.org/IWS-Chapter02 to brush up on my terminology and understanding.
Description was changed from
==========
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.
- Enables multi-line text selection for views::Labels.
- Corrects the behavior for RTL when mouse is dragged above/below the text
bounds on MacViews.
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.
==========
to
==========
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.
==========
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #13 (id:540001) 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 msw@chromium.org Link to the patchset: https://codereview.chromium.org/2541313002/#ps560001 (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: 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.
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 Link to the patchset: https://codereview.chromium.org/2541313002/#ps580001 (title: "Fix test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 580001, "attempt_start_ts": 1483495642417910,
"parent_rev": "7dfa2dac2e56f0f01ab51a775fbefc31f9ad9820", "commit_rev":
"cf8cf60306b7d6b9e77bdc2adbd4640fa6da7b37"}
Message was sent while issue was closed.
Description was changed from
==========
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.
==========
to
==========
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
==========
Message was sent while issue was closed.
Committed patchset #14 (id:580001)
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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.
Committed: https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4
Cr-Commit-Position: refs/heads/master@{#441290}
==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/4e4ca175171eb6c46603799904a624e6797773e4 Cr-Commit-Position: refs/heads/master@{#441290} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
