Last Comment Bug 499621 - [@font-face] synthetic bolding not working for downloadable fonts under Windows
: [@font-face] synthetic bolding not working for downloadable fonts under Windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 1 vote (vote)
: ---
Assigned To: John Daggett (:jtd)
:
Mentors:
http://people.mozilla.org/~jdaggett/f...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-21 22:13 PDT by John Daggett (:jtd)
Modified: 2009-12-01 17:10 PST (History)
9 users (show)
wgianopoulos: wanted1.9.2?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, v.0.1, set weight with user fonts to force synthetic bolding (2.65 KB, patch)
2009-06-22 01:57 PDT, John Daggett (:jtd)
no flags Details | Diff | Review
patch, v.0.2, maintain needsBold array (10.38 KB, patch)
2009-07-14 19:18 PDT, John Daggett (:jtd)
no flags Details | Diff | Review
patch, v.0.3, fix lighter problem and add reftest (23.34 KB, patch)
2009-07-16 14:07 PDT, John Daggett (:jtd)
jfkthame: review+
Details | Diff | Review
patch, v.0.3a, changes based on review comments (23.06 KB, patch)
2009-07-23 14:26 PDT, John Daggett (:jtd)
no flags Details | Diff | Review
follow-up patch, remove incorrect mFontNeedsBold reference (930 bytes, patch)
2009-08-14 21:00 PDT, John Daggett (:jtd)
jfkthame: review+
Details | Diff | Review

Description John Daggett (:jtd) 2009-06-21 22:13:32 PDT
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.
Comment 1 John Daggett (:jtd) 2009-06-22 01:57:42 PDT
Created attachment 384366 [details] [diff] [review]
patch, v.0.1, set weight with user fonts to force synthetic bolding

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 2 Bill Gianopoulos [:WG9s] 2009-07-02 12:46:28 PDT
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.
Comment 3 John Daggett (:jtd) 2009-07-14 19:18:59 PDT
Created attachment 388611 [details] [diff] [review]
patch, v.0.2, maintain needsBold array

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.
Comment 4 John Daggett (:jtd) 2009-07-16 14:07:36 PDT
Created attachment 388998 [details] [diff] [review]
patch, v.0.3, fix lighter problem and add reftest

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 5 Jonathan Kew (:jfkthame) 2009-07-22 03:02:01 PDT
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?)
Comment 6 John Daggett (:jtd) 2009-07-23 14:26:12 PDT
Created attachment 390324 [details] [diff] [review]
patch, v.0.3a, changes based on review comments

Changes based on review comments and disable reftest on Linux (bug 419962).
Comment 7 John Daggett (:jtd) 2009-07-23 14:27:07 PDT
Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/b2d3327fb4cc
Comment 8 Jonathan Kew (:jfkthame) 2009-08-14 04:08:45 PDT
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?
Comment 9 John Daggett (:jtd) 2009-08-14 20:43:57 PDT
(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.
Comment 10 John Daggett (:jtd) 2009-08-14 20:53:42 PDT
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.
Comment 11 John Daggett (:jtd) 2009-08-14 21:00:53 PDT
Created attachment 394619 [details] [diff] [review]
follow-up patch, remove incorrect mFontNeedsBold reference

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 12 John Daggett (:jtd) 2009-08-25 10:02:24 PDT
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 13 Daniel Veditz [:dveditz] 2009-08-28 12:58:49 PDT
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 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-12-01 17:10:29 PST
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.

Note You need to log in before you can comment on or make changes to this bug.