Closed
Bug 499621
Opened 15 years ago
Closed 15 years ago
[@font-face] synthetic bolding not working for downloadable fonts under Windows
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jtd, Assigned: jtd)
References
()
Details
Attachments
(3 files, 2 obsolete files)
23.34 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
23.06 KB,
patch
|
Details | Diff | Splinter Review | |
930 bytes,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 3•15 years ago
|
||
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.
Attachment #384366 -
Attachment is obsolete: true
Assignee | ||
Comment 4•15 years ago
|
||
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.
Attachment #388611 -
Attachment is obsolete: true
Attachment #388998 -
Flags: review?(vladimir)
Attachment #388998 -
Flags: review?(vladimir) → review?(joe)
Assignee | ||
Updated•15 years ago
|
Attachment #388998 -
Flags: review?(joe) → review?(jfkthame)
Updated•15 years ago
|
Flags: wanted1.9.2?
Comment 5•15 years ago
|
||
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?)
Attachment #388998 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 6•15 years ago
|
||
Changes based on review comments and disable reftest on Linux (bug 419962).
Attachment #390324 -
Flags: approval1.9.1.3?
Assignee | ||
Comment 7•15 years ago
|
||
Pushed to trunk
http://hg.mozilla.org/mozilla-central/rev/b2d3327fb4cc
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 8•15 years ago
|
||
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?
Assignee | ||
Comment 9•15 years ago
|
||
(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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
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.
Attachment #394619 -
Flags: review?(jfkthame)
Updated•15 years ago
|
Attachment #394619 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 12•15 years ago
|
||
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.
Attachment #394619 -
Flags: approval1.9.2?
Comment 13•15 years ago
|
||
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?
Attachment #390324 -
Flags: approval1.9.1.3?
Comment 14•15 years ago
|
||
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.
Attachment #394619 -
Flags: approval1.9.2?
You need to log in
before you can comment on or make changes to this bug.
Description
•