Seak, Teng-Fong Reporter
	Description • 24 years ago

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.

	Asa Dotzler [:asa]
	Comment 1 • 24 years ago

updating component

	Seak, Teng-Fong Reporter
	Comment 2 • 24 years ago

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

	Frank Tang
	Comment 3 • 24 years ago

good polishing issue. Mark this as future.

	Keyser Sose
	Comment 4 • 24 years ago

Marking NEW.

	Shanjian Li Assignee
	Comment 6 • 24 years ago

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.

	Shanjian Li Assignee
	Comment 7 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 8 • 23 years ago

Let's get this one into the branch.

	Shanjian Li Assignee
	Comment 10 • 23 years ago

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:

	Shanjian Li Assignee
	Comment 12 • 23 years ago

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. 

	Frank Tang
	Comment 13 • 23 years ago

move into 93.

	Frank Tang
	Comment 14 • 23 years ago

rbs, can you review this ?

	rbs
	Comment 15 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 16 • 23 years ago

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. 

	Frank Tang
	Comment 17 • 23 years ago

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

	rbs
	Comment 18 • 23 years ago

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

	Shanjian Li Assignee
	Comment 19 • 23 years ago

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. 

	rbs
	Comment 20 • 23 years ago

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?

	rbs
	Comment 21 • 23 years ago

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

	Shanjian Li Assignee
	Comment 22 • 23 years ago

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. 

	rbs
	Comment 23 • 23 years ago

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;

	Shanjian Li Assignee
	Comment 24 • 23 years ago

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. 

	Shanjian Li Assignee
	Comment 26 • 23 years ago

Chris, can you do a sr?

	Chris Waterson
	Comment 27 • 23 years ago

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?

	Shanjian Li Assignee
	Comment 28 • 23 years ago

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. 

	David Baron :dbaron:
	Comment 29 • 23 years ago

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?

	Shanjian Li Assignee
	Comment 30 • 23 years ago

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. 

	Frank Tang
	Comment 32 • 23 years ago

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 ?

	Shanjian Li Assignee
	Comment 33 • 23 years ago

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. 

	石庭豐 (Seak, Teng-Fong)
	Comment 34 • 23 years ago

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.

	Hixie (not reading bugmail)
	Comment 35 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 36 • 23 years ago

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.

	David Baron :dbaron:
	Comment 39 • 23 years ago

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

	Hixie (not reading bugmail)
	Comment 40 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 41 • 23 years ago

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.  

	David Baron :dbaron:
	Comment 44 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 46 • 23 years ago

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

	David Baron :dbaron:
	Comment 48 • 23 years ago

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

	David Baron :dbaron:
	Comment 49 • 23 years ago

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

	Shanjian Li Assignee
	Comment 50 • 23 years ago

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. 

	Shanjian Li Assignee
	Comment 51 • 23 years ago

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

	rbs
	Comment 52 • 23 years ago

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.

	rbs
	Comment 53 • 23 years ago

"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

	rbs
	Comment 54 • 23 years ago

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

	Shanjian Li Assignee
	Comment 55 • 23 years ago

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. 

	David Baron :dbaron:
	Comment 57 • 23 years ago

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

	Hixie (not reading bugmail)
	Comment 58 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 59 • 23 years ago

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. 

	David Baron :dbaron:
	Comment 60 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 61 • 23 years ago

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. 

	Shanjian Li Assignee
	Comment 62 • 23 years ago

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 

	rbs
	Comment 63 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 65 • 23 years ago

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.  

	rbs
	Comment 66 • 23 years ago

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

	Shanjian Li Assignee
	Comment 67 • 23 years ago

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. 

	Chris Waterson
	Comment 68 • 23 years ago

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

	Shanjian Li Assignee
	Comment 69 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 70 • 23 years ago

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

	kill this account
	Comment 71 • 23 years ago

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

	石庭豐 (Seak, Teng-Fong)
	Comment 73 • 23 years ago

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?

	Frank Tang
	Comment 74 • 23 years ago

nsbeta1+ per triage meeting. 

	Shanjian Li Assignee
	Comment 76 • 23 years ago

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? 

	kill this account
	Comment 77 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 78 • 23 years ago

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

	Shanjian Li Assignee
	Comment 79 • 23 years ago

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

	Hixie (not reading bugmail)
	Comment 80 • 23 years ago

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

	Shanjian Li Assignee
	Comment 81 • 23 years ago

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. 

	Hixie (not reading bugmail)
	Comment 82 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 83 • 23 years ago

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. 

	Hixie (not reading bugmail)
	Comment 84 • 23 years ago

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.

	rbs
	Comment 86 • 23 years ago

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.

	kill this account
	Comment 87 • 23 years ago

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

	Frank Tang
	Comment 88 • 23 years ago

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

	Frank Tang
	Comment 89 • 23 years ago

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

	Shanjian Li Assignee
	Comment 91 • 23 years ago

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

	rbs
	Comment 92 • 23 years ago

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

	Shanjian Li Assignee
	Comment 93 • 23 years ago

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. 

	Shanjian Li Assignee
	Comment 95 • 23 years ago

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?

	rbs
	Comment 96 • 23 years ago

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

	Shanjian Li Assignee
	Comment 97 • 23 years ago

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?

	rbs
	Comment 98 • 23 years ago

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

	Shanjian Li Assignee
	Comment 99 • 23 years ago

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.   

	rbs
	Comment 100 • 23 years ago

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

	Shanjian Li Assignee
	Comment 102 • 23 years ago

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

	rbs
	Comment 103 • 23 years ago

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

looks good, r=rbs

	Shanjian Li Assignee
	Comment 104 • 23 years ago

marc, could you sr my patch?

	Marc Attinasi
	Comment 105 • 23 years ago

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

	Frank Tang
	Comment 107 • 23 years ago

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

	Frank Tang
	Comment 108 • 23 years ago

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: 

	David Baron :dbaron:
	Comment 117 • 23 years ago

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.

	Frank Tang
	Comment 118 • 23 years ago

shanjian- please make screenshot with IE also. 

	Frank Tang
	Comment 119 • 23 years ago

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

	Jaime Rodriguez, Jr.
	Comment 120 • 23 years ago

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.

	Frank Tang
	Comment 122 • 23 years ago

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

	Shanjian Li Assignee
	Comment 123 • 23 years ago

fix checked in to trunk with switch turned on. 

	Frank Tang
	Comment 124 • 23 years ago

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

	David Baron :dbaron:
	Comment 125 • 23 years ago

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.

	Ricky Webb
	Comment 126 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 127 • 23 years ago

No, this feature applies to all languages/fonts. 

	Frank Tang
	Comment 128 • 23 years ago

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 ?

	Frank Tang
	Comment 129 • 23 years ago

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

	Frank Tang
	Comment 130 • 23 years ago

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 (ftang@netscape.com) and put down what I should do
for mac ? (also cc sfraser@nescape.com please) Thanks. 

	Yuying Long
	Comment 131 • 23 years ago

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

	Shanjian Li Assignee
	Comment 132 • 23 years ago

bug 137817 was filed for mac. 

	Yuying Long
	Comment 133 • 23 years ago

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.

	Frank Tang
	Comment 134 • 23 years ago

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. 

	Hideo Saito
	Comment 136 • 23 years ago

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.

	Shanjian Li Assignee
	Comment 137 • 23 years ago

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.  

	Hideo Saito
	Comment 138 • 23 years ago

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

	Yuying Long
	Comment 139 • 23 years ago

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

	nhottanscp
	Comment 140 • 22 years ago

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

	Yuying Long
	Comment 141 • 22 years ago

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

	Shanjian Li Assignee
	Comment 142 • 22 years ago

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.

	Frank Tang
	Comment 143 • 22 years ago

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. 

	Shanjian Li Assignee
	Comment 144 • 22 years ago

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

	rbs
	Comment 145 • 22 years ago

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.

	Roland Mainz
	Comment 146 • 22 years ago

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
?

	Hideo Saito
	Comment 147 • 22 years ago

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.

	Hideo Saito
	Comment 148 • 22 years ago

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

	rbs
	Comment 149 • 22 years ago

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

	Shanjian Li Assignee
	Comment 150 • 22 years ago

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. 

	Katsuhiko Momoi
	Comment 151 • 22 years ago

topembed. needed for embedding clients.

	rbs
	Comment 152 • 22 years ago

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.

	Jaime Rodriguez, Jr.
	Comment 153 • 22 years ago

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?

	bobj
	Comment 154 • 22 years ago

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

	Shanjian Li Assignee
	Comment 155 • 22 years ago

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. 

	Jaime Rodriguez, Jr.
	Comment 156 • 22 years ago

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!

	bobj
	Comment 157 • 22 years ago

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?
   

	Shanjian Li Assignee
	Comment 158 • 22 years ago

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

	Yuying Long
	Comment 159 • 22 years ago

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

	Jaime Rodriguez, Jr.
	Comment 160 • 22 years ago

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!

	Roland Mainz
	Comment 161 • 22 years ago

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 ?

	Shanjian Li Assignee
	Comment 162 • 22 years ago

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

	Shanjian Li Assignee
	Comment 164 • 22 years ago

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

carry over r/sr

	Frank Tang
	Comment 165 • 22 years ago

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

	Jaime Rodriguez, Jr.
	Comment 166 • 22 years ago

posthumus adt1.0.1+

	Teruko Kobayashi
	Comment 167 • 22 years ago

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.

	Shanjian Li Assignee
	Comment 168 • 22 years ago

Yes, this bug is for windows only.

	David Baron :dbaron:
	Comment 169 • 22 years ago

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

	Hixie (not reading bugmail)
	Comment 170 • 22 years ago

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

	Shanjian Li Assignee
	Comment 171 • 22 years ago

see comment 155, this bug is for windows only. 

	Hixie (not reading bugmail)
	Comment 172 • 22 years ago

So where are the bugs for the other platforms?

(comment 155 is presumably not the one you meant)

	David Baron :dbaron:
	Comment 173 • 22 years ago

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.

	Jaime Rodriguez, Jr.
	Comment 174 • 22 years ago

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

	Yuying Long
	Comment 175 • 22 years ago

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.

	David Baron :dbaron:
	Comment 176 • 22 years ago

OK, restoring fixed resolution.

	Yuying Long
	Comment 177 • 22 years ago

Verified as such.