Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
371413002:
Don't split text runs on underline style change (Closed)
|
Created:
6 years, 5 months ago by Tomasz Moniuszko Modified:
6 years, 3 months ago Reviewers:
msw CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDon't split text runs on underline style change
BUG=392154
Committed: https://crrev.com/a7fd79d2cddafb635489bd5a2a65dc647af6a927
Cr-Commit-Position: refs/heads/master@{#295444}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Don't split text runs on underline style change (with code review fixes) #Patch Set 3 : Don't split text runs on underline style change (CharRangeToGlyphRange fixed) #Patch Set 4 : Avoid applying style change in the middle of grapheme #
Total comments: 3
Patch Set 5 : Avoid applying style change in the middle of grapheme #Patch Set 6 : Avoid breaking text runs in the middle of grapheme #Patch Set 7 : Avoid applying style change mid-grapheme #
Total comments: 6
Patch Set 8 : Avoid applying style change mid-grapheme #Patch Set 9 : Avoid applying style change mid-grapheme #Messages
Total messages: 32 (3 generated)
Does this defect still exist with --enable-harfbuzz-rendertext? Are the menu mnemonic key underline ranges actually correct? (ie. the range doesn't include surrogates/accents, but should) https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:894: position < intersection.end(); ) { nit: the indent here should match the inside of the parentheses above. https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) nit: add {} https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) Why do we get an empty glyph range for something that should be valid? https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:902: decorated_glyphs.set_start(decorated_glyphs.end() - 1); Hmm, adjusting the range by one glyph seems odd, what's going on here? https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:968: std::vector<BreakList<bool> > breaking_styles = styles(); nit: add a comment like the one above for colors (or extend that to mention the reason for modifying the underline BreakList).
On 2014/07/08 17:20:54, msw wrote: > Does this defect still exist with --enable-harfbuzz-rendertext? Yes, it does. What is more, rectangles are drawn instead of font glyphs on my machine. But I think this can be caused by missing fonts. > Are the menu mnemonic key underline ranges actually correct? > (ie. the range doesn't include surrogates/accents, but should) Native Hindi speaker confirmed that ranges are correct. Unfortunately I don't know Hindi and I'm not able to verify that.
https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:894: position < intersection.end(); ) { On 2014/07/08 17:20:53, msw wrote: > nit: the indent here should match the inside of the parentheses above. Done. https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) On 2014/07/08 17:20:53, msw wrote: > nit: add {} Done. https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) On 2014/07/08 17:20:53, msw wrote: > Why do we get an empty glyph range for something that should be valid? We can get empty glyph range for non-empty character range when script is complex. Example from ScriptShape() documentation (http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564(v=vs.85).aspx): Character array, where c<n>u<m> means cluster n, Unicode code point m: | c1u1 | c2u1 | c3u1 c3u2 c3u3 | c4u1 c4u2 | Glyph array, where c<n>g<m> means cluster n, glyph m: | c1g1 | c2g1 c2g2 c2g3 | c3g1 | c4g1 c4g2 c4g3 | Cluster array, that is, the offset (in glyphs) to the cluster containing the character: | 0 | 1 | 4 4 4 | 5 5 | With this example CharRangeToGlyphRange() would return glyph range [4, 4) (empty) for character range [3, 4) (non-empty). https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:902: decorated_glyphs.set_start(decorated_glyphs.end() - 1); On 2014/07/08 17:20:53, msw wrote: > Hmm, adjusting the range by one glyph seems odd, what's going on here? Yes, I agree. The issue is that BreakList contains some underline range (in this case the range consists of single underlined character which is a keyboard accelerator) but we get an empty glyph range for this range. BreakList indicates that we should underline something but glyph range is empty so there's nothing to underline. I adjust the glyph range so one glyph gets underlined.
https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) On 2014/07/16 10:35:45, Tomasz Moniuszko wrote: > On 2014/07/08 17:20:53, msw wrote: > > Why do we get an empty glyph range for something that should be valid? > > We can get empty glyph range for non-empty character range when script is > complex. > > Example from ScriptShape() documentation > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564(v=vs.85).aspx): > > Character array, where c<n>u<m> means cluster n, Unicode code point m: > | c1u1 | c2u1 | c3u1 c3u2 c3u3 | c4u1 c4u2 | > Glyph array, where c<n>g<m> means cluster n, glyph m: > | c1g1 | c2g1 c2g2 c2g3 | c3g1 | c4g1 c4g2 c4g3 | > Cluster array, that is, the offset (in glyphs) to the cluster containing the > character: > | 0 | 1 | 4 4 4 | 5 5 | > > With this example CharRangeToGlyphRange() would return glyph range [4, 4) > (empty) for character range [3, 4) (non-empty). I suspect that RenderTextWin::CharRangeToGlyphRange is wrong and needs a fix, like Cem's http://crrev.com/275754 for RenderTextHarfBuzz. WDYT?
On 2014/07/16 19:52:39, msw wrote: > https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc > File ui/gfx/render_text_win.cc (right): > > https://codereview.chromium.org/371413002/diff/1/ui/gfx/render_text_win.cc#ne... > ui/gfx/render_text_win.cc:900: if (decorated_glyphs.is_empty()) > On 2014/07/16 10:35:45, Tomasz Moniuszko wrote: > > On 2014/07/08 17:20:53, msw wrote: > > > Why do we get an empty glyph range for something that should be valid? > > > > We can get empty glyph range for non-empty character range when script is > > complex. > > > > Example from ScriptShape() documentation > > > (http://msdn.microsoft.com/en-us/library/windows/desktop/dd368564(v=vs.85).aspx): > > > > Character array, where c<n>u<m> means cluster n, Unicode code point m: > > | c1u1 | c2u1 | c3u1 c3u2 c3u3 | c4u1 c4u2 | > > Glyph array, where c<n>g<m> means cluster n, glyph m: > > | c1g1 | c2g1 c2g2 c2g3 | c3g1 | c4g1 c4g2 c4g3 | > > Cluster array, that is, the offset (in glyphs) to the cluster containing the > > character: > > | 0 | 1 | 4 4 4 | 5 5 | > > > > With this example CharRangeToGlyphRange() would return glyph range [4, 4) > > (empty) for character range [3, 4) (non-empty). > > I suspect that RenderTextWin::CharRangeToGlyphRange is wrong and needs a fix, > like Cem's http://crrev.com/275754 for RenderTextHarfBuzz. WDYT? I fixed CharRangeToGlyphRange so it now returns non-empty glyph range whenever char range is non-empty.
On 2014/07/16 10:35:33, Tomasz Moniuszko wrote: > On 2014/07/08 17:20:54, msw wrote: > > Does this defect still exist with --enable-harfbuzz-rendertext? > > Yes, it does. What is more, rectangles are drawn instead of font glyphs on my > machine. But I think this can be caused by missing fonts. Do you think I should apply this solution also to RenderTextHarfBuzz?
Hi msw, Do you think this patch is ready for integration now? Should RederTextHarfBuzz also be fixed before integration? Best Regards, Tomasz
I think this is the wrong approach. The underline range supplied for the single character "ब" should should apply to the entire grapheme "बु", then run breaking should be okay. Yes, we will need a similar fix for RenderTextHarfBuzz. Perhaps in the long-term, we'll avoid run-breaking for decorations and draw them in a separate pass from text rendering altogether, using GetSubstringBounds (as long as it respects text color ranges, etc.)
On 2014/09/02 20:18:21, msw wrote: > I think this is the wrong approach. The underline range supplied for the single > character "ब" should should apply to the entire grapheme "बु", then run breaking > should be okay. Do you mean something I did in Patch Set 4? I tried to correct index in RenderTextWin::ItemizeLogicalText() but RenderText::IndexOfAdjacentGrapheme() calls EnsureLayout() which shouldn't be called from ItemizeLogicalText(). Range correction from RenderText::ApplyStyle() works also for RenderTextHarfBuzz. Do you think it's a good place to do this?
On 2014/09/03 14:25:02, Tomasz Moniuszko wrote: > On 2014/09/02 20:18:21, msw wrote: > > I think this is the wrong approach. The underline range supplied for the > single > > character "ब" should should apply to the entire grapheme "बु", then run > breaking > > should be okay. > > Do you mean something I did in Patch Set 4? > > I tried to correct index in RenderTextWin::ItemizeLogicalText() but > RenderText::IndexOfAdjacentGrapheme() calls EnsureLayout() which shouldn't be > called from ItemizeLogicalText(). Range correction from RenderText::ApplyStyle() > works also for RenderTextHarfBuzz. Do you think it's a good place to do this? Yeah, actually Patch Set 4's ApplyStyle fixup seem pretty good, nice work. Can anyone CC'ed imagine a case where we really want to apply a style (bold, italic, underline, strike, etc.) to a limited portion of a grapheme? Or does anyone see any flaws in the proposed use of IndexOfAdjacentGrapheme? I can't think of anything offhand. Tomasz, could you add a unittest for this behavior? https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:654: // Don't change style in the middle of grapheme to avoid breaking of ligature. nit: "Do not change styles mid-grapheme to avoid breaking ligatures." https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:655: const size_t corrected_end = We should only correct the range bounds as needed: size_t end = IsValidCursorIndex(range.end()) ? range.end() : IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD); https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... ui/gfx/render_text.cc:657: styles_[style].ApplyValue(value, Range(range.start(), corrected_end)); We should also correct the range start (using CURSOR_BACKWARD).
On 2014/09/03 19:17:33, msw wrote: > On 2014/09/03 14:25:02, Tomasz Moniuszko wrote: > > On 2014/09/02 20:18:21, msw wrote: > > > I think this is the wrong approach. The underline range supplied for the > > single > > > character "ब" should should apply to the entire grapheme "बु", then run > > breaking > > > should be okay. > > > > Do you mean something I did in Patch Set 4? > > > > I tried to correct index in RenderTextWin::ItemizeLogicalText() but > > RenderText::IndexOfAdjacentGrapheme() calls EnsureLayout() which shouldn't be > > called from ItemizeLogicalText(). Range correction from > RenderText::ApplyStyle() > > works also for RenderTextHarfBuzz. Do you think it's a good place to do this? > > Yeah, actually Patch Set 4's ApplyStyle fixup seem pretty good, nice work. Can > anyone CC'ed imagine a case where we really want to apply a style (bold, italic, > underline, strike, etc.) to a limited portion of a grapheme? Or does anyone see > any flaws in the proposed use of IndexOfAdjacentGrapheme? I can't think of > anything offhand. > > Tomasz, could you add a unittest for this behavior? > > https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc > File ui/gfx/render_text.cc (right): > > https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... > ui/gfx/render_text.cc:654: // Don't change style in the middle of grapheme to > avoid breaking of ligature. > nit: "Do not change styles mid-grapheme to avoid breaking ligatures." > > https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... > ui/gfx/render_text.cc:655: const size_t corrected_end = > We should only correct the range bounds as needed: > size_t end = IsValidCursorIndex(range.end()) ? range.end() : > IndexOfAdjacentGrapheme(range.end(), CURSOR_FORWARD); > > https://codereview.chromium.org/371413002/diff/60001/ui/gfx/render_text.cc#ne... > ui/gfx/render_text.cc:657: styles_[style].ApplyValue(value, Range(range.start(), > corrected_end)); > We should also correct the range start (using CURSOR_BACKWARD). Unfortunately this approach doesn't work properly in all cases. Everything is OK when text is first set and styles are applied then. But this solution doesn't work if the text is changed when styles are already applied. Style ranges are adjusted in RenderText::SetText() to accommodate a new text length only but style ranges which were valid for the old text may be not valid for the new one. Currently RenderTextTest.ApplyColorAndStyle test fails because BOLD and ITALIC styles are already there when complex text is being set in order to test UNDERLINE style. The test succeeds when I clear BOLD and ITALIC styles before applying UNDERLINE style. I see two possible solutions to solve this problem: 1. Clear styles every time the text is changed in RenderText::SetText(). It might be OK if style applied to whole text is preserved but I guess style ranges are invalid if new text is not a substring of (or doesn't contain) the old text. OR 2. Avoid breaking texts in the middle of grapheme in RenderTextWin::ItemizeLogicalText() (and in similar method in RenderTextHarfBuzz). This would be a combination of my original idea from Patch Set 1 and the idea of non-breaking grapheme from Patch Set 5. Could you please let me know what do you think about this?
On 2014/09/04 14:46:43, Tomasz Moniuszko wrote: > Unfortunately this approach doesn't work properly in all cases. Everything is OK > when text is first set and styles are applied then. But this solution doesn't > work if the text is changed when styles are already applied. Style ranges are > adjusted in RenderText::SetText() to accommodate a new text length only but > style ranges which were valid for the old text may be not valid for the new one. > > Currently RenderTextTest.ApplyColorAndStyle test fails because BOLD and ITALIC > styles are already there when complex text is being set in order to test > UNDERLINE style. The test succeeds when I clear BOLD and ITALIC styles before > applying UNDERLINE style. > > I see two possible solutions to solve this problem: > 1. Clear styles every time the text is changed in RenderText::SetText(). It > might be OK if style applied to whole text is preserved but I guess style ranges > are invalid if new text is not a substring of (or doesn't contain) the old text. > OR > 2. Avoid breaking texts in the middle of grapheme in > RenderTextWin::ItemizeLogicalText() (and in similar method in > RenderTextHarfBuzz). This would be a combination of my original idea from Patch > Set 1 and the idea of non-breaking grapheme from Patch Set 5. > > Could you please let me know what do you think about this? Let's do (2), unless it is prohibitively difficult or complex.
Patch set 5 + either solution sounds good to me. Mike, is there a reason we're not resetting styles when the text is changed? Is that a useful property?
On 2014/09/04 18:12:50, ckocagil wrote: > Patch set 5 + either solution sounds good to me. > > Mike, is there a reason we're not resetting styles when the text is changed? Is > that a useful property? I'm not sure if anything sets the styles then adjusts the text... We can try clearing styles on setting text, but it might break something. It seems like regardless, we shouldn't break runs mid-grapheme.
On 2014/09/04 18:17:03, msw wrote: > On 2014/09/04 18:12:50, ckocagil wrote: > > Patch set 5 + either solution sounds good to me. > > > > Mike, is there a reason we're not resetting styles when the text is changed? > Is > > that a useful property? > > I'm not sure if anything sets the styles then adjusts the text... > We can try clearing styles on setting text, but it might break something. > It seems like regardless, we shouldn't break runs mid-grapheme. This patchset + resetting styles should prevent mid-grapheme breaks. But since it's an API change we can easily break something, so it will probably be harder to correctly implement than (2).
Patch Set 6 is the solution (2) applied to RenderTextWin. There are some drawbacks of this approach. I think that the biggest issue here is that we need to call LayoutTextRun() and ScriptPlace() because GetGlyphXBoundary() relies on TextRun data members and fails otherwise. These methods are called later from LayoutVisualText() anyway so everything is calculated twice. I'm using |temp_run| object here. I tried to use |run| object which is already created but it caused RenderTextTest.SameFontForParentheses() test to fail. I haven't investigated this issue much and it's possible that temp_run can be avoided if you decide to go with this solution anyway. Also Patch Set 5 seems to be redundant if this solution is applied. Text is itemized in such way that runs aren't broken in the middle of grapheme regardless of style ranges. I think that correcting style ranges in RenderText::ApplyStyle() but allowing style ranges to be invalid after text is changed in RenderText::SetText() is quite inconsistent. What do you think about this solution? Do you have any idea how to avoid calling call LayoutTextRun() and ScriptPlace() from RenderTextWin::ItemizeLogicalText()?
On 2014/09/05 14:21:08, Tomasz Moniuszko wrote: > Patch Set 6 is the solution (2) applied to RenderTextWin. > > There are some drawbacks of this approach. I think that the biggest issue here > is that we need to call LayoutTextRun() and ScriptPlace() because > GetGlyphXBoundary() relies on TextRun data members and fails otherwise. These > methods are called later from LayoutVisualText() anyway so everything is > calculated twice. > > I'm using |temp_run| object here. I tried to use |run| object which is already > created but it caused RenderTextTest.SameFontForParentheses() test to fail. I > haven't investigated this issue much and it's possible that temp_run can be > avoided if you decide to go with this solution anyway. > > Also Patch Set 5 seems to be redundant if this solution is applied. Text is > itemized in such way that runs aren't broken in the middle of grapheme > regardless of style ranges. I think that correcting style ranges in > RenderText::ApplyStyle() but allowing style ranges to be invalid after text is > changed in RenderText::SetText() is quite inconsistent. > > What do you think about this solution? Do you have any idea how to avoid calling > call LayoutTextRun() and ScriptPlace() from RenderTextWin::ItemizeLogicalText()? Dang, let's avoid (2) for now, due to the complications you encountered. Instead, try "clearing [ranged] styles on SetText" (1), if that's simple. Sorry for the trouble :( My reasoning: GetGlyphXBoundary is only required to determine grapheme boundary indices in RenderTextWin, afaik. We can enforce "not breaking runs on mid-grapheme indices" (2) in RenderTextHarfBuzz without that complication, but let's delay that more comprehensive fix until RenderTextHarfBuzz ships and we delete the other RenderText implementations. I'd prefer to avoid incurring performance regressions with this change to RenderTextWin, and would prefer to keep parity between the RenderText implementations (where reasonable) for now. Alternately, We could potentially just fix (2) for RenderTextHarfBuzz, and hope to land that for M-39 (or encourage users experiencing this defect to use --enable-harfbuzz-rendertext).
> Instead, try "clearing [ranged] styles on SetText" (1), if that's simple. Done in Patch Set 7. Style from the first break is applied to the whole text now.
Looks pretty good; just some minor comments. https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:420: // Clear style ranges as they might break new text graphemes. nit: Merge this comment with the one on line 414. https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:421: // Apply first style to the whole text instead. nit: "Apply the first". https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:423: break_list.SetValue(break_list.breaks().begin()->second); Do this unconditionally, before calling SetMax.
https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:420: // Clear style ranges as they might break new text graphemes. On 2014/09/08 19:29:34, msw wrote: > nit: Merge this comment with the one on line 414. Done. https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:421: // Apply first style to the whole text instead. On 2014/09/08 19:29:34, msw wrote: > nit: "Apply the first". Done. https://codereview.chromium.org/371413002/diff/120001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:423: break_list.SetValue(break_list.breaks().begin()->second); On 2014/09/08 19:29:34, msw wrote: > Do this unconditionally, before calling SetMax. Done.
Excellent, thank you for your help and patience. LGTM
The CQ bit was checked by tmoniuszko@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tmoniuszko@opera.com/371413002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2014/09/10 09:07:59, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_rel_swarming on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) I disabled the failing part of the test on Mac because RenderTextMac::IsValidCursorIndex method currently doesn't check anything more than IsValidLogicalIndex.
The CQ bit was checked by tmoniuszko@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/371413002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as cd4ff0d3609e32bdf44da9588e12bc1e092534e6
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a7fd79d2cddafb635489bd5a2a65dc647af6a927 Cr-Commit-Position: refs/heads/master@{#295444} |
