Support kerning with SVG web fonts

• Component: Text
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

When an SVG font is used as a web font, it will not use the regular SVG layout engine but instead go through the fast font path. While there is support for kerning of platform fonts in the fast path now, there is still support missing for SVG fonts.

Note that kerning in SVG fonts is currently also absurdly slow and will need to be greatly improved since SVG web fonts are used on many website since it is the only web font allowed on iOS.

[Allan Sandfeld Jensen]

Created attachment 204433 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204433 [details]
Patch

Attachment 204433 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/809185

[EFL EWS Bot]

Comment on attachment 204433 [details]
Patch

Attachment 204433 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/652919

[Allan Sandfeld Jensen]

Created attachment 204437 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204437 [details]
Patch

Attachment 204437 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/760450

[EFL EWS Bot]

Comment on attachment 204437 [details]
Patch

Attachment 204437 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/827091

[Allan Sandfeld Jensen]

Created attachment 204440 [details]
Patch

Add handling of Cairo glyphs

[Stephen Chenney]

Comment on attachment 204433 [details]
Patch

Mid-air collision so not sure on line numbers etc. Where are the tests?

View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:103
> +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace)

Why not make this non-static since you now need the "this" object?

> Source/WebCore/svg/SVGFontElement.cpp:243
> +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))

return (a || b || c);

> Source/WebCore/svg/SVGFontElement.cpp:256
> +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))

return d && !(a | b | c)

[Allan Sandfeld Jensen]

(In reply to comment #8)
> (From update of attachment 204433 [details])
> Mid-air collision so not sure on line numbers etc. Where are the tests?
> 
I haven't figured out a way to test this reliably cross-port. A pixel-test is obviously possible, but I would prefer a layout or reference test.

> View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review
> 
> > Source/WebCore/platform/graphics/WidthIterator.cpp:103
> > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace)
> 
> Why not make this non-static since you now need the "this" object?
> 

That was my original plan, but would require moving the definitions of OriginalAdvancesForCharacterTreatedAsSpace and CharactersTreatedAsSpace to the header file, so the end result wouldn't be any cleaner netto.

> > Source/WebCore/svg/SVGFontElement.cpp:243
> > +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))
> 
> return (a || b || c);
> 
> > Source/WebCore/svg/SVGFontElement.cpp:256
> > +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))
> 
> return d && !(a | b | c)

That would make it cleaner I agree.

[Alexey Proskuryakov]

Is this the same as bug 92949?

[Allan Sandfeld Jensen]

(In reply to comment #10)
> Is this the same as bug 92949?

Yes, would be the same bug.

[Stephen Chenney]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 204433 [details] [details])
> > Mid-air collision so not sure on line numbers etc. Where are the tests?
> > 
> I haven't figured out a way to test this reliably cross-port. A pixel-test is obviously possible, but I would prefer a layout or reference test.

In my experience it's almost impossible to test this kind of thing using a ref test, because different platforms will still render things at slightly different locations, enough to make it hard to generate a reliable reference.

I think you'll need a pixel test, much as it pains me to say it. Someone from an actual WebKit using team should weigh in (I'm on Blink these days).
 
> > View in context: https://bugs.webkit.org/attachment.cgi?id=204433&action=review
> > 
> > > Source/WebCore/platform/graphics/WidthIterator.cpp:103
> > > +static inline float applyFontTransforms(GlyphBuffer* glyphBuffer, bool ltr, int& lastGlyphCount, const SimpleFontData* fontData, WidthIterator& iterator, TypesettingFeatures typesettingFeatures, CharactersTreatedAsSpace& charactersTreatedAsSpace)
> > 
> > Why not make this non-static since you now need the "this" object?
> > 
> 
> That was my original plan, but would require moving the definitions of OriginalAdvancesForCharacterTreatedAsSpace and CharactersTreatedAsSpace to the header file, so the end result wouldn't be any cleaner netto.

Fair enough.
 
> > > Source/WebCore/svg/SVGFontElement.cpp:243
> > > +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))
> > 
> > return (a || b || c);
> > 
> > > Source/WebCore/svg/SVGFontElement.cpp:256
> > > +        && !stringMatchesUnicodeRange(u2, svgKerning.unicodeRange2))
> > 
> > return d && !(a | b | c)
> 
> That would make it cleaner I agree.

[Allan Sandfeld Jensen]

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

[Allan Sandfeld Jensen]

Created attachment 204572 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204572 [details]
Patch

Attachment 204572 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/846103

[EFL EWS Bot]

Comment on attachment 204572 [details]
Patch

Attachment 204572 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/849118

[Allan Sandfeld Jensen]

Created attachment 204574 [details]
Patch

Sign of iterator again

[Build Bot]

Comment on attachment 204574 [details]
Patch

Attachment 204574 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/858194

New failing tests:
fast/text/svg-font-face-with-kerning.html

[Build Bot]

Created attachment 204645 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204574 [details]
Patch

Attachment 204574 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/851261

New failing tests:
fast/text/svg-font-face-with-kerning.html

[Build Bot]

Created attachment 204681 [details]
Archive of layout-test-results from APPLE-EWS-2 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-2  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Allan Sandfeld Jensen]

(In reply to comment #18)
> (From update of attachment 204574 [details])
> Attachment 204574 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/858194
> 
> New failing tests:
> fast/text/svg-font-face-with-kerning.html

I should note, that the fails are missing baselines. The baseline is intentional missing and not marked as missing, because I will let the bots created the baseline and land that using garden-o-matic, but the bots will not create new pixel baselines for tests marked 'missing', so I will have to land the test failing in order to get the correct baselines after landing.

[Allan Sandfeld Jensen]

Comment on attachment 204574 [details]
Patch

Fails due to missing baseline is intentional.

[Allan Sandfeld Jensen]

Created attachment 205488 [details]
Patch

Uploaded basic test results. Hopefully it will match since it is SVG rendering and not system font rendering.

[Allan Sandfeld Jensen]

With test and test expections, is there somebody who want to review the patch now?  Or are there still issues to be solved?

[Allan Sandfeld Jensen]

Ping for review?

[Stephen Chenney]

Comment on attachment 205488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review

One clarification requested. Otherwise good to go.

> Source/WebCore/platform/graphics/WidthIterator.cpp:122
> +    if (fontData->isSVGFont()) {

Shouldn't this be "if (fontData->isSVGFont() && (typesettingFeatures & Kerning))" so that the applyTransforms gets called like it was previously? Or is that what the comment implies?

As it is SimpleFontData::applyTransforms just returns false, and a code search in my checkout does not reveal any other implementations.

[Allan Sandfeld Jensen]

(In reply to comment #27)
> (From update of attachment 205488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review
> 
> One clarification requested. Otherwise good to go.
> 
> > Source/WebCore/platform/graphics/WidthIterator.cpp:122
> > +    if (fontData->isSVGFont()) {
> 
> Shouldn't this be "if (fontData->isSVGFont() && (typesettingFeatures & Kerning))" so that the applyTransforms gets called like it was previously? Or is that what the comment implies?
> 
applySVGKerning is supposed to replace applyTransforms, but should not be called if only ligatures but not kerning is enabled.

> As it is SimpleFontData::applyTransforms just returns false, and a code search in my checkout does not reveal any other implementations.

ApplyTransforms used to have an ASSERT against handling SVG fonts but now just bails out. I would prefer not to calling it rather than relying on the method bailing out when asked to do something it can not handle.

[Allan Sandfeld Jensen]

(In reply to comment #27)
> (From update of attachment 205488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review
> 
> Or is that what the comment implies?
> 
The comment is just an explaination for why ligatures are not handled here. Since ligatures are handled as part of the glyph selection algorithm, when we have list of glyphe instead of a list of characters, any ligature transformation should already have been handled.

[Allan Sandfeld Jensen]

Committed r156393: <http://trac.webkit.org/changeset/156393>

[Csaba Osztrogonác]

Comment on attachment 205488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review

> Source/WebCore/platform/graphics/WidthIterator.cpp:125
> +            iterator.run().renderingContext()->applySVGKerning(fontData, iterator, glyphBuffer, lastGlyphCount);

It broke the !ENALBE(SVG_FONTS) build, because applySVGKerning is inside in this block:
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp: In function 'float WebCore::applyFontTransforms(WebCore::GlyphBuffer*, bool, int&, const WebCore::SimpleFontData*, WebCore::WidthIterator&, WebCore::TypesettingFeatures, WebCore::CharactersTreatedAsSpace&)':
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp:129:48: error: 'class WebCore::TextRun::RenderingContext' has no member named 'applySVGKerning'

[Allan Sandfeld Jensen]

(In reply to comment #31)
> (From update of attachment 205488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205488&action=review
> 
> > Source/WebCore/platform/graphics/WidthIterator.cpp:125
> > +            iterator.run().renderingContext()->applySVGKerning(fontData, iterator, glyphBuffer, lastGlyphCount);
> 
> It broke the !ENALBE(SVG_FONTS) build, because applySVGKerning is inside in this block:
> /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp: In function 'float WebCore::applyFontTransforms(WebCore::GlyphBuffer*, bool, int&, const WebCore::SimpleFontData*, WebCore::WidthIterator&, WebCore::TypesettingFeatures, WebCore::CharactersTreatedAsSpace&)':
> /ramdisk/qt-linux-release-minimal/build/Source/WebCore/platform/graphics/WidthIterator.cpp:129:48: error: 'class WebCore::TextRun::RenderingContext' has no member named 'applySVGKerning'

Right, fix commited in https://trac.webkit.org/r156399