The way we get vertical (ascent, descent, leading) font metrics from X11 gives bad results. In particular, we assume that ascent+descent gives the recommended line spacing and that the character boundaries in max_bounds will give something smaller than that that excludes leading. However, it is almost always the reverse (even for TrueType fonts that I know on Windows to have good metrics). The ascent+descent is generally smaller than the max bounds of the characters. If we use max_bounds to mean recommended line spacing, we end up with very large line spacing for certain fonts (in particular Courier or Courier New or something of that sort). I'm beginning to think that the best solution may be to ignore max_bounds entirely and get everything we need from ascent and descent. One of the big problems with the way we do things now is that it leads to a class of X11-only bugs because text frames routinely overflow their parent because we use a smaller line spacing than the size of the text frame (since the normal line spacing is correctly derived from |GetNormalLineHeight| while the size of a text frame is correctly derived from |GetHeight| which should always be the sum of |GetMaxAscent| and |GetMaxDescent|). This probably causes performance problems in event handling, it causes scrollbars to appear on textareas after typing a single character, it probably causes some quasi-infinite loop problems in sprocket layout, and it causes a number of visual problems. On other platforms, the result of |GetHeight| is the ascent and descent of the font minus the internal leading, but X doesn't have a concept of internal leading.

Erik van der Poel had some interesting comments on this issue on bug 59825.

Seems reasonable to me. It'd be interesting to compare this with what 4.x and Opera do given the same font. (Not that web designers are writing a lotta content for X browsers, but...) Regardless, r/sr=waterson

I am a little worried about the possible impact to websites of other languages. My unix box is offline, so I just checked with some locally stored traditional chinese web pages. The normal line-height is a little bit too small. But it is that way before you patch. So probably it is a good idea to get rid of max_bounds, and add some extra leading (20% of mEmHeight?) to normal line-height. Any way, if you can check some CJK websites to make sure it didn't make things worse before your checking in, that will be appreciated.

Any idea how this will interact with fill-in glyphs that are larger/smaller than line size?

The Mozilla font code has a lot of code to do glyph fill in if the default glyph does not have the glyphs. Because of this it is often very hard to determine where a glyph came from. The font debug macros are an important tool I frequently ask users to enable so thay can help me determine what their system is doing. Unless someone else wants to take responsibility for solving problems where one or more glyphs seem wrong then please do not turn these debug feature off in optimized builds.

shanjian: This change doesn't affect line spacing or 'line-height', it only affects the extent of backgrounds and borders drawn on inline elements (including things like selection). bstell: I don't understand your 17:03 comment. bstell: I'm really uncomfortable shipping debugging code that's very rarely needed in the default binaries. It bloats the code and slows down performance. It doesn't make much of a difference if it's only done in one place, but if everybody else did what you want here the binaries would be much bigger and we'd be slower. So why not set a good example and remove it? Couldn't you provide your own (debug?) builds for people to test when they actually need to use this debugging code?

Re shipping code that helps debug problems in the field: Netscape for a long time has included code to help track crashes in the field so we could debug them. These 36 "if (gDebug&bit) printf(...)" are very small in comparison. I believe I *am* setting a good example by having tools in place to diagnose and resolve customer problems. Because the font code is complicated it is often very hard for a user to give meaningful information on what is really happening. They often just say the glyphs are "wrong" and should be "fixed". While I sympathize with the user's feeling that they are looking at "wrong" glyphs I cannot fix a problem without information on what is actually happening. If someone else wants to take responsibility for determining the cause(s) when a poor looking glyph was selected I will gladly hand over this problem to them and they would be free to remove this code. IN the mean time I use that code so on a regular basis so please do not remove it. Re performance: These "if (val&1)..." tests are only passed when fonts are being looked up. Once the fonts are found all the characters in those fonts are displayed without passing these tests again. One less malloc/free (which C++ does alot) would save more CPU cycles than these few if statements. Re code bloat: Do you really feel the 36 debug "if (val&1) printf()" statements are a lot of code bloat? I use the code fairly often. To my knowledge I have not heard of efforts to remove useful code such as talkback to reduce code bloat. You may not like the code but it definitely helps resolve bugs just like the talkback code does. Re shipping my own builds: Engineer's shipping their own executables is a very poor idea. There are many variables and problems associated with this which is why we have a team that does just that. Re: changing the way we calc metrics: If it does not change the line spacing it should be okay. Hwever, if it does affect the drawing near the top or bottom of the glyphs this may cause problems. Be careful of playing a "zero-sum" game: make one group more happy and another group less happy. Shrinking the metrics will make one group of web pages look better but may well make others unusable when the glyphs collide. At present when the primary font does not have a desired character/glyph the font system searches for a readable glyph from other fonts. Because most X fonts are not scalable the code often has to settle for glyphs that are a different size. It is my experinece that many pages "work" because of this extra space (or said the other way: fail when the space is reduced). If all X fonts were scalable we could control the size of the fill in glyphs. (This is way I'm taking a month of personal time to all TrueType support to Linux/Mozilla, bug 82076). Until we can control the size of fill in glyphs (glyphs not in the primary font) shrinking the line spacing may cause the (often oversized) fill in glyphs to collide with the line above or below. Sadly, I have had to back out some nice pieces of code to because of this problem.

I'll attach a new patch in a few days with the NS_FONT_DEBUG always on again (but one that will build with it off, unlike the current code). I think changing the position of the background that we paint will solve more problems than it will cause -- see all the dependencies of this bug, plus the performance issues with event target determination and overflow. There is a chance that in some cases the background won't be painted behind the entire glyph when we use glyphs from a different font, but that chance existed before as well -- it's just the scaling required to hit the problem was a little bigger. However, I think the real (long term) solution to the problem of using multiple fonts is to consider all fonts in the determination of line spacing and background drawing, as I discussed in http://www.people.fas.harvard.edu/~dbaron/css/2000/01/dibm . Once we consider multiple fonts properly it will be much easier to solve various issues, such as the desire to have Western fonts override Asian fonts in Asian documents when the Western fonts have the glyphs, without departing from the CSS2 font selection algorithm.

"the real (long term) solution to the problem of using multiple fonts is to consider all fonts in the determination of line spacing and background drawing" BTW, that's what I have done over at bug 74186 with GetDimensions(). My patch is now giving the overall dimensions to nsTextFrames, and nsLineLayout make use of these when doing the placements. So, that should likely solve the issues with the Asian fonts as erik mentionned in bug 20394. The placement of lines that I now see ressembles that of TeX.

My preference would be to get my stuff in so that other tweaks like this bug could be added from there. bstell, you wrote: "Until we can control the size of fill in glyphs (glyphs not in the primary font) shrinking the line spacing may cause the (often oversized) fill in glyphs to collide with the line above or below. Sadly, I have had to back out some nice pieces of code to because of this problem." That's one aspect that my font branch addresses. Care to give it a try and see what you think?

rbs: Well, I'd like to get something in to fix this for 0.9.4, and I'm not sure whether your changes are going to make it -- in fact, they seem like they're probably a bit risky for 0.9.4 at this point. So I'll attach a new patch shortly with NS_FONT_DEBUG still enabled all the time. Note that this patch does not affect line spacing at all -- it only affects the height of backgrounds (for things like inline element backgrounds and selection) that we paint.

I'm unclear on why the debug macros are moved: (unless perhaps you want cvsblame to say these are your lines?) +#define DEBUG_PRINTF_MACRO(x, type) \ + PR_BEGIN_MACRO \ ... perhaps we could just do something like this: #if !defined SMALLEST_BINARY_AT_ALL_COSTS # define NS_FONT_DEBUG 1 #endif Switching "#if DEBUG" to "#if REALLY_NOISY_FONTS" seems good (although a PRINTF_REALLY_NOISY_FONTS(x) would be far more compact and could be enabled by a compile time flag) As far as changing the font metrics eg: - mEmHeight = PR_MAX(1, nscoord(mWesternFont->mSize * f)); - mEmAscent = nscoord(fontInfo->ascent * mWesternFont->mSize * f / ... - mEmDescent = mEmHeight - mEmAscent; + mMaxHeight = nscoord((fontInfo->ascent + fontInfo->descent) * f); + mMaxAscent = nscoord(fontInfo->ascent * f); + mMaxDescent = nscoord(fontInfo->descent * f); I do not really understand the subtlies so I cannot say one way or the other.

I moved the macros because it made more sense to define first what was used by the later ones. We don't have anything like SMALLEST_BINARY_AT_ALL_COSTS, so there's no point making up a macro. The metrics diff you cited was just re-ordering of code to make the computation a little simpler.

The Xlib fontmetrics patch works OK (for both Xlib toolkit and Xprint module) ... :-)

David: the changes to nsFontMetricsGTK.cpp in attachment 46394 [details] [diff] [review] change the way mMaxHeight, mMaxAscent, and mMaxDescent are calculated. - mMaxHeight = nscoord((fontInfo->max_bounds.ascent + - fontInfo->max_bounds.descent) * f); + mMaxHeight = nscoord((fontInfo->ascent + fontInfo->descent) * f); - mMaxAscent = nscoord(fontInfo->max_bounds.ascent * f) ; + mMaxAscent = nscoord(fontInfo->ascent * f); - mMaxDescent = nscoord(fontInfo->max_bounds.descent * f); + mMaxDescent = nscoord(fontInfo->descent * f); Have you had a chance to see how this works on a large variety of fonts out in the field? The reason I ask is that we are very near the 0.9.4 cutoff. If you intend to put this in post 0.9.4 then I will would be comfortable adding a "r=". If you are looking to do the pre 0.9.4 then I would like to discuss these details a bit more.

If my memory is correct -- I did the work on this patch over a month ago -- I did test on quite a few fonts, and my memory is that the max_bounds always returned larger extents than the ascent/descent. Even if this were not the case, what problems would this cause? Using the same number for the background color that we use, and have always used, for the line spacing seems like a change that has very little risk, since we know the metrics provide reasonable numbers, whereas we know that the ones we get from max_bounds are often bad. I just don't see why this is risky.

I cannot say if it is risky or not. It's just an unknown for me. Are you aiming to get this in pre 0.9.4?

yes.

a=tor on behalf of drivers

Fix checked in 2001-08-23 06:44 PDT.

+ // XXXldb Shouldn't we get mEmHeight from the metrics? + mEmHeight = PR_MAX(1, nscoord(mWesternFont->mSize * f)); I am about to sync my changes and remove the XXX comment. As I reported in bug 74186, the CSS metrics 'em', 'ex', ..., come from an ASCII (Western) font. [Rather than hunting for an ASCII font, I have been wondering what might happen if the first font was considered, so that <font face="Symbol">...</font> truly gets its 'em' from the 'Symbol' font. For fonts that don't have the 'm' character, that might be the cause for some interesting surprises, tough...]

tough or though?

Marking verified per last comments

changing from max_bounds.ascent,max_bounds.descent to ascent/descent causes clipping of Japanese chars. See bug 96928

re-opening

>changing from max_bounds.ascent,max_bounds.descent to ascent/descent >causes clipping of Japanese chars. See bug 96928 I checked the documentation and it is okay to use the ascent/descent in the way this bug is doing. I suspect therefore that the regression might perhaps be just some side-effects of the fact that an unrelated ASCII font is determining the height, instead of the actual J-font being used for the text. If that's the case then my patch in bug 99010 makes even more sense. I suggest to wait and try to see if my patch caters for the regression.

If this did cause bug 96928, then the fix would not be to back it out, but to fix invalidation code so that it works in this case. (If bug 96928 was a side-effect of this fix, then it was merely that this change exposed an existing bug that would previously have been visible for text with a 'line-height' less than 'normal' -- one that we should fix anyway.) So why should this bug be REOPENED?

I attached testcases to bug 96828 that confirm rbs's theory. Marking FIXED again.

Sorry, bug 96928.

I reopened this bug because the checkin of its patch caused broken display of CJK characters. As you may remember, when this bug was being reviewed I asked: > ------- Additional Comments From Brian Stell 2001-08-21 14:21 ------- > > David: the changes to nsFontMetricsGTK.cpp in attachment 46394 [details] [diff] [review] change the > way mMaxHeight, mMaxAscent, and mMaxDescent are calculated. > > - mMaxHeight = nscoord((fontInfo->max_bounds.ascent + > - fontInfo->max_bounds.descent) * f); > + mMaxHeight = nscoord((fontInfo->ascent + fontInfo->descent) * f); > > - mMaxAscent = nscoord(fontInfo->max_bounds.ascent * f) ; > + mMaxAscent = nscoord(fontInfo->ascent * f); > > - mMaxDescent = nscoord(fontInfo->max_bounds.descent * f); > + mMaxDescent = nscoord(fontInfo->descent * f); > > Have you had a chance to see how this works on a large variety of fonts out > in the field? My experience is that fonts in the field are a somewhat messy business and I have been burned many times. It does no good for a user with a busted display if I say that "what I did was right". It is not clear whether the patch was incorrect or it uncovered a pre-existing bug. Either way, until this is clearly resolved it seems to me that this bug needs to stay open. If it is some other bug, then once we have identified it and we are comfortable that it can be fixed promptly, then we can close this bug.

Sorry I change the status, I missed your last comments, I will change it back. I will make a build with the patches in bug 99010 and reopen it if I see problems.

Based on last comments, marking verified with the Sept 28th branch.