Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18848002:
Shows Japanese and English mixed queries correctly. (Closed)
|
Created:
7 years, 5 months ago by Yuki Modified:
7 years, 5 months ago CC:
chromium-reviews, tfarina, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShows Japanese and English mixed queries correctly.
This CL respects the common height and baseline of the fonts in the given font list, and draw text according to the baseline.
The cause of vertically-misaligned queries was that
1. ASCII characters have 23 pixels in height
2. Japanese characters have 17 pixels in height
pango_layout_get_pixel_size(), which is called in RenderTextLinux::GetStringSize(), returns the above size.
Also see:
https://docs.google.com/a/chromium.org/document/d/1e2n9lEM_usn37Pld8tMeo_qpfLRQQjkx-O1hTHi3sPo/edit?usp=sharing
BUG=244323
TEST=Test manually.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211664
Patch Set 1 #Patch Set 2 : Fixes RenderText::GetAlignmentOffset(). #
Total comments: 2
Patch Set 3 : Switched the way to fix the issue. #Patch Set 4 : Adds a unit test. #
Total comments: 16
Patch Set 5 : Fixes based on review comments. #
Total comments: 10
Patch Set 6 : Fixed code style and comments. #Patch Set 7 : Synced. #Patch Set 8 : Disables the test on Android. #
Messages
Total messages: 19 (0 generated)
Could you review this CL? Cheers, Yuki Shiino
You can't change to VCENTER. I introduced the entire concept of vertical alignment precisely to change this location to TOP. RenderText needs to be calculating a consistent height for different characters in the same font so that, when we position based on the font metrics, the actual character layout for different-height characters happens correctly.
On 2013/07/08 19:20:33, Peter Kasting wrote: > You can't change to VCENTER. I introduced the entire concept of vertical > alignment precisely to change this location to TOP. > > RenderText needs to be calculating a consistent height for different characters > in the same font so that, when we position based on the font metrics, the actual > character layout for different-height characters happens correctly. Okay, I've changed my way to fix RenderText::GetAlignmentOffset() so it respects the specified font size and takes care of the difference of character height between Latin and Japanese characters. If you're okay with this way, I'll add test cases and ask owner review.
https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc#newc... ui/gfx/render_text.cc:861: offset.set_y(display_rect().height() - string_size.height()); I think we should be subtracting the font height here, since your new code suggests that if the two differ, the font height should be larger? This may mean we need to use the font height instead of the string size in some other places, e.g. GetCursorBounds()? Or perhaps using GetFont() isn't right here and we need to get a particular font for a run? You should rope in msw/asvitkine and ask them about all this.
On 2013/07/09 17:28:36, Peter Kasting wrote: > https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc > File ui/gfx/render_text.cc (right): > > https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc#newc... > ui/gfx/render_text.cc:861: offset.set_y(display_rect().height() - > string_size.height()); > I think we should be subtracting the font height here, since your new code > suggests that if the two differ, the font height should be larger? This may > mean we need to use the font height instead of the string size in some other > places, e.g. GetCursorBounds()? Or perhaps using GetFont() isn't right here and > we need to get a particular font for a run? > > You should rope in msw/asvitkine and ask them about all this. I think subtracting the font height wouldn't work well. I wrote a memo to explain what I'm thinking. https://docs.google.com/a/chromium.org/document/d/1e2n9lEM_usn37Pld8tMeo_qpfL... To msw, asvitkine Could you take a look on this CL? I'd like to hear you guys' thought.
After reading your doc I realize that your change doesn't actually fix the problem of text shifting around when someone adds latin characters, it just reduces it by half. I don't think that's the right answer. It seems like if we're positioning the overall text rect we should use the largest size to do so (which seems like the font height), and then inside that rect, individual pieces that have different string heights should be bottom- or baseline-aligned. This would mean the glyphs in this case would never shift around. I await the experts' reactions here, though.
On 2013/07/10 18:29:01, Peter Kasting wrote: > After reading your doc I realize that your change doesn't actually fix the > problem of text shifting around when someone adds latin characters, it just > reduces it by half. I don't think that's the right answer. > > It seems like if we're positioning the overall text rect we should use the > largest size to do so (which seems like the font height), and then inside that > rect, individual pieces that have different string heights should be bottom- or > baseline-aligned. This would mean the glyphs in this case would never shift > around. > > I await the experts' reactions here, though. Peter's latest suggestion here seems the most correct. Try adjusting individual PangoLayoutRun vertical alignments in RenderTextLinux::DrawVisualText. Simply using bottom- or center-alignment by comparing string height to font height seems sub-optimal. A common baseline should be found if possible, but I suppose bottom-alignment as suggested by your doc is better than the status-quo. This problem is also present on RenderTextWin (via NativeTextfieldViews) and NativeTextfieldWin. I would appreciate your help solving this issue for RenderTextWin as well, but encourage you to at least file a bug for me to fix that when I have time (or expand the scope of Issue 244323 and reassign that to me once you have a Linux/Pango fix).
Thanks for all the comments and suggestions. I've completely changed a way as you guys suggested. Could you take another look? I'll fix render_text_win.cc as well after this CL. https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/18848002/diff/4001/ui/gfx/render_text.cc#newc... ui/gfx/render_text.cc:861: offset.set_y(display_rect().height() - string_size.height()); On 2013/07/09 17:28:36, Peter Kasting wrote: > I think we should be subtracting the font height here, since your new code > suggests that if the two differ, the font height should be larger? This may > mean we need to use the font height instead of the string size in some other > places, e.g. GetCursorBounds()? Or perhaps using GetFont() isn't right here and > we need to get a particular font for a run? > > You should rope in msw/asvitkine and ask them about all this. Done.
Does this actually fix the issue? I would have thought that RenderTextLinux::DrawVisualText would actually need per-run changes to the vertical positioning depending on it's height/baseline versus the common height/baseline. But if this is all it takes, it looks like a great fix. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:159: return ascent + descent; Does this meaningfully differ from max(i->GetHeight())? Why is this better? i.e. does max(h) =?= max(b) + max(h-b) in this case? https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:164: nit: remove blank line. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:352: font_list_height_ = font_list_.GetHeight(); Init these values to 0 in the ctor. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:505: int font_list_height_; Can FontList cache these values itself? https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:86: // pango_layout_get_pixel_size() returns the minimal size to render the given DCHECK that |height| is smaller, and just return GetFontListHeight(). https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:103: return std::max(PANGO_PIXELS(pango_layout_get_baseline(layout_)), DCHECK that the pango value is smaller, and just return GetFontListBaseline().
Thanks for the review. We don't need per-run tweaks. If the text is mixture of several fonts/scripts, PangoLayout takes care of the alignment and positioning of them. We only need to take care the baseline shift of the whole text. I've moved the cache to FontList. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:159: return ascent + descent; On 2013/07/11 22:47:29, msw wrote: > Does this meaningfully differ from max(i->GetHeight())? Why is this better? > i.e. does max(h) =?= max(b) + max(h-b) in this case? I supposed there could be two fonts. Spec of Font A: ascent = 5, descent = 0, height = 5 Spec of Font B: ascent = 0, descent = 5, height = 5 In this case, we need 5 pixels for ascent and another 5 pixels for descent to draw text on the same baseline. So font height must be 5 + 5 = 10 pixels, and baseline must be 5 pixels. In order to respect the baseline, we should take care ascent and descent separately. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:164: On 2013/07/11 22:47:29, msw wrote: > nit: remove blank line. Done. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.cc#new... ui/gfx/render_text.cc:352: font_list_height_ = font_list_.GetHeight(); On 2013/07/11 22:47:29, msw wrote: > Init these values to 0 in the ctor. Done. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text.h#newc... ui/gfx/render_text.h:505: int font_list_height_; On 2013/07/11 22:47:29, msw wrote: > Can FontList cache these values itself? Yes, I've moved these cache to FontList. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:86: // pango_layout_get_pixel_size() returns the minimal size to render the given On 2013/07/11 22:47:29, msw wrote: > DCHECK that |height| is smaller, and just return GetFontListHeight(). I'm not sure if it's guaranteed that the pango value is not greater than the font size. What happens if... 1. You specified only Latin font. (e.g. Arial) 2. You drew non-Latin characters. (e.g. Japanese "あ") and, what happens especially if 1-b. You specified 10px Arial 2-b. You don't have 10px (or smaller) Japanese font. If the minimum size of Japanese font were 20px, what happens? I'm afraid that Pango would use 20px Japanese font even though you specified 10px Arial. I thought there is no guarantee. You may have only fixed-size font (no scalable font) and may not be able to draw some text in the specified size. I thought the font metrics is just a hint. I think it's safe to return the max of font height or actual pango value. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:103: return std::max(PANGO_PIXELS(pango_layout_get_baseline(layout_)), On 2013/07/11 22:47:29, msw wrote: > DCHECK that the pango value is smaller, and just return GetFontListBaseline(). Ditto.
LGTM with nits and Qs; thanks! https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:159: return ascent + descent; On 2013/07/12 08:25:53, Yuki wrote: > On 2013/07/11 22:47:29, msw wrote: > > Does this meaningfully differ from max(i->GetHeight())? Why is this better? > > i.e. does max(h) =?= max(b) + max(h-b) in this case? > > I supposed there could be two fonts. > Spec of Font A: ascent = 5, descent = 0, height = 5 > Spec of Font B: ascent = 0, descent = 5, height = 5 > In this case, we need 5 pixels for ascent and another 5 pixels for descent to > draw text on the same baseline. So font height must be 5 + 5 = 10 pixels, and > baseline must be 5 pixels. > > In order to respect the baseline, we should take care ascent and descent > separately. Perfect example and explanation, thanks! But text may still jarringly vertically re-align when input yields new, e.g. taller font. Should FontList or some user anticipate fonts by locale or past/system languages? Do you think that's worth filing a bug or adding a TODO? https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:86: // pango_layout_get_pixel_size() returns the minimal size to render the given On 2013/07/12 08:25:53, Yuki wrote: > On 2013/07/11 22:47:29, msw wrote: > > DCHECK that |height| is smaller, and just return GetFontListHeight(). > > I'm not sure if it's guaranteed that the pango value is not greater than the > font size. > > What happens if... > 1. You specified only Latin font. (e.g. Arial) > 2. You drew non-Latin characters. (e.g. Japanese "あ") > and, what happens especially if > 1-b. You specified 10px Arial > 2-b. You don't have 10px (or smaller) Japanese font. If the minimum size of > Japanese font were 20px, what happens? > > I'm afraid that Pango would use 20px Japanese font even though you specified > 10px Arial. > > I thought there is no guarantee. You may have only fixed-size font (no scalable > font) and may not be able to draw some text in the specified size. I thought > the font metrics is just a hint. > > I think it's safe to return the max of font height or actual pango value. Fair enough; I thought FontLists might track fallback fonts, but I was wrong. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:73: : common_height_(-1), common_baseline_(-1) { This should fit on the line above. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:79: common_height_(-1), common_baseline_(-1) { The style guide recommends one initializer per line when the whole signature doesn't fit on a single line. That may apply to the ctors below as well: www.corp.google.com/eng/doc/cppguide.xml#Constructor_Initializer_Lists https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.h#newcode97 ui/gfx/font_list.h:97: // We cache these values so we don't need to scan font metrics for all the I think this sentence is unnecessary, just say "Cached common height..." above. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list_unittest.cc File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list_unittest... ui/gfx/font_list_unittest.cc:251: // Bypass DCHECK in a constructor of FontList which checks that all the fonts Hmm, if FontList behavior for mixed font sizes is okay, remove the DCHECK? https://codereview.chromium.org/18848002/diff/22001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:86: // pango_layout_get_pixel_size() returns the minimal size to render the given nit: these could be more concise; like "Keep a consistent [height|baseline] between this particular string's PangoLayout and potentially larger text supported by the FontList."
Thanks for the review. If you really want a TODO comment, I'm happy to add it, but I don't think we need it. https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:159: return ascent + descent; On 2013/07/12 09:36:04, msw wrote: > On 2013/07/12 08:25:53, Yuki wrote: > > On 2013/07/11 22:47:29, msw wrote: > > > Does this meaningfully differ from max(i->GetHeight())? Why is this better? > > > i.e. does max(h) =?= max(b) + max(h-b) in this case? > > > > I supposed there could be two fonts. > > Spec of Font A: ascent = 5, descent = 0, height = 5 > > Spec of Font B: ascent = 0, descent = 5, height = 5 > > In this case, we need 5 pixels for ascent and another 5 pixels for descent to > > draw text on the same baseline. So font height must be 5 + 5 = 10 pixels, and > > baseline must be 5 pixels. > > > > In order to respect the baseline, we should take care ascent and descent > > separately. > > Perfect example and explanation, thanks! But text may still jarringly vertically > re-align when input yields new, e.g. taller font. Should FontList or some user > anticipate fonts by locale or past/system languages? Do you think that's worth > filing a bug or adding a TODO? I'm optimistic for that case. Most of Latin fonts have enough space above and below the baseline. I don't remember any character needs extra space. If a FontList has only a Japanese font (smaller), and the text includes Latin characters (taller), the problem occurs. However, I think it's better to add your favorite Latin font to the FontList rather than anticipating some default fonts. For better UX, we should choose the best Latin font for Japanese text. At least, Serif or Sans Serif matters. Summary 1. Most users type/read Latin characters in addition to their native language. 2. We should specify Latin font in addition to their native language's font. 3. Latin font should have enough space. I don't think it's worth to file a bug or add a TODO. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:73: : common_height_(-1), common_baseline_(-1) { On 2013/07/12 09:36:04, msw wrote: > This should fit on the line above. Done. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:79: common_height_(-1), common_baseline_(-1) { On 2013/07/12 09:36:04, msw wrote: > The style guide recommends one initializer per line when the whole signature > doesn't fit on a single line. That may apply to the ctors below as well: > http://www.corp.google.com/eng/doc/cppguide.xml#Constructor_Initializer_Lists Good to know. Done. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list.h#newcode97 ui/gfx/font_list.h:97: // We cache these values so we don't need to scan font metrics for all the On 2013/07/12 09:36:04, msw wrote: > I think this sentence is unnecessary, just say "Cached common height..." above. Done. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list_unittest.cc File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/font_list_unittest... ui/gfx/font_list_unittest.cc:251: // Bypass DCHECK in a constructor of FontList which checks that all the fonts On 2013/07/12 09:36:04, msw wrote: > Hmm, if FontList behavior for mixed font sizes is okay, remove the DCHECK? Technically we can remove it, but I'm not sure if we really want to remove it. What's good for having different size fonts? The original problem is that, Japanese font and Latin font have the same font size, but Pango returns different height actually. This DCHECK may be useful to avoid accidentally specifying different size fonts. https://codereview.chromium.org/18848002/diff/22001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): https://codereview.chromium.org/18848002/diff/22001/ui/gfx/render_text_linux.... ui/gfx/render_text_linux.cc:86: // pango_layout_get_pixel_size() returns the minimal size to render the given On 2013/07/12 09:36:04, msw wrote: > nit: these could be more concise; like "Keep a consistent [height|baseline] > between this particular string's PangoLayout and potentially larger text > supported by the FontList." Thanks.
https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc File ui/gfx/font_list.cc (right): https://codereview.chromium.org/18848002/diff/14001/ui/gfx/font_list.cc#newco... ui/gfx/font_list.cc:159: return ascent + descent; On 2013/07/12 10:29:24, Yuki wrote: > On 2013/07/12 09:36:04, msw wrote: > > On 2013/07/12 08:25:53, Yuki wrote: > > > On 2013/07/11 22:47:29, msw wrote: > > > > Does this meaningfully differ from max(i->GetHeight())? Why is this > better? > > > > i.e. does max(h) =?= max(b) + max(h-b) in this case? > > > > > > I supposed there could be two fonts. > > > Spec of Font A: ascent = 5, descent = 0, height = 5 > > > Spec of Font B: ascent = 0, descent = 5, height = 5 > > > In this case, we need 5 pixels for ascent and another 5 pixels for descent > to > > > draw text on the same baseline. So font height must be 5 + 5 = 10 pixels, > and > > > baseline must be 5 pixels. > > > > > > In order to respect the baseline, we should take care ascent and descent > > > separately. > > > > Perfect example and explanation, thanks! But text may still jarringly > vertically > > re-align when input yields new, e.g. taller font. Should FontList or some user > > anticipate fonts by locale or past/system languages? Do you think that's worth > > filing a bug or adding a TODO? > > I'm optimistic for that case. Most of Latin fonts have enough space above and > below the baseline. I don't remember any character needs extra space. > > If a FontList has only a Japanese font (smaller), and the text includes Latin > characters (taller), the problem occurs. However, I think it's better to add > your favorite Latin font to the FontList rather than anticipating some default > fonts. For better UX, we should choose the best Latin font for Japanese text. > At least, Serif or Sans Serif matters. > > Summary > 1. Most users type/read Latin characters in addition to their native language. > 2. We should specify Latin font in addition to their native language's font. > 3. Latin font should have enough space. > > I don't think it's worth to file a bug or add a TODO. sgtm, we can just followup if any actual defects are observed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/18848002/31001
So does this also fix the bug on Windows, or is that still broken?
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
On 2013/07/12 20:15:27, Peter Kasting wrote: > So does this also fix the bug on Windows, or is that still broken? Sorry for the late reply. This CL fixes the issue on Chrome OS and GNU/Linux (i.e. Pango), and we need extra work for Windows. I'll work on it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/18848002/58001
Message was sent while issue was closed.
Change committed as 211664 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
