Bug 60556

Sorry, should be font-family css attribute. 

Correcting the title accordingly 

*** Bug 60976 has been marked as a duplicate of this bug. ***

I too have encountered this bug, and I think I have discovered the root cause.  
(Source path and line numbers are from the kdelibs-3.1-10.src.rpm package  
distributed with Red Hat Linux 9.) The code for font family selection appears  
in khtml/css/cssstyleselector.cpp lines 2432-2474. That code correctly includes  
a loop over the stylesheet's font-family value, but it handles the "not found"  
case incorrectly.  
  
    QString face = static_cast<FontFamilyValueImpl *>(val)->fontName();  
    if ( !face.isNull() || face.isEmpty() ) {  
	if(face == "serif") {  
	    face = settings->serifFontName();  
	}  
	else if(face == "sans-serif") {  
	    face = settings->sansSerifFontName();  
	}  
	else if( face == "cursive") {  
	    face = settings->cursiveFontName();  
	}  
	else if( face == "fantasy") {  
	    face = settings->fantasyFontName();  
	}  
	else if( face == "monospace") {  
	    face = settings->fixedFontName();  
	}  
	else if( face == "konq_default") {  
	    face = settings->stdFontName();  
	}  
	if ( !face.isEmpty() ) {  
	    fontDef.family = face;  
	    if (style->setFontDef( fontDef ))  
		fontDirty = true;  
	}  
        return;  
    }  
  
The goal is apparently to look up the font-family in the Qt font database and  
set the font-family if there's a match. However, the "if" condition is  
incorrect: It's always true, even if there's no match! Because of this, the 
first font-family always "matches"; if the font doesn't actually exist, 
Konqueror apparently uses its default font. 
 
It looks like there's a set of parentheses missing. I think the following code 
would be correct: 
  
    if ( ! (face.isNull() || face.isEmpty()) ) {  
  
However, I think the test can be simplified further. All Null QStrings are also  
Empty. Therefore, the following test should be equivalent: 
 
    if ( !face.isEmpty() ) { 
 
Unfortunately, I don't have a good test system to test this change. The error 
is simple, though, and this one-line change should be sufficient to fix it, 
unless I'm overlooking something. Hopefully, the KDE developers can fix it 
easily. 
 
Note that I also discovered another possible bug while investigating this. At 
first, I thought this might be an Xft bug. I tried creating an alias for the 
missing font, but that made no difference. Qt's font database does not include 
aliases in the db.families() call (used by KHTMLSettings::AvailableFamilies() 
and indirectly by the CSS routines). Therefore, a user cannot supply aliases 
for fonts used in font-family declarations, which is a potential problem for 
Linux users who have URW Palladio installed instead of a true Palatino font 
(for example). I don't know whether there's a way to fix this using Qt; a brief 
skim of the manpages didn't turn anything up. Therefore, this problem might be 
a Qt bug rather than a KHTML bug. 

Subject: Re:  [PATCH] Konqueror doesn't parse font-family CSS correctly if first font doesn't exist [testcase]

Thanks Bradd for your detective work and clear description of the problem!

The following patch against current KDE_3_1_BRANCH fixes the problem for me:
If anyone with CVS privileges could apply the patch, or something very similar
this bug can be closed. I don't know if it the problem exists in HEAD as I'm not running it, 
and the code seems to have changed quite a bit.

Jason


Index: cssstyleselector.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtml/css/cssstyleselector.cpp,v
retrieving revision 1.242.2.4
diff -u -3 -p -r1.242.2.4 cssstyleselector.cpp
--- cssstyleselector.cpp        30 Apr 2003 21:41:07 -0000      1.242.2.4
+++ cssstyleselector.cpp        5 Aug 2003 08:13:00 -0000
@@ -2451,7 +2451,8 @@ void CSSStyleSelector::applyRule( DOM::C
             CSSPrimitiveValueImpl *val = static_cast<CSSPrimitiveValueImpl *>(item);
             if(!val->primitiveType() == CSSPrimitiveValue::CSS_STRING) return;
             QString face = static_cast<FontFamilyValueImpl *>(val)->fontName();
-           if ( !face.isNull() || face.isEmpty() ) {
+
+           if ( !face.isEmpty() ) {
                if(face == "serif") {
                    face = settings->serifFontName();
                }

unfortunately the code in HEAD looks slightly different. But I attach a test case that breaks.

Created attachment 2858 [details]
test case

should display in courier, displays in default sans font

sent patch to kfm-devel for review

Subject: kdelibs/khtml

CVS commit by coolo: 

do not break on non existant fonts in css font lists
CCMAIL: 60556-done@bugs.kde.org


  M +5 -0      ChangeLog   1.44
  M +3 -2      css/cssstyleselector.cpp   1.294


--- kdelibs/khtml/ChangeLog  #1.43:1.44
@@ -1,2 +1,7 @@
+2003-10-23  Stephan Kulow  <coolo@kde.org>
+
+        * css/cssstyleselector.cpp (applyRule): if the first font isn't found,
+                continue looking (#60556)
+
 2003-10-22  Dirk Mueller  <mueller@kde.org>
 

--- kdelibs/khtml/css/cssstyleselector.cpp  #1.293:1.294
@@ -2928,8 +2928,9 @@ void CSSStyleSelector::applyRule( int id
             if ( !face.isEmpty() ) {
                 fontDef.family = face;
-                if (style->setFontDef( fontDef ))
+                if (style->setFontDef( fontDef )) {
                     fontDirty = true;
-            }
             return;
+                }
+            }
         }
         break;

This change created bug #67768

Subject: kdelibs/khtml

CVS commit by coolo: 

slightly modified patch by Paul Sprakes to fix his own bug report which 
turned out to be a regression introduced by my fix for #60556 (see #67768
for detailed descriptions)
CCMAIL: 60556@bugs.kde.org
CCMAIL: 67768-done@bugs.kde.org
CCMAIL: 67973-done@bugs.kde.org

(will work out regression tests now :)


  M +6 -0      ChangeLog   1.102
  M +7 -14     css/cssstyleselector.cpp   1.301


--- kdelibs/khtml/ChangeLog  #1.101:1.102
@@ -1,2 +1,8 @@
+2003-11-23  Stephan Kulow  <coolo@kde.org>
+
+        * css/cssstyleselector.cpp (applyRule): fixing my fix for #60556, which 
+        introduced quite some regressions (thanks to Paul Sprakes for working
+        that out with me)
+
 2003-11-22  Stephan Kulow  <coolo@kde.org>
 

--- kdelibs/khtml/css/cssstyleselector.cpp  #1.300:1.301
@@ -1852,6 +1852,5 @@ void CSSStyleSelector::applyRule( int id
             }
         }
-        if (style->setFontDef( fontDef ))
-        fontDirty = true;
+        fontDirty |= style->setFontDef( fontDef );
         break;
     }
@@ -1874,6 +1873,5 @@ void CSSStyleSelector::applyRule( int id
                 return;
         }
-        if (style->setFontDef( fontDef ))
-            fontDirty = true;
+        fontDirty |= style->setFontDef( fontDef );
         break;
     }
@@ -1918,6 +1916,5 @@ void CSSStyleSelector::applyRule( int id
             }
         }
-        if (style->setFontDef( fontDef ))
-            fontDirty = true;
+        fontDirty |= style->setFontDef( fontDef );
         break;
     }
@@ -2773,6 +2770,5 @@ void CSSStyleSelector::applyRule( int id
 
         fontDef.size = size;
-        if (style->setFontDef( fontDef ))
-        fontDirty = true;
+        fontDirty |= style->setFontDef( fontDef );
         return;
     }
@@ -2982,10 +2978,8 @@ void CSSStyleSelector::applyRule( int id
             if ( !face.isEmpty() ) {
                 fontDef.family = face;
-                if (style->setFontDef( fontDef )) {
-                    fontDirty = true;
+                fontDirty |= style->setFontDef( fontDef );
                     return;
                 }
             }
-        }
         break;
     }
@@ -3164,6 +3158,5 @@ void CSSStyleSelector::applyRule( int id
             FontDef fontDef = parentStyle->htmlFont().fontDef;
             style->setLineHeight( parentStyle->lineHeight() );
-            if (style->setFontDef( fontDef ))
-                fontDirty = true;
+            fontDirty |= style->setFontDef( fontDef );
         } else if ( value->isFontValue() ) {
             FontValueImpl *font = static_cast<FontValueImpl *>(value);