Closed Bug 74186 Opened 24 years ago Closed 23 years ago

Unable to choose different size for serif and sans-serif fonts

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 99010
Milestone:
mozilla0.9.5

People

(Reporter: hniksic, Assigned: rbs)

References

(Blocks 1 open bug)

Details

(Keywords: fonts, Whiteboard: [Hixie-PF] WG: actual (font-size-adjusted values of) font-size should be used for 'em', 'ex', '%', ratios and inline box height)

Attachments

(27 files)

demo of font-size 'Auto'
3.59 KB, image/gif
Details
demo2 - same as above but with 'x' added on the first line
45.34 KB, image/gif
Details
real demo2 !
3.79 KB, image/gif
Details
demo based on ongoing spec compliant implementation
8.40 KB, image/gif
Details
FYI - Template of the callback mechanism as I have it in my tree
2.90 KB, text/plain
Details
testcase - shows how ugly overlapping can happen if GetDimensions() isn't used - need MathML-enabled builds
3.94 KB, text/html
Details
patch in gfx
280.19 KB, text/plain
Details
patch in content
4.85 KB, text/plain
Details
patch in layout
6.15 KB, text/plain
Details
patch in layout (ignore the previous one, I picked the wrong diff.txt)
39.38 KB, text/plain
Details
rendering of the testcase 1)with vs. 2)without GetDimensions()
21.02 KB, image/gif
Details
impact of font-size-adjust
7.32 KB, image/gif
Details
patch in content
41.38 KB, text/plain
Details
patch in layout
60.43 KB, text/plain
Details
patch in gfx
297.78 KB, text/plain
Details
final patch in content - just sync:ing with the tip
41.65 KB, text/plain
Details
final patch in layout - just sync:ing with the tip
61.38 KB, text/plain
Details
patch in gfx [win32 (tested), gtk (untested), xlib (untested)]
307.85 KB, text/plain
Details
patch in gfx - with an iteration to remove the local min-size hacks
307.85 KB, text/plain
Details
fix-up patch for GfxPS
5.77 KB, text/plain
Details
fix-up patch for GfxMac
6.03 KB, text/plain
Details
patch in layout (diff -wu)
61.10 KB, text/plain
Details
patch in content (diff -wu)
41.52 KB, text/plain
Details
patch in gfx-win (diff -wu)
233.40 KB, patch
Details | Diff | Splinter Review
patch in gfx-elsewhere (diff -wu)
44.06 KB, text/plain
Details
small patch for the prefs - will be needed to sync the prefs for gfx-win
3.11 KB, text/plain
Details
updated patch in gfx/{gtk,xlib,mac}
42.06 KB, patch
Details | Diff | Splinter Review 
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Nominating for mozilla0.9.1. Looks there is already a nice spot on the 
pref. dialog box awaiting this.
Keywords: mozilla0.9.1
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
Component: Preferences → Layout
Oops. Reassign. (Is this right?)

Gerv
Assignee: mcafee → erik
OS: Linux → All
Hardware: PC → All
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.
QA Contact: sairuh → petersen
This is a good one. But it mean we need to change the UI. Reassign to nhotta and 
mark it as Future for now.
Assignee: erik → nhotta
Target Milestone: --- → Future
Blocks: 61883
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.
Target Milestone: Future → ---
Depends on: 84398
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.
Assignee: nhotta → ftang
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
Assignee: ftang → rbs
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.
Whiteboard: WONTFIX ? -- what's the point?
> 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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3

    
    

    
  
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."

    
    

    
  

      
      
      
      

        Attached image
          
        real demo2 !
        — 
      

    


  

    
    

    
  
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...! :-)
Keywords: fonts
Whiteboard: WONTFIX ? -- what's the point? → [Hixie-PF]

    
    

    
  
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.
Depends on: 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.

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        patch in gfx
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        patch in content
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        patch in layout
        — 
      

    


  

    
    

    
  
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.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

    
    

    
  
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?
Depends on: 41847
Whiteboard: [Hixie-PF] → [Hixie-PF] WG: actual (font-size-adjusted values of) font-size should be used for 'em', 'ex', '%', ratios and inline box height

    
    

    
  
>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.

    
    

    
  

      
      
      
      

        Attached file
          
        patch in content
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        patch in layout
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        patch in gfx
        — 
      

    


  

    
    

    
  
...[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".

    
  
Blocks: 95860

    
    

    
  
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.
Target Milestone: mozilla0.9.4 → mozilla0.9.5

    
    

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

    
    

    
  
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.


    
    

    
  

      
      
      
      

        Attached file
          
        fix-up patch for GfxPS
        — 
      

    


  

    
    

    
  
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 ***
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: