156943 - CJK text underline is positioned too near the text

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Shanjian Li]

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.

Comment 1

[Yuying Long]

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

Comment 2

[Shanjian Li]

Attachment: patch

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 3

[rbs]

Comment on attachment 91017 [details] [diff] [review]
patch

ok, I understand what you are trying, let's give it a stab
r=rbs

Comment 4

[Jaime Rodriguez, Jr.]

ftang - can you nsbeta1+ this one, and provide an ETA in the status whiteboard.
thanks!

EDT - can we get a topembed+?

Comment 5

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

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

Comment 6

[Shanjian Li]

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

Comment 7

[Hideo Saito]

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

Comment 8

[Shanjian Li]

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. 

Comment 9

[Shanjian Li]

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

Comment 10

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

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

Comment 11

[Shanjian Li]

Attachment: Chinese website, before

Comment 12

[Shanjian Li]

Attachment: chinese website, after

Comment 13

[Shanjian Li]

Attachment: espn , before

Comment 14

[Shanjian Li]

Attachment: espn after

Comment 15

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

What about attachment 41729 [details]?

Comment 16

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

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?

Comment 17

[Shanjian Li]

Attachment: screen shot for the testcase

Comment 18

[Shanjian Li]

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

Comment 19

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

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?

Comment 20

[Shanjian Li]

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. 

Comment 21

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

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.

Comment 22

[Shanjian Li]

Attachment: espn after

Comment 23

[Shanjian Li]

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?

Comment 24

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

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.

Comment 25

[Shanjian Li]

Attachment: testcase

Comment 26

[Shanjian Li]

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

Comment 27

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

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

Comment 28

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

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

Comment 29

[Shanjian Li]

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. 

Comment 30

[Frank Tang]

>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 

Comment 31

[Frank Tang]

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

Comment 32

[Frank Tang]

david: how can we make this bug forward?

Comment 33

[Shanjian Li]

Attachment: same patch but only applies to CJK language

Comment 34

[Shanjian Li]

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 35

[Shanjian Li]

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

carry over review

Comment 36

[bobj]

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 37

[Chris Waterson]

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

sr=waterson

Comment 38

[Jaime Rodriguez, Jr.]

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

Comment 39

[Shanjian Li]

Attachment: actual patch that get into trunk. (previous one has 2 other unrelated patch).

Comment 40

[Shanjian Li]

patch checked into trunk. 

Comment 41

[Asa Dotzler [:asa]]

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
[e-mail] 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.

Comment 42

[Jaime Rodriguez, Jr.]

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!

Comment 43

[Yuying Long]

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. 

Comment 44

[Yuying Long]

Attachment: screen shot after checked into trunk

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.

Comment 45

[Yuying Long]

Attachment: correct screen shot in previous comment.

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

Comment 46

[rbs]

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);
+    }

Comment 47

[Frank Tang]

rbs- can you submit a right fix for it?

Comment 48

[rbs]

I will r= a fix if provided.

Comment 49

[Frank Tang]

Attachment: make sure all shanjian's change is inside if CJK

rbs- is this the right fix?

Comment 50

[rbs]

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

Comment 51

[Frank Tang]

waterson, can you sr= it ?

Comment 52

[Johnny Stenback (:jst)]

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

sr=jst

Comment 53

[chris hofmann]

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.

Comment 54

[Jaime Rodriguez, Jr.]

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.

Comment 55

[Frank Tang]

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

Comment 56

[Frank Tang]

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

Comment 57

[Jaime Rodriguez, Jr.]

posthumus adt1.0.1+. 

Comment 58

[Shanjian Li]

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.

Comment 59

[Jaime Rodriguez, Jr.]

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

Comment 60

[Yuying Long]

Verified fixed with win32 1.0 branch build.

Comment 61

[Koike Kazuhiko]

The fix for this bug brought about bug 167001.

Comment 62

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

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

Comment 63

[Hideo Saito]

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

Comment 64

[Hideo Saito]

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.