CJK text underline is positioned too near the text

VERIFIED FIXED in mozilla1.1beta

Status

()

--
major
VERIFIED FIXED
17 years ago
7 years ago

People

(Reporter: shanjian, Assigned: shanjian)

Tracking

({intl, topembed+})

Trunk
mozilla1.1beta
x86
Windows XP
intl, topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2 RTM] [ETA 07/23])

Attachments

(10 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED

Updated

17 years ago
Keywords: intl
QA Contact: ruixu → ylong

Comment 1

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

Updated

17 years ago
Blocks: 154896
Keywords: mozilla1.0.1, topembed
Whiteboard: [adt2 RTM] [ET Needed]
(Assignee)

Comment 2

17 years ago
Created attachment 91017 [details] [diff] [review]
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

17 years ago
Comment on attachment 91017 [details] [diff] [review]
patch

ok, I understand what you are trying, let's give it a stab
r=rbs
Attachment #91017 - Flags: review+

Comment 4

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

EDT - can we get a topembed+?
Blocks: 143047
Whiteboard: [adt2 RTM] [ET Needed] → [adt2 RTM] [ETA Needed]
Target Milestone: --- → mozilla1.1beta
(Assignee)

Updated

17 years ago
Depends on: 76097
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?)
(Assignee)

Comment 6

17 years ago
> 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. 

Updated

17 years ago
Severity: normal → major
Whiteboard: [adt2 RTM] [ETA Needed] → [adt2 RTM] [ETA 07/16]

Comment 7

17 years ago
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);
(Assignee)

Comment 8

17 years ago
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. 
(Assignee)

Comment 9

17 years ago
david, could you take one more look at this bug? thx. 
(Assignee)

Comment 11

17 years ago
Created attachment 91672 [details]
Chinese website, before
(Assignee)

Comment 12

17 years ago
Created attachment 91673 [details]
chinese website, after
(Assignee)

Comment 13

17 years ago
Created attachment 91674 [details]
espn , before
(Assignee)

Comment 14

17 years ago
Created attachment 91676 [details]
espn after
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?
(Assignee)

Comment 17

17 years ago
Created attachment 91695 [details]
screen shot for the testcase
(Assignee)

Comment 18

17 years ago
> 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.

Updated

17 years ago
Whiteboard: [adt2 RTM] [ETA 07/16] → [adt2 RTM] [ETA 07/19]
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?
(Assignee)

Comment 20

17 years ago
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.
(Assignee)

Comment 22

17 years ago
Created attachment 91711 [details]
espn after
Attachment #91676 - Attachment is obsolete: true
(Assignee)

Comment 23

17 years ago
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.
(Assignee)

Comment 25

17 years ago
Created attachment 91724 [details]
testcase
(Assignee)

Comment 26

17 years ago
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?
(Assignee)

Comment 29

17 years ago
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

17 years ago
>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

17 years ago
>>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

17 years ago
david: how can we make this bug forward?
(Assignee)

Comment 33

17 years ago
Created attachment 92044 [details] [diff] [review]
same patch but only applies to CJK language
(Assignee)

Comment 34

17 years ago
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? 
(Assignee)

Comment 35

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

carry over review
Attachment #92044 - Flags: review+

Updated

17 years ago
Keywords: topembed → topembed+

Comment 36

17 years ago
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?)

Updated

17 years ago
Keywords: approval
Whiteboard: [adt2 RTM] [ETA 07/19] → [adt2 RTM] [ETA 07/22] [needs sr=, and approval]

Comment 37

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

sr=waterson
Attachment #92044 - Flags: superreview+

Comment 38

17 years ago
shanjian - can you pls land this on the trunk for baking over the weekend? thanks!
Keywords: nsbeta1 → adt1.0.1, nsbeta1+
(Assignee)

Comment 39

17 years ago
Created attachment 92074 [details] [diff] [review]
actual patch that get into trunk. (previous one has 2 other unrelated patch).
Attachment #92044 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #92074 - Flags: superreview+
Attachment #92074 - Flags: review+
(Assignee)

Comment 40

17 years ago
patch checked into trunk. 

Comment 41

17 years ago
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.

Comment 42

17 years ago
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [adt2 RTM] [ETA 07/22] [needs sr=, and approval] → [adt2 RTM] [ETA 07/22] [needs drivers' and ADT approval for the branch]

Comment 43

17 years ago
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. 
Status: RESOLVED → VERIFIED
OS: All → Windows XP

Comment 44

17 years ago
Created attachment 92252 [details]
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

17 years ago
Created attachment 92255 [details]
correct screen shot in previous comment.

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

Updated

17 years ago
Attachment #92252 - Attachment is obsolete: true

Comment 46

17 years ago
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

17 years ago
rbs- can you submit a right fix for it?

Comment 48

17 years ago
I will r= a fix if provided.

Comment 49

17 years ago
Created attachment 92310 [details] [diff] [review]
make sure all shanjian's change is inside if CJK 

rbs- is this the right fix?

Comment 50

17 years ago
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 ) {
Attachment #92310 - Flags: review+

Comment 51

17 years ago
waterson, can you sr= it ?

Updated

17 years ago
Whiteboard: [adt2 RTM] [ETA 07/22] [needs drivers' and ADT approval for the branch] → [adt2 RTM] [ETA 07/23] [needs drivers' approval for the trunk and branch]
Comment on attachment 92310 [details] [diff] [review]
make sure all shanjian's change is inside if CJK 

sr=jst
Attachment #92310 - Flags: superreview+

Updated

17 years ago
Keywords: adt1.0.1 → adt1.0.1+

Updated

17 years ago
Keywords: adt1.0.1+ → adt1.0.1

Comment 53

17 years ago
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.
Attachment #92310 - Flags: approval+

Updated

17 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 54

17 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 55

17 years ago
land the last patch into trunk. will land it into m1.0.1 branch this afternoon
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 56

17 years ago
land into 1.0.1 by using nhotta's account from ftang for shanjian
Keywords: adt1.0.1 → fixed1.0.1

Comment 57

17 years ago
posthumus adt1.0.1+. 
Keywords: adt1.0.1+
Whiteboard: [adt2 RTM] [ETA 07/23] [needs drivers' approval for the trunk and branch] → [adt2 RTM] [ETA 07/23]
(Assignee)

Comment 58

17 years ago
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

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

Comment 60

17 years ago
Verified fixed with win32 1.0 branch build.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.1 → verified1.0.1

Comment 61

16 years ago
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).

Comment 63

16 years ago
> 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

16 years ago
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.
You need to log in before you can comment on or make changes to this bug.