WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
3315
CSS3: Implement @font-face rule (master bug)
https://bugs.webkit.org/show_bug.cgi?id=3315
Summary
CSS3: Implement @font-face rule (master bug)
Nicholas Shanks
Reported
2005-06-07 14:44:16 PDT
Bug to track implementation of the CSS2 @font-face rule.
Attachments
Snapshot of work in progress
(39.10 KB, patch)
2007-05-04 02:41 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Another snapshot (compiles at least)
(69.37 KB, patch)
2007-05-05 02:49 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
More work in progress
(70.44 KB, patch)
2007-05-05 03:49 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Work in progress
(73.25 KB, patch)
2007-05-05 16:06 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Make local() work
(80.44 KB, patch)
2007-05-05 21:50 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
And with this patch, downloadable fonts live.
(93.27 KB, patch)
2007-05-06 00:16 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Add support for format(), don't download urls that end with .eot
(99.18 KB, patch)
2007-05-06 14:59 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Start work on memory cleanup
(99.70 KB, patch)
2007-05-06 17:24 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Patch that cleans up the GlyphPageTreeNodes.
(105.23 KB, patch)
2007-05-06 21:31 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Patch that improves page load when custom fonts are present
(108.54 KB, patch)
2007-05-07 01:29 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Include cache statistic reporting for CachedFonts
(111.30 KB, patch)
2007-05-07 01:58 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Make GlyphPage refcounted.
(114.51 KB, patch)
2007-05-07 22:21 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Fix memory management model for the font selector.
(115.98 KB, patch)
2007-05-09 14:08 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Make other platforms build
(116.51 KB, patch)
2007-05-09 15:23 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Patch that can be applied to trunk on Mac only
(121.11 KB, patch)
2007-09-27 13:02 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Patch that implements font-face on Mac and Windows
(139.68 KB, patch)
2007-09-28 13:56 PDT
,
Dave Hyatt
eric
: review+
Details
Formatted Diff
Diff
Address mitz and eric comments
(142.54 KB, patch)
2007-10-01 10:06 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Maybe I should attach something that actually compiles.
(142.54 KB, patch)
2007-10-01 10:31 PDT
,
Dave Hyatt
no flags
Details
Formatted Diff
Diff
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Joost de Valk (AlthA)
Comment 1
2006-01-20 05:13:11 PST
This should get done :)
Nicholas Shanks
Comment 2
2007-04-16 10:07:09 PDT
Here are the corresponding Mozilla and KHTML bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=70132
http://bugs.kde.org/show_bug.cgi?id=128920
And here is some Qt code to download and use a font:
http://labs.trolltech.com/blogs/2006/08/04/fun-with-fonts/
Dave Hyatt
Comment 3
2007-05-04 02:41:19 PDT
Created
attachment 14324
[details]
Snapshot of work in progress What I've done so far (doesn't compile).
Dave Hyatt
Comment 4
2007-05-05 02:49:23 PDT
Created
attachment 14349
[details]
Another snapshot (compiles at least)
Dave Hyatt
Comment 5
2007-05-05 03:49:58 PDT
Created
attachment 14351
[details]
More work in progress This patch gets loading notification hooked up correctly. Fonts will now properly delay the display of a page in the same way that stylesheets do.
Dave Hyatt
Comment 6
2007-05-05 16:06:14 PDT
Created
attachment 14368
[details]
Work in progress Handle checking for the existence of local fonts to avoid downloading remote resources. Make sure to hash bold and italic variants separately so that they don't blow away their counterparts.
Dave Hyatt
Comment 7
2007-05-05 21:50:56 PDT
Created
attachment 14370
[details]
Make local() work This patch actually compiles and gets the remapping of local fonts completely working. Now a stylesheet can actually define what families to use for serif, sans-serif, etc.
Dave Hyatt
Comment 8
2007-05-06 00:16:36 PDT
Created
attachment 14373
[details]
And with this patch, downloadable fonts live. Downloadable fonts live! There's still some cleanup work to do. (1) Should look for the .eot extension and short-circuit EOT loads to prevent clashing with Internet Explorer most of the time. (2) Should add a FontData cache for various sizes to CSSFontFaceSources. (3) When CSSFontFaceSources die, they need to delete these FontDatas and prune the glyph tree. (4) The glyph tree should ideally be decorated with a count of deletable branches, so that pruning will be quick. (5) Other platforms need to have some stuff stubbed out to keep building, since there are a number of new methods/classes that require native implementations.
Dave Hyatt
Comment 9
2007-05-06 00:17:14 PDT
Also, don't try to apply this or build it, as WebKitSystemInterface had to be changed to allow for display/measuring with no NSFont present.
Nicholas Shanks
Comment 10
2007-05-06 00:23:33 PDT
Ahh, I was ust about to try :-) Does this allow one to set the -webkit-body font from CSS too?
Dave Hyatt
Comment 11
2007-05-06 00:30:15 PDT
No, that's just an internal value that we use when resetting tables. It's not important.
Nicholas Shanks
Comment 12
2007-05-06 00:48:45 PDT
I thought it was the font selected in the preferences, the one above monospace, and was used for rendering runs that have no author-specified font. (e.g. "<html>This text will be in the body font.") The reason I would like this to work with @font-face is so that I can restrict the unicode range of my body font, Gentium, to exclude Cyrillic characters, because many of the Cyrillic characters are missing from Gentium Italic, and a run of italic will often have odd characters revert to Lucida Grande. I would rather force Gentium never to be selected for Cyrillic so that this doesn't happen. It also has several missing characters used in Serbian and Old Church Slavonic, even in the upright, with the same ugly substitutions occuring.
Dave Hyatt
Comment 13
2007-05-06 00:56:21 PDT
I don't have unicode-range working yet anyway heh. :)
Nicholas Shanks
Comment 14
2007-05-06 01:04:44 PDT
Yep, I know. Just thinking ahead :-) One more thing that should be easy to do, a way to disable use of a font by websites (yet still have it enabled in FontBook) by setting the src to nil. You can also implement the remapping functions (e.g. Arial to Helvetica) as CSS now, instead of hard-coding them into WebKit.
Dave Hyatt
Comment 15
2007-05-06 14:59:12 PDT
Created
attachment 14377
[details]
Add support for format(), don't download urls that end with .eot This patch implements support for the format() qualifier as a way of avoiding the download of unsupported font format types. In addition, just for compatibility with WinIE, we go ahead and assume that URLs ending with ".eot" are Embedded OpenType and refuse to download them.
Dave Hyatt
Comment 16
2007-05-06 15:02:26 PDT
"You can also implement the remapping functions (e.g. Arial to Helvetica) as CSS now, instead of hard-coding them into WebKit." Yeah, that would be neat, except that I didn't make @font-face work in user agent sheets. The difficulty is that I have this CSSFontSelector object that is per-document, but font face rules in the user agent sheet are parsed before you have a document. I'm also a bit concerned that having a font selector instantiated will cause a slight performance hit (it's nice that in the common case the object is null, so a simple pointer check avoids doing any more work). I'll add that to the list of open issues.
Dave Hyatt
Comment 17
2007-05-06 15:06:34 PDT
"The reason I would like this to work with @font-face is so that I can restrict the unicode range of my body font, Gentium, to exclude Cyrillic characters." Added unicode-range to the list of open issues. I think I'll morph -webkit-body into what you suggest (I'll change the name to -webkit-default or -webkit-standard or something a little less HTML-centric in name).
Dave Hyatt
Comment 18
2007-05-06 17:24:06 PDT
Created
attachment 14381
[details]
Start work on memory cleanup This patch implements the FontData size cache for each CSSFontFaceSource. This allows repeated requests for the same size of custom font to avoid making extra font datas. New in this patch also is a general memory cleanup. All structures now delete themselves properly on document teardown. Custom fonts are now also deactivated the moment they are no longer referenced by a document. The one open issue with memory usage is that FontDatas need to be pruned out of the glyph tree. I even have a page that demonstrates the bug, since a new FontData can be malloc'd with the exact same address of an old one, and suddenly you start handing back the wrong glyphs.
Dave Hyatt
Comment 19
2007-05-06 21:31:04 PDT
Created
attachment 14383
[details]
Patch that cleans up the GlyphPageTreeNodes. This patch implements the cleanup of FontDatas in the GlyphPageTreeNode tree. Everything seems to clean up properly now. I implemented my "custom font count" optimization for avoiding branches of the tree that don't have custom fonts in them.
Dave Hyatt
Comment 20
2007-05-07 01:29:57 PDT
Created
attachment 14384
[details]
Patch that improves page load when custom fonts are present This patch is cool. Instead of holding up the display of the page, I now make a loading placeholder FontData*. Once the font does load, the placeholder is swapped out for the real thing. The placeholder font will measure but will not render. The overall effect is similar to what you might see if images had been used in the same place as the downloadable fonts.
Dave Hyatt
Comment 21
2007-05-07 01:58:31 PDT
Created
attachment 14385
[details]
Include cache statistic reporting for CachedFonts
Nicholas Shanks
Comment 22
2007-05-07 07:41:28 PDT
A document body is a body regardless of whether it's HTML or not. That's why TimBL chose <body> to represent it ;-) I don't think it needs changing to something else just because that other name is not an HTML element. It also makes it easier to understand for web authors, just leaving it as -webkit-body. FWIW Gecko makes users select either "serif" or "sans-serif" as the default body text, and then uses the font for that style, since all five generic families must be defined. "This patch is cool." :o) Regarding CSS3 being smelly, or however you phrased it on public-html, you can always implement it your preferred way, and then advocate that to the css3-web-fonts editors. With an existing implementation behind you, your opinions will be much stronger. But you also have to remain compatible with IE4-7 and Prince, the only extant implementations I know of. I have no idea how much of IE's design the W3C took on board, but Prince seems to follow CSS 2 closely AFAICT. And regarding restricting the unicode-range of a font, it would be lovely to have a unicode-range-exclusions property whereby I can just define U+400 to U+52F as excluded, and allow the font to be updated and add new codepoints, without having to update my CSS. Or alternativly allow an inverse() function for the unicode-range property (probably better for inheritance). Without either of these, I have to enumerate every codepoint in the font that I want to be allowed, a nightmare for large fonts.
Dave Hyatt
Comment 23
2007-05-07 17:30:24 PDT
Deleting the GlyphTreePageNode's m_page is bad here, since a child can be pointing to a parent's page. I think I'll need to fix glyph pages to be reference counted to resolve this.
Dave Hyatt
Comment 24
2007-05-07 22:21:39 PDT
Created
attachment 14408
[details]
Make GlyphPage refcounted. Glyph tree pruning doesn't work right unless glyph pages are reference counted. The code reads better with this change anyway! :)
Dave Hyatt
Comment 25
2007-05-07 22:25:32 PDT
Open issues list: (1) Need to support "all" value for font-weight, font-style as well as lists of values. (2) Need to cull out unsupported properties from @font-face rules. (3) Need to support font-size in the descriptor (4) Need to support unicode-range in the descriptor (5) Rename -webkit-body to -webkit-default and make it set on the document so that it's possible to override from a user sheet (6) Need to make sure other platforms build
mitz
Comment 26
2007-05-07 22:42:12 PDT
(In reply to
comment #24
)
> Created an attachment (id=14408) [edit] > Make GlyphPage refcounted. >
Isn't it enough to check for ownership by testing if you share the page with your parent?
Dave Hyatt
Comment 27
2007-05-07 22:52:54 PDT
While I think you are right, I think the code reads bettter with the reference counting in place (and it made the three spots that deleted pages read more nicely).
Dave Hyatt
Comment 28
2007-05-07 22:55:22 PDT
Actually I think this case gets you in trouble if you don't refcount: else if (parentPage && parentPage->owner() != m_parent) { // The page we're overriding may not be owned by our parent node. // This happens when our parent node provides no useful overrides // and just copies the pointer to an already-existing page (see // below). // // We want our override to be shared by all nodes that reference // that page to avoid duplication, and so standardize on having the // page's owner collect all the overrides. Call getChild on the // page owner with the desired font data (this will populate // the page) and then reference it. m_page = parentPage->owner()->getChild(fontData, pageNumber)->page(); } In the above case you aren't sharing a page with your parent but with, e.g., a cousin.
Dave Hyatt
Comment 29
2007-05-09 14:08:07 PDT
Created
attachment 14446
[details]
Fix memory management model for the font selector. This patch passes all the layout tests. An SVG layout test that tried to use an SVG font-face exposed a major bug in the lifetime of FontSelectors. I made the FontSelector reference counted (both by the CSSStyleSelector and by FontFallbackLists). The Font == operator now compares the font selectors in an old and new font and will return false if they are different. I also made sure to delete the font selector if it ends up not containing any real font-face rules (e.g., because none of the formats are supported).
Dave Hyatt
Comment 30
2007-05-09 15:23:45 PDT
Created
attachment 14448
[details]
Make other platforms build Fix a bunch of stupidity in CSSFontFaceSource where I was just using Mac code directly. Push it down behind FontCustomPlatformData. Add a few ifdefs to CachedFont for now so that every other platform can just ignore FontCustomPlatformData.
Dave Hyatt
Comment 31
2007-05-09 15:25:10 PDT
Comment on
attachment 14448
[details]
Make other platforms build This is ready for a first round of comments and suggestions.
Dave Hyatt
Comment 32
2007-05-09 15:26:47 PDT
I'm looking for review to land on the new feature branch.
Nicholas Shanks
Comment 33
2007-05-09 16:17:08 PDT
Hunk #9 of WebCore/WebCore.xcodeproj/project.pbxproj fails when applying to revision 21342. Can be overcome fairly easily using a text editor. Other hunks are offset by a few lines too. Will run build in the morning.
Dave Hyatt
Comment 34
2007-05-09 16:21:19 PDT
You still won't be able to build. I'm going to produce a patch that can be landed on trunk that updates WebKitSystemInterface to use the changed APIs. Then I'll make a new patch that should easily apply to trunk.
Nicholas Shanks
Comment 35
2007-05-09 16:34:46 PDT
Okay, noted. Be aware there is at least one other build problem: FontPlatformDataMac.mm is missing. It would also be considerate if you could have the bold synthesis use unsigned ints representing weight instead of a bool, to be compatible with
bug #6484
when that gets finished.
Dave Hyatt
Comment 36
2007-05-09 17:14:04 PDT
Comment on
attachment 14448
[details]
Make other platforms build Clear review bit, since I missed a file.
Dave Hyatt
Comment 37
2007-09-27 13:02:21 PDT
Created
attachment 16419
[details]
Patch that can be applied to trunk on Mac only This patch can be applied to trunk (for anyone interested in trying out font-face). I'm working now on making font-face work on Win32.
Dave Hyatt
Comment 38
2007-09-28 13:56:13 PDT
Created
attachment 16434
[details]
Patch that implements font-face on Mac and Windows This is ready for an initial review as basic examples now work on both Mac and Windows.
mitz
Comment 39
2007-09-28 15:04:26 PDT
Comment on
attachment 16434
[details]
Patch that implements font-face on Mac and Windows This is unused (in CSSFontFaceSource.h) + bool m_tableInvalid; (In CSSFontSelector::addFontFaceRule) I think this should be initialized to false! + bool foundLocal = true; WHOA! + if (familyName.lower() == "port credit") + printf("WHOA!\n"); Can this leak the old fontFace if you have another rule for the same family and style? + m_fonts.set(hashForFont(familyName.lower(), fontDescription.bold(), fontDescription.italic()), fontFace);
mitz
Comment 40
2007-09-28 15:48:55 PDT
Comment on
attachment 16434
[details]
Patch that implements font-face on Mac and Windows Additional comments... This needs at least a comment about the secret value 3 + ATSFontActivateFromMemory((void*)buffer->data(), buffer->size(), 3, kATSFontFormatUnspecified, NULL, kATSOptionFlagsDefault, &containerRef); Don't you mean (fontCount != 1) here? + // Don't support multiple fonts for now. + if (fontCount == 0) {
Eric Seidel (no email)
Comment 41
2007-09-30 13:03:48 PDT
I wonder if this should return true as long as one of the sources is valid? It seems getFontData() only cares about the first source which returns font data. bool CSSFontFace::isValid() const 57 { 58 unsigned size = m_sources.size(); 59 if (!size) 60 return false; 61 for (unsigned i = 0; i < size; i++) { 62 if (!m_sources[i]->isValid()) 63 return false; 64 } 65 return true; 66 }
Eric Seidel (no email)
Comment 42
2007-09-30 14:20:10 PDT
Comment on
attachment 16434
[details]
Patch that implements font-face on Mac and Windows Is there a Ptr type (like OwnPtr) which could make more clear the fact that: void addSource(CSSFontFaceSource*); takes ownership of the source? Yet another area of code where
bug 15331
would be useful. This comment makes the if feel backwards: // If we are still loading, then we let the system pick a font. I think you could just change the return type here to be AtomicString and it would still perform the same "uniquing" trick that you seem to be after: static String hashForFont(const String& familyName, bool bold, bool italic) 69 { 70 String familyHash(familyName); 71 if (bold) 72 familyHash += "-webkit-bold"; 73 if (italic) 74 familyHash += "-webkit-italic"; 75 return AtomicString(familyHash); 76 } I believe these .get() calls are unnecessary: 84 if (!fontFamily.get() || !src.get() || !fontFamily->isValueList() || !src->isValueList()) AFAIK RefPtr has a ! operator, as well as should be able to convert to a bool automagically for things like ptr && otherPtr. I would almost write a "static bool isNonEmptyValueList(CSSValue*)" function to perform the checks, since it seems to take several lines each. But such is certainly not necessary. Typo: 251 else if (familyName == "-webkit-fantasy") 252 genericFamily = settings->cursiveFontFamily(); It's hard to imagine we don't have a cleaner way to express this with the string classes: 2480 parsedValue = new CSSFontFaceSrcValue(String(KURL(styleElement->baseURL().deprecatedString(), value.deprecatedString()).url()), m_systemFallbackChild must have been leaking before and we didn't notice. The deletion in the destructor is new in this patch. Isn't there a CFRetainPtr or something? I could have sworn... if (f && objc_collecting_enabled()) 34 CFRetain(f); // fix for <
rdar://problems/4223619
> That's all. Other than Mitz's comments, it looks great. I very much look forward to seeing this on feature-branch. :)
Dave Hyatt
Comment 43
2007-10-01 10:06:10 PDT
Created
attachment 16483
[details]
Address mitz and eric comments This patch addresses comments. I don't think the hash can leak, Mitz, since it's a hash of RefPtrs?
Dave Hyatt
Comment 44
2007-10-01 10:31:35 PDT
Created
attachment 16484
[details]
Maybe I should attach something that actually compiles.
Eric Seidel (no email)
Comment 45
2007-10-02 09:41:41 PDT
Ahha! There is actually a RetainPtr, which you could certainly use in this patch to get rid of the need for explicit CFRetain/CFRelease. RetainPtr.h is the file in question.
Darin Adler
Comment 46
2007-10-04 20:07:34 PDT
(In reply to
comment #42
)
> Is there a Ptr type (like OwnPtr) which could make more clear the fact that: > void addSource(CSSFontFaceSource*); > takes ownership of the source?
Yes, std::auto_ptr.
Dave Hyatt
Comment 47
2007-10-04 23:38:01 PDT
Fixed on feature branch.
Nicholas Shanks
Comment 48
2008-09-25 12:14:14 PDT
Dave: how many of the to-dos mentioned in
comment #25
are left open? Have you filed bugs for each of them?
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug