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

Status: RESOLVED WORKSFORME

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

Description

[Takanori MATSUURA]

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

Comment 1

[Takanori MATSUURA]

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?

Comment 2

[Atsushi Sakai]

Attachment: testcase

Comment 3

[Atsushi Sakai]

Attachment: screenshot of the testcase

font-family: "Bitstream Vera Sans", "IPAPGothic";
is good, but sans-serif (fontconfig switches fonts) is not good.

Comment 4

[Atsushi Sakai]

Attachment: 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.

Comment 5

[Karl Tomlinson (:karlt)]

(Transferring MATSUURA's blocking request from firefox3 to 1.9.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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....

Comment 7

[Takanori MATSUURA]

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.

Comment 8

[Takanori MATSUURA]

Checkins between 2008021004 and 2008021104

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-10+04%3A00%3A00&maxdate=2008-02-11+04%3A59%3A59&cvsroot=%2Fcvsroot

Comment 9

[Atsushi Sakai]

Attachment: <span> testcase

Comment 10

[Atsushi Sakai]

Attachment: screenshot of <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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, so not a form control issue.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we can block on this. However, Karl can probably look at this if priorities allow.

Comment 13

[Damon Sicore (:damons)]

-'ing based on Comment #12.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: patch

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why is it ok to ScrollToInitialTarget() in SetValue() even though we haven't reflowed the new text yet?

Comment 16

[Karl Tomlinson (:karlt)]

(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?

Comment 17

[Karl Tomlinson (:karlt)]

(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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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.

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: patch with more comments

Added comments suggested in comment 18.

Comment 20

[Atsushi Sakai]

Attachment: comparison with and without patch

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."

Comment 21

[Karl Tomlinson (:karlt)]

(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"

Comment 22

[Karl Tomlinson (:karlt)]

(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.

Comment 23

[Karl Tomlinson (:karlt)]

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.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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

Comment 25

[Karl Tomlinson (:karlt)]

(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}

Comment 26

[Karl Tomlinson (:karlt)]

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.

Comment 27

[Atsushi Sakai]

Attachment: testcase for 20% more space

(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?

Comment 28

[Atsushi Sakai]

Attachment: Screenshot of attachment 368855 on Windows

We don't have "20% more space" with MS PGothic 32px.

Comment 29

[Karl Tomlinson (:karlt)]

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.)

Comment 30

[Karl Tomlinson (:karlt)]

(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.

Comment 31

[Atsushi Sakai]

(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.

Comment 32

[Takanori MATSUURA]

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.

Comment 33

[Karl Tomlinson (:karlt)]

(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.

Comment 34

[Takanori MATSUURA]

Attachment 312030 [details], attachment 311770 [details] seems to be shown properly.

However I don't know which bug fix this issue.