Mozilla's font preferences only allow a single size setting for both serif and sans-serif fonts. The problem is that the font sizes don't really match for different font. For example, I use Times New Roman (TT version, grabbed from Windows) as my default serif font, and Verdana as my default sans-serif fonts. Times New Roman at size 18 is right about the size I want, while Verdana is huge! If I were only able to tell Mozilla to use size 16 for Verdana, they would be even. Please note that this is not about personal pickiness: fonts rarely match perfectly. But for some fonts, the difference in actual size is really striking, and it makes specification of different sizes a necessity.

confirming.

Nominating for mozilla0.9.1. Looks there is already a nice spot on the pref. dialog box awaiting this.

This is definitely a back-end issue in: http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsPresContext.cpp#199 because we currently don't understand the concept of having more font sizes than "variable" and "fixed". So, this bug is in the wrong place. erik was the last person to touch this code, so we'll start with him. Here's the elevator story: for CSS compliance, I think we need to support different default font sizes for the five CSS font families - Serif, Sans-Serif, Monospace, Cursive and Fantasy. Currently we just have fixed and variable. I can do the UI for this in five minutes when the back end is in. Gerv

Oops. Reassign. (Is this right?) Gerv

Yes, we have had endless debates on this one. The sizes in the back-end are for variable and fixed because that is all we had in the old HTML (before CSS came along). We continued to support 2 different sizes for variable and fixed since the old Netscape browser has always defaulted to Times New Roman at 12pt and Courier New at 10pt (since they looked strange when they were at the same size). Take a look at the old messages in the mozilla.layout newsgroup. Look for Todd Fahrner and myself.

This is a good one. But it mean we need to change the UI. Reassign to nhotta and mark it as Future for now.

Taking this bug while I am looking at bug 61883. Now that I am seeing the big picture of the back-end, things appear brighter in... hmm, their intricacy. Setting the size is non ambiguous only if the font-family list has exactly _one_ entry, and that unique entry is a generic font, right? E.g., "font-family: serif", "font-family: sans-serif". Otherwise, I don't see how to fix this bug in the context of multiple entries in the font list (remember that there is font-switching in GFX and the size cannot be changed half-way during the search if they are several entries). If there is only one generic entry, then yes, a pref value can be taken in nsPresContext and later examined during style resolution (in nsRuleNode.cpp). Here are the two categories of situations: <docA> font-family: serif-font1, sans-serif-font2, serif; or any other author's provided _list_ <docA> Here, what to do? It is ambiguous. (Possibly: just take the font-size from nsPresContext::mDefaultFont.) <docB> <!-- OK, can be handled: --> font-family: unspecified; or: font-family: generic; <docB> Here, all is fine, a default list can taken with a corresponding font-size.

rbs: I think this bug is asking for the ability to set a different size for each font family in the UI. I thought that this feature was included in the work you are currently doing? It's not ambiguous - the font size you set for e.g. Serif is the size you want for whichever Serif font you have selected. Gerv

Yep, the ability to set different font-sizes for generic family fonts is included in what I am doing. That's why I became interested in this bug. What I wanted to make clear was a situation like the following one. Suppose an author has only specified the font-family list, using this pseudo-markup for convenience: <docA> {font-family: serif-font1, sans-serif-font2, serif} <docA> i.e., there is no author's provided font-size. And so it is up to the UA to pick the default size. Of the different font-sizes that will now be supported, which one should be picked? As I noted, you can only pick one size for the whole font-family property. It is like an incomplete '{font: ...}' property that has to be completed by the UA. And just like it isn't possible to specify diffent font-size per each font in the font-family list, it won't be possible to pick the user's preferred serif's font-size for the serif-font1, and the user's preferred sans-serif's font-size for the sans-serif-font2, etc. What could be done is to pick the default variable font-size. Is it what is expected by this bug? === The other case, e.g., <docB> {font-family: serif} <docB>, is OK, since it suggests to just pick the user's preferred serif's font-size.

My comments are more related to how to interprete the sizes in the back-end. (There is no ambiguity in the front-end to select different font-sizes.)

Reassign to ftang, cc to bstell, yokoyama.

Hixie, mpt: heads-up. Your opinion is wanted here on how to choose the font size to use for fonts which don't have one set (through not being one of the six selected in the UI.) rbs: that's a good question :-) The algorithm I'd suggest is as follows, although I don't know if you have enough info where you are in the code to implement it. If the font you end up choosing is one of the six actively mentioned in the UI, then use the size associated with it. Otherwise, use the size associated with the first generic family mentioned in the CSS. For example: font-family: serif-font1, font2, serif If you had serif-font1 as your Serif font, you use the Serif size. If you'd never heard of serif-font1, but had font2 available (but not one of your six), pick that. Then, keep looking, and use the font size associated with serif, being the first generic font family specified. If all else fails, fall back on the size specified for Proportional. You see, when specifying font-families, the normal thing to do is put a bunch of related fonts, then a catch-all family at the end. I think the above algorithm has the best chance of producing something that works for the user. Given that rbs is actively working on this bug, reassigning back to him. Unless Frank has a fix already? :-) Gerv

My personal opinion is that there should only be one font size setting, the "medium" font size, in pixels.In CSS it makes no sense to have different font sizes for different font families. I've said this before various times, and people always disagree. So I'll just wait until it is implemented, and then file some standards compliance bugs... The "correct" solution is to implement font-size-adjust and @font-face and then give the aspect ratios for the user's selected fonts in @font-face blocks.

> In CSS it makes no sense to have different font sizes for different font > families. Why? What happens when the author doesn't specify a size, and your Fantasy font happens to be the same size in 16pt as your Serif font in 14pt? Or you like your Script font bigger because Script fonts are unreadable? > The "correct" solution is to implement font-size-adjust and @font-face and > then give the aspect ratios for the user's selected fonts in @font-face > blocks. I don't understand this. Is this a solution for content providers or for Mozilla? Is it highly-advanced CSS fu that I should be reading about somewhere? :-) Gerv

> [...] and your Fantasy font happens to in 16pt as your Serif > font in 14pt? [...] Is it highly-advanced CSS fu that I should be reading > about somewhere? I would recommend reading these sections: http://www.w3.org/TR/REC-CSS2/fonts.html#propdef-font-size-adjust http://www.w3.org/TR/REC-CSS2/fonts.html#font-descriptions http://www.w3.org/TR/REC-CSS2/fonts.html#descdef-x-height In CSS, "16pt" and "14pt" are different sizes, so a 16pt font cannot "be the same size" as a 14pt font. It can have a different em:ex aspect ratio, though, and that is what font-size-adjust is about. > Is this a solution for content providers or for Mozilla? Mozilla.

Got an idea how to simulate this in Mozilla for the 5 basic CSS fonts. It doesn't involve implementing the font-size-adjust property, and it isn't meant to be a perfect solution. But it might make Mozilla to shine even brighter... The idea is to add an 'Auto' option in the front-end as a possible font-size. And once this is selected, the internal size should be set to (see Hixie's link above for the formula): c <- y*(a/a') where: y = 'font-size' of first-choice font: i.e., in Mozilla, this should be the current size selected by the user for the current 'Proportional' font a' = aspect value of available font, i.e the ratio em/xheight from the current font selected by the user for that generic font family c = 'font-size' to apply to available font, i.e., the size that the back-end will use. So if font.current.[langroup].[generic].size = 0 [Auto], then I can override and pass this actual 'c' to GFX, leaving 0 for the menu... Remember that when the support for these generic fonts is complete, with the ability to set different font-sizes for each group, it is going to take a lot of gymnastics to adjust all these numerous font-sizes. If one is to be changed, then the user has to adjust the whole lot for consistency. So this 'Auto' option is going to be also useful in that respect. See the link that Hixie gave above to see a screenshot of how the actual font-sizes will be automatically homogenized with this 'Auto' option.

The screenshot is for the rending of: <html><head><title>Bug 74186</title></head> <body> <span style="font-family: Verdana">Verdana</span> <span style="font-family: Flemish Script BT">Flemish Script BT</span> <hr /> <span style="font-family: serif">Verdana-x</span> <span style="font-family: cursive">x-Flemish Script BT</span> </body> </html> On the first line, there is no generic family so no adjustment happens. The default font-size is the size set for the variable font (aka 'Proportional'). On the second line, the fonts are generic are therefore, the new code is doing the adjustments. My prefs.js has: user_pref("font.current.x-western.-moz-variable.size", 16); //i.e, basesize=16px user_pref("font.current.x-western.cursive.size", 0); // [Auto] !! user_pref("font.current.x-western.cursive.name", "Flemish Script BT"); user_pref("font.current.x-western.serif.size", 16); //just to see if preserved user_pref("font.current.x-western.serif.name", "Verdana"); As you can see, it helps a lot. The idea of the font-size-adjust is just to homogenize the 'x'-height as I illustrated on the screenshot. In fact, the CSS spec says "This property allows authors to specify an aspect value for an element that will preserve the x-height of the first choice font in the substitute font."

Gerv: what you described in "Additional Comments From Gervase Markham 2001-06-08 11:48" could allow to stabilize the other case of the first-line, i.e., the general case where the font-family is not necessarily a generic name. But to that would require a special property in the Style System, something like 'font-size: -moz-auto-adjust' [inherit] to instruct the Style System to do the work during style resolution... What I am doing is in nsPresContext::GetFontPreferences(). It is a well-confined place where the default generic fonts are initialized based on the current prefs values. When the size is 'Auto' I override and set the proper adjusted font-size to be used internally. With -moz-auto-adjust, a similar calculation can be done during style resolution. So you got to file another bug and convince the style people that it is worth doing it at some stage :-)

Err... Actually, the best thing to do is the spec... Implement the CSS 'font-size-adjust' property by itself, as: font-size-adjust: <number> | none | inherit | -moz-auto

Hixie, there is something not very clear to me in the CSS spec about this 'font-size-adjust' property when the value is 'none'. Unlike other properties that support the 'none' value, the 'font-size-adjust' property is a bit peculiar because it specifies a relationship with child fonts, i.e., when a font-adjust-property is encountered, it merely tells how new fonts should be treated, and has no impact on the current element whatsoever. So should 'none' do the same? I.e., 'none' should only stop the adjustments in subsequent fonts without resetting the current font-size? Looking at some examples, it seemed somewhat against the intuition and that's why I wanted to double-check. See the following example with my indicated XXX comments: <div style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> <!-- i.e., for all new fonts that may arise from now on, make sure to pick a font-size such that their actual x-height is 16*0.55 ~= 9px --> <!-- adjust:none is the initial value from the UA, it applies... --> This is an _unadjusted_ text in Verdana @ 16 px <span style="font-family:Arial"> <!-- child with new font... --> <!-- adjust:0.55 from the parent applies... --> This is an _adjusted_ text in Arial such that the x-height is 9px </span> <p style="font-family:Arial; font-size-adjust:none"> <!-- i.e., for all new fonts that may arise from now on, don't enforce any constraint on their actual x-height --> <!-- should adjust:0.55 from the parent apply? --> XXX This is a text in Arial without a new font being specified. XXX What size is inherited right here? <span style="font-family:Times"> <!-- child with new font... --> <!-- adjust:none from the parent applies... --> This is an _unadjusted_ text in Times @ 16px </span> </p> </div>

rbs: The first problem I found when trying to answer your question is that there is an error in the spec which has not yet been added to the errata. I have brought this up with the working group. I hope to have it resolved ASAP. In the meantime, you should assume that David's proposal in: http://lists.w3.org/Archives/Public/www-style/1999Feb/0032.html ...is accepted. What that means is that font-size-adjust applies to _all_ the fonts, not just the second and subsequent fonts. To answer your questions -- There is no way to tell (from a CSS point of view) if a property was inherited or setm so in your example the style attributes are IDENTICAL to those in: <div style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> ... <span style="font-family:Arial; font-size:16px; font-size-adjust:0.55"> ... </span> <p style="font-family:Arial; font-size: 16px; font-size-adjust:none"> ... <span style="font-family:Times; font-size: 16px; font-size-adjust:none"> ... </span> </p> </div> As I understood it, the only way font-size-adjust affects children is through inheritance. For example: <div style="font-family: Non-Existent, HandScript; font-size: 16px; font-size-adjust: 0.5625;"> <!-- The font Non-Existent is assumed to have an ex-height of 16px*0.5625, which is 9px. Since it is not found, the font HandScript is used instead. It has an ex-height:em-height ratio of 0.3, and therefore the ACTUAL font-size is 16px*(0.5625/0.3) = 30px. --> This is adjusted HandScript with an actual font-size of 30px. <p style="font-family: Verdana; font-size: 32px;"> <!-- Verdana has an ex-height:em-height ratio of 0.55, and therefore the ACTUAL font-size is 32px*(0.5625/0.55) = 33px, because the font-size-adjust is inherited from the parent element. --> This is adjusted Verdana with an actual font-size of 33px. </p> </div> Of course you can cause weird effects that way: <div style="font-family: Verdana; font-size: 1px; font-size-adjust: 100;"> <!-- Verdana has an ex-height:em-height ratio of 0.55, and therefore the ACTUAL font-size is 1px*(100/0.55) = 181px, because the font-size-adjust is applied to the first font even if the first font is available and font-size-adjust was not inherited. --> This is some very large "1px" text! :-) </div> Filling in your example and correcting the comments to match how I understand the spec (including the assumed errata): <div style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> <!-- i.e., for this element, make sure to pick an actual font-size such that their actual x-height is 16*0.55 ~= 9px --> <!-- adjust:none is the initial value from the UA, however it is irrelevant at this point. --> This is an _adjusted_ text in Verdana -- however since Verdana has the same ex:em ratio as the value of font-size-adjust *right here*, the actual font size is the computed font size is 16px. <span style="font-family:Arial"> <!-- child with new font... --> <!-- adjust:0.55 from the parent applies... --> This is an _adjusted_ text in Arial such that the x-height is 9px </span> <p style="font-family:Arial; font-size-adjust:none"> <!-- i.e., for all new fonts that may arise from now on, don't enforce any constraint on their actual x-height --> <!-- adjust:0.55 from the parent is irrelevant here --> This is a text in Arial without a new font being specified. It has inherited the computed font-size of 16px. <span style="font-family:Times"> <!-- child with new font... --> <!-- adjust:none from the parent is inherited... --> This is an _unadjusted_ text in Times @ 16px </span> </p> </div> If you have some peculiar scenarios, such as: <div style="font-family: HebrewFont, RomanFont; font-size: 16px; font-size-adjust: 1.0;"> Some Hebrew Text Some Roman Text </div> ...and the HebrewFont has an aspect ratio of 2, and the RomanFont has an aspect ratio of 0.5, and the relevant pieces of text are drawn in their respective fonts after glyph lookup, then the computed font size will be 16px all the way through, but "Some Hebrew Text" will have an actual font size of 8px, and "Some Roman Text" will have an actual font size of 32px. Everything else (other than font rendering) uses the computed value of font-size. This includes the "em" and "ex" units, percentages and integers on some properties, inheritance, and so on. I hope I didn't make any errors in this reply...! :-)

Great explanation which shows also how useful the feature is. Although it seems tough to implement -- re: your latest bit on size switching... It reminds me of bug 20394.

Fixing this bug requires to: - add support of 'font-size-adjust' in the Style System (done -- modulo cleanup, see ongoing patch in bug 84398). Based on Hixie's comments, sizeAdjust can become a field of nsFont so that it can be queried anywhere in GFX when switching between each of the fonts in the font family-list. In other words, nsFont::size will be the computed size, and the actual adjustments using nsFont::sizeAdjust will take place in GFX based on the particular metrics of the current existing and selected font in the font family-list. For this to work requires fixing bug 20394, i.e., - add GetDimensions() in nsIRenderingContext, and newer versions of DrawString() as per the comments in that bug. The goal being that a string with differently sized glyphs should remain neatly aligned. (This is the kind of things that has been happening in MathML BTW, it is just a pity that bug 20394 -- note the number -- has been repeatedly pushed out from the mainline.) - Teach nsTextFrame to stop using GetHeight() which not only may not be the metrics that is used for the string at hand (case of I18N texts), but would be unadjusted if/when nsFont::sizeAdjust becomes operational. nsTextFrame needs to instead use GetDimensions() which will return the ACTUAL dimensions of its string argument, while switching between fonts as appropriate, and adjusting according to nsFont::sizeAdjust.

rbs: Ok, the CSS Working Group has discussed this and we agreed to the errata. The errata text will be: : : 'font-size-adjust' should be applied to the first choice font as : well. (This should have no effect since the value of : 'first-size-adjust' typically will be the ex:em ratio of the first : choice font.) This corrects an oversight related to cases where the : property is inherited into children inline elements.

I got a patch that is working so-so. But seeing how the patch is coming along, I am not very keen to complete it and clean it up. The primary concern is that it is getting large, thus scary. And since I am working on Windows, I find it hard to believe that other platforms can quickly catch up. Anyway, here is what I did for the record: - corrected the patch in the Style System to exactly function as Hixie described above. Along with this, I added a |float sizeAdjust| in nsFont. The Style System only computes the value, and the actual adjustments are made in GFX. - on WinGFX: - I modified nsFontMetricsWin so that each of the native nsFonWin (which are the actual fonts created for each family of the font-family list) keeps its own, possibly adjusted, metrics. - Added CreatedAjustedFont(), and replaced CreateFontIndirect() with this where _relevant_. - Added a GetDimensions() in nsRenderingContextWin, and tweaked DrawString() to handle different nsFontWins with different metrics [the gymnastics here resemble what is presently happening in nsMathMLChar::DrawGlyph()] - Tweaked nsTextFrame (_really tweaked_ just for testing purposes) to use GetDimensions. While I was there, I made the following simplifying changes too: If you look at nsFontMetricsWin, you will see lots of place with hand-rolled growing arrays like this: [...] if (mLoadedFontsCount == mLoadedFontsAlloc) { int newSize = 2 * (mLoadedFontsAlloc ? mLoadedFontsAlloc : 1); nsFontWin** newPointer = (nsFontWin**) PR_Realloc(mLoadedFonts, newSize * sizeof(nsFontWin*)); if (newPointer) { mLoadedFonts = newPointer; mLoadedFontsAlloc = newSize; } else { ::DeleteObject(hfont); return nsnull; } } [...] By just using a nsVoidArray, this whole chunk of code can be removed. While it is true that this makes the code smaller, it is equally true that it does make the opposite on the patch :-) The patch looks big at the end. Also, if I were to continue, I will have to change other things -- like what is happening with GetWidth/DrawString/GetBoundingMetrics and now GetDimensions()... You have all these functions that loop over a string to find contiguous segments that are representable in the same font. I came to the thought that they can all be folded in a scanner-like function. The idea being that the scanner will take a font-switching callback function as argument, find the segements, and callback with the current segment and its length... This way GetWidth will just set up a callback function that gets the width of the segment and accumulates it, DrawString will draw the segment, etc. That could lead to less duplication of code, and ease maintainance. Hence, adding a new function like GetDimensions() for example will amount to setting up a callback function that just knows how to to get the dimensions of a string segment representable in the _same_ font. Basically the patch is getting too long, and if I were to continue, there will still be more changes. All of these make the overall picture (re: other platforms) blur. I will think a day or two about what to do, and future this bug in the light that this is m0.9.3 where such large changes should be minimized.

rbs: the errata is now official: http://www.w3.org/Style/css2-updates/REC-CSS2-19980512-errata.html#x21

Typo? 'first-size-adjust'

Attachment 40234 [details] is for the following fragment: <!-- no adjustment --> <p style="border: solid 1px red; font-family:Verdana; font-size:16px;"> Verdana-x <span style="font-family: Flemish Script BT"> x-Flemish-x &alpha; <!-- not available in the Flemish font - will cause font-switching --> <span style="font-family: Book Antiqua"> x-Book Antiqua-x </span> &delta; <!-- not available in the Flemish font - will cause font-switching --> x-Script </span> </p> <!-- adjustment without border --> <p style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> Verdana-x <span style="font-family: Flemish Script BT"> x-Flemish-x &alpha; <!-- not available in the Flemish font - will cause font-switching --> <span style="font-family: Book Antiqua"> x-Book Antiqua-x </span> &delta; <!-- not available in the Flemish font - will cause font-switching --> x-Script </span> </p> <!-- adjustment with border --> <p style="border: solid 1px red; font-family:Verdana; font-size:16px; font-size-adjust:0.55"> Verdana-x <span style="font-family: Flemish Script BT"> x-Flemish-x &alpha; <!-- not available in the Flemish font - will cause font-switching --> <span style="font-family: Book Antiqua"> x-Book Antiqua-x </span> &delta; <!-- not available in the Flemish font - will cause font-switching --> x-Script </span> </p>

The reason why the changes involved are getting intricate is because this is something that should have been considered earlier. Quoting Erik from bug 20394, "since we need to change the APIs in a relatively sensitive area, my suggestion is to make this change sooner rather than later." It might perhaps be too late now. If this bug was to be fixed, the Font prefs could just have an innovative 'Adjust' value for each of the generic fonts (as a replacement for the idea of individual font-sizes). User could "pick" a font-size-adjust value to scale-up or scale-down their effective size compared to the 'Proportional' font as they wish. ("pick" is any means here, numeric values, or a 'slider' with a text preview.) Because this attribute would then be handled at the Style System layer, the chrome could be exempted from user-defined generic values, and the min-size would be enforced as needed.

"a 'slider' with a text preview" should read "a 'slider' with a dynamic text preview as you slide upwards or downwards" (the slider could be the arrow heads '<|>' reversed vertically).

> It might perhaps be too late now. No. Having been involved in the 1.0 definition document, standards support is going to be a big part of that. This includes decent font handling. I say you should go ahead and write this stuff. If it gets checked in on Windows only first, then that's 90% of Mozilla's users who have decent support. Also, after you finish the code, platform gurus can see what to do on their platform. Now Netscape have branched for their release, this is exactly the time for these changes. For goodness sake, please don't stop this excellent work! :-) Are you planning to fix bug 20394? Gerv

> the Font prefs could just have an innovative 'Adjust' value for each of the > generic fonts (as a replacement for the idea of individual font-sizes). They could, but I don't think they should. Users would not understand why, when they were able to specify basic sizes for Proportional and Monospace, they had to use sliders instead for the other families besides Monospace. If you really need a proportion to work with, how about taking the user's chosen size for the CSS family and dividing it by the user's chosen size for Proportional? And again, for Hixie's benefit, the reason we need to specify different sizes for different families rather than just tweaking based on x-height: Because There Is More To Legibility Than X-Height. As an author I should be able to just say {font-family: fantasy; font-size: medium}, and not worry about it not being legible. Therefore, sizes for CSS familes should be set in the UA such that {font-size: medium} is equally legible for each. Yes, x-height can affect legibility, but so can width, slope, use of ligatures, extent of swashes and flourishes, cursiveness, weight, closedness of hooks, shape of bowls ... It will be a decade or three before UAs are capable of factoring in all those attributes to harmonize sizes for each family automatically, and until then users will need to do it themselves with their eyes.

Gerv: "Are you planning to fix bug 20394?" That's a required link - it is targetted at m0.9.3 - but not sure if that means anything. mpt: "If you really need a proportion to work with, how about taking the user's chosen size for the CSS family and dividing it by the user's chosen size for Proportional?" That wouldn't work quite the same because when the scaling based on the x-height is made later, the new size may be different from the intended size -- in order to fully rely on just the font-size, one would need a non-spec compliant patch like the one sitting in bug 84398 :(

In addition to what I summarized in "Additional Comments From rbs@maths.uq.edu.au 2001-06-26 13:24", what I have do so far includes: - fixing all leaks in nsFontMetricsWin, it is 'all' because it is by systematic inspection: greping all places with 'new|calloc|malloc', and finding their corresponding delete/free. By adding comments on all the allocation lines that have their matching de-allocation lines, it became possible to pinpoint the culprits (i.e., the allocation lines which didn't get comments) and these could be fixed after a few iterations of this systematic process. It was like finding closing tags in an XML document. Fixing was possible because the font sub-system is pretty much self-contained, and the memory allocated there is also deleted there. - setting the callback mechanism as I described in attachment 40963 [details], and some cleanup here and there.

Got GetDimensions() hooked up [bug 20394], modulo cleanup and some issues that arise when slowly re-sizing the window. Here is an interesting problem that came up... Consider a string: abcd efgh ijkl xyz..., and assume that each 'letter' is a Unicode point coming from different fonts, each with different metrics. Then GetDimensions() measures the string (the width and the maxAscent and maxAscent of all fonts), using an adapted version of GetWidth() with break indices that Troy added in nsRenderingContext. But layout says the string doesn't fit, and wants to line-break at some point. What is the ascent and descent to use? How to efficiently get the ascent and descent at the breakpoint without re-measuring the string. Very interesting because I didn't think of this kind of problem until I got things running and saw how the unwanted ascent/descent could 'leak' pass the break point. Watch out on the patch (someday :-) for the work-around the problem.

So, this is what it takes to fix this bug! attachment 41752 [details] - patch in gfxwin: ==================================- * Provides the foundation for handling 'font-size-adjust' * Implements what I said in "Additional Comments From rbs@maths.uq.edu.au 2001-06-21 04:01" [with CreateAdjustedFont() now called CallFontHandle() to remind that not all fonts are actually adjusted]. Also did some major cleanup, e.g., by using nsVoidArray() instead of hand-rolled growing arrays, re-worked the determination of substrings that are representable in the same font. This led to a zappier code, and even allowed removing nsRenderingContextWinA which was a hack for a Japanense Win95 problem. I intensively tested these changes by forcibly setting 'useAFunctions = 1' in nsGfxFactoryWin.cpp. Fixed memory leaks, etc... There were also some changes of a typogrpahical nature, like renaming 'CharSet' to 'charset' because the latter is easier to read, or like prefixing globals variables with 'g'. * Added the back-end for the much wanted GetDimensions() that will allow layout to get font heights dynamically attachment 41753 [details] - patch in content: =================================== * enable 'font-size-adjust' in the Style System attachment 41756 [details] - patch in layout: =================================== * make layout understand GetDimensions() and use it to dynamically render text in the Unicode world (I18n) - rather than relying on the implicit assumption the font is an ASCII font with a constant height, the layout engine now gets the string heights dynamically. This fixes bug 20394 which was a prerequisite to supporting font-size-adjust with 'size-switching' inside the fonts. Performance impact: the overhead of GetDimensions() vs. GetWidth() consists in finding the maxAscent and maxDescent. For a text-run coming from a _single_ font, like an ASCII text, this is negliglible from looking at the code. Moroever, the optimization in ResolveForwords() should probably balance that and perhaps pay back a little bit, although I haven't done any tests re:performance measurements. Over to you guys...

These patches fix only the Windows or not?

The patches in content & layout are XP. As for the patch in gfxwin, it is possible to do a minimalist implemention of GetDimensions() in other platforms (never mind the mess of the patch). The version of GetDimensions() with break indices has superseded the former GetWidth() with break indices. This wasn't/isn't implemented on *nix. So no issues there. However, I would recommend switching to ResolveForwards() & ResolveBackwards() on all platforms. This gives so much air to the code, and the bulk from this template can be copy-pasted on all plaforms, with an update to the calls to platform-specific functions as appropriate. The actual handling of nsFont::sizeAdjust involves deeper changes.

But nsFont::sizeAdjust could remain a no-op for some time as well.

I have cut a branch for others to try things out and detect problems: cvs co -r FONT_20011307_BRANCH mozilla/client.{mak,mk} And proceeding as usual from there will grab what is in the branch. (The Unix version wont compile though.) The branch now has all the changes mentioned here plus the hooks for 'font-size-adjust' for each generic family, and the handling of the minimum font-size [these need dummy values in prefs.js -- see the code in nsPresContext::GetFontPreferences() for the possible ones]. BTW, the same code would allow to set different values of the font-size if that wasn't deemed non-spec compliant. What the code is doing is to set different values of 'font-size-adjust' now that this is supported. Since -moz-fixed is internal, I am using this ability of setting a different font-size for it. This reduced the size of nsStyleFont by half as I previously mentioned in bug 84398. My prefs.js used for experimentation had: user_pref("font.current.x-western.cursive.size-adjust", "0.55"); user_pref("font.current.x-western.min-size", 10); user_pref("font.default.x-western.-moz-fixed.name", "Courier New"); user_pref("font.default.x-western.-moz-variable.name", "Times New Roman"); user_pref("font.default.x-western.cursive.name", "Flemish Script BT"); user_pref("font.default.x-western.monospace.name", "Courier New"); user_pref("font.default.x-western.sans-serif.name", "Arial"); user_pref("font.default.x-western.serif.name", "Times New Roman");

Checkins in the branch so far to see the patches in a less messy format: http://bonsai.mozilla.org/cvsquery.cgi?module=allrepositories&branch=&dir=&file=&who=rbs%25maths.uq.edu.au&sortby=Date&hours=2&date=week

Some testing action is needed before the branch dries out (are you seeing all the checkins that are happening on the trunk?!) What I have been doing on the branch ever since has essentially consisted in fixing some long standing "XXX Write me" for the 'A' functions used for the workaround to the problem with Win95-Japanese.

CCd kmcclusk who, I believe, owns that kind of problems and might be able to review the patch on Windows and Unix.

Updated link for all checkins into the branch (unlike the previous link which expires after a week, this one will never expire since it has "date=all") http://bonsai.mozilla.org/cvsquery.cgi?module=allrepositories&branch=FONT_20011307_BRANCH&dir=&file=&who=&sortby=Date&hours=2&date=all

I have landed a minimalist port of GetDimensions() in GfxGTK (untested). Unix folks, please have have a go... The support of GetDimensions() in and of itself allows to handle the kind of problems that I have illustrated on the screenshot. It allows layout to use dynamic font metrics based on the _selected_ font rather than the fixed CSS metrics that comes from an ASCII font which may be irrelevant to the current text run (e.g., an i18n text). Then, it provides a cornerstone to subsequently support 'font-size-adjust' because 'font-size-adjust' involves dynamic size-switching within the fonts.

Milestone shift... Need interested folks to try out the branch that has the fix.

rbs: You may have to be more active in seeking testers - for example, by doing a build to be put up on ftp.mozilla.org. I, for one, don't have the bandwidth to pull and build the tree for testing. Gerv

Regression Tests: Block: Passed... Table: Passed except the following 2 mismatches (not counting the false positives) file:///s|/mozilla/layout/html/tests/table/bugs/bug19061-1.html Description: Style difference (bold vs non-bold). Reason: This doesn't appear to be related to my changes - looks like a regression on the trunk at the time I branched. file:///s|/mozilla/layout/html/tests/table/bugs/bug2886-2.html Description: Different renderings. Reason: There is a .css file associated to this testcase and it happens to have rules with |font-size-adjust|... This led to different renderings since is |font-size-adjust| is now supported. So it is not really a bug, but a feature. In addition, I had to tweak an 'if' clause a little bit to fix this one: file:///s|/mozilla/layout/html/tests/table/bugs/bug83786.html Description: The gap between the lines were larger than expected, and this caused lines not to fit vertically in the <div> element with a fixed height. Reason: The testcase has the following rule: div { border: green solid; width: 20em; height: 10em; font: 1em/1em monospace; /* i.e., font-size:1em and line-height:1em */ overflow: hidden; } I previously removed an 'if' test in nsLineLayout. The test was stopping the height of text frames from contributing in the determination of the height of the line. In nsLineLayout::VerticalAlignFrames(PerSpanData* psd), I had changed: - if (!pfd->GetFlag(PFD_ISTEXTFRAME)) { + if (PR_TRUE)) { The motivation of this change is that when there is <font-size-adjusted>an ASCII Text, a Hebrew Text, a Tiny Script</font-size-adjusted>, such wrapped lines just jam together due to the varying levels of zooming of |font-size-adjust| in bewteen fonts. However, the testcase passes if I don't remove the 'if', but I relax it a little bit as indicated below. What is happening now is that text frames contribute only if there is no specified line-height (the logicalheight flag in the code). I am giving details on this particular point because that's also where nsDimensions comes into play. If this 'if' is not relaxed in some way, then layout will also get the dynamic font heights with the newly added GetDimensions() and then throw them away when aligning frames... #if 0 if (!pfd->GetFlag(PFD_ISTEXTFRAME)) { #else if (!pfd->GetFlag(PFD_ISTEXTFRAME) || (pfd->GetFlag(PFD_ISNONWHITESPACETEXTFRAME) && !logicalHeight)) { #endif

That 'if' test proved not to be reliable enough and I had to tweak it even more using this time the actual information in the style context. The change now read: #if 0 if (!pfd->GetFlag(PFD_ISTEXTFRAME)) { #else // Only consider non empty text frames when there is no explicit line-height PRBool canUpdate = !pfd->GetFlag(PFD_ISTEXTFRAME); if (!canUpdate && pfd->GetFlag(PFD_ISNONWHITESPACETEXTFRAME)) { const nsStyleText* textStyle; frame->GetStyleData(eStyleStruct_Text, (const nsStyleStruct*&)textStyle); canUpdate = textStyle->mLineHeight.GetUnit() == eStyleUnit_Null || textStyle->mLineHeight.GetUnit() == eStyleUnit_Normal; } if (canUpdate) { #endif [...] } I will be attaching a screenshot that shows that 'font-size-adjust' is of dubious value if the ASCII metrics are to remain the sole yardstick by which to determine the line height. With my above changes the text metrics stashed in nsDimensions are considered unless there is an explicit 'line-height'. The change also passes the regression tests (not suprising since the regression tests are based on ASCII documents, and the values stashed in nsDimensions in these circumstances match the ASCII metrics). Notice that it would be unrealistic to ask authors to specify the 'line-height' in conjunction with 'font-size-adjust' because they cannot tell in advance which of the fonts in their 'font-family' list will be present in a particular configuration. So it is helpful to let the UA do a reasonable 'default'. For a first implementation in a browser, the rationale behind my above changes seemed okay to me. Upon doing my fair share of testings, I don't see much else to add in this initial round. So reviews are already welcome -- though I still greatly need some assistance with the other platforms. It is hard to imagine that all this was prompted by the need to set different font-sizes for different generic fonts... To recap and keep things in perspective: GetDimensions() was added to allow layout to use dynamic font metrics. Then, this allows to support 'font-size-adjust' because 'font-size-adjust' involves dynamic size-switching within the fonts. Then, setting different 'font-size-adjust' for each generic font produces the effect of different font-sizes in a spec compliant manner. (Note: the machinery in this code allows to easily activate the former non-spec approach with a few extra lines to read a hidden pref for example.)

Temporary experimental self-exatracting executable: ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font.exe (need the new expected font prefs keys!)

hixie, dbaron, got some comments about the weirdness of font-size-adjust vs. the default line height? You have being following me as I was facing the problem, is font-size-adjust something that is flawed by design, or am I missing something, or is my 'work-around' something that was expected and is acceptable per the CSS spec?

(CSS has no concept of a property not being set. All properties are 'set', even if just by virtue of having an initial value.) I don't really understand why your change causes the difference in the rendering. Could you explaing that a bit more? I understand what the three renderings are, and I think the third is what we want. Both the second and third are arguably valid interpretations of the spec. The way we should achieve the third, however, is by using the actual font-size (i.e. the font-size after adjustment by font-size-adjust; note that is not the font-size that would be inherited) for the calculation of 'em' units, 'ex' units, vertical-align and line-height percentages and line-height ratios, as well as for the calculations of the height of the line box. This is the opposite of what I said earlier, but in retrospect I think it is the only sensible implementation choice. (It also turns out that this is what I said in bug 41847 a few months ago, so maybe when I wrote the above I was not thinking straight or something...) http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/009.xml This, however, would not be fixed by your |if| statement change above, as far as I can tell and as far as I can see using your test build. Basically, behaviour should be nearly identical for 'line-height: normal' and 'line-height: 1.2' except that the ratio in the first case should be taken from the fonts used in the inline box: http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/003.xml normal http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/004.xml ratio http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/005.xml % http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/006.xml em http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/007.xml ex http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/008.xml px (control) BTW I found a bug in your implementation... The two cases in the URI given below should render identically but do not. It seems that font-size-adjust is affected by the value on its parent element (?). font-size-adjust should only affect the actual font-size, which will affect the cases listed above. http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/002.xml A second bug is that the font zoom (View | Font Size) does not seem to interact very well with font-size-adjust. The font zoom should be a factor applied to the font size calculation after everything else has been done, uniformly across all fonts. (And changing the font zoom should also affect the actual font size and thus the font size used for 'em', 'ex', '%', ratios and inline height calculations, although we only do this right for inline height calculations right now.) Hitting Ctrl++ and Ctrl+- a lot on a page with font-size-adjust being used shows this problem quite clearly; e.g. on: http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/001.xml And finally, is it known that the font prefs dialog does not seem to have an effect on the font picked? I'm getting Flemish Script BT being used for all my unstyled text now. Ever tried reading a directory listing in HandScript? :-/ dbaron: I would very much appreciate your comments on this bug regarding the issues I mention above! :-D rbs: Have I mentioned recently how much you rock?

>And finally, is it known that the font prefs dialog does not seem to have an >effect on the font picked? Let me digest your post a bit more... but for the above, the Font prefs dialog is out of sync with my changes (since the changes aim at allowing multiple fonts, etc, as per that other bug 61883. You might need to hand-edit your prefs as detailed in bug 30910). Did you try the min-size, my favorite...

Minimum size is sweet! Unfortunately, it interacts with the font-size-adjust code in an unfortunate way... As far as I can tell what you are doing is changing the computed value of font-size to ensure the minimum... this then means that if I have a font-size of 3px containing a font-size of 2em and my minimum font size is 10px, the actual font-size of the contained block will be 20px and not 10px (the minimum, raised from 6px) as it should be. For an extreme example of this, see: http://www.hixie.ch/tests/adhoc/css/fonts/size/minimum/001.xml ...which should just have the single green word "PASS" in your minimum font size or 10 pixels, whichever is biggest. As far as I can tell, this is how you are doing the maths at the moment: stylesheet cascade | \|/ specified value <-------------------------------------+ | | \|/ | minimum font-size calculation | | | \|/ | computed value ---------------------------------> inheritance | \ | \ | _\| | calculation of 'em', 'ex', '%', ratios \|/ | font zoom \|/ font-size-adjust inline box model | /|\ \|/ | actual value ----> font rasterising ----> inline box line height Here is how I think it should work (dbaron: please correct me!!!): stylesheet cascade | \|/ specified value <-------------------------------------+ | | \|/ | computed value ---------------------------------> inheritance | \|/ minimum font-size calculation | \|/ font-size-adjust | \|/ font zoom | \|/ actual value -------> calculation of 'em', 'ex', '%', ratios / \ | |/_ _\| \|/ font rasterising inline box ---------> inline box model line height Please feel free to tell me that makes no sense or is intelligible. :-)

The minimum size should not interfere with the calculation of the computed value. The question is: do we want to calculate the minimum size before font-size-adjust and font zoom, or just before getting the actual value? Opinions may differ here: when bug 30910 was filed, the request was that the minimum size should be calculated just before the actual value. Personally, I think there may be some value in letting Zoom reduce the font size without limit (to do some kind of thumbnails of web pages, for instance) so I would vote for: font-size-adjust --> minimum size ---> zoom ---> actual value

"there may be some value in letting Zoom reduce the font size without limit" I haven't yet got into reflecting on hixie's comments but that was exactly what I thought at the time of writing the code. If the minimum size affects the zoom, the user may have to disable it to get other niceties of zoom (for reduce-size printing for example). Since both prefs are controlled by the user, it might be best to pick the most convenient. I think the way things work at the moment is like: computed-value ---> minimum size ---> font-size-adjust & zoom ---> actual value where (zoom might be 1). font-size-adjust is acting on the final would-be actual value to achieve these visually identical x-height on disparate fonts. Also, the reason why I didn't do 'font-size-adjust --> minimum size' as pierre suggested is because I thought that it might be expensive to compute multiple x-heights during the already critical style resolution process. (Two x-heights are needed to settle the font-size corresponding to a font-size-adjust, and I am using very large trial font-sizes for improved accuracy -- although the metrics are cached). But even more critical is that it requires resolving the characters to see the actual fonts from where the glyphs will be taken at the rendering stage. That's why I thought that all this can be deferred until the last minute, and the adjustment is an 'add-on' where the other measuring operations are already been done.

pierre: minimum font size musn't be done after font-size-adjust, or else you'll end up with fonts with low ex:em ratios being blown way out of proportion, confusing the user and causing him to lower his minimum without need. rbs: agreed on principle, however this test shows that you are currently forcing the computed value up to the minimum value, as I described in my diagrams above: http://www.hixie.ch/tests/adhoc/css/fonts/size/minimum/002.xml The test passes on the trunk.

Hixie, just noted that your suggested ASCII art is what I am doing -- except that I was preserving the unadjusted CSS metrics (based on your previous comments). Need some time to update and see how your testcases go. Re: you latest testcase - I noted that it uses JS to get the font-size. But since I added a mSize field in nsStyleFont to distinguish which size is which during the CSS cascade, getComputedStyle needs to be updated so that it returns nStyleFont::mSize (the computed value). It is currently returning nsFont:size which the actual value intended for GFX. The trunk doesn't know about computed-value vs actual-value, that's why the test passes there.

re: http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/002.xml I could reproduce with min-size=0. The following rule that you have is taking precedence: .a { font-size-adjust: 1; } /* not used by anything in this test case */ when I change the value from '1' to '20' I can see the difference. I also noted that the problem goes away if I change: - .a span, .b, .b span, .c { font-size-adjust: 20; } + .a, .b, .b span, .c { font-size-adjust: 20; } Not yet sure what is going on. Looks like a rule problem higher-up in the Style System. re: http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/001.xml I could see the bug clearly when not setting the min-size. I have fixed the bug in my local tree. As you pointed out, the zoom should be an after-effect. re: http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/003.xml normal http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/004.xml ratio http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/005.xml % http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/006.xml em http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/007.xml ex http://www.hixie.ch/tests/adhoc/css/fonts/size-adjust/008.xml px (control) After changing the code to also get the CSS metrics from the adjusted size, all pass, except 'em'. That's because nsFont::size is used all over the place as an approximation of 'em'! People (me included of course :-) should really be using the GetEmHeight() API. (BTW, that's where bug 41847 comes from... where is the effect of the zoom if nsFont::size is being used?!) But note that I still need that 'if' for the cases where the CSS metrics differ from those used to render the string. The problem is this: in order to get 'em', 'ex', ..., GFX always searches for an ASCII font. For example, if you have <span style="font-family:Symbol">a symbol text</span>. GFX will hunt for the first ASCII font, forgetting your 'Symbol' font. If the font-family list includes a font that contains the ASCII set, that's the one used. If not, your symbol fonts are forgotten and the CSS metrics ('em', 'ex', ...) may be picked from any other font with the ASCII set. For less verbiage, I referred to this earlier by saying that GFX picks an ASCII font. Higher up, the CSS metrics are then used to get other computed-values, Then, comes the layout stage where the actual-values are needed, the glyphs are resolved and are now taken from 'Symbol'. Add to the sauce a few font-switching operations... And nsDimensions (the final combined 'font-box') is different from the idealized CSS rectangle where a plain ASCII text would fit. Layout insists to use that rectangle whereas with my 'if', I am letting the resolved nsDimensions interact as well, enabling therefore dynamic font heights (bug 20394). Because layout wasn't expecting text frames in that 'if', some weirdness occurs if not used with care w.r.t. whitespace, e.g., <br> uses generated content to simulate line-breaking, and we don't want the height of the '\A' used in <br>. That's the story behind that 'if' and my attempts at finding the good condition to enter there in. Maybe I am not using the right terminology. I am testing if the current value is line-height:normal (default UA), and if it happens that users have overridden with something else, they take full control, and their specified value of line-height takes precedence. But this means that there could be particular strings where line-height:normal and, for example, 'line-height:1.2' differ since the latter is a computed-value that may come from an unrelated ASCII font whereas the former is the actual nsDimensions font-box resulting from the (Unicode) string.

Attachment 42985 [details]: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=42985 is a vivid illustration of the problem I described above. The left image shows the expected rendering (that's the one obtained when using the computed nsDimensions through that 'if'). The right image shows the rendering on the trunk _without_ GetDimensions. As the image shows, the insufficient height that layout is allocating to the strings is in fact the height where ASCII characters would fit.

New drop -- overwritting the previous one: ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font.exe Of note (something easily forgotten): when the min-size is set, e.g., if min-size=10px, style sheets values like font-size:1px are filtered out early on while computing the CSS cascade. So if there is a font-size-adjust around, GFX will later act on the min-size. (For testcases related to pure compliance, the min-size should be set to 0 to avoid its interference.) The new drop include hixie's feedback and fixes the reported problems, except the 'em' testcase -- that one requires a jihad in the tree to fix bad users of nsFont:size (I would think such a bug needed a high priority because the sooner it is cleaned, the better. Who knows the dependencies that might have been built on such a bad bug). The rule problem was because I had not hooked the check for 'font-size-adjust' in nsRuleNode::CheckFontProperties(). I also fixed getComputedStyle() to make the JS testcase happy.

... also regression-tested again --that's how life is hard in the layout land :(

hixie, re: http://www.hixie.ch/tests/adhoc/css/fonts/size/minimum/001.xml I think this one is causing an arithmetic overflow. To get the test to pass with min-size=10px, I had to reduce the font-size: <style type="text/css"> div { font-family: Verdana; } .a { font-size: 0.01px; height: 0.5em; background: red; } - .a span { font-size: 1000em; color: green; } + .a span { font-size: 10em; color: green; } </style> In accordance with the graphs shown ealier, the algo takes 'font-size: 0.01px' from the CSS computation and converts it to the nsFont expected by GFX to get the current font metrics. During this process 'font-size: 0.01px' is converted to 'nsFont:size = min-size' so that the 'em', 'ex', ..., that GFX returns are relative to the min-size. Then, when the next rule comes it, the 1000em value is blowed out of proportion. This is okay as per the algo agreed upon.

> we don't want the height of the '\A' used in <br>. In standard mode, we do. > when the min-size is set, e.g., if min-size=10px, style sheets values like > font-size:1px are filtered out early on while computing the CSS cascade. That sounds wrong. Minimum size stuff should be done just before font-size- adjust, WAY after the cascade. > re http://www.hixie.ch/tests/adhoc/css/fonts/size/minimum/001.xml I guess that test case is actually invalid then. :-) My bad! Please remind me to test your build at some point next week, I'm supposed to be testing the IMGLIB1-REMOVAL stuff first... thanks.

> > we don't want the height of the '\A' used in <br>. > In standard mode, we do. My patch wouldn't break it then. Since the 'if' test used to exclude all text frames. Now, some text frames are let in. But I will let see how your tests go. >That sounds wrong. Minimum size stuff should be done just before font-size- >adjust, WAY after the cascade. That's what is happening (my explanation was not complete -- refer to your ASCII art for what is going on). The actual value comes after the computed value, i.e., after the CSS cascade. However, actual values can impact computed values because units ('em', 'ex', ...) are fed back in the CSS cascade, and units are obtained from actual values.

My bad, when you said "computing the CSS cascade" I thought you meant the actual cascade bit (working out what rules match) and not the computation of the values bit (filling in the structs). Sounds good. I'll download your test build and try it out.

I have brought my changes in sync with the trunk. I will attach the complete set of up-to-date patches that can be applied. I am ready for r/sr to secure clearance on this part since the other platforms are beyond my reach and would better be handled by the respective platform gurus. Getting s/sr on what I did might motivate them. bstell is already hooking up GetDimensions() in bug 95721. Here is a brief summary of what I did: 1. [Feature] Added support for multiple fallback fonts with factory pre-built defaults as discussed in bug 61883 -> non-GfxWin status: pending (the layout part is XP). However, the new font prefs keys constitute a superset of the current ones so that other platforms can continue without this and support it later. 2. [I18N Consolidation] Brought the support of the 'A' functions used in Win95-J in parity with the other functionalities provided in the font subsystem (notably, the support of the substitute fallback font for missing glyphs and the handling of buffer overrun when converting a long Unicode string to the ANSI code page of each font subset). Also eliminated nsRenderingContextWinA with a re-work. -> Specific to GfxWin. No action needed on other platforms. 3. [Code optimization] Switched to ResolveForwards() and ResolveBackwards() -> non-GfxWin status: pending, but only ResolveForwards() is needed -- since ResolveBackwards() is for BiDi. This is not essential for a start. Other platforms can operate without this and support it later. 4. [Serious bug / Feature] Added support for GetDimensions() which is needed to allow layout to support dynamic font heights - the long standing bug 20304 - the layout part is XP. -> non-GfxWin: initiated only in GTK - bug 95721. This is _the_ BLOCKER because all zillions versions of GFX are required to support it. Now that the dreaded layout part is complete, I am hoping that platform gurus can chime in... Your help will be much appreciated. See the template in bug 95721 for an example of how it is being hooked in GfxGTK. 5. [Accessibility / Feature] Added support of a font's min-size -> Entirely handled in the Style System while leaving the chrome alone and honoring the CSS notion of "actual value" vs. "computed value" 6. [CSS2 / Feature] Added support for |font-size-adjust| -> non-GfxWin status: pending. This is not essential for a start, but will be desirable for platform parity. Other platforms can operate without this and support it later. 7. [Outcome] Fix for this bug and related friends. For background on the whole story, please read the comments in this bug (and the other bugs too). Reminder: the minimalist thing needed to hook my patch on other platforms is to hook GetDimensions(). On these platforms, other extra things (like |font-size-adjust|) would result in a no-op for the moment, although XP parts (like the font's min-size) already benefit all platforms.

...[8] also notable is the systematic fix for memory leaks as described in "Additional Comments From rbs@maths.uq.edu.au 2001-07-02 16:54".

I took a quick look at the changes -- hopefully I'll get a chance to look more later. A few comments: I'm concerned that the changes to nsLineLayout will break any line heights less that 1 -- or perhaps (depending on how things are defined) any less than 'normal'. Why does nsIRuleNode::GetBits need to be added to the interface? Why should it be public? Why should it be virtual?

re: nsLineLayout Actually, it won't break the case that you refer to. Any value (whether smaller or larger) than 'lineheight: normal' -- the default value of the UA, takes precedence. My understanding is that 'lineheight: normal' amounts to relinquishing the decision to the UA to do what is deemed appropriate depending on the situation at hand. For fonts with widely different metrics, the computed nsDimensions (the resulting combined 'font-box') does the appropriate job as the screenshot shows. In the case where the piece of text is entirely within an ASCII font (or if all the fonts involved during glyph fill-in have the same height), nsDimensions and the idealized box coincide, hence authors won't notice a difference. (But in any case, authors can get specific renderings by superseding the default 'lineheight: normal' of the UA.) re: GetBits This is for optimization similar to that in WalkRuleTree. Given a style context, I wanted to get the CSS rules from where the resolved font data came from. The whole set of rules associated to a style context are obtained by walking from its seed node up to the root of the rule tree. However, whenever a path (from a given node to the root) has no rule relevant to a certain CSS property, a bit is set in the node to say so. Hence when looking for rules that affect font data, if it is possible to query the state of the font bit, one could quickly bail out if it turns out that the remaining path has no rule that will affect font data. For example, if the rest of rules is only about 'color' and such, there is no point trying to update font data since it will result in a no-op. It is public because it is called from the static SetGenericFont() which isn't part of the nsIRuleNode API. It is virtual because I am just using NS_IMETHOD:-)

nsFontMetricsGTK.cpp: I don't think we should be using XFontStruct::max_bounds.ascent and XFontStruct::max_bounds.descent at all, since they are generally wrong. See bug 91794. nsRuleNode.cpp: The change to CheckFontProperties may not be sufficient - is font-size-adjust part of what's set by setting a system font? (I remember discussing this in the past with the CSS WG, I think, so the spec may not have the right answer -- I should double-check this.)

re: nsFontMetricsGTK.cpp + mMaxAscent = xFont->max_bounds.ascent; + mMaxDescent = xFont->max_bounds.descent; For consistency, I will update as appropriate with what ends up in nsFontMetricsGTK.cpp. re: nsRuleNode.cpp: Had another look and saw that system fonts can only be set with the short hand "font: caption | icon | ..." and the spec is also saying that "for reasons of backwards compatibility, it is not possible to set 'font-stretch' and 'font-size-adjust' to other than their initial values using the 'font' shorthand property", so that would imply that it isn't part of what is settable by a system font. <spec @ http://www.w3.org/TR/REC-CSS2/fonts.html#font-shorthand http://www.w3.org/TR/REC-CSS2/fonts.html#propdef-font-size-adjust > 15.2.5 Shorthand font property: the 'font' property 'font' Value: [ [ <'font-style'> || <'font-variant'> || <'font-weight'> ]? <'font-size'> [ / <'line-height'> ]? <'font-family'> ] | caption | icon | menu | message-box | small-caption | status-bar | inherit Initial: see individual properties Applies to: all elements Inherited: yes Percentages: allowed on 'font-size' and 'line-height' Media: visual The 'font' property is, except as described below, a shorthand property for setting 'font-style', 'font-variant', 'font-weight', 'font-size', 'line-height', and 'font-family', at the same place in the style sheet. The syntax of this property is based on a traditional typographical shorthand notation to set multiple properties related to fonts. All font-related properties are first reset to their initial values, including those listed in the preceding paragraph plus 'font-stretch' and 'font-size-adjust'. Then, those properties that are given explicit values in the 'font' shorthand are set to those values. For a definition of allowed and initial values, see the previously defined properties. For reasons of backwards compatibility, it is not possible to set 'font-stretch' and 'font-size-adjust' to other than their initial values using the 'font' shorthand property; instead, set the individual properties. </spec>

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

I just filed bug 96609 Tracker - add GetDimensions() in GFX

I applied the patches to a current tree on my Redhat 6.2 system and got this error: nsDeviceContextPS.cpp: In method `nsresult nsDeviceContextPS::CreateRenderingContext(nsIRenderingContext *&)': nsDeviceContextPS.cpp:108: cannot allocate an object of type `nsRenderingContextPS' nsDeviceContextPS.cpp:108: since the following virtual functions are abstract: ../../../dist/include/nsIRenderingContext.h:579: nsresult nsIRenderingContext::GetDimensions(const char *, unsigned int, nsDimensions &) ../../../dist/include/nsIRenderingContext.h:581: nsresult nsIRenderingContext::GetDimensions(const PRUnichar *, unsigned int, nsDimensions &, PRInt32 * = 0) ../../../dist/include/nsIRenderingContext.h:657: nsresult nsIRenderingContext::DrawString2(const char *, unsigned int, int, int, const nscoord * = 0) ../../../dist/include/nsIRenderingContext.h:677: nsresult nsIRenderingContext::DrawString2(const PRUnichar *, unsigned int, int, int, int = -1, const nscoord * = 0)

I am patching GfxPS (which is built when GfxGTK is built - I had forgotten about that...) I will attach a fix-up patch shortly.

BTW, the minimum is a depend build from mozilla/ since the patch alters core layout & content APIs.

The fix-up patches are not getting the individual font heights during font-switching. In other words, their rendering is going to be the _same_ as with the current trunk (no regression, but no dynamic font heights either). Since I hope to be done with this bug early in m0.9.5, I am afraid that's all I can offer on these platforms. I hope that platform gurus can step in at some stage and provide the right fix for parity with other platforms. Status: win32 - tested gtk - bstell? xlib - gisburn? (i.e, Roland.Mainz@informatik.med.uni-giessen.de -- in full!) mac - anyone?

I'm confused by the nsRenderingContextGTK::DrawString function in nsRenderingContextXlib.cpp in attachment 46961 [details]: 8732 class nsFontMetricsXlib : public nsIFontMetrics 8733 Index: src/xlib/ 8734 =================================================================== 8735 RCS file: /cvsroot/mozilla/gfx/src/xlib/nsRenderingContextXlib.cpp,v 8736 retrieving revision 1.59 8737 diff -u -r1.59 nsRenderingContextXlib.cpp 8738 --- nsRenderingContextXlib.cpp 2001/08/15 02:48:54 1.59 8739 +++ nsRenderingContextXlib.cpp 2001/08/23 23:04:50 ... 8818 +nsRenderingContextXlib::DrawString2(const char *aString, PRUint32 aLength, 8819 nscoord aX, nscoord aY, 8820 const nscoord* aSpacing) 8821 { 8822 @@ -1329,12 +1402,6 @@ 8823 8824 nscoord x = aX; 8825 nscoord y = aY; 8826 - 8827 - // Substract xFontStruct ascent since drawing specifies baseline 8828 - if (mFontMetrics) { 8829 - mFontMetrics->GetMaxAscent(y); 8830 - y += aY; 8831 - } 8832 8833 UpdateGC(); 8834 8835 @@ -1420,23 +1487,39 @@ 8836 } 8837 8838 NS_IMETHODIMP 8839 +nsRenderingContextGTK::DrawString(const char *aString, PRUint32 aLength, 8840 + nscoord aX, nscoord aY, 8841 + const nscoord* aSpacing) 8842 +{ 8843 + nscoord y; 8844 + mFontMetrics->GetMaxAscent(y); 8845 + return DrawString2(aString, aLength, aX, aY + y, aSpacing); 8846 +} 8847 + 8848 +NS_IMETHODIMP 8849 nsRenderingContextXlib::DrawString(const PRUnichar *aString, PRUint32 aLength, 8850 nscoord aX, nscoord aY,

Could we get a short discussion on why the patch has DrawString and DrawString2?

Unlike DrawString(), the DrawString2() methods are drawing a string without tampering with the aY parameter supplied by the caller. They are aimed at allowing to draw multiple string fragments with mixed fonts in separate calls, yet while still keeping them baseline-aligned. Somebody higher up is using them. However, in order not to break existing call sites of DrawString(), it is simply reformulated in terms of DrawString2(aX, aY + ASCIIFont.ascent). I am envisaging eliminating these DrawString() and renaming DrawString2() to DrawString() once the switch to GetDimensions() is complete and all callers migrated. Did your build complete successfully? If you are not getting the correct rendering, then there might a typo. Algorithmically, the needed functionality is what I described above. Since this bug is getting very long, let's discuss further issues specific to porting in bug 95721 or bug 96609 which are aimed at specifically dealing with porting issues arising from GetDimensions().

rbs: Could you explain how the "default font size per generic font family" and the 'font-size-adjust' code will interact?

If you look at the patch in layout, you will see that the presentation context now holds several default nsFonts. It used to hold only two nsFonts: one for the default "proportional" (variable) font, and the other for "-moz-fixed". Currently, only the font-size is different in the initialization of these nsFonts but technically it is possible to initialize with different attributes (e.g., the variable font could be initialized as a bold font...). All these nsFonts have |font-size-adjust| = 0, and this means no adjustment by default. Setting a |font-size-adjust| other than zero in one of these default nsFonts amounts to overriding the initial 0 (in a similar way as one could choose to initialize the variable font as a bold font). Then, starting from the presentation context, there is a lazy CSS cascade as I explained in bug 84398 using the analogy with "threads". The final nsStyleFont that ends up in an element is the result of the usual CSS cascade -- just that the starting values from the presentation context may be different depending on whether an element is in the "monospace thread" or the "cursive thread" for example. So there is no special casing of |font-size-adjust| in my lazy code as far as the CSS machinery is concerned. Other attributes can be given different initializations and the code would cater for them too.

New drop to exercise the code, in sync with the tip -- overwriting the last one: ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font.exe

> I am envisaging eliminating these DrawString() and renaming DrawString2() > to DrawString() once the switch to GetDimensions() is complete and all > callers migrated. I see from your comments that 2 or more checkins are planned. When I have had a large change I sometimes have decided to break the patches into 2 or more parts to make the review and checkin process easier. The patches here are quite long. A 'wc' on attachments 46957, 46957, and 46957 gives ~2500 lines (which of course is somewhat bloated by 'diff -u').

Yes, converting callers will help to avoid to unnecessarily clutter the API with multiple functions only used in specific places. (In fact, apart from the usual potential fix-ups that may ensue, that's the main follow-up tree-wide cleanup patch that I see at the moment.) OK, will break the patches into - layout - content - gfxXXX

I hope I did not miscommunicate here. I did not mean to imply that long patches were necessarily bad. Actually, long patches often mean that the developer is attacking a large (and probably long neglected) problem. I just find that sometimes it is hard to "herd all the cats" together. As such, I sometimes solve larger problems by breaking up the changes in to more review'able/checkin'able sizes. Then I handle several pieces serially. It is often a fair bit more work for me but (I think) it makes the reviewer's and super-reviewer's jobs easier which I often find to be a major bottleneck on big changes.

Since the tree is ever changing and the number of patches attached to the bug is getting large (there could be more patches than comments if it goes on like this :-), I have put the whole set of changes in sync with the current tree at: ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-all-wu.txt -> For review (the path was obtained with diff -wu from mozilla/) ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-all-w.txt -> For application from mozilla/ by those interested. Simply run "patch -p0 < bug74186-diff-all-w.txt" from mozilla/ to apply all of the changes on a tip of a tree as of today, and with a depend build, you are all set. Since I have been carrying this bug for quite some time now, I did run the block and table regression tests several times, and have recently stepped through my changes in the debugger and refine them (as well as fixing other subtle preceding bugs that I discovered in the area). Thus I have got a higher degree of confidence in my changes now and do not have further pending algorithmic issues to resolve. Consequently, I am tentatively "finger-pointing" the following people for review/super-review: * changes in content/ attinasi, dbaron, hyatt, pierre. * changes in layout/nsTextFrame and gfxwin/nsRenderingContext::GetDimensions() attinasi, dbaron, waterson. [This is the crux of the measuring code. Sorry guys, the measuring code with break indices is inherently cryptic :-) I have added helpful comments to detail what is happening. I can provide more details if needed.] * changes in gfxwin/nsFontMetrics ftang, brendan, kevin (kmcclusk). * changes in gfxmac ftang, pierre. [Testing is still pending here, but note that only GetDimensions() is being hooked up as per bug 96609. Other changes are entirely XP and covered elsewhere.] * changes in gfxgtk (and other ports) blizzard, bstell, ftang [Testing is still pending here, but note that only GetDimensions() is being hooked up as per bug 96609. Other changes are entirely XP and covered elsewhere.] Of course, this is just a subdivision to balance the workload. Feel free to r/sr any area you read and deem OK on the way. I am collecting the first r/sr that come along, thanks!

New drop to further exercise the code, the last one is gone... ftp://ftp.maths.uq.edu.au/pub/rbs/mozilla-win32-font-20010904.exe Testers always welcome. Should I recall that you don't miss anything?! The drop is in sync with the tip as of now...

re: line-height, looking again at the spec http://www.w3.org/TR/REC-CSS2/visudet.html#propdef-line-height I gather that when line-height = normal, it is okay to use nsDimensions with font-switching of varying metrics to get a "reasonable" value that achieves a better legibility. The spec says: <spec> normal Tells user agents to set the computed value to a "reasonable" value based on the font size of the element. </spec> IMO, had that not be possible, it would have probably deserved an errata.

Now that I have reviewed attachment 47450 [details] I want to meet with the i18n department to consider the i18n implications. In the meantime let me address some simpler items. It is my policy not to *add* tabs (At least in the non-Mac code). Would the name "IsGeneric" be more readable than "GenericIs"? I am unclear on the intent of "sizeAdjust". I did not see where this default constructor was needed: +nsFont::nsFont() +{ +} + I'm curious why we do not remove the "ifdef MOZ_MATHML" from GetDimensions and make it a standard build item. If we did this could we then remove GetWidth? While getting the actual text height will help a lot I believe that this code in nsFontMetricsGTK.cpp will still be needed for other reasons. Even it we felt it was not needed after we get the text height I would still strongly recommend we leave it in and change the prefs to lower/zero the minimum. Once that is out in the field for a while we could consider removing it. - if (mLangGroup) { - nsCAutoString name("font.min-size."); - if (mGeneric->Equals("monospace")) { - name.Append("fixed"); - } - else { - name.Append("variable"); - } - name.Append(char('.')); - const PRUnichar* langGroup = nsnull; - mLangGroup->GetUnicode(&langGroup); - name.AppendWithConversion(langGroup); - PRInt32 minimum = 0; - res = gPref->GetIntPref(name.get(), &minimum); - if (NS_FAILED(res)) { - gPref->GetDefaultIntPref(name.get(), &minimum); - } - if (minimum < 0) { - minimum = 0; - } - if (mPixelSize < minimum) { - mPixelSize = minimum; - } - } - Ditto for the similar code in nsFontMetricsXlib.cpp Ditto for this code in nsFontMetricsMac.cpp +#if 0 if (textSize < 9 && !nsDeviceContextMac::DisplayVerySmallFonts()) textSize = 9; +#endif The ascent/descent in the first font is certainly *not* the *max* ascent/descent. + + mMaxAscent = xFont->ascent; + mMaxDescent = xFont->descent; +

rbs: I've proposed that we explicitly list 'normal' as an allowable computed value, which would mean that it could be interpreted in a font-dependent way. So yes, this seems like a good idea, and it will hopefully be explicitly allowed by CSS3 (although one *could* argue that it isn't permitted by CSS2).

From my (non exhaustive) reading of this bug and attachment 47450 [details] there seem to be several distinct problems being solved: Get string height in addition to string width since glyph fill-in can change the actual ascent and descent. Allow user to to specify a "adjust-size" in addition to a pixel height since some fonts do not appear to be the specified pixel height. Change the DrawString() API to pass x,y at the left side base line instead of the left side top side. Is this correct?

Correct. Reading your summary, the problems may look distinct but they really fit together as per the history of this bug. Since font heights can now change dynamically, DrawString2() is a vital cornerstone of the framework because without it the text would have to be measured twice in order to keep all the pieces baseline-aligned. As for the change induced by the sizeAdjust stuff: the issue is that during font-switching, sizeAdjust can itself entail different ascent and descent of the physical fonts involved in unpredictable ways. So at the end, one also need to pass back the final resulting ascent to allow the rendering code to keep all the pieces baseline-aligned. re: your ealier comments >It is my policy not to *add* tabs (At least in the non-Mac code). Any tab is an oversight as I don't like tabs myself. In fact, it is against mozilla.org's coding styles to use tabs. Kindly point any tabs (that I added against local customs :-) and I will clean them up. > Would the name "IsGeneric" be more readable than "GenericIs"? The second conveys the intention: what generic font is this? To which one might reply: serif | sans-serif, .... As opposed to "is this a generic font?" To which one might reply: true | false. > I am unclear on the intent of "sizeAdjust". Hope it is now clear after your reading of the bug. > I did not see where this default constructor was needed: In the Style System. > I'm curious why we do not remove the "ifdef MOZ_MATHML" from > GetDimensions and make it a standard build item. The patch may be confusing. It is not in #ifdef. It is part of the default build. > If we did this could we then remove GetWidth? Maybe a later item to be looked at. > While getting the actual text height will help a lot I believe > that this code in nsFontMetricsGTK.cpp will still be needed for > other reasons. As I described in bug 30910, these local handling of the min-size are not necessary anymore. They have some weaknesses. Let my patch have a chance. It is not difficult to bring back these chunks of code :-) >The ascent/descent in the first font is certainly *not* the *max* >ascent/descent. I simply used a naming scheme that followed the local custom.

> Reading your summary, the problems may look distinct but > they really fit together as per the history of this bug. I'm glad to see these items being addressed. I do however find that having all of these solved in one patch makes it much harder to understand exactly what each change / new subroutine is actually doing. Not understanding exactly what each part does makes me very reluctant to add a "r=". With some many things happening in one patch it makes the discussion in the bug report very long. With so many things changing at once it will be much harder when bugs come for QA to isolate exactly what effected what since they all got checked in on one day. (Please note that I do want this stuff to happen.) > Since font heights can now change dynamically, DrawString2() > is a vital cornerstone of the framework because without it the > text would have to be measured twice in order to keep all the > pieces baseline-aligned. Changing GetWidth to return Ascent/Descent/Width (rename) and changing DrawString's Y parameter from the top to the text base line is good. I am very happy to see the layout get the height of each text segment. This has been a lingering problem that seriously affect i18n. I spent much of yesterday working with yokoyama, bobj, and momoi making sure that this would not impact future forms of i18n layout such as vertical text, Urudu text (glyphs move vertically intra-word), Ruby (a Japanese variant). > As for the change induced by the sizeAdjust stuff: the > issue is that during font-switching, sizeAdjust can itself entail different > ascent and descent of the physical fonts involved in unpredictable ways. So at > the end, one also need to pass back the final resulting ascent to allow the > rendering code to keep all the pieces baseline-aligned. I am unclear on whether the sizeAdjust should be per-element or per page. The example I saw earlier (2001-06-26 20:29) makes me uncomfortable in that it seems to imply this would setting would be repeated every time a font was specified; not once per document. I will however note that I am not an expert in this area. <!-- adjustment without border --> <p style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> > > re: your ealier comments > > >It is my policy not to *add* tabs (At least in the non-Mac code). > Any tab is an oversight as I don't like tabs myself. In fact, it is against > mozilla.org's coding styles to use tabs. Kindly point any tabs (that I added > against local customs :-) and I will clean them up. Kindly search your patches for tabs in the lines you are changing. > > Would the name "IsGeneric" be more readable than "GenericIs"? > The second conveys the intention: what generic font is this? To which > one might reply: serif | sans-serif, .... As opposed to "is this a generic > font?" To which one might reply: true | false. Okay, then how about "GetGeneric" or "GetGenericID"? > > I'm curious why we do not remove the "ifdef MOZ_MATHML" from > > GetDimensions and make it a standard build item. > The patch may be confusing. It is not in #ifdef. It is part of the default > build. I guess I would be repeating myself to say that making a patch that just removed the ifdef would be a much easier to review and would if checked in a few days apart from the other changes would allow QA to tell if a bug was related to that or the other parts of this patch. > > While getting the actual text height will help a lot I believe > > that this code in nsFontMetricsGTK.cpp will still be needed for > > other reasons. > As I described in bug 30910, these local handling of the min-size are not > necessary anymore. They have some weaknesses. Let my patch have a chance. > It is not difficult to bring back these chunks of code :-) Last time I said it as softly as I could so let me be a little more firm this time: Please do not remove that code. It is used to solve other i18n problems. I also strongly recommend you get nhotta or ftang to review the change in nsFontMetricsMac.cpp before you check it in. > > >The ascent/descent in the first font is certainly *not* the *max* > >ascent/descent. > I simply used a naming scheme that followed the local custom. The current name conveys something that *cannot* be done so how about we change it's name to something like GetPrimaryFontAscent() ? Does this function provide an optimization: +NS_IMETHODIMP +nsRenderingContextGTK::GetDimensions(const PRUnichar* aString, + PRUint32 aLength, + nsDimensions& aDimensions, + PRInt32* aFontID) If so I would strongly recommend that since this checkin has several API and function changes this optimization be handled in a separate bug, patch, and checkin. Since DrawString2() is a bridge function to allow the change to be staged in can we open a bug to remove it after the changes are in?

> With some many things happening in one patch it makes the > discussion in the bug report very long. > > With so many things changing at once it will be much harder > when bugs come for QA to isolate exactly what effected what > since they all got checked in on one day. If all the problems are connected, fixing them piecemeal makes no sense. Hyatt's style system whackage wasn't checked in incrementally, because it was all interconnected. In the entirety of Bugzilla, rbs is the best I've seen at explaining exactly how his code works. If you need to get on the phone with him to talk about the patch while you are reviewing it, I'm sure Netscape will foot the bill; we dial Ben Goodger into meetings all the time. Gerv

>I am very happy to see the layout get the height of each text >segment. This has been a lingering problem that seriously affect >i18n. Great. But this doesn't happen with one-liner patches. And there are headache-functions to deal with without breaking anything (the measuring code is a chief example -- the code with break indices is a challenge in itself. The line-height code was another. For the record, it wasn't that simple to figure out that |if| problem in the mass of code out there and come-up with that one-liner fix in nsLineLayout). And there is the guilt of adding an |mAscent| to nsTextFrame. And what about the blockage due to changes on platforms which I have no access to. etc, etc. If I was looking for excuses to give-up like other attempts in the past, I will have no troubles finding them :-) >I spent much of yesterday working with yokoyama, bobj, and momoi >making sure that this would not impact future forms of i18n layout >such as vertical text, Urudu text (glyphs move vertically >intra-word), Ruby (a Japanese variant). Glad to see that all is clear in that corner. >I am unclear on whether the sizeAdjust should be per-element or per >page. It is inherited per-element like other CSS properties. The spec for it is at: http://www.w3.org/TR/REC-CSS2/fonts.html#propdef-font-size-adjust If you have <span style="font-family:Verdana; font-size:16px; font-size-adjust:0.55"> a Verdana text... a Hebrew text.... </span> then, sizeAdjust has to infiltrate everything during font-switching. >Last time I said it as softly as I could so let me be a little more firm >this time: Please do not remove that code. It is used to solve other i18n >problems. Based on my understanding of the whole picture, I still maintain that these local hacks are not necessary anymore. Without you testing my patch, I find it hard to accept these hacks. Please, after testing, let me know what I am missing. >I also strongly recommend you get nhotta or ftang to review the >change in nsFontMetricsMac.cpp before you check it in. I have already requested ftang to have a look. Other people eyes are welcome on all of the changes, BTW. >The current name conveys something that *cannot* be done so how about >we change it's name to something like GetPrimaryFontAscent() ? I think there is a confusion here. It is not only the first font. I am considering the ascent/descent of all of the nsFontGTKs involved during font-switching. Perhaps, I should just chop 'Max' and call them ascent/descent? >Does this function provide an optimization: > > +NS_IMETHODIMP > +nsRenderingContextGTK::GetDimensions(const PRUnichar* aString, > + PRUint32 aLength, > + nsDimensions& aDimensions, > + PRInt32* aFontID) The layout code uses this to measure successive text runs -- to determine the amount of text that fits in a nsTextFrame and the final combined height of that nsTextFrame. >Since DrawString2() is a bridge function to allow the change to be >staged in can we open a bug to remove it after the changes are in? Yes, as noted earlier, there is a follow-up clean-up patch to fix-up callers, and remove the old DrawString(), and rename the new DrawString2() back to DrawString(). As for GetWith(), it is still needed because the drawing code uses it to advance the 'x' offset during font-switching.

> If all the problems are connected, fixing them piecemeal > makes no sense. I am not asking for piecemeal fixes. I am observing that mixing many different items in one bug report makes it very long and makes the review process slower. I note that this bug is already over 2400 lines long. Also the title Unable to choose different size for serif and sans-serif fonts seems unrelated to dynamic text height measurement. Even if the problems seems interconnected the solution need not be done in a single checkin. Build the foundation, build the first floor, build the second floor, ... Building in stages is quite different from piecemeal fixes. > >Last time I said it as softly as I could so let me be a > >little more firm this time: Please do not remove that code. > >It is used to solve other i18n problems. > > Based on my understanding of the whole picture, I still > maintain that these local hacks are not necessary anymore. > Without you testing my patch, I find it hard to accept these > hacks. Please, after testing, let me know what I am > missing. My last discussion (about a month ago) of this being used was to control the size differences between mixed Japanese and English text due to fill in glyphs being from different fonts. See bug 89520. I have made similar changes in the past where the effects did not show up in testing but users did eventually report in problems. Let me state my "review" of this part formally now: I explicitly disapprove of taking out the min-size code in nsFontMetricsGTK.cpp. I strongly caution against to disabling the similar code in the mac unless it is approved by ftang or nhotta. If you have a strong need to continue to address this then please open a bug. After the other part of this code is in we can consider disabling that code by setting the pref to zero and after that has some soak time with users with no reported problems we can then remove the code. > >The current name conveys something that *cannot* be done so > >how about we change it's name to something like > >GetPrimaryFontAscent() ? > > I think there is a confusion here. It is not only the first > font. I am considering the ascent/descent of all of the > nsFontGTKs involved during font-switching. Perhaps, I should > just chop 'Max' and call them ascent/descent? To get the ascent one needs to consider all the fonts used for glyph fill-in. Because we lazily build the list of fill-in fonts as we measure/render this information changes over time and hence does not have a constant value. What can be known is the ascent/descent of a particular run of text. Since it is useful but not accurate perhaps we should call it GetMaxAscentHint(). > Yes, as noted earlier, there is a follow-up clean-up patch to > fix-up callers, Could you remind me what the number is and if I am cc:'d on that bug?

>If you have a strong need to continue to address this It is one of my favorites :-) Guess what, I am simply going to rename my key to font.minimum-size.[generic]. This way, the other key, font.min-size.[generic], will continue to be associated to these hacks and you can keep them for the moment. >Since it is useful but not accurate perhaps we should call it >GetMaxAscentHint(). It is just now that I am seeing that you seem to be referring to ***nsIFontMetrics::***GetMaxAscent(), right? That's not what I am referring to. I didn't change nsIFontMetrics::GetMax[Ascent/Descent]() which continue to be the ascent and descent of the ASCII font, like GetXHeight(), GetEmHeight(), etc, are still liaised to the ASCII font. GetDimensions() needs and uses the ascent and descent of the native fonts as they gradually become known during glyph resolution. (There are no nsFontGTK::GetMax[Ascent/Descent]() functions). >Could you remind me what the number is and if I am cc:'d on that bug? There is no bug number yet because there is nothing to be changed in the trunk. I want to be done with these changes first.

> I am simply going to rename my key to font.minimum-size.[generic]. This way, > the other key, font.min-size.[generic], will continue to be associated to > these hacks and you can keep them for the moment. thanks > It is just now that I am seeing that you seem to be referring to > ***nsIFontMetrics::***GetMaxAscent(), right? How/where is GetMaxAscent() defined? Will it be used across multiple text segments?

>How/where is GetMaxAscent() defined? When the font associated to an <element>...</element> is realized, GFX hunts for the first ASCII (Western) font, and it is that ASCII font that is used to get the CSS metrics ('em', 'ex', ...), as well as the values that are stashed away and subsequently returned in GetMaxAscent/Descent, etc. That part of the code hasn't changed: NS_IMETHODIMP nsFontMetricsGTK::GetMaxAscent(nscoord &aAscent) { aAscent = mMaxAscent; return NS_OK; } NS_IMETHODIMP nsFontMetricsGTK::GetMaxDescent(nscoord &aDescent) { aDescent = mMaxDescent; return NS_OK; } >Will it be used across multiple text segments? No, the height that is now used is computed by GetDimensions(). If it happens that all segments are entirely ASCII text, then the resulting ascent/descent will actually match the ASCII GetMaxAscent/Descent. However, something else can result in the case of intl text involving intl fonts with different metrics.

For reviewers and those interested in trying things out, I have put whole diffs that are in sync with the tree as of now at: ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-for-review.txt ftp://ftp.maths.uq.edu.au/pub/rbs/bug74186-diff-for-apply.txt

re: attachment 48545 [details] [diff] [review] I r=bstell@netscape.com (approve) the code in nsFontMetricsGTK.cpp nsFontMetricsGTK.h nsRenderingContextGTK.cpp nsRenderingContextGTK.h I do ask that we open a bug at this time as a marker to clean up the extra DrawString call. Although my experience is more limited in the gfx/src/ps and gfs/src/xlib areas, it seems okay. If you have trouble finding a reviewer for these file let me know. At the present I make no statement about these files: () nsRenderingContextPS.cpp (katakai could you check this?) nsRenderingContextPS.h (katakai could you check this?) nsFontMetricsXlib.cpp nsFontMetricsXlib.h nsRenderingContextXlib.cpp nsRenderingContextXlib.h Although I see no immediate problems in the following, I have no expertise in these areas so I cannot express an opinion either positive or negative of the changes to: nsFont.h nsFont.cpp nsIRenderingContext.h nsRenderingContextMac.cpp nsRenderingContextMac.h nsUnicodeRenderingToolkit.cpp

re: nsIFontMetrics::GetMaxAscent/Descent Since the subject is fresh, I thought I should add that these are the functions that are currently used to determine the height of nsTextFrame. Together with the way the line-height is determined, no wonder therefore that the final height is always the height of the ASCII font. The following is an excerpt from my changes in nsTextFrame to correct this and use textData.mAscent/mDescent which are now obtained using GetDimensions: - ts.mNormalFont->GetHeight(aMetrics.height); - ts.mNormalFont->GetMaxAscent(aMetrics.ascent); - ts.mNormalFont->GetMaxDescent(aMetrics.descent); + aMetrics.ascent = textData.mAscent; + aMetrics.descent = textData.mDescent; + aMetrics.height = aMetrics.ascent + aMetrics.descent;

Since this bug is getting too long, I have opened bug 99010 for continuation. Head over to that bug for what's coming up... *** This bug has been marked as a duplicate of 99010 ***