Closed Bug 243966 Opened 16 years ago Closed 15 years ago

getPropertyValue truncates values for computed system font value

Categories

(Core Graveyard :: GFX, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mnyromyr, Assigned: mnyromyr)

Details

(Keywords: fixed1.7, Whiteboard: fixed on trunk and 1.7 branch, fixed-aviary1.0)

Attachments

(4 files, 5 obsolete files)

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...
Attached file Test case
Attached image Screen shot
Attachment #148760 - Attachment description: Scrren shot → Screen shot
(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. ;-)
Assignee: general → mnyromyr
Status: NEW → ASSIGNED
Component: DOM: CSSOM → GFX
Attachment #148900 - Flags: review?(ere)
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
Attachment #148900 - Flags: review?(ere) → review+
Attachment #148900 - Flags: superreview?(dbaron)
Attached patch Fix for BeOS (1 line) (obsolete) — Splinter Review
Attachment #149040 - Flags: review?(timeless)
Attached patch Fix for Mac (1 line) (obsolete) — Splinter Review
Attachment #149043 - Flags: review?(timeless)
Attached patch Fix for OS/2 (1 line) (obsolete) — Splinter Review
Attachment #149045 - Flags: review?(mkaply)
Attached patch Fix for Photon (1 line) (obsolete) — Splinter Review
Attachment #149047 - Flags: review?(amardare)
JFTR: Even though PS and XPrint define a function GetSystemFont, that just calls
the parent device context's one - no need to act there.
Attachment #148900 - Attachment description: Possible fix (for Windows and GTK) → Base fix for all OS + fix for Windows (1 line)
Comment on attachment 149045 [details] [diff] [review]
Fix for OS/2 (1 line)

r=mkaply
Attachment #149045 - Flags: review?(mkaply) → review+
Attachment #149045 - Flags: superreview?(dbaron)
Attachment #149047 - Flags: review?(amardare) → review+
Attachment #149047 - Flags: superreview?(dbaron)
Attachment #149040 - Flags: review?(timeless) → review+
Attachment #149043 - Flags: superreview?(sfraser)
Attachment #149043 - Flags: review?(timeless)
Attachment #149043 - Flags: review+
Attachment #149040 - Flags: superreview?(dbaron)
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.
Attachment #148900 - Attachment is obsolete: true
Attachment #149040 - Attachment is obsolete: true
Attachment #149043 - Attachment is obsolete: true
Attachment #149045 - Attachment is obsolete: true
Attachment #149047 - Attachment is obsolete: true
Attachment #148900 - Flags: superreview?(dbaron)
Attachment #149040 - Flags: superreview?(dbaron)
Attachment #149043 - Flags: superreview?(sfraser)
Attachment #149045 - Flags: superreview?(dbaron)
Attachment #149047 - Flags: superreview?(dbaron)
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.
Attachment #149952 - Flags: superreview+
Attachment #149952 - Flags: review+
Attachment #149952 - Flags: approval1.7?
Attached patch patch for XLibSplinter Review
Sorry, I missed XLib... :-/
Attachment #149953 - Flags: review?(timeless)
Comment on attachment 149953 [details] [diff] [review]
patch for XLib

r=roland.mainz@nrubsig.org
Attachment #149953 - Flags: review?(timeless) → review+
Comment on attachment 149953 [details] [diff] [review]
patch for XLib

Supplementary patch to patch 149952
Attachment #149953 - Flags: approval1.7?
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: fixed on trunk, asked for 1.7 approval
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.
Attachment #149952 - Flags: approval1.7? → approval1.7+
checked in on 1.7 branch
Keywords: fixed1.7
Whiteboard: fixed on trunk, asked for 1.7 approval → fixed on trunk and 1.7 branch
Whiteboard: fixed on trunk and 1.7 branch → fixed on trunk and 1.7 branch, needed-aviary1.0
Attachment #149953 - Flags: approval1.7?
Whiteboard: fixed on trunk and 1.7 branch, needed-aviary1.0 → fixed on trunk and 1.7 branch, fixed-aviary1.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.