Bug 10649 – WebKit SVG needs SVG Fonts support

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

WebKit SVG needs SVG Fonts support

This is a rather low priority issue.  Although would be incredibly cool to have.  Web authors would love to be able to use custom fonts in their pages, especially if we had @font-face-like CSS/HTML integration of SVG fonts.

This is the master bug, I'll fill other smaller bugs to outline the various steps.  This bug covers actually turning on some very very minimal font support in WebKit.

[Eric Seidel (no email)]

I was thinking this weekend, that after all of WildFox's work on <text> and once the changes for <font-face> land this should be totally within the realm of possibility.  We just need to teach Font how to point off into the SVG DOM for it's glyphs and the SVG text painting code path how to use them.  Since I don't plan (initially) to modify the HTML code painting path, we'd have to hack the FontSelector code to avoid selecting SVG fonts for HTML.

[Eric Seidel (no email)]

Ok, now that bug 10652 is in, I'm ready to implement a real SVG fonts system.  I waded through the WebCore font system today and have come up with something resembling a plan.

Some background on the CSS @font-face system:
CSSFontFace elements own their CSSFontFaceSource elements, which in turn own a CachedFont object which does the actual loading.  CSSFontFaceSource elements provide the actual FontData objects into the font system (FontCache, etc).  Actual users hold onto Font objects, which build a FontFallbackList object behind the scenes, and try and grab the fonts in that list from the FontCache using the FontSelector.  The FontSelector (actually CSSFontSelector) which holds all the CSSFontFaces under a hash (by family name, weight, etc.) If the font selector has a CSSFontFace in its cache, it will try and grab the FontData from that CSSFontFace (which in turn tries to grab it off the CSSFontFaceSource).  If none of the standard variants for the font family/weight/etc. are found in the CSSFontSelector's hash, then it gives up, and the FontCache keeps walking down the FontFamily list (held by the FontFallbackList).  If none of those return anything, then there is a last resort font which is pulled from the platform.

So, that's how font lookup works... more or less, as I understand it.

I see SVG hooking into the system by subclassing from CSSFontFaceSource, an SVGFontFaceSource object, perhaps.  SVGFontFaceSource can handle loading/storing the font data (local or remote) and create the proper FontData subclass (SVGFontData) without calling out to any platform code.

The rest of WebCore is only really exposed to the Font object.  This will remain true in SVG Fonts.

Once a render has a Font object, it will eventually call Font::drawText to do the drawing.  Font::drawText makes a choice (based on the glyphs) as to which font path to go down. The "simple" or "complex" path.  SVG would require adding a 3rd path here.  This 3rd path would look identical to simple text drawing, except that the drawGlyphs call would use the glyph data out of the SVG font instead of the platform font data.

Providing advanced functionality for SVG fonts (like cascading style from current text run onto the actual glyphs) could be done at a later point.  (This, for example is, how SVG Fonts get coloring -- default fill is black, no stroke.  But SVG Fonts might come with their own intrinsic colors.  crazy, eh?)

[Eric Seidel (no email)]

Hum... one problem with this is that currently there is no way to get sources into the CSSFontFace except through CSSPropery values, as CSSFontFace is rebuilt from a CSSFontFaceRule every time the style selector is recalculated.

[Nikolas Zimmermann]

Created attachment 17397 [details]
Initial work-in-progress patch

First SVG fonts screenie:
http://flickr.com/photo_zoom.gne?id=2047231824&size=l

[Nikolas Zimmermann]

Created attachment 18072 [details]
Updated to ToT

Updating patch to work on ToT. Fixes several other things:
- Proper integration with the new SVG
- no more "// FIXME\n return true;" in Font::canUseGlyphCache, now proper implemented for SVG fonts
- Remove 'drawGlyphsWithSVGFont' instead created 'drawGlyphsRespectingSVGFont' function, which just calls drawGlyphs() for non-SVG builds, and also for SVG builds, if a non-SVG-font is used to render.

Only patched WebCore.xcodeproj build system, .pro & vcproj/ not updated anymore - will be done once this is commitable. (Wx doesn't support SVG builds at all)

M      WebKit/mac/MigrateHeaders.make
M      WebCore/DerivedSources.make
M      WebCore/platform/graphics/FontData.cpp
M      WebCore/platform/graphics/FontData.h
M      WebCore/platform/graphics/Font.cpp
M      WebCore/platform/graphics/Font.h
M      WebCore/bindings/js/JSSVGElementWrapperFactory.cpp
M      WebCore/bindings/objc/DOM.mm
M      WebCore/bindings/objc/DOMInternal.h
M      WebCore/bindings/objc/DOMSVG.h
M      WebCore/css/CSSFontFace.h
M      WebCore/css/CSSFontSelector.cpp
A      WebCore/css/SVGCSSFontFace.cpp
A      WebCore/css/SVGCSSFontFace.h
A      WebCore/svg/SVGFontElement.cpp
A      WebCore/svg/SVGFontElement.h
A      WebCore/svg/SVGFontElement.idl
M      WebCore/svg/SVGFontFaceElement.cpp
M      WebCore/svg/SVGFontFaceElement.h
M      WebCore/svg/SVGFontFaceFormatElement.h
A      WebCore/svg/SVGGlyphElement.cpp
A      WebCore/svg/SVGGlyphElement.h
A      WebCore/svg/SVGGlyphElement.idl
A      WebCore/svg/SVGMissingGlyphElement.cpp
A      WebCore/svg/SVGMissingGlyphElement.h
A      WebCore/svg/SVGMissingGlyphElement.idl
M      WebCore/svg/svgtags.in
M      WebCore/WebCore.xcodeproj/project.pbxproj

Comment on attachment 18072 [details]
Updated to ToT

I realize that the patch is not up for review yet so you may be aware of some of the following, but anyway:

+            fprintf(stderr, "drawGlyphsWithSVGFont !! i=%i, from=%i, to=%i\n", i, from, to);

Make sure to remove this.

+    // TODO: This is ridicolous. Glyphs shouldn't be collected when painted. The problem is

Typo: ridicolous

+    void drawGlyphsRespectingSVGFont(GraphicsContext*, const FontData*, const GlyphBuffer&, int from, int to, const FloatPoint&) const;

Seems better to me to keep drawGlyphs as the name of the method drawGlyphBuffer() calls and add whatever #ifdef'ed code needed there to check for the SVG font case and call out to a different function for that case if needed.

+#if PLATFORM(CG)
+            CGContextFillPath(context->platformContext());
+#endif

Probably not the right way to do this.

[Nikolas Zimmermann]

Hey mitz,

thanks for the pre-review :-)
Going to upload a new patch soon, all wrapped in ENABLE(SVG_FONTS) blocks - not influencing the default build anymore.

> I realize that the patch is not up for review yet so you may be aware of some
> of the following, but anyway:
> 
> +            fprintf(stderr, "drawGlyphsWithSVGFont !! i=%i, from=%i, to=%i\n",
> i, from, to);
> 
> Make sure to remove this.
I've outcommented all fprintfs on my hdd.
 
> +    // TODO: This is ridicolous. Glyphs shouldn't be collected when painted.
> The problem is
> 
> Typo: ridicolous
Fixed.
 
> +    void drawGlyphsRespectingSVGFont(GraphicsContext*, const FontData*, const
> GlyphBuffer&, int from, int to, const FloatPoint&) const;
> 
> Seems better to me to keep drawGlyphs as the name of the method
> drawGlyphBuffer() calls and add whatever #ifdef'ed code needed there to check
> for the SVG font case and call out to a different function for that case if
> needed.
You were reviewing the first patch, not the updated, as I already fixed it there?
 
> +#if PLATFORM(CG)
> +            CGContextFillPath(context->platformContext());
> +#endif
> 
> Probably not the right way to do this.
Right it's a hack, because the SVGPaintServer logic needs a RenderObject/RenderStyle to operate on. I had to choose a rather ugly solution to store a RenderObject pointer in the Font object, identifying the RenderObject which invoked the drawText method (in the SVG case, it will store a RenderText pointer) - you can comment on that once I uploaded the new patch. Maybe we can find a better solution.
 
Greetings,
Niko

[Nikolas Zimmermann]

Created attachment 18160 [details]
Updated patch

Final version of the first SVG Fonts patch. Updated all build systems - wrapped all new code in ENABLE(SVG_FONTS) blocks, so it's disabled by default (build-wekbit --svg-fonts enables it).

Comment on attachment 18160 [details]
Updated patch

+    mutable RenderObject* m_referencingRenderObject;

I don't think this transient data belongs in the permanent Font object. You can consider sticking it in TextRun, which may also allow you to get rid of Font::drawSVGText.

+            if (SVGPaintServer* fillPaintServer = SVGPaintServer::fillPaintServer(style, renderObject)) {
+            if (SVGPaintServer* strokePaintServer = SVGPaintServer::strokePaintServer(style, renderObject)) {

Is it really necessary to set up and tear down these every time through the loop?

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 18160 [details] [edit])
> +    mutable RenderObject* m_referencingRenderObject;
> 
> I don't think this transient data belongs in the permanent Font object. You can
> consider sticking it in TextRun, which may also allow you to get rid of
> Font::drawSVGText.
Ok, will check that.
 
> +            if (SVGPaintServer* fillPaintServer =
> SVGPaintServer::fillPaintServer(style, renderObject)) {
> +            if (SVGPaintServer* strokePaintServer =
> SVGPaintServer::strokePaintServer(style, renderObject)) {
> 
> Is it really necessary to set up and tear down these every time through the
> loop?
Nope, good catch.

Uploading new patch soon, after Eric checked it.

[Eric Seidel (no email)]

Comment on attachment 18160 [details]
Updated patch

We talked about various fixes over IRC, including some from mitz.  There was also a leak of getFontFace() which I remember.  I look forward to seeing the next patch.

[Nikolas Zimmermann]

Created attachment 18161 [details]
Updated patch v2

Incorporated all comments by Eric & Mitz.

[Eric Seidel (no email)]

Comment on attachment 18161 [details]
Updated patch v2

This looks like an OK first-cut to me.

I'm sad to see <font-face> being disabled for the normal build, but I guess it's OK.  <font-face> isn't actually that useful w/o <font> anyway.

[Nikolas Zimmermann]

(In reply to comment #13)
> (From update of attachment 18161 [details] [edit])
> This looks like an OK first-cut to me.
> 
> I'm sad to see <font-face> being disabled for the normal build, but I guess
> it's OK.  <font-face> isn't actually that useful w/o <font> anyway.

Thanks - let's hope we can enable SVG_FONTS by default soon.
Landing soon...

Niko

[Nikolas Zimmermann]

Landed in r29012.

[Eric Seidel (no email)]

*** Bug 10691 has been marked as a duplicate of this bug. ***