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

RESOLVED FIXED in mozilla0.9.5

Status

()

Core
Layout
P2
major
RESOLVED FIXED
18 years ago
13 years ago

People

(Reporter: Erik van der Poel, Assigned: rbs)

Tracking

({css1, fonts, testcase})

Trunk
mozilla0.9.5
css1, fonts, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
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.
(Reporter)

Updated

18 years ago
Assignee: erik → rickg
Priority: P3 → P2
(Reporter)

Comment 1

18 years ago
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.
(Reporter)

Comment 2

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

Updated

18 years ago
Assignee: rickg → troy

Comment 3

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

Updated

18 years ago
Assignee: troy → kipp

Comment 4

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

Comment 5

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

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

Comment 7

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

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

Updated

18 years ago
Target Milestone: M15 → M14

Updated

18 years ago
Blocks: 11779

Updated

18 years ago
Summary: change layout engine to get font heights dynamically → [TEXT] change layout engine to get font heights dynamically
(Reporter)

Comment 9

18 years ago
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.
(Reporter)

Comment 10

18 years ago
Steve, take a look at this bug report. I answered your questions.
(Reporter)

Updated

18 years ago
Assignee: kipp → erik
(Reporter)

Comment 11

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

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 12

18 years ago
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.
Keywords: css1
Summary: [TEXT] change layout engine to get font heights dynamically → [TEXT] change layout engine to get font heights dynamically {ll}
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.

Updated

18 years ago
Blocks: 24854
(Reporter)

Comment 14

18 years ago
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.
Keywords: beta1

Comment 15

18 years ago
Putting on the PDT+ radar for beta1.
Whiteboard: [PDT+]
(Reporter)

Comment 16

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

18 years ago
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.
(Reporter)

Comment 18

18 years ago
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.
(Reporter)

Updated

18 years ago
Whiteboard: [PDT+] → [PDT+] 2/14
(Reporter)

Updated

18 years ago
Whiteboard: [PDT+] 2/14 → [PDT+] 2/16
(Reporter)

Updated

18 years ago
Whiteboard: [PDT+] 2/16 → [PDT+] erik deems fix too dangerous for M14
(Reporter)

Comment 19

18 years ago
Created attachment 5374 [details]
line-height test case
(Reporter)

Comment 20

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

Updated

18 years ago
Whiteboard: [PDT+] erik deems fix too dangerous for M14 → [PDT+] ETA 3/3

Comment 21

18 years ago
Added notation that this bug will become PDT- for beta on 3/3
Whiteboard: [PDT+] ETA 3/3 → [PDT+] ETA 3/3 w/b minus on 3/3

Comment 22

18 years ago
With no update in the bug... we passed the 3/3 target... so I'm changing this to 
PDT-
Whiteboard: [PDT+] ETA 3/3 w/b minus on 3/3 → [PDT-] ETA 3/3 plus for beta2
(Reporter)

Updated

18 years ago
Whiteboard: [PDT-] ETA 3/3 plus for beta2 → [PDT-] plus for beta2
Target Milestone: M14 → M15
(Reporter)

Comment 23

18 years ago
Will try to get this in for M16, i.e. after Beta 1 branch.
Target Milestone: M15 → M16

Comment 24

18 years ago
Putting on beta2 keyword radar since this was "plus for beta2" during beta1 
triaging
Keywords: beta2
Removing the [PDT-] to trigger re-evaluation.
Whiteboard: [PDT-] plus for beta2 → plus for beta2
(Reporter)

Comment 26

18 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Keywords: beta1, beta2
Resolution: --- → WONTFIX
Whiteboard: plus for beta2

Comment 27

18 years ago
Marking verified won't fix based on last comments.
Status: RESOLVED → VERIFIED

Comment 28

18 years ago
Marking verified won't fix based on last comments.

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.
Blocks: 63316
Status: VERIFIED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 30

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

Comment 31

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

17 years ago
erik resign. reassign all his bug to ftang for now.
Assignee: erik → ftang
Status: REOPENED → NEW

Comment 33

17 years ago
shanjian- can you take over this one. I am not sure how soon we need to address 
this one.
Assignee: ftang → shanjian
Target Milestone: M16 → ---

Comment 34

17 years ago
mark it as moz0.9.3
Target Milestone: --- → mozilla0.9.3
(Assignee)

Updated

17 years ago
Blocks: 74186
(Assignee)

Comment 35

17 years ago
I now have a fix for this - see bug 74186

Comment 36

17 years ago
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!
Assignee: shanjian → rbs
(Assignee)

Comment 37

17 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 38

17 years ago
Another Milestone shift... Hopefully should be over soon.

Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Updated

17 years ago
Depends on: 96609
Keywords: fonts, testcase
(Assignee)

Comment 39

17 years ago
Fixed as part of my font changes in bug 99010.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.