362428 - [Cairo] 'normal' keyword for line-height is too tall with 'Lucida Grande'.

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[philippe (part-time)]

The Computed value for 'line-height' using the 'normal' keyword is too large, resulting in a line box that is too tall.

This affects the 'Lucida Grande' font mostly. Other fonts are on par with Safari 2.0, although different from Firefox 2.0.

Comment 1

[philippe (part-time)]

Attachment: test case

Test case using Lucida Grande.

Comment 2

[philippe (part-time)]

Attachment: screenshot

From Left to Right: Camino 20061130 Trunk build (Cairo) - Firefox 2.0 - Safari 2.0.4

Comment 3

[philippe (part-time)]

Attachment: screenshot using 'helvetica'

Using 'Helvetica' instead of 'Lucida Grande'. Still taller than Safari, but much closer.

---
For all fonts tested, Opera 9.0 matches Safari.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 247130 [details]
screenshot using 'helvetica'

Changing the MIME type

Comment 5

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

Are the line-height:1.3 cases same result? If so, the patch of bug 353185 changes this behavior. That patch needs for Japanese system font, because they don't have suitable metrics. In the XP code, we are resolving this problem for Win too. (Windows has same issue.)

Comment 6

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

And the line-height: normal; is UA default. And it depends on UA. We are setting the line-height to 1.2 if the font doesn't have suitable internal leading.

Comment 7

[philippe (part-time)]

Masayuki,
* I know 'line-height:normal' is the UA default. And there absolutely no problems with Japanese Fonts on Mac (since the patch for bug 353185, thank you for that one).
* when you look at the screenshot for 'Lucida Grande' (attachment #247128 [details]), you can see how with Cairo/Mac the line-height is much larger than in Safari (or Opera).
* with any other value for 'line-height' (number, %, ems,..) there are no problems.

I'll attach another testcase, using line-height:normal and line-height:1.2. There is a huge difference when using 'Lucida Grande'.

Comment 8

[philippe (part-time)]

Attachment: test case #2

'Lucida Grande', 'line-height:normal' and 'line-height:1.2'

Comment 9

[philippe (part-time)]

Attachment: screenshot, testcase #2

Minefield (2006113017) on the left, Safari 2.04 on the right.

Comment 10

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

Um.. looks like the line-height: normal and line-height: 1.2 are different results. By this results, we can know the 'Lucida Grande' has non-zero internal leading. Therefore, Gecko uses em height + internal leading + external leading(always zero on Mac) for normal line-height...

Comment 11

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

http://lxr.mozilla.org/mozilla/source/layout/generic/nsHTMLReflowState.cpp#2218

> 2218   case eCompensateLeading:
> 2219     if (!internalLeading && !externalLeading)
> 2220       normalLineHeight = NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR);
> 2221     else
> 2222       normalLineHeight = emHeight+ internalLeading + externalLeading;
> 2223     break;

This is current calculating code, should this be:

 case eCompensateLeading:
   preferredLineHeight = NSToCoordRound(emHeight * NORMAL_LINE_HEIGHT_FACTOR);
   if (!internalLeading && !externalLeading)
     normalLineHeight = preferredLineHeight;
   else {
     normalLineHeight = emHeight+ internalLeading + externalLeading;
     if (normalLineHeight > preferredLineHeight)
         normalLineHeight = preferredLineHeight;
   }
   break;

Comment 12

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

Or we should hack in gfx.

Comment 13

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

http://lxr.mozilla.org/mozilla/source/gfx/thebes/src/gfxAtsuiFonts.cpp#132

> 132     if (mMetrics.maxHeight - mMetrics.emHeight > mMetrics.internalLeading)
> 133         mMetrics.emHeight = mMetrics.maxHeight - mMetrics.internalLeading;

if (mMetrics.maxHeight - mMetrics.emHeight > mMetrics.internalLeading) {
    gfxFloat emHeight = mMetrics.maxHeight - mMetrics.internalLeading;
    if (emHeight * 1.2f < mMetrics.maxHeight)
        mMetrics.emHeight = emHeight;
}

??

Comment 14

[Felix Miata]

If you do anything special for Lucida Grande, you probably need to do the same for Lucida Sans Unicode. I don't think there's any significant difference between them other than the names and what platforms you find them on. Check it out with this testcase: http://mrmazda.no-ip.com/auth/Font/font-lsansuni-lh.html

Comment 15

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

Wow, we have a mistake. We treat the 'leading' as internal leading. But it's wrong. The leading of Mac means external leading. We need to check the current logic.

Comment 16

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

taking this.

Comment 17

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

Attachment: Patch rv1.0

This uses the 'leading' as external leading.
This patch works fine for both western font and Japanese font.

Comment 18

[philippe (part-time)]

(In reply to comment #17)
> Created an attachment (id=247330) [edit]
> Patch rv1.0

I've used this patch on my home-made Camino build. It fixes all issues, and while visiting webpages, haven't seen or found any problems.

Comment 19

[Phil Ringnalda (:philor)]

*** Bug 361922 has been marked as a duplicate of this bug. ***

Comment 20

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

checked-in.