243966 - getPropertyValue truncates values for computed system font value

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Karsten Düsterloh]

Netscape/7.1 [Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.4)
Gecko/20030624]
Mozilla 1.7a [Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7a) Gecko/20040129]
Mozilla 1.8a [Mozilla/5.0 (Windows; U; Windows NT 5.0; de-AT; rv:1.8a)
Gecko/20040512]

Setting a system font like |menu| or |message-box| via the CSS font: property
works, but the computed font-family value is truncated when reading it with
getPropertyValue and hence unusable.

Steps to reproduce:
- Make sure that your system font for menu or message-box is different from the
default font (default should be |serif|) and that the new font's name is longer
than the length of |serif| plus 1. 
Eg: under Windows 2000, change the value of Start->Settings->Control
Panel->Display->Apperance->Item: Messagebox to "Lucida Handwriting"
- View the attached test case:
The test case applies the styles 
      #family     {font: 100% monospace}
      #menu       {font: menu;}
      #messagebox {font: message-box;}
to a first line of text, tries to read the resulting font-family value and to
style a second line of text accordingly - and succeeds *only* in the #family case.

Example (see screenshot): Windows 2000 with Menu="Old English Text MT" and
Messagebox="Lucida Handwriting"; Mozilla Modern Theme
---
Some text with a given font: menu.
Some text with the copied font-family: Old English T

Some text with a given font: message-box.
Some text with the copied font-family: Lucida Handw
---

The last six characters (length of the default font "serif" + 1) are cut from
the font-family name and hence reapplying that font to the second line fails. 

The actual error occurs in nsComputedDOMStyle::GetFontFamily (l.515++):

    PRUint8 generic = font->mFlags & NS_STYLE_FONT_FACE_MASK;
    if (generic == kGenericFont_NONE) { 
      const nsFont* defaultFont =
        presContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID);

      PRInt32 lendiff = fontName.Length() - defaultFont->name.Length();
      if (lendiff > 0) {
        val->SetString(Substring(fontName, 0, lendiff-1)); // -1 removes comma
      } else {
        val->SetString(fontName);
      }
    } else {
      val->SetString(fontName);
    }

(generic == kGenericFont_NONE) and (lendiff > 0) are true and the characters at
the end of the string are cut even though there's no default font name attached.
It seems as if the correct flag (here: NS_STYLE_FONT_MENU resp.
NS_STYLE_FONT_MESSAGE_BOX) isn't set...

Comment 1

[Karsten Düsterloh]

Attachment: Test case

Comment 2

[Karsten Düsterloh]

Attachment: Screen shot

Comment 3

[Karsten Düsterloh]

Attachment: Base fix for all OS + fix for Windows (1 line)

(Taking if the proposed fix is at least somewhere near the target. ;-) )

The problem seems to be in nsRuleNode::SetFont
(http://lxr.mozilla.org/mozilla/source/content/base/src/nsRuleNode.cpp#1595):
If we have a list of fonts, we append the default font.
If we have a system font, we just set that font.
And when the font-family property is read in nsComputedDOMStyle::GetFontFamily
(http://lxr.mozilla.org/mozilla/source/content/html/style/src/nsComputedDOMStyle.cpp#497),
the default font is removed - even if it hasn't added...

Since the nsFonts' systemFont flag is never set (except in GTK, see
http://lxr.mozilla.org/mozilla/source/gfx/src/gtk/nsDeviceContextGTK.cpp#980)
and is never checked anywhere, I think that this maybe a solution:
- system fonts should set the systemFont flag to true
- GetFontFamily should check the systemFont flag and, if it is set to true,
don't make removals

My proposed patch does this for Windows and it should work with GTK also, all
other platforms should be fixed accordingly.
This patch is actually a one-and-a-half-liner. ;-)

Comment 4

[Ere Maijala (slow)]

Comment on attachment 148900 [details] [diff] [review]
Base fix for all OS + fix for Windows (1 line)

> +    if ((generic == kGenericFont_NONE) && (!font->mFont.systemFont)) { 

You can remove the extra parenthesis. This would be better:

    if (generic == kGenericFont_NONE && !font->mFont.systemFont) { 

I don't know this font stuff very well, but with the above change and provided
that you get a knowledgeable sr, r=ere

Comment 5

[Karsten Düsterloh]

Attachment: Fix for BeOS (1 line)

Comment 6

[Karsten Düsterloh]

Attachment: Fix for Mac (1 line)

Comment 7

[Karsten Düsterloh]

Attachment: Fix for OS/2 (1 line)

Comment 8

[Karsten Düsterloh]

Attachment: Fix for Photon (1 line)

Comment 9

[Karsten Düsterloh]

JFTR: Even though PS and XPrint define a function GetSystemFont, that just calls
the parent device context's one - no need to act there.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 149045 [details] [diff] [review]
Fix for OS/2 (1 line)

r=mkaply

Comment 11

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

sr=dbaron on all 5 patches (I'm not going to mark it on all of them -- they
could easily have been attached as one), if you take ere's comment and probably
drop the three line comment in each place.  If you want a comment about what
|systemFont| is used for, put it in nsFont.h.  However, this patch makes that
comment no longer the whole truth, since you're adding a second use -- so you
could probably fix the GTK version as well.

Comment 12

[Karsten Düsterloh]

Attachment: collective patch, incorporating ere's and dbaron's comments

Comment 13

[Karsten Düsterloh]

Comment on attachment 149952 [details] [diff] [review]
collective patch, incorporating ere's and dbaron's comments

Taking over r+ of the previous one line patches and sr+ of dbaron from comment
#11.
Asking for a1.7 because this is a collection of simple one liners that
improve/correct standard compliance.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on trunk

Comment 15

[Karsten Düsterloh]

Attachment: patch for XLib

Sorry, I missed XLib... :-/

Comment 16

[Roland Mainz]

Comment on attachment 149953 [details] [diff] [review]
patch for XLib

r=[e-mail]

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

xlib patch checked in

Comment 18

[Karsten Düsterloh]

Comment on attachment 149953 [details] [diff] [review]
patch for XLib

Supplementary patch to patch 149952

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 149952 [details] [diff] [review]
collective patch, incorporating ere's and dbaron's comments

a=asa (on behalf of drivers) for checkin to Mozilla 1.7. We're wrapping things
up and so this will need to land today if it's going to make the release.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in on 1.7 branch