402524 - Need to correct the Metrics for fonts in XP level

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

We have several reports (e.g., bug 396809, bug 400141 and bug 380026) which are underline correctness issues. They are not platform issue, so *they are font issue*. Therefore, we should fix the bugs in XP level. The patch will come soon.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

This should fix the all problems. But this patch cannot fix following case:

<span lang="en-US">漢字</span>

This is an invalid case, but this case is existing on the Web. For this bug, we need to check whether the font supports CJK, but it cannot do now. (bug 366664 should be fixed first.)

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

fmmm... the patch makes damage to UI on CJK locale when using western fonts for menus. We need another approach...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

This patch changes the underline offset if the font has 'あ' or '一' or '가' on Win/Mac/Pango.

'あ' is U+3042, this is Hiragana of Japanese. '一' is U+4E00, this is CJK ideograph's first character. '가' is U+AAC0, this is Hangul's first character.

I need to test on Linux and to write the code for OS/2.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.5

This should work fine.

If the font is have CJK glyphs, we ignore the underline offset of font. Instead of it, we use the bottom of the descent.

gfxFont::CorrectMetrics has the hack for bug 353632. The same problem can be reproduced on Linux, but not on Mac. I think that Mac might corrects the values in their APIs. So, the issue should be XP level, and we should hack it in XP level.

And gfxFont::CorrectMetrics has following functions:
1. it guarantees the underline height and the strikeline height. they are one pixel at minimum.
2. the maximum of underline offset is -1, not 0. This guarantees the underline doesn't adhere to the characters.
3. But finally, underline offset is moved to descent space. (Or if it is CJK fonts, it might be moved to the bottom of descender, and it makes wider line by spreading the descent and font height.)

And I removed the changes by bug 380026. It is wrong approach. And this patch also re-fixes bug 380026.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

the patch is changing the OS/2 code, OS/2 team should check it.

Comment 6

[Peter Weilbacher]

masayuki: thanks for thinking about us. :-) The OS/2 part of the code makes sense to me and compiles.

However, I am not sure that in the end this patch really helps. At least on OS/2 we replace many glyphs from standard Unicode fonts (well, at least soon, my patch for that isn't finished yet). And because those fonts usually contain CJK glyphs this patch causes very low underlines for languages as different as Thai, Ivrit, and Eesti (I checked this on the http://wikipedia.org/ page) which before looked much better.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #6)
> However, I am not sure that in the end this patch really helps. At least on
> OS/2 we replace many glyphs from standard Unicode fonts (well, at least soon,
> my patch for that isn't finished yet). And because those fonts usually contain
> CJK glyphs this patch causes very low underlines for languages as different as
> Thai, Ivrit, and Eesti (I checked this on the http://wikipedia.org/ page) which
> before looked much better.

It's really good point. I didn't think about Unicode fonts. I'll post new patch which also checks the lang of font style.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.6

This should work fine with Unicode font.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.7

This only checks the actual glyph if the content is CJK. So, other language doesn't have large damage.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

using FontEntry on Mac.

Comment 11

[Peter Weilbacher]

Thanks, rv3.0 is much better. :-) You should use gfxOS2Font::HasCJKGlyphs (not gfxPangoFont) in the OS/2 file and I think you also have to cairo_ft_scaled_font_unlock_face() in that function. Otherwise I'm happy.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.1

thank you!

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 287668 [details] [diff] [review]
Patch rv3.1

this has a bug, next patch is coming soon.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 287668 [details] [diff] [review]
Patch rv3.1

Oops, sorry. I mistook the bug number.

Comment 15

[Stuart Parmenter]

+    return mFontEntry->SupportsRange(sHiragana) ||
+           mFontEntry->SupportsRange(sIdeograph) ||
+           mFontEntry->SupportsRange(sHangul);

you should be using mFontEntry->mCharacterMap->test() instead of these.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.2

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that the cause of bug 406729 is this bug. Stuart, I hope that you restart the review ASAP.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.2.1

updating for latest trunk.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.3

Updating for latest trunk too.

Stuart, please hurry up to review this. This bug blocks other bugs!

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And note that this bug's changing is already used on legacy gfx for Win32. The corrects are not Win32 API dependent issues, they depends on fonts. Therefore, we need the code in XP level.

Comment 21

[John Daggett (:jtd)]

I'm still looking at this but to these heuristics apply to all ideographic fonts?  Beyond just MS Gothic?  And do these metrics render Latin text rendered with glyphs from these fonts correctly?  

I wish there was a way of pulling this more directly out of the font information, something in the OS/2 perhaps?  Relying on "does the font have glyphs in cmap range xxx?" as a test makes me a bit nervous.

Also, which of the dependent bugs is best to look at see problems that this bug will help fix?

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that the risk of new side effects is low. Because this patch's logic is already used in legacy win32 gfx. (legacy mac gfx didn't refer to font metrics for underline/strikeout line.)

And this patch has the patches for bug 353632, bug 361576 and bug 380026. They are only fixed on one platform, but they are not API issues, so, we should fix it on all platforms.

bug 406729 is a regression of bug 392785. That is fixed by this patch. So, I cannot check-in the patch for bug 392785 before this.

bug 396809 and bug 400141 are fixed by this patch.

Comment 23

[Stuart Parmenter]

(In reply to comment #21)
> I'm still looking at this but to these heuristics apply to all ideographic
> fonts?  Beyond just MS Gothic?  And do these metrics render Latin text rendered
> with glyphs from these fonts correctly?

What is the answer to this?

> 
> I wish there was a way of pulling this more directly out of the font
> information, something in the OS/2 perhaps?  Relying on "does the font have
> glyphs in cmap range xxx?" as a test makes me a bit nervous.
> 

I agree with this.  We should certainly change the mac code to be faster like the windows code for this lookup if we're to keep it, but I really don't like the idea of "does this have any CJK glyphs."  There really needs to be a better way.

Comment 24

[John Daggett (:jtd)]

Added another testcase to 400141.  Also, I think the patch needs to be updated, there were some changes made to gfxPangoFonts.h.

Comment 25

[John Daggett (:jtd)]

Attachment: disjoint underline problem screenshot

Screenshot shows FF2 vs. FF3 w/ patch 3.3 on Mac.  Under FF2 the underline was displayed very close against the baseline while in FF3 with patch 3.3 it displays a disjoint underline.

Comment 26

[John Daggett (:jtd)]

Attachment: same example in safari, screenshot

Safari renders the baseline tight against the text.

Comment 27

[John Daggett (:jtd)]

Attachment: same example in IE6, screenshot

Looks like IE simply renders the underline lower than FF/Safari for all text.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> (In reply to comment #21)
> > I'm still looking at this but to these heuristics apply to all ideographic
> > fonts?  Beyond just MS Gothic?  And do these metrics render Latin text rendered
> > with glyphs from these fonts correctly?
> 
> What is the answer to this?

Is this underline issue? Or Superscript/Subscript rendering? I think that the underline issue is not only for MS Gothic. But Superscript and Subscript issue is only for it. If the font has CJK glyphs, the underline is rendered to bottom of descent even if the text is Latin text. However, I believe that CJK users like such rendering.

> > I wish there was a way of pulling this more directly out of the font
> > information, something in the OS/2 perhaps?  Relying on "does the font have
> > glyphs in cmap range xxx?" as a test makes me a bit nervous.
> > 
> 
> I agree with this.  We should certainly change the mac code to be faster like
> the windows code for this lookup if we're to keep it, but I really don't like
> the idea of "does this have any CJK glyphs."  There really needs to be a better
> way.

Do you have the idea?

Note that Safari/IE/Opera don't prefer the underline offset/size of fonts.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #28)
> Note that Safari/IE/Opera don't prefer the underline offset/size of fonts.

And legacy Mac gfx code is too.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Current my patch breaks Meiryo's underline position, but Fx2 doesn't
break it. I failed to port the underline position logic from legacy win gfx :-(

The reason is that the legacy gfx uses otmDescent for computing. Most CJK fonts returns same value for otmDescent and tmDescent, However, Meiryo doesn't so...

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv3.4

updating for comment 30.

In gfxWindowsFont, it is assumed that internal leading is always included only in ascent. But Meiryo is not so, it has very large internal leading and only a part of it is in ascent. We should prefer the otmAscent and otmDescent for emAscent and emDescent. (On Linux and Mac, we don't assume that the internal leading is only in ascent.)

Comment 32

[Stuart Parmenter]

+
+PRBool
+gfxAtsuiFont::HasCJKGlyphs()
+{
+    return mFontEntry->TestCharacterMap(PRUnichar(0x3042)) ||
+           mFontEntry->TestCharacterMap(PRUnichar(0x4E00)) ||
+           mFontEntry->TestCharacterMap(PRUnichar(0xAAC0));
+}
+

I still don't like doing this for any platform.  We need to come up with a better way to tell if we need to adjust the metrics.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #32)
> +
> +PRBool
> +gfxAtsuiFont::HasCJKGlyphs()
> +{
> +    return mFontEntry->TestCharacterMap(PRUnichar(0x3042)) ||
> +           mFontEntry->TestCharacterMap(PRUnichar(0x4E00)) ||
> +           mFontEntry->TestCharacterMap(PRUnichar(0xAAC0));
> +}
> +
> 
> I still don't like doing this for any platform.  We need to come up with a
> better way to tell if we need to adjust the metrics.

The reason of I added it is for Linux UIs. Linux UIs are rendered Western Font + user locale's font. And lang of font of UIs is system locale. So, if we don't check it, Linux UI's underline is broken. I'll try to use system font checking instead of glyph checking.

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv4.0

Using system font checking instead of CJK glyph checking.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv4.1

minimized patch.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

would you continue to review?

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Stuart:

Would you continue to review for new patch?

Comment 38

[Stuart Parmenter]

i don't understand this new patch either.  the langgroup is often wrong coming from the page or defaulting to western.  if this enough?  also, correcting these metrics make in this way make you end up with different baselines for mixed japanese/english text which seems out of whack.  Imho, we either need to lower the entire baseline for all fonts or not do this.  If we really want some similar approach we need to detect in some way, from the font, that we should do this and not try to make guesses from the language group or other typically poor information we have about the state we're in.  Imagine mixed text runs here -- things aren't going to look very good.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #38)
> the langgroup is often wrong coming from the page or defaulting to western.  if this enough?

The lang of the elements of UIs and the lang of lang non-specified elements in UTF-8 pages are same as system locale.

> also, correcting these metrics make in this way make you end up with
> different baselines for mixed japanese/english text which seems out of
> whack.

It's true in quriks mode if each languages text are in different textframe (e.g., in different element). However, standards mode only uses first font metrics of text-decoration specified.

> Imho, we either need to lower the entire baseline for all fonts or not do
> this.  If we really want some similar approach we need to detect in some way,
> from the font, that we should do this and not try to make guesses from the 
> language group or other typically poor information we have about the state 
> we're in.  Imagine mixed text runs here -- things aren't going to look very good.

Your only worry is decoration lines disjoint problems between different fonts? If so, it should be fixed on layout level. not gfx. And note that it's rare case I think. Because layout code uses first font's metrics for decoration lines. So, actually, we can look the problem with between different font element, e.g., one is lang="en-US" and other is lang="ja".

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #38)
> i don't understand this new patch either.  the langgroup is often wrong coming
> from the page or defaulting to western.  if this enough?

So, I meant in previous comment that for the users whose system locale is CJK, they can look the corrected decoration lines in most pages, but UIs are not broken by the new checking. I think that this is a best workaround for now.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Stuart:

How do you think for my comment?

Comment 42

[Stuart Parmenter]

If we use the first font's lowered metrics followed by non-cjk don't we end up with the underline now running through english text?

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #42)
> If we use the first font's lowered metrics followed by non-cjk don't we end up
> with the underline now running through english text?

No.

The latest patch doesn't check whether the font is CJK or not. And the first font's lower point is used for underline offset only if the lang attr of the element is CJK. If the lowest point of first font is higher than second or later CJK font's lowest point, the line might be overlapped to the text, however, I think that the case is very rare. Because CJK fonts don't have larger descent then Western fonts generally (CJK characters doesn't need descent).

And the case of non-CJK fonts are listed after CJK fonts is very rare case. Because most CJK fonts have Western characters, and other characters are not used in most documents.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And even if you think we should check the all font metrics for the text, it's not in gfx level issue, it should be layout level.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase for ja and en

This is testcase for underline position of some font-family cases.

1. font-family has Japanese font at first, and Western font at second.
   This case is very rare case. Because second font might be ignored.
2. font-family has Western font at first, and Japanese font at second.
   This case is major case in Japan.
3. font-family only has Japanese font.
   This case is used in most sites in Japan.

And for each cases, lang attr is set "en-US" or "ja" or "ar".

And for experimental, the case of Japanese new system font on Vista is last. It has very large internal leading, it is larger than general western fonts.

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screenshot of current trunk

The underline is too near the Kanji character("MS PGothic is Japanese default font on Windows, and the case (font-size: 19px) is worst case). And the glyph in descent is overlapped by the underline. Therefore, the text is not pretty.

"Meiryo" cases (in most bottom table row) are good, it has suitable values for underline offset.

Note that in the first cases (font-family: ja, en, others;), the western font (Times New Roman) is not used. Because most CJK fonts have western glyphs. (If they don't have the western characters, we cannot use them.)

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screenshot of patched build

This is a screenshot of patched build.

Only when the lang is ja, the underline positions are corrected. Kanji characters are not overlapped by the underline. And descent of western characters are very clear. Note that Japanese people might not like the underline being positioned to baseline position. This screenshot is similar to Japanese typographic.

And "Meiryo" has very large internal leading, but this patch suppresses to the underline being positioned too far from the text.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know much about this issue, but if the problem is just that certain popular fonts have a bad underline-offset value, then I'm comfortable with just blacklisting certain font names at an XP level, giving them special treatment such as increasing the underline offset to the descent of the font.

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

roc:

Thank you for you advice. However, the blacklist approach cannot save following case:

font-family: "non-blacklisted western font", "blacklisted CJK font";

We decide the underline position only from first font metrics, Therefore, non adjusted underline might overlap to CJK characters. Some major sites (generally, official sites of major companies) uses such font-family specification. Therefore, I bet your approach cannot solve this problem...

Ideally, we should check all used fonts for the text (or all fonts for the fontgroup) for deciding the underline position, but it is not realistic now.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And I hope that the underline of Western fonts is also positioned at bottom of descent space. It's natural for CJK users (or CJK typography), I think.

Comment 51

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #50)
> And I hope that the underline of Western fonts is also positioned at bottom of
> descent space. It's natural for CJK users (or CJK typography), I think.

Of course, only in CJK context.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> it is not realistic now.

Why not? It doesn't sound very hard to me.

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #52)
> > it is not realistic now.
> 
> Why not? It doesn't sound very hard to me.

We are in b4 cycle, now. Is there enough testing term? How many weeks we have for Gecko1.9?

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

O.K. I'll post a new patch which doesn't have the underline adjusting for CJK fonts. It should be landed in early time for other bugs. And the CJK fonts issue will be filed to new bug, thanks.

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv5.0

This patch is removed the part for CJK fonts underline offset adjusting from previous patch.

So, by this landing, bug 380026 will be reopened, but bug 392785 and bug 396809 are will be marked as FIXED.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Stuart:

gfxFont::AdjustFontMetrics is better name?

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

karlt suggested "SanitizeMetrics". It sounds good.

Thank you, karlt!

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv5.0.1

only renames.

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv5.0.2

Oops, I forgot to change the comment, sorry for the spam.

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in!

Thank you stuart!