Bug 76097 fixed the lineheight problem for CJK text. Underline position is left over and I file this bug for it. Basicly, (for windows), the underline position suggested by font itself is too near the text. Using decent is a good option, but that will extend over the boundary if current line, and will cause some regression. Bug 136935 is an example.

Nominate as nsbeta1 -> the characters are touched together with underline in most of CJK pages with this problem.

I found this easy way to raise the baseline so that lowered underline will stay within its territory. rbs, could you review my patch?

Comment on attachment 91017 [details] [diff] [review] patch ok, I understand what you are trying, let's give it a stab r=rbs

ftang - can you nsbeta1+ this one, and provide an ETA in the status whiteboard. thanks! EDT - can we get a topembed+?

Comment on attachment 91017 [details] [diff] [review] patch I'm somewhat concerned about these changes, for two reasons: 1) It's going to cause us to ignore the underline position metrics in all fonts (right?) just because a few fonts (generally CJK ones) have badly-designed underline positions (or at least ones. This is going to make a bunch of western fonts look bad, no? 2) My understanding of what you're doing is that it's going to affect the size that's returned by GetNormalLineHeight, etc. (Or is it?) If so, is this going to affect the line spacing on pages? Will it create new evangelism bugs? Or are you not affecting the line spacing? Or am I misunderstanding things? How often do we hit the condition |mInternalLeading + mExternalLeading > mUnderlineSize|? (Do we even hit this condition for typical CJK fonts?) If this correction only applies to some fonts, will this cause the baseline to be misaligned with fonts that don't have the correction made? (Perhaps this patch needs to be commented more clearly so that it's clear what you're doing and why?)

> 1) It's going to cause us to ignore the underline position metrics in all >fonts (right?) just because a few fonts (generally CJK ones) have >badly-designed underline positions (or at least ones. This is going to make a >bunch of western fonts look bad, no? On the contrary, we also make western fonts looks better. In my testing, after lower the underline, western characters looks better than before. The reason is the same as CJK fonts. You might ask why fonts does not provide a better suggestion for underline position. My understanding is that the underline position suggested by font has to apply to all situation. If user agent don't use external leading between line, or there is no external leading and user agent does not create a leading, lower the underline will mess up thing and thus not feasible. > 2) My understanding of what you're doing is that it's going to affect the size >that's returned by GetNormalLineHeight, etc. (Or is it?) If so, is this going >to affect the line spacing on pages? Will it create new evangelism bugs? Or >are you not affecting the line spacing? No. I decrease the ascent for the same amount when I increase the descent. That way the line height (normal or not ) won't change a bit. Besides I also check the leading to make sure we have enought space to raise baseline so that the upper part of glyph will not extend out of boundary. >|mInternalLeading + mExternalLeading > mUnderlineSize|? (Do we even hit this >condition for typical CJK fonts?) This is true for must CJK fonts. For western font, about half half? just a guess. >If this correction only applies to some fonts, will this cause the baseline to >be misaligned with fonts that don't have the correction made? No, when I am talking about raise the baseline, that is relative to em box. The relative position of baseline and glyph does not change at all.

Please let me know. In order to detect the damage of an underline in patch v2 (Attachment 84022 [details] [diff]) of Bug 76097, the added value of kidRect.lineheight is used as follows. Does the value of kidRect.height spread in height including the underline in the case of your patch? --- layout/html/base/src/nsContainerFrame.cpp.org Fri Apr 5 21:13:23 2002 +++ layout/html/base/src/nsContainerFrame.cpp Fri May 17 13:55:06 2002 @@ -234,6 +234,9 @@ // rect (both are in our coordinate space). This limits the // damageArea to just the portion that intersects the childs // rect. + if (kidRect.height != 0 && (kidRect.lineheight) > kidRect.height) { + kidRect.height = kidRect.lineheight; + } overlap = damageArea.IntersectRect(aDirtyRect, kidRect);

In my patch, I didn't change anything to existing line Rect. I only raise the glyph one pixel up relative to line Rect when permitted.

david, could you take one more look at this bug? thx.

Could you attach before and after screenshots of attachment 41729 [details]? (I don't have access to a Windows build.)

What about attachment 41729 [details]?

One other question: have you checked that we're not hitting the codepath in nsFontMetricsWin::RealizeFont that handles the failure of GetOutlineTextMetrics? Any chance the differences with IE aren't just the differences in the backup code in case the font doesn't have the right metrics?

> One other question: have you checked that we're not hitting the codepath in > nsFontMetricsWin::RealizeFont that handles the failure of GetOutlineTextMetrics? descentPos is assigned to 0, and mUnderlineOffset is negative, descentPos < mUnderlineOffset will not be true in that code path. > Any chance the differences with IE aren't just the differences in the backup > code in case the font doesn't have the right metrics? I don't quite understand your question.

The screenshot in comment 17 looks like a big difference. Is that because of the presence of the Chinese fonts in the testcase, or does the same change happen if it's only Western fonts?

The presence of chinese characters certainly contributes to the difference. As my screen shot for espn shown, there is difference for western fonts as well. As long as the difference is towards good direction or acceptable, it shouldn't be a concern.

The espn before and espn after images don't look any different. Did you attach the right image? I really don't think we should be changing the metrics for Western fonts, and I'm not sure how the presence of Chinese characters would affect things in attachmnt 91695, although it could be some of the work that rbs did.

Sorry, the espn after screen shot is incorrect. I don't know what went wrong. I have to do a series of clumsy operation to take a screen shot. Anyway, I reposted it and please take a look. Please pay attention to character "g". Don't you agree that it looks better after my patch?

IMO, the correct underline position across a p or g should really be such that the bottom of the descender extends clearly below the underline, rather than the reverse. However, there isn't always room for the underline there at small sizes. I'm really hesitant to think we should make this big a change right before a release without knowing what people would think about it. (Also, what would the effects be on fonts in scripts that have larger descenders, such as Arabic and perhaps some Indic scripts?) After all, who are we to say that all the font designers, for all scripts, were wrong, and that we should make up some different underline position? Finally, is it possible that this could re-create a bug like bug 136935, except with the parts of the glpyhs at the top rather than the underline at the bottom? Consider a testcase like: <title>:hover testcase</title> <style type="text/css"> :link:hover, :visited:hover { background: yellow; } </style> <p><a href="http://mozilla.org/">A link to mozilla.org</a> <br>&Eacute;&Aacute;</p> where you hover over the link.

Well base on the screen shot I attached, it looks to me that after the patch, even the western fonts looks better. that is my personal opinion anyway, I will leave it for other to decide. I did consider the possible regression while I made this patch. Normally, the internal leading + external leading should provide enough padding space. In the worst situation, when a font fully uses the internal leading for those diacritics, one pixel would be chopped off on top when hovering. Since the main part is still there, glyph should be still readable. In my testing, I couldn't find such a situation yet. I posted a testcase based on your comment, and I make it extreme by force the line-height to be 1.0, and everything still looks fine. (I could safe guard this situation by not decreasing the ascent,but that will change the font size.)

Shanjian, what about those sites viewed with IE? Could you give some screen shots please?

One other thought -- is it possible that we're using the underline metrics for an ascii font when drawing the CJK fonts?

No, I tested it with CJK specific font metric change before, and I was sure the metrics change in CJK affect the final display result.

>I'm really hesitant to think we should make this big a change right before a release I don't think we plan to do that. We want to see this land into trunk first and give to one of our internal embedding customer

>>I'm really hesitant to think we should make this big a change right before a release >I don't think we plan to do that. We want to see this land into trunk first and >give to one of our internal embedding customer sorry, I take that part back. It looks like jaimejr did want this one get into the recent release.

david: how can we make this bug forward?

We had a conference call today discussed this issue. One of the decision is to provide a CJK specific patch as candidates. I just posted the patch. Waterson, could you sr?

Comment on attachment 92044 [details] [diff] [review] same patch but only applies to CJK language carry over review

Shanjian's updated patch only affects CJK documents and does not change the behavior for other language documents. This addresses the issue raised in dbaron's comment #24 > IMO, the correct underline position across a p or g should really be such > that the bottom of the descender extends clearly below the underline, > rather than the reverse. However, there isn't always room for the > underline there at small sizes. > > I'm really hesitant to think we should make this big a change right > before a release without knowing what people would think about it. > (Also, what would the effects be on fonts in scripts that have larger > descenders, such as Arabic and perhaps some Indic scripts?)

Comment on attachment 92044 [details] [diff] [review] same patch but only applies to CJK language sr=waterson

shanjian - can you pls land this on the trunk for baking over the weekend? thanks!

patch checked into trunk.

Shanjian, this patch was not approved by drivers to land on the trunk. It clearly says at the top of tinderbox that you are required to get approval from drivers@mozilla.org before checking in. Jaime, please make your directives more clear. Saying something like "please get drivers' approval and land on the trunk" would be a lot less likely to cause these problems. We're trying to do a release on Monday and we can't afford these unknowns landing without approval.

asa, i had added the Status Whiteboard marking "[needs sr=, and approval]", and thought that would've been sufficient. but, yes i can be more explicit in the future. my apologies ... resolving as fixed, since this has been checked into the trunk. ylong: can you pls verify this fix on the trunk? thanks! Shajian: Pls ask for Drivers' and and ADT approval for the branch, once yuying has verified the fix on the trunk. thanks!

On 07-22 trunk build: 1. First, if I understand correctly, this bug is for windows only, so I change OS from All to WindowsXP(no item for all windows). 2. Display finw with English pages, although compare to branch build there is more space between characters text and underline. 3. CJK pages: Very good with Chinese(SC and TC) pages and Korean pages. 4. CJK pages: seems with Japanse pages (yahoo.co.jp, lycos.co.jp, excite.co.jp...etc.), I can see the line highlight is larger but I still see the chracters text string is too close to underline. (I'll attach a screen shot later) I'm marking this one as verified cause we have resolved the majority issue here. Need more investigate for left over problem with Japanese, if it is something we don't do very good, we can file a separate bug for that.

This screen shot is for Japanese pages on trunk build after checked in. Notice the text string still too close to underline. This problem not exisits in Chinese and Korean pages.

Sorry, I attached a wrong image file in previous comment.

Comment on attachment 92074 [details] [diff] [review] actual patch that get into trunk. (previous one has 2 other unrelated patch). shanjian forgot the added langGroup check in one place (there is a convenience macro, line 3053 in the file, IsCJKLangGroupAtom, that can be used for that): @@ -3282,9 +3283,11 @@ mStrikeoutSize = PR_MAX(onePixel, NSToCoordRound(oMetrics.otmsStrikeoutSize * dev2app)); mStrikeoutOffset = NSToCoordRound(oMetrics.otmsStrikeoutPosition * dev2app); mUnderlineSize = PR_MAX(onePixel, NSToCoordRound(oMetrics.otmsUnderscoreSize * dev2app)); - if (gDoingLineheightFixup) - mUnderlineOffset = NSToCoordRound(PR_MIN(oMetrics.otmsUnderscorePosition*dev2app, - oMetrics.otmDescent*dev2app + mUnderlineSize)); + if (gDoingLineheightFixup) { + mUnderlineOffset = NSToCoordRound(PR_MIN(oMetrics.otmsUnderscorePosition, oMetrics.otmDescent + oMetrics.otmsUnderscoreSize) * dev2app); + // keep descent position, use it for mUnderlineOffset if leading allows + descentPos = NSToCoordRound(oMetrics.otmDescent * dev2app); + }

rbs- can you submit a right fix for it?

I will r= a fix if provided.

rbs- is this the right fix?

Comment on attachment 92310 [details] [diff] [review] make sure all shanjian's change is inside if CJK r=rbs I am also ok if you use the isCJK macro here: + if (mLangGroup.get() == gJA || + mLangGroup.get() == gKO || + mLangGroup.get() == gZHTW || + mLangGroup.get() == gZHCN ) {

waterson, can you sr= it ?

Comment on attachment 92310 [details] [diff] [review] make sure all shanjian's change is inside if CJK sr=jst

Comment on attachment 92310 [details] [diff] [review] make sure all shanjian's change is inside if CJK a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking in on the branch.

reopened because there needs to be a change to protect western characters. the new patch is attached and approved for checkin to the branch by drivers.

land the last patch into trunk. will land it into m1.0.1 branch this afternoon

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

posthumus adt1.0.1+.

mUnderlineOffset will only be replaced by descentPos for CJK later, so it is not really necessary to check language group when calculating mUnderlineOffset and descentPos. Using mUnderlineSize to replace otmsUnderscoreSize can make it safer, but it can be over done in certain situation. Anyway, if the latest patch make people feel more comfortable, I have no object to it either.

yuying: can you pls verify this as fixed on the 1.0 branch. thanks!

Verified fixed with win32 1.0 branch build.

The fix for this bug brought about bug 167001.

Could someone take a look at bug 160697 ? It's still not yet confirmed one month after I've filed it. I don't even know if there's anybody taking care of that component. (Sorry for the off-topic to this bug, but I don't know what to do else).

> Could someone take a look at bug 160697 ? This is a platform specific issue. A mSubscriptOffset variable at gfx/src is asked for a gap of the vertical direction of a small font in <sub> and <sup> tags. In the case of windows. gfx/src/windows/nsFontMetricsWin.cpp nsFontMetricsWin::RealizeFont() if (0 < ::GetOutlineTextMetrics(dc, sizeof(oMetrics), &oMetrics)) { mSubscriptOffset = NSToCoordRound(oMetrics.otmptSubscriptOffset.y * dev2app) } else { mXHeight = NSToCoordRound((float)metrics.tmAscent * dev2app * 0.56f); // 56% of ascent, best guess for non-true type mSubscriptOffset = mXHeight; // XXX temporary code! } In a true case of above program, the value of mSubscriptOffset is calculated small and in a false case, the value of mSubscriptOffset is calculated a big value. In any case, it is thought that adjustment of a value is required.

This is off-topic. > In any case, it is thought that adjustment of a value is required. The variable of mSubscriptOffset is related to <sub> tag and the variable of mSuperscriptOffset is related to <sup> tag. In any case, it is thought that adjustment of both values are required.