Closed
Bug 243966
Opened 20 years ago
Closed 20 years ago
getPropertyValue truncates values for computed system font value
Categories
(Core Graveyard :: GFX, defect)
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)
1.30 KB,
text/html
|
Details | |
11.38 KB,
image/png
|
Details | |
6.49 KB,
patch
|
mnyromyr
:
review+
mnyromyr
:
superreview+
asa
:
approval1.7+
|
Details | Diff | Splinter Review |
860 bytes,
patch
|
roland.mainz
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #148760 -
Attachment description: Scrren shot → Screen shot
Assignee | ||
Comment 3•20 years ago
|
||
(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•20 years ago
|
Component: DOM: CSSOM → GFX
Assignee | ||
Updated•20 years ago
|
Attachment #148900 -
Flags: review?(ere)
Comment 4•20 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•20 years ago
|
Attachment #148900 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 5•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #149040 -
Flags: review?(timeless)
Assignee | ||
Comment 6•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #149043 -
Flags: review?(timeless)
Assignee | ||
Comment 7•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #149045 -
Flags: review?(mkaply)
Assignee | ||
Comment 8•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #149047 -
Flags: review?(amardare)
Assignee | ||
Comment 9•20 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•20 years ago
|
Attachment #148900 -
Attachment description: Possible fix (for Windows and GTK) → Base fix for all OS + fix for Windows (1 line)
Comment 10•20 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•20 years ago
|
Attachment #149045 -
Flags: superreview?(dbaron)
Attachment #149047 -
Flags: review?(amardare) → review+
Assignee | ||
Updated•20 years ago
|
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+
Assignee | ||
Updated•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #148900 -
Flags: superreview?(dbaron)
Assignee | ||
Updated•20 years ago
|
Attachment #149040 -
Flags: superreview?(dbaron)
Assignee | ||
Updated•20 years ago
|
Attachment #149043 -
Flags: superreview?(sfraser)
Assignee | ||
Updated•20 years ago
|
Attachment #149045 -
Flags: superreview?(dbaron)
Assignee | ||
Updated•20 years ago
|
Attachment #149047 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 13•20 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?
Comment 14•20 years ago
|
||
checked in on trunk
Assignee | ||
Comment 15•20 years ago
|
||
Sorry, I missed XLib... :-/
Assignee | ||
Updated•20 years ago
|
Attachment #149953 -
Flags: review?(timeless)
Comment 16•20 years ago
|
||
Comment on attachment 149953 [details] [diff] [review] patch for XLib r=roland.mainz@nrubsig.org
Attachment #149953 -
Flags: review?(timeless) → review+
Comment 17•20 years ago
|
||
xlib patch checked in
Assignee | ||
Comment 18•20 years ago
|
||
Comment on attachment 149953 [details] [diff] [review] patch for XLib Supplementary patch to patch 149952
Attachment #149953 -
Flags: approval1.7?
Assignee | ||
Updated•20 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: fixed on trunk, asked for 1.7 approval
Comment 19•20 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+
Comment 20•20 years ago
|
||
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•20 years ago
|
Whiteboard: fixed on trunk and 1.7 branch → fixed on trunk and 1.7 branch, needed-aviary1.0
Updated•20 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
Updated•16 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•