Version: (using KDE Devel) Installed from: Compiled sources Slashdot (and other slashcode sites) displays article titles with the following attribute: FACE="arial,helvetica" This should cause the title to be displayed with arial if found, however for some reason helvetica is picked, which isn't scalable and looks ugly. The rest of the article is displayed correctly in arial. If i save the page as html and edit the above to read: FACE="arial" then the title will display correctly in arial. This problem is not in 3.1.4 but has appeared in cvs.
I've figured out what introduced this bug. It's from revision 1.294 commited by coolo. Attached is a patch that will revert the change (and reintroduce #60556 - so it isn't a real fix). I'm a java programmer and don't know eneough c++ to create a real fix.
Created attachment 3163 [details] Patch which shows the bug location
Hmm, so you have arial installed? Because with your patch, it will simply default to the default font if arial can't be found, otherwise it will go on to use helvetica. Try fc-list | grep -i arial
Yes, I have the Microsoft font pack installed which includes arial. What happens for you without the patch? Does it go straight to Helvetica?
It displays in the default font. Just set the default sans-serif font to courier to see the effect with and without the patch.
Created attachment 3342 [details] try this Just look here. The second and third show up in arial, the fourth displays in the default font (I have no helvetica installed) and the first displays in the default sans serif font (verdana for me)
Created attachment 3344 [details] test case There was a slight typo in your test - here it is corrected. I'll have to recompile without the patch so I'll get back to you later. With the patch the test case works as expected. That is the first uses whatever I set in konq's options, the second and third both use arial and the last uses helvetica.
OK, without the patch: First is correct. It uses my sans-serif settings Second is wrong. It ignores arial and uses helvetica Third is correct. It uses arial Fourth is correct. It uses helvetica Could you change helvetica in the test to another font that you have installed. Hopefully you will then see the same as me.
Comment on attachment 3344 [details] test case <font face="sans-serif" size="+5">Default font</font><br> <font FACE="arial,helvetica" size="+5">Arial or Helvetica</font><br> <font FACE="arial" size="+5">Arial or default</font><br> <font FACE="helvetica" size="+5">Helvetica</font><br> <font FACE="fancynothing,arial" size="+5">Should be Arial</font><br>
it didn't work as I tried to edit the attachment. But if you add the last line of #9 to your test case, you should see a definite difference what the patch fixes.
Right, I've figured out what is really happening here: Without my patch i.e. from cvs, If the first font in the face attribute is the same as the font I have set as the standard font in konq's preferences, then it is ignored and the second font is used. If I change the standard font to anything but arial, then the second test (face="arial,helvetica") will display in arial. So to recap: I start off with standard font == arial (in konq's settings). If the face attribute is face="arial,helvetica" then arial is ignored and helvetica chosen. If I change standard font to helvetica then change the face attribute to face="helvetica,arial" then helvetica is ignored and arial chosen. As long as my standard font setting is not equal to the first attribute value then it is displayed in the correct font. Hope that makes sense.
More info. I think the problem is this line (cssstyleselector.cpp:2984): if (style->setFontDef( fontDef )) { This seems to say: If set font is succesful then we have the right one and can return. However, the setFontDef() method in render_style.h is this: bool setFontDef(const khtml::FontDef & v) { // bah, this doesn't compare pointers. broken! (Dirk) if (!(inherited->font.fontDef == v)) { inherited.access()->font = Font( v ); return true; } return false; } So the inherited font will be arial and the arg font will also be arial. The comparison will return false since the font doesn't need changing therefore this method will also return false. This means that the calling method will think it has failed and will loop round to the next font value.
Created attachment 3354 [details] Proposed fix I think I have fixed it (see patch). style->setFontDef( fontDef ) returns true if the font changed and false if it is the same. However it was called with the assumption that it returns false if it failed. Therefore if the current font was the same as the default font it would think it failed. The correct call would be: fontDirty = style->setFontDef( fontDef ); return; This will only set the dirty flag if the font had changed. I'm testing the attached patch and it seems to be OK
There may be a problem with the last patch. I think fontDirty is global so my patch may set it back to false if it had previously been set to true. Would this be better: fontDirty = fontDirty || style->setFontDef( fontDef );
Oops, that should be: fontDirty = style->setFontDef( fontDef ) || fontDirty;
Created attachment 3358 [details] Tests both 67768 and 60556 This is an updated test which tests both this bug and bug 60556.
Created attachment 3359 [details] updated fix This is an updated fix which should pass the last test case with flying colours. I have updated each line where fontDirty is set, however it is only needed in one place. I did this for the sake of consistency (in case of future copy/pasting).
Created attachment 3361 [details] rendering for me without the patch would you accept that as WORKSFORME? I'm looking at your patch though
thanks for the detailed description. Now that you point me to it, it's obvious why having the same families in a:link and a:hover changes the font on hover.
If you mean bug 67973 I can confirm that my patch also fixes that too.
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);
Cool, I didn't know there was a |= operator. Does it guarantee that style->setFontDef( fontDef ) is always called (even if fontDirty is already true)?
yep
*** Bug 67834 has been marked as a duplicate of this bug. ***