Closed Bug 425004 Opened 17 years ago Closed 8 years ago

Text descenders of "g, j, p, q, y" are hidden when using some fonts

Categories

(Core :: Layout: Text and Fonts, defect)

x86
Linux
defect
Not set
major

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: t.matsuu, Assigned: karlt)

References

(Depends on 2 open bugs)

Details

Attachments

(8 files, 2 obsolete files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre If certain fonts are used for desktop or web contents, text descenders of "g, j, p, q, y" are hidden. As far as I tested, the following fonts affect to this bug. IPA明朝,IPAMincho IPA P明朝,IPAPMincho IPAゴシック,IPAGothic IPA Pゴシック,IPAPGothic IPA UIゴシック,IPAUIGothic IPA X0208 明朝,IPAX0208Mincho IPA X0208 P明朝,IPAX0208PMincho IPA X0208 ゴシック,IPAX0208Gothic IPA X0208 Pゴシック,IPAX0208PGothic IPA X0208 UIゴシック,IPAX0208UIGothic available from: http://ossipedia.ipa.go.jp/ipafont/ (Information only in Japanese is available. But we can redistribute them if no modification is applied.) The fonts above are higher quality Japanese fonts than ones (ie. Sazanami-mincho, Sazanami-gothic font etc.) which are distributed by most Linux distributors. So some users prefer to use them and should be fixed before released. Reproducible: Always Mozilla-gumi bugzilla: http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=6083 Screenshot of gedit and Minefield with ja_JP.UTF-8 locale http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3707 Screenshot of gedit and Minefield with C locale http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3708 The height of URL bar is 2 dots shorter than the search dialog in gedit when Minefield is run with ja_JP.UTF-8 locale. Text descenders are shown if the input area is scrolled by using mouse. Work status: 2008020718 OK 2008021119 NG bonsai: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-02-07+18%3A00%3A00&maxdate=2008-02-11+19%3A59%3A59&cvsroot=%2Fcvsroot
Flags: blocking-firefox3?
The problem is not occured if firefox is run as the following locales: LANG=C firefox LANG=en_US.UTF-8 firefox LANG=it_IT.UTF-8 firefox And it is occured if firefox is run as the following locales: LANG=ja_JP.UTF-8 firefox LANG=ko_KR.UTF-8 firefox LANG=zh_CN.UTF-8 firefox Is the calculation of the font height different between CJK locale and non-CJK ones?
Attached file testcase (deleted) —
Attached image screenshot of the testcase (deleted) —
font-family: "Bitstream Vera Sans", "IPAPGothic"; is good, but sans-serif (fontconfig switches fonts) is not good.
Attached file my ~/.fonts.conf (deleted) —
This ~/.fonts.conf is used when previous screenshot taken. This is similar to the style "font-family: "Bitstream Vera Sans", "IPAPGothic";" but the height of input is different.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Layout: Form Controls
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: general → layout.form-controls
(Transferring MATSUURA's blocking request from firefox3 to 1.9.)
Flags: blocking1.9?
Is this actually a form controls issue? Or would a <span> with such text in it also not extend far enough? The latter seems much more likely to me....
Atsushi reported the more refined regression period at Mozilla-gumi bugzilla. 2008021004 is good and 2008021104 is NOT good. And he has suggetset this is the regression of the following checkin: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&date=explicit&mindate=2008-02-10+22%3A45&maxdate=2008-02-10+22%3A45&cvsroot=%2Fcvsroot I have reverted the bonsai checkin above and built. Attachment 317770 [details] shows properly but it is not affected to URL bar.
Attached file <span> testcase (deleted) —
Attached image screenshot of <span> testcase (deleted) —
(In reply to comment #6) > Is this actually a form controls issue? Or would a <span> with such text in it > also not extend far enough? The latter seems much more likely to me.... outline of <span> is good. But, background-color of <span> is sometimes narrow.
Attachment #312030 - Attachment mime type: text/html → text/html; charset=Shift_JIS
OK, so not a form control issue.
Component: Layout: Form Controls → Layout: Fonts and Text
QA Contact: layout.form-controls → layout.fonts-and-text
I don't think we can block on this. However, Karl can probably look at this if priorities allow.
-'ing based on Comment #12.
Flags: blocking1.9? → blocking1.9-
Assignee: nobody → mozbugz
Status: NEW → ASSIGNED
Blocks: 349259
Blocks: 481751
Blocks: 483558
Attached patch patch (obsolete) (deleted) — Splinter Review
This fixes this bug and bug 481751 by using a natural baseline for the text control frames and setting the initial scroll position of the content so that the baselines match. This patch doesn't fix bug 483558, though.
Attachment #367550 - Flags: review?(bzbarsky)
Hmm. Why is it ok to ScrollToInitialTarget() in SetValue() even though we haven't reflowed the new text yet?
(In reply to comment #15) > Why is it ok to ScrollToInitialTarget() in SetValue() even though we > haven't reflowed the new text yet? It wouldn't be OK if the new text hasn't been reflowed, but the reflow is performed due to this FlushPendingNotifications #0 PresShell::FlushPendingNotifications (this=0xf8e8f0, aType=Flush_Layout) at /home/karl/moz/dev/layout/base/nsPresShell.cpp:4627 #1 0x00007f7ee9442ae6 in nsEditor::EndUpdateViewBatch (this=0x13d3f10) at /home/karl/moz/dev/editor/libeditor/base/nsEditor.cpp:4351 #2 0x00007f7ee94494d7 in nsEditor::EndPlaceHolderTransaction (this=0x13d3f10) at /home/karl/moz/dev/editor/libeditor/base/nsEditor.cpp:956 #3 0x00007f7ee942c7ad in ~nsAutoPlaceHolderBatch (this=0x7fff02ccdf90) at /home/karl/moz/dev/editor/libeditor/base/nsEditorUtils.h:66 #4 0x00007f7ee9426abb in nsPlaintextEditor::InsertText (this=0x13d3f10, aStringToInsert=@0x7fff02cce030) at /home/karl/moz/dev/editor/libeditor/text/nsPlaintextEditor.cpp:792 #5 0x00007f7ee8f375c3 in nsTextControlFrame::SetValue (this=0x13b80c0, aValue=@0x7fff02cce300) That only happens when the editor doesn't have eEditorUseAsyncUpdatesMask, but I see now that bug 151882 seems to want to add that flag. If we want something that will work with eEditorUseAsyncUpdatesMask, then I guess the best thing for SetValue() is to set a flag on the nsTextControlFrame to indicate that a ScrollToInitialTarget() is required and mark the nsTextControlFrame dirty so that it gets a reflow, and check for the flag in DoLayout(). I guess it would then make sense to shift the reflow root from the scroll frame to the nsTextControlFrame?
(In reply to comment #16) > That only happens when the editor doesn't have eEditorUseAsyncUpdatesMask, but > I see now that bug 151882 seems to want to add that flag. I mean bug 174823.
Yeah, ideally we want to not do a sync flush there. It's probably ok to leave as-is with a comment explaining why it's ok and another comment in the flag-munging code pointing out that this needs to be addressed if we start using eEditorUseAsyncUpdatesMask. At that point we might want to just use a reflow callback or something.
Attached patch patch with more comments (obsolete) (deleted) — Splinter Review
Added comments suggested in comment 18.
Attachment #367550 - Attachment is obsolete: true
Attachment #368197 - Flags: review?(bzbarsky)
Attachment #367550 - Flags: review?(bzbarsky)
Attached image comparison with and without patch (deleted) —
These screenshots are taken with old nightly before this regression, recent nightly and build with this patch. My ~/.fonts.conf is same as comment #4 and LANG is ja_JP.UTF-8. This patch improves 1px, but it's still hard to read as "gooqle."
Flags: blocking1.9.0.9?
(In reply to comment #4) > Created an attachment (id=311773) [details] > my ~/.fonts.conf > This is similar to the style "font-family: "Bitstream Vera Sans", > "IPAPGothic";" > but the height of input is different. Similar, but quite different. ><?xml version="1.0"?> ><!DOCTYPE fontconfig SYSTEM "fonts.dtd"> ><fontconfig> > <alias> > <family>sans-serif</family> > <prefer> > <family>Bitstream Vera Sans</family> > <family>IPAPGothic</family> > </prefer> > </alias> ></fontconfig> fontconfig gives aliases defined in this way a "weak" "binding", which means that these fonts are only considered if they support the requested language, or after aliases that do support the requested language. % fc-match -s "sans\-serif:lang=en" | head -n 3 Vera.ttf: "Bitstream Vera Sans" "Roman" ipagp.ttf: "IPAPGothic" "Regular" DejaVuSans.ttf: "DejaVu Sans" "Book" % fc-match -s "sans\-serif:lang=ja" | head -n 3 ipagp.ttf: "IPAPGothic" "Regular" Vera.ttf: "Bitstream Vera Sans" "Roman" DejaVuSans.ttf: "DejaVu Sans" "Book" This is intentional so that, for example, Japanese fonts don't get used to render Chinese text (when fonts for both are installed) even with something like: <alias> <family>sans-serif</family> <prefer> <family>Bitstream Vera Sans</family> <family>IPAPGothic</family> <family>SimSun</family> </prefer> </alias> The "regression" that makes this bug apparent would most likely be that Mozilla now interprets the fontconfig setting correctly, whereas previously Bitstream Vera Sans would have always been the primary font for all languages and IPAPGothic would have been used for Chinese text. fontconfig can be persuaded to make Bitstream Vera Sans the primary font for Japanese text (which would work-around this bug): <match target="pattern" > <test name="lang" compare="contains"> <string>ja</string> </test> <test name="family" > <string>sans-serif</string> </test> <edit mode="prepend" binding="same" name="family" > <string>Bitstream Vera Sans</string> </edit> <edit mode="prepend" name="family" > <string>IPAPGothic</string> </edit> </match> % fc-match -s "sans\-serif:lang=ja" | head -n 3 Vera.ttf: "Bitstream Vera Sans" "Roman" ipagp.ttf: "IPAPGothic" "Regular" DejaVuSans.ttf: "DejaVu Sans" "Book" What out that either your gtk/gnome font needs to be "sans-serif", not "sans" or you need to add another fontconfig rule to test for family "sans", or this won't work: fc-match -s "sans:lang=ja" | head -n 3 ipagp.ttf: "IPAPGothic" "Regular" Vera.ttf: "Bitstream Vera Sans" "Roman" DejaVuSans.ttf: "DejaVu Sans" "Book"
(In reply to comment #20) > Created an attachment (id=368380) [details] > comparison with and without patch Thanks for this. When I was testing the patch, I foolishly neglected to remove another change that was causing the code to use different font metrics. What's happening here is that the text input area is being sized according to the primary system font for the locale (from GTK and fontconfig), which in this case is IPAPGothic for Japanese. The metrics of this font as used on Linux and Mac give the following: (The metrics in the hhea table and the typo metrics in the OS/2 table are the same in this font.) ascent/ascender: 0.88 em descent/descender: -0.12 em line height: 1.00 em This is a small descent when compared with typical Latin fonts. I assume this descent was chosen to be representative of the Japanese glyphs in the font and it would be a reasonable value I think for a font that had no Latin glyphs. (This font does have Latin glyphs actually and the descent is not low enough to fit the descenders of Latin glyphs within this font. I don't know whether that really makes it "wrong" though, as the font is designed primarily for Japanese text.) Further there is no vertical spacing between the em heights for each line. Is this a normal feature of Japanese fonts? (It is not common for Latin fonts.) (On Windows, different metrics are used, and I had been incorrectly testing with those metrics: The winAscent is the same as the other ascent metrics, but the winDescent is 0.196em, which was giving a line height of 1.076em, which happened to provide enough space to see another pixel. Changing the Linux/Mac code to use these Windows metrics is not the solution here because these metrics have their own sets of problems, and other fonts will still have the same problem as seen here.) The input area is one line-height high. Normally most Latin letters fit between the ascent and descent, and the line height sometimes provides a little padding beyond that for unusual glyphs. When Latin words are entered into the text input, the system's (fontconfig's) font for English text is used for those words. That font has a similar em height (modulo hinting and non-scalable bitmap fonts), but other metrics differ. In this case, that font is Bitstream Vera Sans, and the metrics used from that font are: ascender: 0.928 em (from hhea table) descender: -0.236 em (from hhea table) line height: 1.200 em (from typo metrics in OS/2 table) Without attachment 368197 [details] [diff] [review], the baseline of the Latin text was moved down ~0.048em to accommodate the additional ascent here. It seems that, after rounding, this amounted to a 1 pixel shift. This shift increased the number of pixels truncated off the bottom of the text input area. With attachment 368197 [details] [diff] [review], the baseline of the Latin text is held consistent with the baseline intended for Japanese text. That means that fewer pixels are truncated at the bottom, but there simply isn't enough space allocated below the baseline in the text input to draw these pixels.
The reason why attachment 368197 [details] [diff] [review] is not enough to make Latin text completely visible here is that baseline alignment is not enough to make Latin text visible in a text input with a baseline intended for Japanese text. Possible improvements to this situation are: a) Position the text so that the em height of the first character is visible. A benefit of this approach is that, because the em height is similar for fonts for all languages, this is something that is achievable even in a text input sized for a different language. Another benefit is that this could be consistent with a ScrollIntoView implementation that makes the em height visible (bug 483558). An issue with this approach is that baselines of text within text-input boxes would sometimes not align with baselines of adjacent text. Another issue with this approach is that the em height only applies to a particular font, so the em ascent and descent could be different for other characters, but those other characters are aligned by baseline and so may not remain visible. (A similar ScrollIntoView implementation would make them visible when the cursor was moved to the obscured characters though.) b) It seems that not all of the space available in the location bar is being made available to the text input. Making the text input occupy the entire available space would seem sensible. I guess we'd want some way to center the text if the text input ends up higher than one line-height. With the current implementation that could be done by applying padding to the child div, but that feels a little messy. c) The height and baseline of the text input could be determined by considering fonts for both the current locale and Latin text. I wonder whether we'd want to do this for all text inputs or only those where we'd expect Latin characters. Ideally, like (b), this would need to have some means of shifting the text down to a baseline lower than the baseline of the child div.
(In reply to comment #22) > Further there is no vertical spacing between the em heights for each line. > Is this a normal feature of Japanese fonts? > (It is not common for Latin fonts.) Yes. Maybe, Internal Leading/External Leading of most major Japanese fonts are zero. Therefore, line-height: auto; was too tight layout before Mozilla 1.0. For the issue, we have special code for such fonts: http://hg.mozilla.org/mozilla-central/file/956071116564/layout/generic/nsHTMLReflowState.cpp#l2024
(In reply to comment #24) > For the issue, we have special code for such fonts: > http://hg.mozilla.org/mozilla-central/file/956071116564/layout/generic/nsHTMLReflowState.cpp#l2024 Thanks, Masayuki. It seems that hinting and rounding is causing us to fail the "if (!internalLeading && !externalLeading)" test. If we pass this test we'll have 20% more space. I'll see if I can find anything fundamentally wrong with the rounding. (gdb) p mMetrics $308 = {xHeight = 7.0000000000000009, superscriptOffset = 7.0000000000000009, subscriptOffset = 2, strikeoutSize = 1, strikeoutOffset = 6, underlineSize = 1, underlineOffset = -1, height = 0, internalLeading = 1, externalLeading = 0, emHeight = 13, emAscent = 11.44, emDescent = 1.5600000000000005, maxHeight = 14, maxAscent = 12, maxDescent = 2, maxAdvance = 13, aveCharWidth = 8, spaceWidth = 4, zeroOrAveCharWidth = 8} (gdb) p this->mStyle $309 = {style = 0 '\0', systemFont = 1 '\001', printerFont = 0 '\0', familyNameQuirks = 0 '\0', weight = 400, stretch = 0, size = 13.333333333333334, langGroup = {<nsACString_internal> = { mData = 0x2540e08 "x-unicode", mLength = 9, mFlags = 5}, <No data fields>}, sizeAdjust = 0}
Attachment #368197 - Flags: review?(bzbarsky)
Comment on attachment 368197 [details] [diff] [review] patch with more comments Removing review request on this. There is a discontinuity in the function for scroll position in ScrollToInitialTarget() wrt the height of the control when textareas have more than one line of text.
Attached file testcase for 20% more space (deleted) —
(In reply to comment #25) > If we pass this test we'll > have 20% more space. I'll see if I can find anything fundamentally wrong with > the rounding. On Windows, we sometimes don't have "20% more space." Is this related? Or should I file another bug?
Attachment #368855 - Attachment mime type: text/html → text/html; charset=Shift_JIS
We don't have "20% more space" with MS PGothic 32px.
No longer blocks: 481751
Depends on: 481751
Comment on attachment 368197 [details] [diff] [review] patch with more comments An updated version of this patch is in bug 349259. (At least it completely fixes that bug, but I think there may be a better solution.)
Attachment #368197 - Attachment is obsolete: true
(In reply to comment #27) > Created an attachment (id=368855) [details] > testcase for 20% more space > On Windows, we sometimes don't have "20% more space." > Is this related? > Or should I file another bug? Yes, this is the same 20% being only added sometimes due to rounding. There's more than one issue in this bug so it would be worth having another bug for that issue.
(In reply to comment #30) > There's more than one issue in this bug so it would be worth having another bug > for that issue. Filed Bug 485335.
Depends on: 485335
Flags: blocking1.9.0.10? → blocking1.9.0.10+
Depends on: 488776
Flags: blocking1.9.0.10+
No longer blocks: 453827
Depends on: 453827
No longer blocks: 483558
The issue at URL bar and status bar now seems to be fixed between ddfecbc93934 and 7c24dc44ca00. http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ddfecbc93934&tochange=7c24dc44ca00 However attachment 312030 [details], attachment 311770 [details], and attachment 376361 [details] (bug 485335) still have this issue with IPAPMincho and IPAPGothic fonts which are proportional sans-serif and serif fonts respectively.
(In reply to comment #32) > The issue at URL bar and status bar now seems to be fixed between ddfecbc93934 > and 7c24dc44ca00. Bug 524107 perhaps. I don't know whether that would be expected/intentional though.
Attachment 312030 [details], attachment 311770 [details] seems to be shown properly. However I don't know which bug fix this issue.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: