The default bug view has changed. See this FAQ.

getPropertyValue truncates values for computed system font value

RESOLVED FIXED

Status

Core Graveyard
GFX
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: Karsten Düsterloh, Assigned: Karsten Düsterloh)

Tracking

({fixed1.7})

Trunk
x86
All
fixed1.7

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

13 years ago
Created attachment 148759 [details]
Test case
(Assignee)

Comment 2

13 years ago
Created attachment 148760 [details]
Screen shot
(Assignee)

Updated

13 years ago
Attachment #148760 - Attachment description: Scrren shot → Screen shot
(Assignee)

Comment 3

13 years ago
Created attachment 148900 [details] [diff] [review]
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. ;-)
Assignee: general → mnyromyr
Status: NEW → ASSIGNED
(Assignee)

Updated

13 years ago
Component: DOM: CSSOM → GFX
(Assignee)

Updated

13 years ago
Attachment #148900 - Flags: review?(ere)

Comment 4

13 years ago
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+
(Assignee)

Updated

13 years ago
Attachment #148900 - Flags: superreview?(dbaron)
(Assignee)

Comment 5

13 years ago
Created attachment 149040 [details] [diff] [review]
Fix for BeOS (1 line)
(Assignee)

Updated

13 years ago
Attachment #149040 - Flags: review?(timeless)
(Assignee)

Comment 6

13 years ago
Created attachment 149043 [details] [diff] [review]
Fix for Mac (1 line)
(Assignee)

Updated

13 years ago
Attachment #149043 - Flags: review?(timeless)
(Assignee)

Comment 7

13 years ago
Created attachment 149045 [details] [diff] [review]
Fix for OS/2 (1 line)
(Assignee)

Updated

13 years ago
Attachment #149045 - Flags: review?(mkaply)
(Assignee)

Comment 8

13 years ago
Created attachment 149047 [details] [diff] [review]
Fix for Photon (1 line)
(Assignee)

Updated

13 years ago
Attachment #149047 - Flags: review?(amardare)
(Assignee)

Comment 9

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

Updated

13 years ago
Attachment #148900 - Attachment description: Possible fix (for Windows and GTK) → Base fix for all OS + fix for Windows (1 line)

Comment 10

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

r=mkaply
Attachment #149045 - Flags: review?(mkaply) → review+
(Assignee)

Updated

13 years ago
Attachment #149045 - Flags: superreview?(dbaron)

Updated

13 years ago
Attachment #149047 - Flags: review?(amardare) → review+
(Assignee)

Updated

13 years ago
Attachment #149047 - Flags: superreview?(dbaron)

Updated

13 years ago
Attachment #149040 - Flags: review?(timeless) → review+

Updated

13 years ago
Attachment #149043 - Flags: superreview?(sfraser)
Attachment #149043 - Flags: review?(timeless)
Attachment #149043 - Flags: review+
(Assignee)

Updated

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

Comment 12

13 years ago
Created attachment 149952 [details] [diff] [review]
collective patch, incorporating ere's and dbaron's comments
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
(Assignee)

Updated

13 years ago
Attachment #148900 - Flags: superreview?(dbaron)
(Assignee)

Updated

13 years ago
Attachment #149040 - Flags: superreview?(dbaron)
(Assignee)

Updated

13 years ago
Attachment #149043 - Flags: superreview?(sfraser)
(Assignee)

Updated

13 years ago
Attachment #149045 - Flags: superreview?(dbaron)
(Assignee)

Updated

13 years ago
Attachment #149047 - Flags: superreview?(dbaron)
(Assignee)

Comment 13

13 years ago
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?
checked in on trunk
(Assignee)

Comment 15

13 years ago
Created attachment 149953 [details] [diff] [review]
patch for XLib

Sorry, I missed XLib... :-/
(Assignee)

Updated

13 years ago
Attachment #149953 - Flags: review?(timeless)

Comment 16

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

r=roland.mainz@nrubsig.org
Attachment #149953 - Flags: review?(timeless) → review+
xlib patch checked in
(Assignee)

Comment 18

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

Supplementary patch to patch 149952
Attachment #149953 - Flags: approval1.7?
(Assignee)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: fixed on trunk, asked for 1.7 approval

Comment 19

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

Updated

13 years ago
Whiteboard: fixed on trunk and 1.7 branch → fixed on trunk and 1.7 branch, needed-aviary1.0

Updated

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