For font families defined via @font-face rules, if no bold face is defined then synthetic bolding is not applied to the regular face. Steps: Open URL in FF 3.5. Result: text displays with no bolding Expected result: the word 'grandpas' should appear bolded This occurs because the code to fill out the LOGFONT struct is using the weight in the FontEntry, which in this case is a regular weight. For normal platform fonts on Windows, an extra entries are created for bold and bold italic when not present in the family.

This is a simple workaround for the problem. It will solve most cases but not the case where 'bolder' is involved, for those cases it will only be correct some of the time. For example, if a family has weights 100, 400 and a user specifies 'bolder' for an element whose inherited weight is 100, this code will incorrectly bold the output. The real solution is to use the 'needsBold' flag. But the windows font group code processes all font entries at once and there's no place to stick this. One solution would be to set up an array of bools for this but, gak.

Comment on attachment 384366 [details] [diff] [review] patch, v.0.1, set weight with user fonts to force synthetic bolding I get compiler errors with this patch. >diff --git a/gfx/thebes/src/gfxWindowsFonts.cpp b/gfx/thebes/src/gfxWindowsFonts.cpp >--- a/gfx/thebes/src/gfxWindowsFonts.cpp >+++ b/gfx/thebes/src/gfxWindowsFonts.cpp >@@ -1173,17 +1183,30 @@ gfxWindowsFont::SetupCairoFont(gfxContex > * except for OOM in which case we do nothing and return null. > */ > already_AddRefed<gfxWindowsFont> > gfxWindowsFont::GetOrMakeFont(FontEntry *aFontEntry, const gfxFontStyle *aStyle) > { > // because we know the FontEntry has the weight we really want, use it for matching > // things in the cache so we don't end up with things like 402 in there. > gfxFontStyle style(*aStyle); >- style.weight = aFontEntry->mWeight; >+ >+ if (aFontEntry->mUserFont && !aFontEntry->IsBold()) { Changing the last line above to: if (aFontEntry->mIsUserFont && !aFontEntry->IsBold()) { corrects the issue for me.

Use a mNeedsBold array of booleans to track which font entries need bolding. Set when looking up the font entry, use within GetOrMakeFont. With this synthetic bolding should be correct for bolder/lighter situations. Needs more testing and a set of reftests.

While making a reftest for this I came across a more general bug, when lighter is used we don't currently compensate for families containing a synthetic bold face (this affects platform fonts on Mac, @font-face fonts on Mac/Windows). Fix is just to detect that situation and adjust for it.

Comment on attachment 388998 [details] [diff] [review] patch, v.0.3, fix lighter problem and add reftest This looks sensible to me. It'll end up getting merged into the font-style unification stuff soon, but that's fine; we'll still want the improved behavior. If I'm thinking straight, it seems like this: + PRUint16 targetWeight = (baseWeight * 100) + (weightDistance * 100); + if ((weightDistance == 0 && targetWeight >= 600) + || (weightDistance > 0)) { + weight = 700; // set to get GDI to synthetic bold this face + } is equivalent to: + if ((weightDistance == 0 && baseWeight >= 6) || weightDistance > 0) + weight = 700; // set to get GDI to synthetic bold this face + } and a similar simplification is possible in gfxWindowsFont::GetOrMakeFont. There's no need to actually compute a targetWeight value. (Maybe the compiler is smart enough to optimize it away?)

Changes based on review comments and disable reftest on Linux (bug 419962).

Pushed to trunk http://hg.mozilla.org/mozilla-central/rev/b2d3327fb4cc

Comment on attachment 390324 [details] [diff] [review] patch, v.0.3a, changes based on review comments If I'm thinking straight, this change to gfxWindowsFonts.cpp is not actually correct: >@@ -2395,17 +2424,17 @@ already_AddRefed<gfxWindowsFont> > already_AddRefed<gfxWindowsFont> > gfxWindowsFontGroup::WhichFontSupportsChar(const nsTArray<nsRefPtr<FontEntry> >& fonts, PRUint32 ch) { > for (PRUint32 i = 0; i < fonts.Length(); i++) { > nsRefPtr<FontEntry> fe = fonts[i]; > if (fe->mSymbolFont && !mStyle.familyNameQuirks) > continue; > if (fe->HasCharacter(ch)) { > nsRefPtr<gfxWindowsFont> font = >- gfxWindowsFont::GetOrMakeFont(fe, &mStyle); >+ gfxWindowsFont::GetOrMakeFont(fe, &mStyle, mFontNeedsBold[i]); > // Check that the font is still usable. > if (!font->IsValid()) > continue; > return font.forget(); > } > } > return nsnull; > } The problem is that the array of fonts passed to WhichFontSupportsChar is not in any way parallel to the mFontNeedsBold array of the font group. So the use of mFontNeedsBold[i] here is incorrect and will give more-or-less arbitrary results; it may even crash, if the mFontNeedsBold array is shorter than the array of fonts found by GetPrefFonts for the language group. I haven't yet constructed a test-case that reliably hits this, but I ran into an array-bounds assertion in a debug build that was triggered here. John, am I understanding things correctly?

(In reply to comment #8) Jonathan, you're reading the signatures incorrectly I think. There's a version of WhichFontSupportsChar in gfxFont.h which takes an array of gfxFont objects, but the function you highlighted above takes an array of Windows-specific FontEntry objects: already_AddRefed<gfxWindowsFont> WhichFontSupportsChar(const nsTArray<nsRefPtr<FontEntry> >& fonts, PRUint32 ch); These are not the same. The mFontNeedsBold list is kept in sync with the list of FontEntry objects in mFontEntries (see gfxWindowsFonts.h). If there's somewhere in the code where those are out of sync, then that's a bug but I don't see one currently. As part of your unification, it would be good to remove this code but for now it's necessary to support synthetic bolding of downloadable fonts. If you still thing there's a bug, a testcase would really help.

Argh, on second thought, you're right, the use of mFontNeedsBold[i] should be omitted from the line of code you pointed out. I modify the patch.

Remove incorrect reference to mFontNeedsBold. The default argument is fine since this function is only called for pref fonts. Startup init code creates fake bold entries for all platform fonts lacking a bold face so these fonts are covered.

Comment on attachment 394619 [details] [diff] [review] follow-up patch, remove incorrect mFontNeedsBold reference Follow-up patch pushed to trunk http://hg.mozilla.org/mozilla-central/rev/bf78b6bb0b03 This is needed on 1.9.2.

Comment on attachment 390324 [details] [diff] [review] patch, v.0.3a, changes based on review comments Is the follow-up patch needed if we take this on 1.9.1 also? This is a big patch, is it fixing something we can't live without on the 1.9.1 branch?

Comment on attachment 394619 [details] [diff] [review] follow-up patch, remove incorrect mFontNeedsBold reference approval1.9.2 requests aren't currently being monitored, since we're nearing RC freeze and there are too many outstanding requests, so I'm clearing this request. Feel free to re-request approval if you are confident that it's worth drivers' time to consider whether this non-blocker needs to land for 1.9.2 at this stage.