20394 - [TEXT] change layout engine to get font heights dynamically {ll}

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Erik van der Poel]

Currently, the layout engine gets the height of a piece of text by calling
nsIFontMetrics::GetHeight(), which currently returns the height of the ASCII
font only. I.e. even if other fonts from the font-family list are loaded into
the nsFontMetrics object, their heights do not affect the return value of
GetHeight. This is a problem when the other fonts are larger than the ASCII
font.

We could change GetHeight to return the max height of the fonts that happen to
be loaded at that time, but that would invalidate the results that were obtained
before those fonts were loaded. A reflow in those parts of the document would
get a different answer the next time around.

Alternatively, we could load all of the fonts needed to display any Unicode
character at nsFontMetrics init time, but that would be slow and waste memory.

A better solution would be to get the layout engine to ask for the max font
height *for a particular string*. Currently, the layout engine calls
nsIRenderingContext::GetWidth to get the width of a string. We could add a new
API to nsIRenderingContext to get not only the width of a string, but also the
max font height used by that string.

The layout engine currently calls nsIFontMetrics' GetHeight, GetMaxAscent and
GetMaxDescent quite near the place where nsIRenderingContext::GetWidth is
called. It might be a good idea to merge those 4 calls into one.

Comment 1

[Erik van der Poel]

Rick, could you assign this bug to an appropriate person in your group? I
believe this bug would normally have landed on Kipp's lap.

Comment 2

[Erik van der Poel]

Also, nsIRenderingContext::DrawString uses nsIFontMetrics::GetMaxAscent to
determine the Y coordinate of the text. We will need to alter the DrawString
methods to take the ascent as an argument, which should be passed in by the
layout engine, which will get this ascent value from the new measuring API
mentioned above.

Comment 3

[rickg]

Troy -- one for you. You might be able to take a quick whack at this, if not,
move it to the virtual kipp list to be attended to in January by Nisheeth and
Buster.

Comment 4

[leger]

Updating to default Layout Assignee...kipp no longer with us :-(

Comment 5

[troy]

Why are you re-reassing layout bugs? Do NOT touch layout bugs.

The bugs are assigned to Kipp so they can stay neatly organized until we have a
new owner for the block/inline code.

Comment 6

[buster]

erik, bobj, ftang: please give us some information about how important this is.
Is the result that some pages lay out incorrectly? Is the effect severe, or
relatively minor? Is the problem common?  Can you provide a simple test
case?
Thanks.

Comment 7

[rbs]

Yes it can lead to incorrect layout when Mozilla goes to the mainstream.
I think the problem is mostly going to hit international users, or
when a string "&Entity1;&Entity2;&Entity3;" requires several fonts
with widely different metrics. I experienced a related difficutlty
in MathML where several symbols are mixed. The problem was solved by
making DrawString use the baseline. As Erik explained, for things to work in
general, the max{of maxAscent of all the fonts involved to draw the string}
is needed. DrawString will then use that information to appropriately shift
the substrings of adjacent chars that are representable in the same font.

Erik is not around now. He posted the following on n.p.m.mathml:

> Erik van der Poel wrote:

[snip..]

> I am also planning to make several changes
> to the font-related APIs. For example, currently, the layout engine
> calls nsIFontMetrics's Height, MaxAscent and MaxDescent methods, but the
> problem is that nsIFontMetrics contains potentially more than one font,
> and we don't want to load them all at init time. (We load lazily.) So I
> intend to move the Height, Ascent and Descent parameters to
> nsIRenderingContext, and change GetWidth to GetDimensions. This makes it
> look more and more like MathML's GetBoundingMetrics, but there is still
> a difference: GetDimensions would return the MaxAscent and MaxDescent of
> the *fonts* required by the given string, while GetBoundingMetrics
> returns the bounds of the *string*.

[snip..]

> Erik

Comment 8

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 9

[Erik van der Poel]

Any East Asian document (e.g. Japanese) currently displays incorrectly (ugly
overlapping) on Unix, because East Asian fonts are much larger than Western
fonts on that platform. I believe both Unix and Japanese are important enough
to justify the proposed change. Also, since we need to change the APIs in a
relatively sensitive area, my suggestion is to make this change sooner rather
than later. I.e. before Beta 1. Since I understand the Windows and Unix font
code, I might be the best owner for those platforms. Frank knows the Mac font
code. We would need to change the TextFrame code too, of course. I think I could
handle that without the Gecko group's help.

Comment 10

[Erik van der Poel]

Steve, take a look at this bug report. I answered your questions.

Comment 11

[Erik van der Poel]

I would like to own this bug (for Windows and Unix). May assign Mac part to
ftang later.

Comment 12

[buster]

Erik:
First, thanks for taking this bug.  That's a major load off my mind.
Second, when you get a solution in mind please run it by Troy and myself.  In
essence, I agree with what you're doing.  I just want to make sure that the
design get's looked at by a couple of set of eyes from the layout team, so we
might suggest ways to maintain (or increase) performance along with the added
functionality.  The main thing to keep in mind is the performance of incremental
reflow.

Comment 13

[Hixie (not reading bugmail)]

The second image in this section of David's inline box model document:
   http://www.fas.harvard.edu/~dbaron/css/2000/01/dibm#heights
...shows what should probably happen here.

Comment 14

[Erik van der Poel]

Adding "beta1" keyword. PDT team, this bug is very important for beta because
Japanese HTML documents and Japanese localized UI look very bad on Unix at the
moment. PDT+ approval strongly urged.

Comment 15

[leger]

Putting on the PDT+ radar for beta1.

Comment 16

[Erik van der Poel]

Steve and Troy, as you can see in the image in David's inline box model doc
mentioned above, the "font boxes" inside the inline box are quite similar to
the inline boxes within the line box. I.e. the font boxes have varying heights,
and are vertically aligned at various positions due to the variation in font
baselines.

However, I think it might be a good idea to avoid creating even more frames for
this. Instead, I'd like to implement this in nsIRenderingContext, in a new
method called GetDimensions. But to add proper leading and to align vertically,
we need to pass the line-height and vertical-align info to GetDimensions. The
problem is that the GFX engine does not currently #include StyleContext, and
I'm not sure that we would want to add such an #include (and run-time)
dependency (from the low-level GFX, to the higher-level Style System).

Should we just have the GFX header define some new datatypes and/or constants
for line-height and vertical-align? Similar to the constants in nsFont.h, but
line-height isn't a simple set of constants. It can be a factor (CSS "number"
and "percentage") or a CSS "length" (e.g. 1.2em).

I'd like your opinions before I go down this route.

Comment 17

[buster]

the style context data structures are definately private to layout.  even though
it'll cause a tiny amount of bloat, I'd suggest creating your own data structure
as needed in gfx.

your general approach sounds fine.

I'd be interested in understanding the impact on performance.  Do you have any
plans for concretely measuring the performance impact of your change?  It would
be good to get some timing code in place, and to make some apples-to-apples
comparisons of a variety of interesting pages.  One goal should be that "normal"
pages that don't include any interesting glyph anomalies should display just as
fast with the new code as with the old, and that the performance price we pay
should at worst be proportional to the number of additional fonts that are loaded.

Comment 18

[Erik van der Poel]

OK, I will create separate data structures for GFX.

I agree that it would be nice to have some way of measuring the performance
concretely. Let me think about that some more. Any ideas and help from the Gecko
team would be appreciated!

The loop in GetWidth stays with a single font if all characters can be displayed
with that font. So we would only check the height and ascent when the font does
actually change. In other words, the performance impact would be zero for
documents (e.g. English) that don't change fonts often.

Comment 19

[Erik van der Poel]

Attachment: line-height test case

Comment 20

[Erik van der Poel]

Times New Roman at 16px has a tmHeight of 19 on Windows, and a tmExternalLeading
of 1. The new test case above shows 10 lines in 190 pixels in WinIE5. This means
that they are using tmHeight for line spacing, not tmHeight + tmExternalLeading.
Nav4 rounds the 16px down, so the 10 lines take up less pixels. Mozilla is
already using tmHeight for "normal" line-height. So we probably want to keep it
that way.

Comment 21

[Jim Roskind]

Added notation that this bug will become PDT- for beta on 3/3

Comment 22

[Jim Roskind]

With no update in the bug... we passed the 3/3 target... so I'm changing this to 
PDT-

Comment 23

[Erik van der Poel]

Will try to get this in for M16, i.e. after Beta 1 branch.

Comment 24

[leger]

Putting on beta2 keyword radar since this was "plus for beta2" during beta1 
triaging

Comment 25

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

Removing the [PDT-] to trigger re-evaluation.

Comment 26

[Erik van der Poel]

After thinking about it for a while, I've come to the conclusion that it would
not be a good idea to make this kind of change to the cross-platform code at
this point in the project, just to work around the issues we have with poor
fonts on Unix. I am instead going to pursue having a minimum font size pref
tied to the language group on Unix. This will cause Latin-1 fonts to be larger
when the language is e.g. Japanese, so that there would no longer be a size
mismatch between Latin-1 fonts and Japanese fonts within the same document.
Marking this bug WONTFIX. Removing beta1 and beta2 keywords. I don't think the
css1 keyword is correct. Leaving it there for now in case someone else has
comments. Removing the beta2+ from status whiteboard.

Comment 27

[Chris Petersen]

Marking verified won't fix based on last comments.

Comment 28

[Chris Petersen]

Marking verified won't fix based on last comments.

Comment 29

[Brian Ryner (not reading)]

reopening:

As far as I can tell, the only real reason for closing this bug as WONTFIX was
lack of time to fix it leading up to the release of NS6.  I think we should
probably re-examine this issue, especially since it may lead to a fix for 63316.

Comment 30

[Erik van der Poel]

bryner: It would be great for Unix Mozilla if we could fix bug 20394 in a
satisfactory manner. I'm not sure, but performance of the other platforms (e.g.
Windows and Mac) might be impacted by such a change. We would be getting not
only the width of a string, but also the height of the largest font used by
that string. So there would be some additional computation, and I don't know
how much that would affect performance. One way around that might be to stub
out the height calculation on Windows and Mac, since those platforms aren't
affected as much by this English/Japanese font size disparity.

A very quick and dirty solution would be to just decrease the minimum font size
settings in the default pref file (modules/libpref/src/unix/unix.js).

A slightly less quick and dirty solution would be to dynamically find out what
the minimum size of the default Japanese font is, and use that for ja. This way,
you would end up with e.g. 16px for Korean on a stock X Windows release, but a
smaller size on a real Korean user's machine, since they have downloaded the
more popular, small fonts.

Comment 31

[rbs]

Lucky start on Win32. I had a quick look in the Win32 code and every place where
GetWidth() is called, the height component is also available, but only size.cx
is returned, dropping size.cy on the floor. So GetWidth() could be changed to
GetSize(..., aWidth, aHeight) at not additional cost.

Comment 32

[Frank Tang]

erik resign. reassign all his bug to ftang for now.

Comment 33

[Frank Tang]

shanjian- can you take over this one. I am not sure how soon we need to address 
this one.

Comment 34

[Frank Tang]

mark it as moz0.9.3

Comment 35

[rbs]

I now have a fix for this - see bug 74186

Comment 36

[Shanjian Li]

rbs, I took a look at 74186. Though I haven't examined your patch yet, my 
understanding is that 74186 depend on this bug, but it will not block this one. 
So I am suggesting to fixed separately, of cause this one should be the first to 
be taken care of. I reassign this one to you for now. Once you are comfortable 
with your patch, we might want to examine it XP impact, and propose proper 
change for unix and mac. 
Thanks for your wonderful work!

Comment 37

[rbs]

Milestone shift... Need interested folks to try out the branch that has the fix.
It may be too late for changes like this soon. See bug 74186.

Comment 38

[rbs]

Another Milestone shift... Hopefully should be over soon.

Comment 39

[rbs]

Fixed as part of my font changes in bug 99010.