76097 - Need to include external leading for normal Line-height

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P1)

Description

[Seak, Teng-Fong]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; 0.8.1) Gecko/20010413
BuildID:    2001041304

When a Chinese word (very likely for Japanese and Korean too) is underlined,
aethestically it's nicer to put the underline one more pixel down.

Reproducible: Always
Steps to Reproduce:
1.Make ah HTML file containing underlined Chinese words (using <A> or <U>)
2.Open it and see it yourself.

Actual Results:  The underline covers the last pixel line of Chinese words.

Expected Results:  The underline is one more pixel down.

Presently, the underline is drawn on the last pixel line of Chinese words, this
isn't very nice to read.

PS: I could make a small screen capture if you want to see what I mean.

Comment 1

[Asa Dotzler [:asa]]

updating component

Comment 2

[Seak, Teng-Fong]

I think you could put this one more pixel down to underline for Latin 
characters too. That makes them nicer.

Comment 3

[Frank Tang]

good polishing issue. Mark this as future.

Comment 4

[Keyser Sose]

Marking NEW.

Comment 5

[Shanjian Li]

Attachment: proposed fix

Comment 6

[Shanjian Li]

Instead of using underscore position returned from font's OUTLINETEXTMETRIC, 
descent works better. I also checked IE's behavior. It seems they use descent
for chinese character and underscore for ASCIIs. From my experient, descent works
better in both case. I do not see any need to complicate the logic.

Comment 7

[Shanjian Li]

Using www.sohu.com as a testcase, we need to adjust the linespace as well. 
I will take a look later at this bug. 

Comment 8

[Jaime Rodriguez, Jr.]

Let's get this one into the branch.

Comment 9

[Shanjian Li]

Attachment: A new patch which fix lineheight problem as well.

Comment 10

[Shanjian Li]

I checked with windows documentation about TEXTMETRIC structure, and it says 
internal leading is included in tmHeight. For normal lineheight, external 
leading should also be included. 
nsFontMetricsWin :: GetNormalLineHeight(nscoord &aHeight)
{
  aHeight = mEmHeight + mLeading;
  return NS_OK;
}
So we might want to add external leading to mLeading as well. 

The previous patch missed internal leading. The right patch should be the 
following one:

Comment 11

[Shanjian Li]

Attachment: This patch count both internal and external leading when calculating line height.

Comment 12

[Shanjian Li]

The bug make mozilla looks bad for Chinese (probably japanese and korean) 
websites. This problem might exist in western language as well if certain
font is used. So I changed the severity from trivial to normal. 

Comment 13

[Frank Tang]

move into 93.

Comment 14

[Frank Tang]

rbs, can you review this ?

Comment 15

[rbs]

This is a delicate change. My understanding is that layout should really be 
using (tmHeight + tmExternalLeading)*(lineNumber-1) when changing lines, but 
this issn't what is presently happening. The tmExternalLeading hasn't been 
considered so for during layout. Since its value is usually 0px or 1px in most 
ASCII fonts, nobody seems to complain.

However, just changing things the way you did may cause line-spacing problems 
because layout uses GetMaxHeight(), GetMaxAscent(), ..., and it expects things
to match up, e.g., it expects that ascent + descent = lineheight. With the 
change, things wouldn't match up.

Do you see any difference when trying the following changes? (this is a 
dubious hand made pseudo-diff):

-mLeading = NSToCoordRound(metrics.tmInternalLeading * dev2app);
+mLeading = NSToCoordRound(metrics.tmExternalLeading * dev2app);

-mMaxHeight = NSToCoordRound(metrics.tmHeight * dev2app);
+mMaxHeight = NSToCoordRound((metrics.tmHeight + metrics.tmExternalLeading) *    
                              dev2app);

-mMaxAscent = NSToCoordRound(metrics.tmAscent * dev2app);
+mMaxAscent = NSToCoordRound((metrics.tmAscent + metrics.tmExternalLeading) * 
                              dev2app);

NB: this change is not really correct w.r.t. the semantics of the API. It is 
just an experimentation to see if it helps the line-spacing problem (it is 
motivated by how layout works). I remember Erik was having long discussions 
about this line-height property.

Comment 16

[Shanjian Li]

I looked a little bit further and examined those line height related code in 
layout module. Function GetMaxHeight in fact seem never got used. In place 
where line height is calculated, it seems we always use normal line height for 
calculation. I didn't find any code which suggests an expection of 
ascent+descent=lineheight. In summary, I did not find any match-up problem.

  -mLeading = NSToCoordRound(metrics.tmInternalLeading * dev2app);
  +mLeading = NSToCoordRound(metrics.tmExternalLeading * dev2app);
I tried this and it does not work well. Since we excluded tmInternalLeading
from tmHeight when calculating mEmHeight, and normalLineHeight = 
mEmHeight+mLeading, this is understandable. 

-mMaxHeight = NSToCoordRound(metrics.tmHeight * dev2app);
+mMaxHeight = NSToCoordRound((metrics.tmHeight + metrics.tmExternalLeading) *    
                              dev2app);
-mMaxAscent = NSToCoordRound(metrics.tmAscent * dev2app);
+mMaxAscent = NSToCoordRound((metrics.tmAscent + metrics.tmExternalLeading) * 
                              dev2app);
I tried these changes, but could not see any difference in several average 
websites. I bet it will make difference in certain situation. Since mMaxHeight 
and mMaxAscent is defined for glyph, I did not see any reason to include 
tmExternalLeading. 

Comment 17

[Frank Tang]

shanjian- can you put an updated patch and let rbs review it ?

Comment 18

[rbs]

"Function GetMaxHeight in fact seem never got used." Currently, it is
GetHeight() that is used. But they are equivalent (they return the same value).

Comment 19

[Shanjian Li]

I did not got any reason to update my patch. It still looks fine for me. I did
check GetHeight. In my understanding of the layout code, its soly expection of 
that function is font height. I did not find any place where this GetHeight 
is misused in calculating lineHeight. 

Comment 20

[rbs]

Actually, your patch makes sense to me -- it is what I understand also. That 
layout should use (tmHeight + tmExternalLeading)*(lineNumber-1), as I wished 
too.

But while investitaging the ramifications of the problem (in anticipation of any
regressions that the patch may cause based on the expected legacy behavior), I 
stumbled on a related bug 84672. Does this patch impacts that bug in a negative 
way?

Comment 21

[rbs]

Also, shouldn't this be All/All for consistency of the fundamental metrics that 
are returned to layout?

Comment 22

[Shanjian Li]

For your first question, I put bug 84672 into my consideration when fixing this 
one. But 84672 will not fixed by this patch as well. 
I didn't quite understand your 2nd question. For windows platforms, I could not 
think of anything else which might make this patch incomplete in consistency. 
For other platforms, I didn't look at them yet. My fix will not affect those 
platforms, but we might have simliar problem in those platforms. 

Comment 23

[rbs]

OK, r=rbs. Now we are better prepared to handle any "regressions".

By 'All/All' I meant that Platform=All/OS=All have the similar problem and if 
they are not yet including the external leading, the fix is relevant for them 
and needs to be applied everywhere for consistency.
Cc:ing bstell.

With the change of semantics, another thing to correct is the description of the
API. With your fix, the current description of nsIFontMetrics::GetLeading() is 
not true anymore.

  /**
   * Returns the amount of internal leading (in app units) for the font. This
   * is computed as the "height  - (ascent + descent)"
   */
  NS_IMETHOD  GetLeading(nscoord &aLeading) = 0;

Comment 24

[Shanjian Li]

That's a catch. I will change that. That also mean we need to make sure 
mac, linux and other platform are consistent. Since this function is 
never called by anybody yet, I will  mark this as for window only now. 
After proper change/verification is done of other platform, we will 
remove that. 

Comment 25

[Shanjian Li]

Attachment: patch for GetLeading comment

Comment 26

[Shanjian Li]

Chris, can you do a sr?

Comment 27

[Chris Waterson]

dbaron and/or hixie may want to comment on the line-height issues you've raised
above. Could you please attach the complete patch that you intend to check in?

Comment 28

[Shanjian Li]

The complete patch I am intending to check in is the last 2 patches combined 
together. Since those 2 patches are applied to different files in different 
directory, I will just leave them that way. 

Comment 29

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

This is a major change to a heavily discussed issue, and the proposal makes it
for only one platform and doesn't change it for all other platforms.

See related bugs:  bug 13072, bug 27164, bug 27873, bug 27874, bug 63650.

I think the correct solution here, as I've said in other bugs, is to change the
meaning of 'line-height: normal' to include external leading as well as internal
leading, but not to change the meaning of scaling factors.  (I'd need to refresh
my memory about our current behavior, too.)  However, such a change (as would
this one) would probably break the layout of some poorly designed web pages.

However, if we have an underline positioning problem, isn't that just a bug in
an implementation of nsIFontMetrics::GetUnderline?

Comment 30

[Shanjian Li]

To answer david's question about underline problem:
Because we did not include external leading when calculating normal line height,
the space between 2 lines is too small. For some CJK fonts, the space is just 
not existing. If I move the underline position lower, this problem will make 
many cjk webpages more ugly. 

This problem seem much more complicated that I was originally thought. I need to 
read those bug reports and past discussions before I post my next comment. 

Comment 31

[Shanjian Li]

Attachment: My current, complete patch (combination of last 2) as base for future discussion

Comment 32

[Frank Tang]

ok, it looks a high risk fix and need more discussion.
>It seems IE use descent for chinese character and underscore for ASCIIs. 
> From my experient, descent works better in both case. 
Can we do that? Will that lower the risk for ASCII ?

Comment 33

[Shanjian Li]

to answer ftang's question:
To change underline position for Chinese characters without adjusting lineheight
as suggested in this bug will make the page looks even worse. 

Comment 34

[石庭豐 (Seak, Teng-Fong)]

No, no, I didn't say putting the line one pixel lower without adjusting other
properties if needed.  As a matter of fact, line spacing in Mozilla is tight and
that's not quite comfortable for reading.  So, maybe add one more pixel to line
spacing as well.

Comment 35

[Hixie (not reading bugmail)]

There are VERY specific rules that Mozilla is trying to follow in order to 
calculate line height, namely those described at:

   http://www.people.fas.harvard.edu/~dbaron/css/2000/01/dibm

We have a good dozen test cases for this which must stay identical to the 
current rendering TO THE PIXEL and ON EVERY PLATFORM if any changes are made, 
including but not limited to:

   http://www.fas.harvard.edu/~dbaron/css/fonts/sizes/
   http://www.fas.harvard.edu/~dbaron/css/test/inlinetest
   http://www.fas.harvard.edu/~dbaron/css/test/linebox4
   http://www.fas.harvard.edu/~dbaron/css/test/emunit
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight1.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight2.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight3.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight4.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight5.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight6.html
   http://www.bath.ac.uk/%7Epy8ieh/internet/eviltests/lineheight7.html
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=32563 (bug 77067)

Note: the whole way we draw underlines is wrong from the start, see bug 1777.
Note also that the last two added lines of the patch do not make sense.

Could the reporter please attach a testcase demonstrating clearly to the non-
native reader why the line height and underlining height should be changed in
ideographic languages? I would much, MUCH rather see this issue resolved first
in the W3C and then in Mozilla rather than change our almost perfect 
implementation of the CSS inline box model without carefully thinking this 
through with its full implications.

PDT: Note that changing this code is risky from a CSS1 standards compliance 
point of view.

Comment 36

[Shanjian Li]

I finished my reading of past discussion today, and there are 3 interesting 
points I would like to added here. 
. This problem is not only exist for in simplified chinese, similar bug has been 
filed against Japanese. (bug 63650)
. Many people agreed that by including external leading, we can get a better 
display result.
  David Baron (in this bug): 
"I think the correct solution here, as I've said in other bugs, is to change the 
meaning of 'line-height: normal' to include external leading   as well as 
internal leading, but not to change the meaning of scaling actors."

  Todd Fahrner<http://lists.w3.org/Archives/Public/www-style/1999Nov/0116.html>
"A little leading is almost always a good idea, and a little more is 
almost always even better, especially when you don't have to cut down 
trees to make room for it. :^)"

.Erik expressed a big concern about backward compability. He said that IE is not 
using this external leading for normal line-height. That is different from my 
assumption. He might be correct since IE may do some hack to handle CJK fonts.
In bug 63650, Erik wrote:
"Times New Roman at 16px gives a tmHeight of 19px, and that's what WinNav4 and
WinIE use. The tmHeight value does not include the external leading, which is
1px (giving a total of 20px = 19 + 1).

So, if we changed WinMoz to include the external leading, our pages would look
different from the old browsers. In normal text, that might not be a problem,
but in forms and tables, these small effects have a way of multiplying several
fold. Ask Rod Spears. I'm sure he can give you some horror stories.

Anyway, the only way to solve this problem is to come up with some hack for
Japanese (and other) fonts, leaving Times New Roman and other Western fonts as
they are. Such a hack would not be clean, since the fonts should be giving us
better leading values. But Mozilla has been known to perform dirty tricks every
now and then to get around legacy issues. Perhaps this is one of those areas."

I need to spend more time to investigate our current behvior and IE's, this 
including writing some testcase to verify my assumption. If this change is 
really too risky, we may have to do as erik suggested, that is to implement some 
hack for CJK characters. Base on my observation of chinese font, at least some 
of them has non-zero external leading which can make line-spacing more 
comfortable to reader if we include them. Erik reported that some japanese 
font's external leading is zero. So I need to check popular CJK fonts as well.

Comment 37

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

Attachment: testcase showing English and Chinese fonts

Comment 38

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

Attachment: longer testcase

Comment 39

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

I was testing font underline position in WordPad and I couldn't find any fonts
that WordPad displays differently from Mozilla.  Windows IE does something a bit
wierd, but it's probably something similar to what would happen once we fixed
bug 1777 (although it also doesn't increase the underline thickness for large
fonts, which I think is wrong).

Comment 40

[Hixie (not reading bugmail)]

I was looking at David playing around with underlined Chinese text in WordPad,
and they really do exactly the same as we do now. I'm less and less convinced
that what we do is wrong.

Comment 41

[Shanjian Li]

I am less worried about underline position before we provide a proper 
line-height  for CJK font. Underline position change only will make things worse 
. Probably we did nothing wrong, but the final display result for chinese 
webpage looks rather uncomfortable to readers, especially compared with IE's 
behavior. 

I modified ian's test case and checked the behavior of both IE and mozilla. Erik 
was correct, IE does not use external leading for calculating normal 
line-height. The most popular japanese fonts (MS Gothic and MS Mincho) has zera 
external leading as well. So Erik was correct at this point as well. However, 
Chinese font (MS Song, MS Hei, MingLiU) and a popular korea font (Gulimche) do 
have non-zero external leading value, and IE does include this value when 
calculating normal line-height. I will post 2 screen snaps will prove this. 

Base on all those information, I am inclined to implement a hack to deal with 
this problem as suggested by erik. That is for CJK fonts with external leading, 
we include it when calculating normal line-height. For those CJK fonts that 
without external leading (and decent internal leading, as bitstream's Cyberbit, 
ie. internal leading is less than 10% of tmHeight), we use a factor of 1.1 
instead of 1.0. This hack will only apply to windows, and to Chinese(both simple 
and traditional)/Korean/Japanese. The underline position will be adjusted 
the same way as my previous patch, but it will only applies to CJK as 
well. David, Ian, rbs, please let me know if you have a different opinion.  

Comment 42

[Shanjian Li]

Attachment: mozilla line-height calculation, no external leading for korean font

Comment 43

[Shanjian Li]

Attachment: IE5.5 line-height calculation, has external leading for korean font

Comment 44

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

Could you attach the test case you used for those screenshots.

Also compare Mozilla and IE on
http://www.people.fas.harvard.edu/~dbaron/css/fonts/sizes/variants
and notice that IE does different things depending on whether images are in the
line.

Comment 45

[Shanjian Li]

Attachment: The html file for generating those screen shots.

Comment 46

[Shanjian Li]

I would like to mention that the above html page is modified based on Ian's 
line-height testcase. 

Comment 47

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

Attachment: modified version of above test case showing IE behaves differently for different types of characters

Comment 48

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

I'd like to suggest that the correct solution would be for us to behave for all
fonts as IE does for Korean fonts.  However, I suspect we might need to limit
that in quirks mode to certain fonts, although I'm not sure.  Whatever we do, we
should be consistent across platforms...

Comment 49

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

... and what I *think* that difference is is including the external leading in
the definition of 'line-height: normal' (but not changing the definition of any
other 'line-height' values.

Comment 50

[Shanjian Li]

David, do you mean to include external leading in normal line-height for all 
fonts? That way we may have serious backward problem as suggested by erik. And 
it will not solve japanese fonts problem, since the 2 popular japanese fonts 
have zero external leading. I am wondering why IE does this only to Korean and 
Chinese fonts. They must have some reasons, and my guess is that it is the 
backward compatibility problem with existing webpages. 

Comment 51

[Shanjian Li]

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

Comment 52

[rbs]

What about doing the "right thing" (TM) in strict mode. The 'tmExternalLeading'
field under consideration here is intended by font designers to be _used_ as
part of the line-spacing. Something which I think is important is missing from
the test cases: an actual dump of the metrics used by the fonts being selected
for rendering. If 'tmExternalLeading' is zero (as in many ASCII fonts), the test
cases wouldn't tell the full story. And the apparent coincidence could just be
that 'tmExternalLeading' is zero. Also, the backward compatibility issue could
be a chain that goes to several generations of browsers. It could be that IE was
tied to this compatibilty issue too.

Comment 53

[rbs]

"External Leading
This is returned as tmExternalLeading in the TEXTMETRIC structure from
GetTextMetrics() and describes how much extra space the font designer expects
the application to leave between rows of the font."

http://support.microsoft.com/support/kb/articles/Q32/6/67.ASP

Comment 54

[rbs]

... "When outputting multiple lines of text, the lines should be separated by
(tmHeight + tmExternalLeading)."                       ^^^^^^

Comment 55

[Shanjian Li]

rbs, the backward compatibility problem caused by including external leading for 
ASCII fonts might be unacceptable. I here proposed a limited patch as I stated 
earlier. 

Comment 56

[Shanjian Li]

Attachment: New patch which only affects CJK fonts

Comment 57

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

1) We shouldn't be doing this on one platform and not the others.

2) Making this depend on the encoding of the font, although convenient, is
wrong. It doesn't handle ISO-10646 fonts. I don't think it's quite as bad as
making things depend on the encoding of the document, though.

3) It *is* the right thing to do, so we should do it in strict mode for all
fonts, and in quirks mode we should not do it for certain situations. (What
exactly should those situations be?)

Comment 58

[Hixie (not reading bugmail)]

I'm with dbaron on this. I see no reason for us (in standard mode) to do this 
for CJK fonts but not other fonts. We should be consistent. If the specs say we
should include internal leading then we should include internal leading. And 
depending on the character encoding of _anything_ is wrong.

I don't care what we do in quirks mode, do what you want there.

Comment 59

[Shanjian Li]

I know it's going to be hard to make everybody agree with my fix, but I haven't 
find a better one so far. I post my arguement here and you are more than welcome 
to suggest a better solution. 

Yes, on MS's true type specification, external leading are supposed to be 
included in default line-height, but the practice varies from font to font. On 
windows platform, we have to consider those most popular ones first. MS is at 
least doing this consistent to popular fonts within same encodings. 
a)For most western fonts, external leading is zero, very few of the fonts (like 
  times new roman) has a very small external leading. Either include or exclude 
  this small leading does not make much different to the final result. For 
  backward compatibility reason, even MS does not include this value when 
  calculating line-height. 
b)For Chinese and Korean fonts, this external leading is significant in 
  calculating line-height. Without this leading, the final appearance looks too  
  tight.
c)For Japanese fonts, the external leading is zero in those popular fonts. The    
  text does not looks nice if no extra leading is added. 

There is no easy solution to satisfy all those situation without considering 
encoding. If we just include external leading as suggested by ttf specs, 
japanese normal line-height is too small. If we added extra space for all fonts 
without external leading, some western fonts will have too much extra space 
between lines. Don't you agree that my patch leads to best user experience? 

BTW, (sorry for my ignorant,) what is quirk/strict/standard mode? Is this mode 
set through user preference? If quirk mode is the default one for users, I would 
not mind implement a strict TTF standard compliant normal line-height 
calculation for strict/standard mode. But either way, we are still CSS 
compliant. 

to answer some questions, 
dbaron: 1) We shouldn't be doing this on one platform and not the others.
  I do not think this is a cross-platform issue. On other platform, we may or 
may not have this problem. We should certainly check those platform and fix 
similar problem after we settle down this for windows. 

dbaron: 2) Making this depend on the encoding of the font, although convenient, 
is wrong. It doesn't handle ISO-10646 fonts. I don't think it's quite as bad as
making things depend on the encoding of the document, though.
  It looks like we were using font encoding, but that's not true. The encoding I 
am used (mLangGroup) is in fact decide by document. For example, a japanese 
webpage is requested and the font specified in the document or generic font set 
in browser is a ISO-10646 font, mLangGroup is set to "ja" because the font is 
requested for that language group. I tried "Bitstream Cyberbit" font. Either 
specified inside document or in browser's generic font setting, it all works 
fine. 

Comment 60

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

Quirks mode is used depending on the page -- we use doctype sniffing (not the
best way, but good enough, at least if we did it right) to determine whether a
page is an existing page that requires backward-compatibility treatment (quirks
mode) or whether it's a new page that should be handled correctly according to
standards (standands mode).

We'd discussed in the past that including the external leading for 'line-height:
normal' was the right thing to do on all platforms.  We should do that, and make
an exception for western characters in quirks mode.  This might require an
addition to the nsIFontMetrics API so that we can get internal leading and
external leading separately.  Then the quirks mode decision could be made in the
layout code that uses the font metrics (nsTextFrame/nsLineLayout).

Could you say what mLangGroup *is*?  You've said what it isn't.  How is it
determined?  Documents certainly don't specify a language group in any clear way.

Comment 61

[Shanjian Li]

I can only answer your question base on my understanding of the code. 
langGroup was set in DeviceContextImpl::GetLocaleLangGroup(void). This piece of 
information should be derived from document encoding. In browser preference 
windows, there is font setting various languages (like japanese, korean, 
chinese, etc). This piece of information will decide which group of font setting 
in preference should be used for a web document. 

Comment 62

[Shanjian Li]

This is a troublesome issue. Unless we are sure to include external leading in 
normal line-height will not cause problem with exsiting page or understand how 
bad it is with, applying this logic to western language is too risky. Since we 
could not reach a consensus at this time, need to push this one to 1.0 

Comment 63

[rbs]

Replacing GetLeading() with two APIs like dbaron suggested: GetInternalLeading()
and GetExternalLeading() might be the way to go, as this will allow layout to
decide what to do based for example on quirks vs. strict mode.

Comment 64

[Shanjian Li]

Attachment: patch suggested by david, ian & rbs

Comment 65

[Shanjian Li]

I implement the patch as suggested by david, ian and rbs. It does not solve our 
problem. Why? Most CJK websites are in quirk more, so the new code does not run 
at all. I also suspect that we may still experience backward compatibility 
problem with some new websites. 

For me, there are 2 options I willing to take. 
1, Use my CJK hack, which resort to encoding to determine difference in 
behavior.
2, Include external leading for all encodings/modes. We need to have a 
understanding about how worse is the backward compatibility problem. We can 
implement a preference and let people try out (after checking in the code).
It might not be as bad as we thought.  

Comment 66

[rbs]

I like the patch because it is forward looking. The idea of a hidden pref 
to see how things go is also nice, and the pref could be turned on in the
Japanese edition in both quirks and strict modes.

What is the story with the gymnastics in:
+    if (externalLeading > 0)
+      normalLineHeight = emHeight+ internalLeading + externalLeading;
+    else if (internalLeading > 0)
+      normalLineHeight = emHeight + internalLeading;
+    else
+      normalLineHeight = emHeight + emHeight /
EM2LEADING_RATIO_FOR_NORMAL_LINE_HEIGHT;

Might be better to let GFX returns the final values ready for consumption.
This way, layout can just do: 
   normalLineHeight = emHeight + internalLeading + externalLeading;
and each platform could slightly tweak as they please (e.g., in the Japanese
case).

Comment 67

[Shanjian Li]

Those popular japanese font found in windows are all with zero external leading 
and zero internal leading. But we have customer complain about normal 
line-height being too tight. So in case no internal and external leading are 
found, addition leading is added. 

I think how much leading should be added is a layout issue. (CSS suggest a value 
between 1.0 to 1.2). I would like font metric just return objective value we got 
from OS and leave all decision making to layout. That way we don't have to 
change nsIFontMetrics in future. 

Comment 68

[Chris Waterson]

Forgive me for jumping in late, but isn't the issue that the OS (well, the
font's meta information that the OS retrieves for us) is giving us poor advice?
If that's the case, oughtn't we keep the hack as close to the font as possible
(i.e., in GFX)?

Comment 69

[Shanjian Li]

Chris,
You are only partially correct. The font advice is to put external leading into 
normal line-height. We are not taking this advice right now. In some 
situation when both external leading and internal leading are zero, you can say 
the font give poor advice. I can also say that the font does not give advice in 
this situation. So the right behavior should be taking the font's advice and 
trust it if it is available, otherwise making decision ourselves.

Comment 70

[Shanjian Li]

Remove nsBranch keyword, we will never risk to check this in branch. 

Comment 71

[kill this account]

we should wait until bug 99010 is in before fiddling with this.

Comment 72

[石庭豐 (Seak, Teng-Fong)]

Attachment: Test case from reporter

Comment 73

[石庭豐 (Seak, Teng-Fong)]

Sorry for my late reaction to this bug.
I've seen those test case (in HTML and image) but they don't really show the
problem.  I've just sent an attachment.
The problem in the other testcases is that they don't show the real problem with
Chinese characters with underlines.  Some characters (as shown in my test case)
have bottom horizontal strokes (the first character) or a stroke almost totally
horizontal (the third character).  To make an analogy, the letter E has this
bottom stroke, the letter B has this almost totally horizontal stroke while the
letter A has none.
The other problem with Chinese characters with respect to Latin characters is
that they always occupy the whole space from top to bottom (I don't know the
technical word) Take an example, the letter T occupy the top and middle space
while the letter p occupy the middle and bottom space. See what I mean? That's
why I also put ABCDE in the test case to give you the contrast.
So, if the understand is too near to the bottom, the bottom horizontal stroke
might be covered too.

PS: I'm using the 20010925 built, and the problem seems less important than
before. Is this fixed already?

Comment 74

[Frank Tang]

nsbeta1+ per triage meeting. 

Comment 75

[Shanjian Li]

Attachment: latest patch in an effort to resolve this problem in 1.0

Comment 76

[Shanjian Li]

dbaron, ian, rbs:
I discuss this issue with ftang and we believe this problem should be addressed
in 1.0 time frame. I post a new patch as an effort to achieve this task. 

1) This patch is based on patch 47300 (posted in 7/26/01). The approach use to
calculate normal line height applies to all languages/characters. This is not a
hacking approach.

2) The normal line height calculation behavior is controled by a hidden
preference. After this patch checked in, QA need to fully test the new behavior
before we switch on to new behavior. 

3) I implement 3 approaches, a. existing one include no external leading in
normal line height; b. strictly rely one font vendor to provide us external
leading and include it in normal line height. c. add extra external leading
(compensation) if font vendor does not provide either internal or external
leading. I suggest to use approach c, and QA should try this first. 
To try approach c, we should have this line in all.js:
pref("browser.display.normal_lineheight_calc_control", 2);

4) I change the way how to calculate line height when a factor is given. Using a
example, factor 1.0 will lead to line_height = emHeight+internalLeading. In
existing code, factor 1.0 is the same as normal line height. Now normal line
include external leading in approach b and c, but factor calculation does not
consider external leading. (My patch is consistent with IE5.5).

5) I change the way how underscore position is calculated. I use the lower of
the position suggest by font vendor and font's descent. The new behavior is also
control by above preference. That's say the underscore position is unchange if
we still use the old way to calculate normal line height. (If we donot count
external leading in normal line height, lower underscore position may make
things worse in certain situation.)

6) The current patch (include font API change) is now only available for windows
platform. After it is accepted by windows, other platforms should follow.

[off-topic] 
NEW_FONT_HEIGHT_APIS is defined for XP_UNIX, XP_PC, XP_MAC, XP_BEOS. Is there
any other platform missing? Is it the right time to eliminate this flag and make
it default? 

Comment 77

[kill this account]

Instead of the term "NEW_FONT_LEADING_APIS" which will be inaccurate in a 
very short time could we use something like "FONT_LEADING_APIS_V2" or some
such?

Terms like "new" always reminds me of the term "Modern Art" which is often
used to mean art from about 1880 to 1980 and leads to such silliness such as
"Post Modern Art" to describe current work.

Comment 78

[Shanjian Li]

I will change it to "FONT_LEADING_APIS_V2". I will repost the patch after it has 
been review. (We already have too many patches.)

Comment 79

[Shanjian Li]

dbaron suggested that comment about internal leading was incorrect. 
By windows documenation, internal leading is part of ascent. 
On windows this is not a calculated value, and it is rather ambiguous in using
"height" 
because we have both emHeight and tmHeight.  I will change it to :
 
  /**
   * Returns the amount of internal leading (in app units) for the font. 
   * Accent marks and other diacritical characters may occur in this area. 
   * It is outside em square.
   */

Comment 80

[Hixie (not reading bugmail)]

Do we really want to add yet more (hidden) prefs? Our font code is getting more
and more complicated with alternative build time and run time options and I
guarentee that they are not all getting QAed. Why don't we remove the behaviours
that we don't want? (e.g. for this patch, why bother leaving the current
behaviour in at all, instead of just switching to what you are proposing?)

Comment 81

[Shanjian Li]

ian, 
This is a risky change and we need to manage the possible risk. I agree that so
many hidden preference is a problem, but this one is just temporary. Once we are
comfortable with the new behavior, we will get rid of the hidden preference. I
will file a new bug for this if we agree to take this approach. 

Comment 82

[Hixie (not reading bugmail)]

If it's risky, let's just check it in without any prefs and then back it out if
we get complaints. We get less work to do that way - assuming all goes well:

   With the pref:
      1. Check it in.
      2. QA the new code.
      e. Remove the pref.

   Without the pref:
      1. Check it in.
      2. QA the new code.

The pref code adds to the risk, anyway.

Comment 83

[Shanjian Li]

I have no problem eliminating the preference, but preference approach does have 
some advantages. It allow QA to switch on and off easily, and we can switch on 
and off without touching much code. It is also possible that we only switch on 
this feature in certain build/release during the migration. Preference itself 
does not add much complexity. I do agree that we should eliminate the preference 
eventually. 

Comment 84

[Hixie (not reading bugmail)]

Fair enough. For what it's worth, as QA I don't think it's much easier to change
a pref than just to use another build. QA frequently have dozens of builds
installed anyway.

Comment 85

[Shanjian Li]

Attachment: repost my last patch, don't know how a typo mixed in my last patch.

Comment 86

[rbs]

Comment on attachment 74020 [details] [diff] [review]
repost my last patch, don't know how a typo mixed in my last patch.

+static eNormalLineHeightControl GetNormalLineHeightCalcControl(void)
+{
+    if (!sPrefIsLoaded) {
+	 // Set up a listener and check the initial value
+	 nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID);
+	 if (prefs) {
+	     prefs->RegisterCallback("browser.blink_allowed", PrefsChanged, 
+				     nsnull);
+	 }
+	 PrefsChanged(nsnull, nsnull);
+	 sPrefIsLoaded = PR_TRUE;
+    }
+    return sNormalLineHeightControl;
+}
+

What is the need for "browser.blink_allowed"? & PrefsChanged(nsnull, nsnull)?
Since the control cannot be changed at run-time, I would leave the blink
stuff alone, and just use a separate self-contained function:

+static eNormalLineHeightControl GetNormalLineHeightCalcControl(void)
+{
  static eNormalLineHeightControl lineheightControl;
  static PRBool firstTime = PR_TRUE;
  if (firstTime) {
    firstTime = PR_FALSE;
    ... init lineheightControl from the pref ...
  }
  return lineheightControl;
}

==============================
+  PRInt32 intPref;
+  if (NS_SUCCEEDED(gPref->GetIntPref(
+      "browser.display.normal_lineheight_calc_control", &intPref)))
+    gMaxUnderscorePos = (intPref != 0);

>gMaxUnderscorePos looks like a cryptic name
>what about "gDoingLineheightFixup" or something like that

================================
+  mInternalLeading =	NSToCoordRound(metrics.tmInternalLeading * dev2app);
+  mExternalLeading =	NSToCoordRound(metrics.tmExternalLeading * dev2app);
+      normalLineHeight = emHeight+ internalLeading + externalLeading;

>nit: whitespace

=======================
 #ifdef NEW_FONT_HEIGHT_APIS
+      nscoord internalLeading;
+      if (!nsHTMLReflowState::UseComputedHeight()) {
+	 fm->GetEmHeight(emHeight);
+	 fm->GetInternalLeading(internalLeading);
+      }
+      lineHeight = NSToCoordRound(factor * (emHeight+internalLeading));
+    } else {

>initialization: internalLeading = 0


=======================================
+    if (unit == eStyleUnit_Factor) {
+      // For factor units the computed value of the line-height property 
+      // is found by multiplying the factor by the font's <b>actual</b> 
+      // em height and internal leading. 
+      float factor;
+      factor = text->mLineHeight.GetFactorValue();
+      // Note: we normally use the actual font height for computing the
+      // line-height raw value from the style context. On systems where
+      // they disagree the actual font height is more appropriate. This
+      // little hack lets us override that behavior to allow for more
+      // precise layout in the face of imprecise fonts.
+      nscoord emHeight = font->mFont.size;
 #ifdef NEW_FONT_HEIGHT_APIS
+      nscoord internalLeading;
+      if (!nsHTMLReflowState::UseComputedHeight()) {
+	 fm->GetEmHeight(emHeight);
+	 fm->GetInternalLeading(internalLeading);
+      }
 #endif
+      lineHeight = NSToCoordRound(factor * (emHeight+internalLeading));
+    } else {
       NS_ASSERTION(eStyleUnit_Normal == unit, "bad unit");
+#ifdef NEW_FONT_HEIGHT_APIS
+      lineHeight = GetNormalLineHeight(aPresContext, fm);
+#else
+      lineHeight = font->mFont.size;
+#endif
     }

>>>>>
I think you are changing the semantics here. I checked the previous
code and it was instead doing:

+    if (unit == eStyleUnit_Factor) {
	...
     }
     else { /* it is here that line-height is normal... */
       /* there is a little gymnastics with  |emHeight = -1|
	 and |normalLineHeight = -1| so that |factor| can end up -1 to
	 keep things positive later, but it boils down the following */

       . check NEW_FONT_HEIGHT_APIS
       . if (nsHTMLReflowState::UseComputedHeight()) {
	   override and use font->size no matter what was computed
	 }
     }
===========
Have you thought of re-organizing things to push the previous #ifdef
NEW_FONT_HEIGHT_APIS
_inside_ your newaly
added |GetNormalLineHeight()|

==========
Hixie had some testcases in bug 27164 to test this line-height stuff to avoid
regressions.

Comment 87

[kill this account]

> I will change it to "FONT_LEADING_APIS_V2". I will repost the patch after 
> it has been review.

Could you do this in the next patch?

Comment 88

[Frank Tang]

please make some real life screen shot that show the problem and how the current
patch (different setting) fix the issue. Thanks.

Comment 89

[Frank Tang]

Is this a P2 or P3 ? set it to P3 for now.

Comment 90

[Shanjian Li]

Attachment: update patch, take suggestions from rbs, bstell, dbaron.

Comment 91

[Shanjian Li]

>>What is the need for "browser.blink_allowed"? & PrefsChanged(nsnull, nsnull)?
>>Since the control cannot be changed at run-time, I would leave the blink
>>stuff alone, and just use a separate self-contained function:
Good suggestion, I just made it self-contained. 

>gMaxUnderscorePos looks like a cryptic name
>what about "gDoingLineheightFixup" or something like that
Done.

>initialization: internalLeading = 0
Done.

>>I think you are changing the semantics here. I checked the previous
>>code and it was instead doing:
Yes, I include internal leading in calculating factor lineheight. I just changed
it back because I thought it was a separated issue.

>>Have you thought of re-organizing things to push the previous #ifdef
>>NEW_FONT_HEIGHT_APIS
Yes. My suggestion is to get rid of this NEW_FONT_HEIGHT_APIS definition. I
filed a separate bug for that. 

>>Hixie had some testcases in bug 27164 to test this line-height stuff to avoid
>>regressions.
Done. Now it works well with my new patch. 

Comment 92

[rbs]

Comment on attachment 74400 [details] [diff] [review]
update patch, take suggestions from rbs, bstell, dbaron.

+// browser.display.normal_lineheight_calc_control is not user changable, so 
+// no need to register callback for it.

move comment where it is most relevant, i.e., next to
GetNormalLineHeightCalcControl()

==================
+    if (unit == eStyleUnit_Factor) {
+      // For factor units the computed value of the line-height property 
+      // is found by multiplying the factor by the font's <b>actual</b> 
+      // em height. 
+      float factor;
+      factor = text->mLineHeight.GetFactorValue();
+      // Note: we normally use the actual font height for computing the
+      // line-height raw value from the style context. On systems where
+      // they disagree the actual font height is more appropriate. This
+      // little hack lets us override that behavior to allow for more
+      // precise layout in the face of imprecise fonts.
+      nscoord emHeight = font->mFont.size;
 #ifdef NEW_FONT_HEIGHT_APIS
+      nscoord internalLeading = 0;
+      if (!nsHTMLReflowState::UseComputedHeight()) {
+	 fm->GetEmHeight(emHeight);
+      }
 #endif
+      lineHeight = NSToCoordRound(factor * emHeight);
+    } else {
       NS_ASSERTION(eStyleUnit_Normal == unit, "bad unit");
+#ifdef NEW_FONT_HEIGHT_APIS
+      lineHeight = GetNormalLineHeight(aPresContext, fm);
+#else
+      lineHeight = font->mFont.size;
+#endif
     }
   }
   return lineHeight;
 }
>>>>>>>>>>>>>>>

+      nscoord internalLeading = 0;
> not used

But looks to me like it is don't cover all the edge cases.
What I think what is needed is something like this (more
readable):

  if (unit == eStyleUnit_Coord) {
    // ok, nothing to see here, it is already fine
    ...
  }
  else { // unit is a factor or normal
    if (unit == eStyleUnit_Factor) {
      // the factor applies to what? To the font-size, tentatively.
      emHeight = font->mFont.size;
#if NEW_FONT_HEIGHT_APIS or NEW_FONT_HEIGHT_APIS_V2 (agree?)
       if (!nsHTMLReflowState::UseComputedHeight()) {
	 -> nope, it applies to GetEmHeight()
       }
#end
      lineHeight = NSToCoordRound(factor * emHeight);
    }
    else { // unit is normal
      lineheight = font->size;
#if NEW_FONT_HEIGHT_APIS or NEW_FONT_HEIGHT_APIS_V2
      if (!nsHTMLReflowState::UseComputedHeight()) {
	lineheight = ComputeLineHeight(aPresContext, aRenderingContext, sc);
	//... and ComputeLineHeight() will sort out between v1 and v2
      }
#end
    }
  }

Comment 93

[Shanjian Li]

rbs, 

This is a very good point I ignored. The logic behind is not very clear to me when 
nsHTMLReflowState::UseComputedHeight() is PR_TRUE. The code was there for bug 6865. 
And very fortunately, there is testcases with it. I am going to revisit that bug 
and to figure out what should be the right way for normal lineheight. The old logic 
seems like :
normalLineHeight = font_size * (fm.emHeight+fm.Leading)/fm.emHeight;

My build is crashing in start every time. I cann't do anything right now. 

Comment 94

[Shanjian Li]

Attachment: update patch (moved comment, removed unused variable)

Comment 95

[Shanjian Li]

rbs, 

I checked the logic behind those code, and I believe my current approach was fine. 
When factor is 1.0, we need to use font size as line height. But in case of normal
line height, since internal leading is calculated and the final line height is not 
strictly controled by font size, I would rather use my current approach instead of 
this "fontSize*(fm.emHeight+fm.leading)/fm.emHeight" approach. What do you think?
Or I still did not understand your point?

Comment 96

[rbs]

You are just missing some points as I mentioned earlier. Your approach was/is 
fine. I did read the code and could see what is happening with 
UseComputedHeight().

>When factor is 1.0, we need to use font size as line height.

That's not accurate. The font-size is used as the lineheight only when 
UseComputedHeight() is true. [UseComputedHeight() could have been given a better 
name perhaps -- that's why I often nitpick about names -- In his checkin v1.59,
kipp intended UseComputedHeight() to mean "use the computed cascading font-size 
from CSS" because fm->GetFontHeight() could have returned a bad value on some 
systems/fonts.]

+    if (unit == eStyleUnit_Factor) {
+      // For factor units the computed value of the line-height property 
+      // is found by multiplying the factor by the font's <b>actual</b> 
+      // em height. 
+      float factor;
+      factor = text->mLineHeight.GetFactorValue();
+      // Note: we normally use the actual font height for computing the
+      // line-height raw value from the style context. On systems where
+      // they disagree the actual font height is more appropriate. This
+      // little hack lets us override that behavior to allow for more
+      // precise layout in the face of imprecise fonts.
+      nscoord emHeight = font->mFont.size;
 #ifdef NEW_FONT_HEIGHT_APIS

>> what I did above was to add |NEW_FONT_HEIGHT_APIS_V2| here too. To put it
>> another way, with your code NEW_FONT_HEIGHT_APIS_V2 cannot be used without
>> NEW_FONT_HEIGHT_APIS. Since the intention is to move gradually to V2,
>> the two settings should work independently. (To test things properly, you
>> should remove the setting of NEW_FONT_HEIGHT_APIS to see what happens.)

+      if (!nsHTMLReflowState::UseComputedHeight()) {
+        fm->GetEmHeight(emHeight);
+      }
 #endif
+      lineHeight = NSToCoordRound(factor * emHeight);
+    } else {
       NS_ASSERTION(eStyleUnit_Normal == unit, "bad unit");
+#ifdef NEW_FONT_HEIGHT_APIS
+      lineHeight = GetNormalLineHeight(aPresContext, fm);
+#else
+      lineHeight = font->mFont.size;
+#endif

>>here too, you will be using |font->mFont.size| all the time if 
>>NEW_FONT_HEIGHT_APIS isn't set. Since font->mFont.size is often equals to
>>fm->GetHeight(), it is hard to see problems when testing with your good fonts.
>>That's why I suggested the pseudo-code above which will avoid nasty suprises
>>further down the track.
     }
   }
   return lineHeight;
 }

[Side note: The existing code is broken if NEW_FONT_HEIGHT_APIS is undefined.
I see some code paths that lead to nowhere when it is unset.]

Comment 97

[Shanjian Li]

rbs, 
Now I understood what you mean. Making NEW_FONT_HEIGHT_APIS_V2 depend on
NEW_FONT_HEIGHT_APIS was part of original intention, and that's why I never
thought it was a problem even when I was reading your comment. Since leading is
returned to layout for calculating lineheight, and emHeight must be also known
to layout before this leading make any sense. That's say NEW_FONT_HEIGHT_APIS_V2
should never be defined if NEW_FONT_HEIGHT_APIS is not. I filed a bug about
removing NEW_FONT_HEIGHT_APIS, the response I received from that bug suggesting
NEW_FONT_HEIGHT_APIS has been defined for all platforms and we can remove it and
make it default. Since I am going to take care of it pretty soon, I don't think
it will cause any problem. Does that sound reasonable to you?

Comment 98

[rbs]

OK, so it remains to fix the following bit:

+#ifdef NEW_FONT_HEIGHT_APIS
+      lineHeight = GetNormalLineHeight(aPresContext, fm);
+#else
+      lineHeight = font->mFont.size;
+#endif

It should instead read:

      lineheight = font->size;
#if NEW_FONT_HEIGHT_APIS
      if (!nsHTMLReflowState::UseComputedHeight()) {
        lineheight = GetNormalLineHeight(); //...will sort out between v1 and v2
      }
#end

(this way, the CSS font-size is used as the lineheight upon checking
useComputedHeight as kipp intended -- unfortunately, he didn't provide further
details regarding the bad fonts that he encountered during his testings.)

Comment 99

[Shanjian Li]

When nsHTMLReflowState::UseComputedHeight() is true, should we use 
"font->mFont.size" or "GetNormalLineHeight()" for normal lineheight? 
I prefer the later. The existing logic:
  normalLineHeight = font->mFont.size *(fm->emHeight+fm->leading)/fm->emHeight
seems suggesting some leading, and that makes me believe "GetNormalLineHeight"
is more appropriate.   

Comment 100

[rbs]

-> font-size (I have double-checked v1.59 of kipp's checkin in the bonsai's log)
as I was reviewing your patch. C.f. comment #96.

Comment 101

[Shanjian Li]

Attachment: update patch as suggested by rbs

Comment 102

[Shanjian Li]

The only change made in my latest patch is the one mentioned in rbs comment #98. 
rbs, could you mark r= ?

Comment 103

[rbs]

Comment on attachment 76289 [details] [diff] [review]
update patch as suggested by rbs

looks good, r=rbs

Comment 104

[Shanjian Li]

marc, could you sr my patch?

Comment 105

[Marc Attinasi]

Comment on attachment 76289 [details] [diff] [review]
update patch as suggested by rbs

Shouldn't this:
+static_cast<eNormalLineHeightControl>(intPref);
use NS_STATIC_CAST instead?
GetNormalLineHeight should PRECONDITION the input arguments against null

Please be sure that you compile with the new define FONT_LEADING_APIS_V2
#undefined too.

So, I'm assuming that with the pref turned OFF, you verified pixel for pixel
identical display... sr=attinasi

Comment 106

[Shanjian Li]

Attachment: patch take attinasi's suggestion

Comment 107

[Frank Tang]

please make a screenshot for a real-life example (top sites) which show you the
problme. Make one before-the-patch, one in IE , one in after-the-patch, and
optionally make one for 4.x

Comment 108

[Frank Tang]

Impact Summery
Impact Platform: ALL
Impact language users: CJK users 132.8 M 23.7%
Probability of hitting the problem: HIGH, show on major websites
Severity if hit the problem in the worst case: Some of the text will be
difficult to read
Way of recover after hit the problem: None
Risk of the fix: HIGH
Potential benefit of fix this problem: 

Comment 109

[Shanjian Li]

Attachment: yahoo china, home page, before

Comment 110

[Shanjian Li]

Attachment: yahoo china home page, with switch on after my patch

Comment 111

[Shanjian Li]

Attachment: yahoo japan home page, before

Comment 112

[Shanjian Li]

Attachment: yahoo japan home page, with switch on after my patch

Comment 113

[Shanjian Li]

Attachment: yahoo japan news page, before

Comment 114

[Shanjian Li]

Attachment: yahoo japan news page, with switch on after my patch

Comment 115

[Shanjian Li]

Attachment: sina home page, before

Comment 116

[Shanjian Li]

Attachment: sina home page, with switch on afte my patch

Comment 117

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

Comment on attachment 76595 [details] [diff] [review]
patch take attinasi's suggestion

a=dbaron for trunk checkin.  

It would be nice to see patches for non-Windows platforms to use the new API,
hopefully not too far in the future.

Comment 118

[Frank Tang]

shanjian- please make screenshot with IE also. 

Comment 119

[Frank Tang]

>Risk of the fix: HIGH
since this is a HIGH impack HIGH risk fix, shanjian put in a pref to turn it to
the origioanl behavior 

After see the screenshot, I think this is a adt2 not a adt3. the underline
touching with CJK characters definitely make it very very difficult to
distingish the text in small (10 and below) size font. 

adt1.0.0, please grant check in permission to land with the pref turn ON, if it
cause any problem after QA test it, we can turn OFF the pref to back up to the
origional behavior. 

Comment 120

[Jaime Rodriguez, Jr.]

removing adt1.0.0 nomination per discussion with ftang and adt.  Once the
Mozilla1.0 branch happens, this should go on the trunk and bake.

Comment 121

[Shanjian Li]

Attachment: yahoo china home page, ie5.5

Comment 122

[Frank Tang]

>------- Additional Comment #120 From Jaime Rodriguez, Jr. 2002-04-03 18:20 -------
>removing adt1.0.0 nomination per discussion with ftang and adt.  Once the
>Mozilla1.0 branch happens, this should go on the trunk and bake.

Here is more detail documentation about the reason of the decision:
1. this is a high risk high impact bug, we should consider take it but it is too
risky
2. There are two kind of risk associate with the patch, 
risk kind 1- coding risk, the patch is big so by nature it have more risk, this
code will be used for every single text display on the screen. therefore it is
high risk
risk kind 2- even all the code do what the developer want to do, we still have a
behavior risk here. The change of the logic itself have risk to break some page
display.

3. however, this patch will greatly address important issue with CJK underline
display to make the display to be reasonable and compatable with IE. so we
really don't want to - this bug

we decide to remove the adt1.0.0 for now. after we branch off m1.0, shanjian
should land the patch into the trunk  with the pref turn on and bake for 3-4
days. Once QA confirm it does not generate any negative impact on top English as
well as top intl sites, then we ask adt to reconsider to take it into the m1.0
branch.

Comment 123

[Shanjian Li]

fix checked in to trunk with switch turned on. 

Comment 124

[Frank Tang]

please pro-actively ask IQA to test trunk mac,linux, win these several days so
we have confidentl to ask for branch check in. 

Comment 125

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

I suspect this caused bug 136935.  There's a chance I caused it with my changes
for bug 5693, but this seems more likely, and bug 136935 has so far been
reported only on Windows.

Comment 126

[Ricky Webb]

Is this feature only going to be on by default on CJK Fonts? Because it seems
that recent builds has the underline change on English fonts.

Comment 127

[Shanjian Li]

No, this feature applies to all languages/fonts. 

Comment 128

[Frank Tang]

so we know if we want land this bug into m1.0 then we need to take also the fix
for bug 136935 . any other issue ?

Comment 129

[Frank Tang]

pmac- can you also verify the trunk is ok after this bug land ?

Comment 130

[Frank Tang]

rbs and shanjian- I notice that you use some #ifdef in the code and different
platform may implement different version of the font methods. Could you nicely
file bug for mac against me ([e-mail]) and put down what I should do
for mac ? (also cc [e-mail] please) Thanks. 

Comment 131

[Yuying Long]

Attachment: yahoo-china on 04-12 trunk build.

On 04-12 trunk build, the line hight looks good.

Comment 132

[Shanjian Li]

bug 137817 was filed for mac. 

Comment 133

[Yuying Long]

Attachment: yahoo-china on 04-16 trunk build

Not very good on 04-16 trunk build with CJK pages.
But I don't see much difference between 04-12 and 04-15 trunk build with
western pages.

Comment 134

[Frank Tang]

we don't have solution to greatly improve the cjk display and the current patch
is too big and risky. 
mark this as nsbeta1- so shanjian can focus on detector issue. 

Comment 135

[Shanjian Li]

Attachment: a problem found by marc and his patch (bug 137112)

Comment 136

[Hideo Saito]

Attachment: v1 patch, line space is extended and an underline is detached

The effect of a patch is in the only case of X11/gtk/bitmap-font. 
Since spacing was narrow in the case of Japanese fonts, it extended 1.2 times.
The underline in the case of Japanese fonts was considered as the place which
fell descent from the baseline.
In this case, by scrolling, since the underline might disappear, it was coped
with.

It has judged whether it is the target font by whether "font size" is equal to
"ascent+descent".
In the case of an English font, it is not usually equal.

Comment 137

[Shanjian Li]

Saito san,
Thanks for addressing this problem. I have some suggestions. This bug report is
too long, so let's leave this just for windows. There are in fact 2 problems
need to be handled here:
1, Underline should be better put under descent, but this can put underline
outside of the line territory. Extending the terrritory or raise the baseline
are 2 options. Bug 136935 is for such a problem, but the fix is not good. We
need to find a good solution to that problem. Your patch seems extending the
lineHeight, but it may have side-effect. Especially when user specify 1.0 as
line height, font size and line height is no longer equal. 
2, Fix this problem for GTK. I would suggesting using the same approach  as for
this bug. That is to implement the new leading API. 

We might want to reopen bug 136935 or file a new one to handle issue No. 1, and
open another bug for issue No. 2.  

Comment 138

[Hideo Saito]

Attachment: patch v2

> Your patch seems extending the lineHeight, but it may have side-effect.

The patche was rewritten based on your advice.
If the new bug in relation with this patch is opened, please evaluate this
patch there.

Comment 139

[Yuying Long]

REnominate as nsbeta1 - we have some regression, and it's really bad when visit
CJK pages.

Comment 140

[nhottanscp]

>REnominate as nsbeta1 - we have some regression, and it's really bad when visit
>CJK pages.
Is the regression seen on the trunk only or branch also?
Was the patch checked in to the trunk only or also to 1.0.0 branch?

Comment 141

[Yuying Long]

>>REnominate as nsbeta1 - we have some regression, and it's really bad when visit
>>CJK pages.
> Is the regression seen on the trunk only or branch also?

Sorry, it was typo - I meant "newer solution" not "regression".

And I think the patch haven't checked into trunk yet.

Comment 142

[Shanjian Li]

Saito san, 
Personally I don't think it is a good idea to modify nsRect. I have no better
idea yet. But in my opinion, your last approach is more acceptable that v2. If
it is too difficult to raise baseline, extending lineheight might be an option.

Comment 143

[Frank Tang]

I don't think the change of nsRect will lead to anywhere. No body in the review
process will accept us changin the definition of nsRect. 
mark it as nsbeta1- for now. We don't have a good solution yet. 

Comment 144

[Shanjian Li]

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

Comment 145

[rbs]

Attachment 86971 [details] from the duplicate bug 150298 shows how Mozilla looks really 
bad compared to IE. This bug needs _something_ to be done.

Modifying nsRect the way it is proposed isn't going to buy anybody in.

Re: comment #142
"If it is too difficult to raise baseline, extending lineheight might be an 
option."

Now that layout also does line-layout using dynamic font's height (bug 99010), I 
think another approach could be to:
1) undo the previous patch (v2)
2) eliminate the GetLeading() API altogether
3) apply the fixup on GetMaxAscent() and/or GetMaxDescent()

Indeed if the font's ascent/descent are tweaked (say, e.g., ascent *= 1.5, 
descent *= 1.5), they will achieve the same effect as v2. However, they will 
have the advantage of abstracting the amplification further down, and limiting 
other regressions.

saito, do mind having a go at what I propose above. The proof-of-concept is just 
to try to use |metrics.tmAscent * 1.5|, |metrics.tmDescent * 1.5|, 
|metrics.tmHeight * 1.5| in nsFontMetricsWin::InitMetricsFor() and 
nsFontMetricsWin::RealizeFont(). If this gives encouraging effects on CJK texts, 
then it would meant that the proposal could be further explored. If you give 
this a go, please report what you see with the proof-of-concept before trying 
further things.

Comment 146

[Roland Mainz]

Comment on attachment 84022 [details] [diff] [review]
patch v2

The patch does not cover  Xlib gfx (e.g. gfx/src/xlib) ... is it OK that I wait
for a new patch or should I file a combined patch for GTK+ gfx and Xlib gfx now
?

Comment 147

[Hideo Saito]

Attachment: patch v3 (test)

I did a test.
As a result, the same effect as patch v2 was acquired.
That is, spacing spreads, an underline comes down downward and, in scrolling,
an underline does not disappear.

Comment 148

[Hideo Saito]

Attachment: patch v3 (test)(new)

I am sorry, the patch had a careless mistake.
A test result does not change.

Comment 149

[rbs]

> spacing spreads, an underline comes down downward and, in scrolling,
> an underline does not disappear.

So this is promising then. Shanjian, mind taking a look and pursuing this?
(from my cursory look at the patch, I had the gut feeling that the computations
could be simplified.)

Comment 150

[Shanjian Li]

This approach (patch v3) does not seems acceptable to me. Yes, we can change
ascent/descent to simulate leading with font metrics, but then we cann't correct
handle non-normal lineheight there after. For example, when css specify
lineheight to be 1.0, which means no leading, how do we know what underline
position to return? Since font metric has no idea about this, it has no way to
figure that out. 

I think font metric should only return font specific data, and let layout code
deal with this issue. 

Comment 151

[Katsuhiko Momoi]

topembed. needed for embedding clients.

Comment 152

[rbs]

Re: comment 150 -- 
Fair enough. You have been consistent with the wish of seeing this bug fixed
from the layout side (e.g., comment #69). But seeing how the fix has been
elusive to us for quite some time now, maybe we can relax the expectations.
(Attachment 86971 [details] speaks for itself -- even loyal users will prefer IE over
Mozilla.)

As before, there can be a pref to experiment the changes and maybe emulating
what IE is doing all the way might be worth it:

> Comment #32 From Frank Tang 2001-07-09 11:14 ------- 
>
> ok, it looks a high risk fix and need more discussion.
> >It seems IE use descent for chinese character and underscore for ASCIIs. 
> > From my experient, descent works better in both case. 
> Can we do that? Will that lower the risk for ASCII ?
>
> Comment #33 From Shanjian Li 2001-07-09 15:38 ------- 
>
> to answer ftang's question:
> To change underline position for Chinese characters without adjusting
> lineheight as suggested in this bug will make the page looks even worse. 

If a little delta is added to the ascent for CJK fonts with 0-leading, then it
will adjust the line-height as well.

Comment 153

[Jaime Rodriguez, Jr.]

Marking this as nsbeta1+, as this has been requested by a major embedding
customer. Let's try and get a fix for this one asap.

Roy/Bob/Shajian - Can you get together on this one in ftang's absence to see of
this can be fixed for 1.0.2?

Comment 154

[bobj]

ftang is back! I told him this has been escalated to topembed and nsbeta1+.

Comment 155

[Shanjian Li]

We are talking about 2 issues in this bug. The first one is the line height, as
suggested by summary. The 2nd one is underline (underscore). If lineheight is the 
major concern, that has been fixed with my patch in trunk. For underline position 
problem, we don't have a fix yet, but I think it is less important and we can
live without it. So please clarify what is user's real concern?

I filed bug 156943 for underline position problem. I just got some idea and but
need more investigation. 
Bug 156945 is filed for line height problem on linux.

I will mark this bug as fixed. I understand that we may have to find a different
solution for this bug when fixing 156943. If so, let's reopen this bug later. 

Comment 156

[Jaime Rodriguez, Jr.]

Momoi/ftnag - Does underline position (bug 156943) best desrcibe the problem for
the major embeddor? If yes, then we should lower the priority of this one, and
not request that it be added to the branch, and pursue the inclusion bug 156943.
Should you disagree, and think this one needs to be on the branch, as well, pls
nominate it, by adding adt1.0.1, and come to an ADT meeting to discuss it. thanks!

Comment 157

[bobj]

Shanjian,
 - This is fixed only in the trunk, right?  How do we track this for the branch?
 - Does the fix for underlining probably require this fix for line height?
   

Comment 158

[Shanjian Li]

 - This is fixed only in the trunk, right?  How do we track this for the branch?
Yes. We track branch checkin with keyword. 

 - Does the fix for underlining probably require this fix for line height?
yes, definitely. So either we want this one alone, or we want them both. I just
found a patch for that problem.    

Comment 159

[Yuying Long]

I'm marking veirfied for this line height problem since it has been checked in
trunk for a while.

Comment 160

[Jaime Rodriguez, Jr.]

Shanjian/Bob - Are you saying that bug 156943, is dependent on this fix? If yes,
then pls mark this one as blocking bug 156943. thanks!

Comment 161

[Roland Mainz]

Yuying Long wrote:
> I'm marking veirfied for this line height problem since it has been checked in
> trunk for a while.

The patch part for mozilla/gfx/src/nsFontMetricsGTK.cpp from attachment 76595 [details] [diff] [review]
was not checked in yet, right ?

Comment 162

[Shanjian Li]

Roland, bug 156945 is filed for GTK/Xlib. But I won't worry about it before bug
156943 is settled.

Comment 163

[Shanjian Li]

Attachment: update the trunk patch for branch.

Comment 164

[Shanjian Li]

Comment on attachment 92041 [details] [diff] [review]
update the trunk patch for branch.

carry over r/sr

Comment 165

[Frank Tang]

land into 1.0.1 by using nhotta's account from ftang for shanjian

Comment 166

[Jaime Rodriguez, Jr.]

posthumus adt1.0.1+

Comment 167

[Teruko Kobayashi]

Shanjian, is this fix for only Win32?  I have to verify this in 1.0.1 branch
build.  Since platform says All in this bug, I tested 8-23 branch Win32, Mac and
Linux build.  I saw the fix in Win32, but there are some problems in Mac and
Linux build.
If this is only for Win32, we need to change the platform to PC.

Comment 168

[Shanjian Li]

Yes, this bug is for windows only.

Comment 169

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

The fix may be for Windows only, but the bug was not.  Where are the bugs on
getting this fixed on other platforms?

Comment 170

[Hixie (not reading bugmail)]

Reopening per comments. Please resolve this bug only if new bugs have been filed
for the other platforms.

Comment 171

[Shanjian Li]

see comment 155, this bug is for windows only. 

Comment 172

[Hixie (not reading bugmail)]

So where are the bugs for the other platforms?

(comment 155 is presumably not the one you meant)

Comment 173

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

This bug is not Windows-only.  It's a major technical issue on all platforms,
and you only fixed it on Windows.  That probably shouldn't even have been
checked in without platform parity, and platform parity needs to be addressed. 
Reopening.

Comment 174

[Jaime Rodriguez, Jr.]

Removing fixed1.0.1, as this wasn't fixed for all platforms. We should fix it
for all platforms, when we get the opportunity.

Comment 175

[Yuying Long]

Bug 137817 is filed for Mac (comment #132) and bug 156945 is filed for linux (
comment #155).

This bug is original filed for all platforms, and then we splitted it as per
each platform.

Comment 176

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

OK, restoring fixed resolution.

Comment 177

[Yuying Long]

Verified as such.