Closed Bug 425004 Opened 13 years ago Closed 4 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
font-family: "Bitstream Vera Sans", "IPAPGothic";
is good, but sans-serif (fontconfig switches fonts) is not good.
Attached file my ~/.fonts.conf
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
(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) — 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) — Splinter Review
Added comments suggested in comment 18.
Attachment #367550 - Attachment is obsolete: true
Attachment #368197 - Flags: review?(bzbarsky)
Attachment #367550 - Flags: review?(bzbarsky)
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.
(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: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.