109176 - Japanese hiragino text slided up within big button/drop-menu/...

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[Ying-Lin Xia]

In Japanese build, on all button, drop-menu, radio/check box, ... the area 
size is too big and the Japanese text is slided up. See screenshot below.

This only happens when change the app vers to Japan. It won't happen if keep 
the vers as English. However, we have to change vers to Japan, for system 
dialogs, copy/paste, ...

Comment 1

[Ying-Lin Xia]

Attachment: screenshot

Comment 2

[Ying-Lin Xia]

And this only happens on OS X.
OS 8/9 build with same files, is fine on OS 8/9.

Comment 3

[Frank Tang]

mass move unimportant m9.8 bug to m9.9 for now. 

Comment 4

[Frank Tang]

give this to nhotta. remove target

Comment 5

[nhottanscp]

I put a break point at nsFontMetricsMac::Init and see what kind of difference we
have regarding font info between different system default languages. I tried two
case "English" and "Japanese", numbers are in device coordinate.

English:
font - "Lucida Grande"
size 11, ascent 11, descent 2, leading 0
size 13, ascent 13, descent 3, leading 0


Japanese:
font - "HiraginoKakuGo Pro-W3"
size 11, ascent 10, descent 1, leading 6
size 13, ascent 11, descent 3, leading 7


A noticeable difference is that Japanese font has leading larger than 0.
So when we include leading for height calculation, Japanese font height is
larger than English because of leading.

In nsFontMetricsMac, we get height including leading.
http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsFontMetricsMac.cpp#104
104   mEmHeight   = mEmAscent + mEmDescent;
105 
106   mMaxHeight  = mEmHeight + mLeading;

Then mMaxHeight is used for line height too.
311 NS_IMETHODIMP nsFontMetricsMac :: GetHeight(nscoord &aHeight)
312 {
313   aHeight = mMaxHeight;
314   return NS_OK;
315 }
316 
317 NS_IMETHODIMP nsFontMetricsMac :: GetNormalLineHeight(nscoord &aHeight)
318 {
319   aHeight = mMaxHeight; // on Windows, it's mEmHeight + mLeading (=
mMaxHeight on the Mac)
320   return NS_OK;
321 }


In the layout, we use ascent as a baseline (but no leading included).

http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsTextBoxFrame.cpp#409
409     fontMet->GetMaxAscent(baseline);


In case of Japanese font, the text box height is larger because of the leading
but baseline does not include leading, and the result we see is the charactes
are drawn shifted up as shown in the screen shot.

Comment 6

[nhottanscp]

I dumped font info of other fonts installed on my machine and found those 
new Japanese fonts for MacOSX have large leading values.
One way to workaround the problem is to use other JA fonts (e.g. Osaka) 
for wigets. In fact, I don't know how we get "HiraginoKakuGo Pro-W3" as a 
widget font instead of "Osaka".

fontInfo of Japanese font at size 12 pt.
id= -29602 fontName= ÉqÉâÉMÉmä€ÉS Pro W4 (HiraginoMaruGo W4)
size= 12, ascent= 11, descent= 1, widMax= 12, leading= 6
id= -896 fontName= ÉqÉâÉMÉmñæí© Pro W3 (Hiragino Mincho W3)
size= 12, ascent= 11, descent= 1, widMax= 12, leading= 6
id= -899 fontName= ÉqÉâÉMÉmñæí© Pro W6 (Hiragino Mincho W6)
size= 12, ascent= 11, descent= 1, widMax= 13, leading= 6
id= -17370 fontName= ÉqÉâÉMÉmäpÉS Pro W3 (Hiragino KakuGo W3)
size= 12, ascent= 11, descent= 1, widMax= 12, leading= 6
id= -17373 fontName= ÉqÉâÉMÉmäpÉS Pro W6 (Hiragino KakuGo W6)
size= 12, ascent= 11, descent= 1, widMax= 13, leading= 6
id= -11512 fontName= ÉqÉâÉMÉmäpÉS Std W8 (Hiragino KakuGo W8)
size= 12, ascent= 11, descent= 1, widMax= 14, leading= 6


id= 16384 fontName= Osaka
size= 12, ascent= 12, descent= 3, widMax= 13, leading= 2
id= 16436 fontName= OsakaÅ|ìôïù (Osaka Tohaba)
size= 12, ascent= 10, descent= 2, widMax= 13, leading= 0

id= 16700 fontName= ï?ê¨ñæí© (Heisei Mincho)
size= 12, ascent= 10, descent= 2, widMax= 13, leading= 3
id= 16701 fontName= ï?ê¨äpÉSÉVÉbÉN (Heisei KakuGothic)
size= 12, ascent= 10, descent= 2, widMax= 13, leading= 3

Comment 7

[rbs]

Might be related to bug 167236

Comment 8

[nhottanscp]

I applied the patch of bug 167236 but the problem still exists. I think the
patch is specific to form button.

Comment 9

[nhottanscp]

I and Shanjian looked at Apple's font metric info in inside mac.
http://developer.apple.com/techpubs/mac/Text/Text-186.html#HEADING186-0

From the figure, ascent includes internal leading. And their leading is for
external leading.

The problem we have for the widgets (see the screen shot), I think we don't want
to include (external) leading to draw text in widget. The widget text is mostly
a single line and including external leading could cause the glyph drawn shifted
up for fonts with large leading value like Hiragino family.

Comment 10

[HARUNAGA Hirotoshi]

*** Bug 142628 has been marked as a duplicate of this bug. ***

Comment 11

[Hideo Saito]

Attachment: patch for adding internal leading

For the textmetrics on Windows, the mMaxAscent has an internal leading as
follows. However, for the textmetrics on mac, the mMaxAscent does not have any
internal leading. I think that the baseline of a font with internal Redding is
shifted upwards.

font height = ascent + descent + internal leading

    --------
	|	  internal leading
font   ----
height	|	  ascent
	|
    ----|------------ baseline
	|	  descent
       ----

For the textmetrics on Windows:
mEmHeight = ascent + descent
mEmAscent = ascent
mEmDescent = descent

mMaxHeight = ascent + descent + internal leading
mMaxAscent = ascent + internal leading
mMaxDescent = descent


For the textmetrics on mac:
mEmHeight = ascent + descent
mEmAscent = ascent
mEmDescent = descent

mMaxHeight = ascent + descent + leading
mMaxAscent = ascent
mMaxDescent = descent

Comment 12

[Hideo Saito]

Comment on attachment 166063 [details] [diff] [review]
patch for adding internal leading

Although I can not test this patch since I do not own mac, our estimators said
that the improvement is clear since the baseline shifts down.

Comment 13

[Hideo Saito]

rbs, do you know why |mMaxAscent| of textmetrics on mac does not have an
internal leading? if you do not know the reason, please let me know a reviewer
who can review the code for mac.

Comment 14

[Hideo Saito]

Attachment: patch

Please refer following document for mac's font metrics.
http://developer.apple.com/documentation/Cocoa/Conceptual/FontHandling/index.html

Figure 1 indicats that leading is line gap, and please refer to following document too.
http://developer.apple.com/documentation/mac/Text/Text-145.html#MARKER-9-250
leading
    The measurement in pixels from the descent line to the ascent
    line below it.

I think that mac's leading is a space between lines, but not internal leading.

I wander if a limitation needs as follows, however document of mac's font metrics concerns with all fonts on mac.

-  mLeading    = NSToCoordRound(float(fInfo.leading) * dev2app);
+  if (0 == nsCRT::strcmp(aLangGroup,  "ja"))
+    mLeading  = 0;
+  else
+    mLeading  = NSToCoordRound(float(fInfo.leading) * dev2app);

Comment 15

[Hideo Saito]

> http://developer.apple.com/documentation/Cocoa/Conceptual/FontHandling/index.html

Please click "Getting Font Metrics" to see Figure 1.

Comment 16

[Hideo Saito]

Attachment: patch2

I think that Mac's leading is external leading and this value should be used for normal line height.

Comment 17

[Hideo Saito]

Our estimator tested patch2 and attached following screenshot. It seems that text position is suitable in the center of each box.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=3001&action=view

Comment 18

[Katsuhiro MIHARA]

Ash-san,
Please introduce a reviewer who can review patches attached in this bug.

Comment 19

[Hideo Saito]

Comment on attachment 204283 [details] [diff] [review]
patch2

roc, could you review this patch?

I think that mac's leading of font metric is external leading, then |mMaxHeight| should not include it and the leading should be used for normal font height.

Comment 20

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

Comment on attachment 204283 [details] [diff] [review]
patch2

Josh needs to look at this.

Comment 21

[Josh Aas]

Comment on attachment 204283 [details] [diff] [review]
patch2

Looks good. I'm not a font expert, but from what I got by reading through and testing this patch, it seems fine.

Comment 22

[Hideo Saito]

Comment on attachment 204283 [details] [diff] [review]
patch2

When the font currently used has a leading value on a Mac, this bug is a pretty serious issue related about the display of a text. I think that this patch is low-risk.

Comment 23

[Hideo Saito]

Masayuki, please check into the trunk.

Comment 24

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

(In reply to comment #23)
> Masayuki, please check into the trunk.

O.K. but I cannot check-in now. Please wait until tonight(JST).

Comment 25

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

checked-in to Trunk.

Comment 26

[philippe (part-time)]

Thank you very much. This works very well. I've tested this on most of the sites I designed, and now get the expected display of Hiragino fonts without need for the 'Lucida Grande' hacks.


Hopefully this can land on the branch as well.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch does seem reasonable; there's a question as to whether we actually want to take changes that affect the display of Web pages for dot releases.

It seems like this patch would change the height of backgrounds on inline elements but would not change line spacing, so it's not too dangerous.  (Anything that changes line spacing could easily break poorly-designed web pages, which we don't want to do in a security release since it would discourage people from getting security fixes.  But that looks like it's not the case here.)

Comment 28

[Josh Aas]

I think this is an important fix for Japanese users. I would recommend taking it for Firefox 1.5.0.1. I agree with dbaron, this patch does not change line spacing and as such it is not so dangerous.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 204283 [details] [diff] [review]
patch2

a=dveditz
Please add fixed1.8.0.1 keyword when checked in.

Comment 30

[Josh Aas]

Landed on MOZILLA_1_8_0_BRANCH and MOZILLA_1_8_BRANCH