Bug 149773 – Unify font-variant-* with font-variant shorthand

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

Currently we have split-brain behavior.

[Radar WebKit Bug Importer]

<rdar://problem/22959952>

[Myles C. Maxfield]

The "font" shorthand also allows you to set font-variant-caps. Therefore, there is a three-way competition between properties which allow you to set small-caps -

font:
font-variant:
font-variant-caps

This competition should be resolved in the same way as the competition between any shorthand property and its constituent sub-properties.

[Myles C. Maxfield]

Font-variant currently only has one value that we support - small caps. Therefore, implementing synthesis for font-variant-caps will include this unification.

Therefore, this bug only includes the appropriate shorthand mechanism.

[Myles C. Maxfield]

We are going to have to unify our handling of font-variant in @font-face with the handling of font-variant-* in @font-face. The former is handled in CSSFontSelector's computeTraitsMask()

[Myles C. Maxfield]

(In reply to comment #4)
> We are going to have to unify our handling of font-variant in @font-face
> with the handling of font-variant-* in @font-face. The former is handled in
> CSSFontSelector's computeTraitsMask()

We place small-caps information in the traits mask, which is consulted in CSSFontSelector::getFontFace().

[Myles C. Maxfield]

(In reply to comment #5)
> (In reply to comment #4)
> > We are going to have to unify our handling of font-variant in @font-face
> > with the handling of font-variant-* in @font-face. The former is handled in
> > CSSFontSelector's computeTraitsMask()
> 
> We place small-caps information in the traits mask, which is consulted in
> CSSFontSelector::getFontFace().

It's used when we have a FontDescription, and we need to find a suitable @font-face (or locally-installed font) which matches it. We try to match the traits mask. I need to look up whether the font-variant-* properties should be consulted when selecting a font, or whether they are applied after selecting a font.

[Myles C. Maxfield]

Another monkey wrench - we allow @font-face to say "font-variant: all", which causes us to not consider font-variant when performing the font match (font-variant: all matches all font variants in any matching request)

We currently signal that by putting the value inside a list of 1 item ....

[Myles C. Maxfield]

We map from longhand property to shorthand property in matchingShorthandsForLonghand(). Because I'm going to need two shorthand properties for the same longhand, I'm going to have to do something custom for this.

[Myles C. Maxfield]

(In reply to comment #8)
> We map from longhand property to shorthand property in
> matchingShorthandsForLonghand(). Because I'm going to need two shorthand
> properties for the same longhand, I'm going to have to do something custom
> for this.

Maybe CSSParser::addProperty() does the right thing here?

[Myles C. Maxfield]

(In reply to comment #9)
> (In reply to comment #8)
> > We map from longhand property to shorthand property in
> > matchingShorthandsForLonghand(). Because I'm going to need two shorthand
> > properties for the same longhand, I'm going to have to do something custom
> > for this.
> 
> Maybe CSSParser::addProperty() does the right thing here?

Yeah it looks like it does the right thing.

[Myles C. Maxfield]

Created attachment 266037 [details]
WIP

[Myles C. Maxfield]

It's not obvious from inspection how font-variant: all works.

[Myles C. Maxfield]

The spec says that font-variant: small-caps in an @font-face declaration "does not affect font selection"

[Myles C. Maxfield]

Created attachment 266046 [details]
Parsing test

[Myles C. Maxfield]

Created attachment 266067 [details]
Patch

[Myles C. Maxfield]

Lots of editing failures.

[Build Bot]

Comment on attachment 266067 [details]
Patch

Attachment 266067 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/464789

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 266068 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266067 [details]
Patch

Attachment 266067 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/464794

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 266069 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 266067 [details]
Patch

Attachment 266067 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/464799

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 266070 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

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

[Myles C. Maxfield]

Created attachment 266072 [details]
Patch

[John Daggett]

(In reply to comment #13)
> The spec says that font-variant: small-caps in an @font-face declaration
> "does not affect font selection"

Right, it's not intended for font selection at all. The original CSS2 spec defined it as a way to indicate that a given font was the small-caps version of that font.

http://www.w3.org/TR/2008/REC-CSS2-20080411/fonts.html#descdef-font-variant

In CSS3 Fonts, the 'font-variant' descriptor defines font features enabled by default for a given face, enabling monospaced digits by default for example. Same for the font-feature-settings descriptor. Neither of these affect font selection.

[Myles C. Maxfield]

(In reply to comment #24)
> (In reply to comment #13)
> > The spec says that font-variant: small-caps in an @font-face declaration
> > "does not affect font selection"
> 
> Right, it's not intended for font selection at all. The original CSS2 spec
> defined it as a way to indicate that a given font was the small-caps version
> of that font.
> 
> http://www.w3.org/TR/2008/REC-CSS2-20080411/fonts.html#descdef-font-variant
> 
> In CSS3 Fonts, the 'font-variant' descriptor defines font features enabled
> by default for a given face, enabling monospaced digits by default for
> example. Same for the font-feature-settings descriptor. Neither of these
> affect font selection.

Way ahead of ya ;-) https://bugs.webkit.org/show_bug.cgi?id=151537

[Darin Adler]

Comment on attachment 266072 [details]
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1826
> +static Ref<CSSValue> fontVariantFromStyle(RenderStyle* style)

This should take RenderStyle&, not RenderStyle*.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2010
> +    return WTF::move(list);

I don’t think this WTF::move is needed or helpful.

> Source/WebCore/css/CSSFontSelector.cpp:307
> -    if (variantPosition)
> +    if (variantPosition && is<CSSPrimitiveValue>(*variantPosition))

The more terse way to write this is:

    if (is<CSSPrimitiveValue>(variantPosition))

> Source/WebCore/css/CSSFontSelector.cpp:310
> -    if (variantCaps)
> +    if (variantCaps && is<CSSPrimitiveValue>(*variantCaps))

Ditto.

> Source/WebCore/css/CSSFontSelector.cpp:316
> -    if (variantAlternates)
> +    if (variantAlternates && is<CSSPrimitiveValue>(*variantAlternates))

Ditto.

> Source/WebCore/css/CSSParser.cpp:3118
> +            return parseFontVariantLigatures(important, true, false);

Ugh. Is there no better way to do this? It’s unclear what “, true, false” means. This is write-only code.

> Source/WebCore/css/CSSParser.cpp:3124
> +            return parseFontVariantNumeric(important, true, false);

Ditto.

> Source/WebCore/css/CSSParser.cpp:3130
> +            return parseFontVariantEastAsian(important, true, false);

Ditto.

> LayoutTests/platform/mac/TestExpectations:1400
> +# Temporarily disable font-features tests unil synthesis for font-variant-caps is implemented.

Typo: "unil"

[Myles C. Maxfield]

Committed r192819: <http://trac.webkit.org/changeset/192819>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 151681

[Ryan Haddad]

This change broke the following layout tests on Windows:

css1/font_properties/font.html
css2.1/t1508-c527-font-04-b.html
css2.1/t1508-c527-font-05-b.html
css2.1/t1508-c527-font-07-b.html
css2.1/t1508-c527-font-10-c.html

Run:
<https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/55293>
Results:
<https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r192819%20(55293)/results.html>

[Myles C. Maxfield]

Also fast/text/small-caps-web-font.html

[Myles C. Maxfield]

(In reply to comment #30)
> Also fast/text/small-caps-web-font.html

Looks like the tests in css1/ and css2.1 should just be rebaselined. However, fast/text/small-caps-web-font.html looks like a real regression.

[Myles C. Maxfield]

(In reply to comment #31)
> (In reply to comment #30)
> > Also fast/text/small-caps-web-font.html
> 
> Looks like the tests in css1/ and css2.1 should just be rebaselined.
> However, fast/text/small-caps-web-font.html looks like a real regression.

This patch didn't cause the bad rendering of fast/text/small-caps-web-font.html; instead, it simply uncovered an existing problem with the complex text codepath & small caps on Windows.

[Myles C. Maxfield]

(In reply to comment #31)
> (In reply to comment #30)
> > Also fast/text/small-caps-web-font.html
> 
> Looks like the tests in css1/ and css2.1 should just be rebaselined.
> However, fast/text/small-caps-web-font.html looks like a real regression.

The rebaseline is necessary because small-caps now goes through the complex text codepath.

[Myles C. Maxfield]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > Also fast/text/small-caps-web-font.html
> > 
> > Looks like the tests in css1/ and css2.1 should just be rebaselined.
> > However, fast/text/small-caps-web-font.html looks like a real regression.
> 
> This patch didn't cause the bad rendering of
> fast/text/small-caps-web-font.html; instead, it simply uncovered an existing
> problem with the complex text codepath & small caps on Windows.

This problem is because, for web fonts, Font::platformCreateScaledFont() only modifies the size inside the FontPlatformData and doesn't touch the actual Windows platform font object. Then, UniscribeController only inspects the Windows platform font object to perform shaping and placement.

At drawing time, we set the font size using the FontPlatformData's size: CGContextSetFontSize(cgContext, platformData.size());

I still need to figure out how the simple text path works, and how OS X's complex path works.

[Myles C. Maxfield]

(In reply to comment #34)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #30)
> > > > Also fast/text/small-caps-web-font.html
> > > 
> > > Looks like the tests in css1/ and css2.1 should just be rebaselined.
> > > However, fast/text/small-caps-web-font.html looks like a real regression.
> > 
> > This patch didn't cause the bad rendering of
> > fast/text/small-caps-web-font.html; instead, it simply uncovered an existing
> > problem with the complex text codepath & small caps on Windows.
> 
> This problem is because, for web fonts, Font::platformCreateScaledFont()
> only modifies the size inside the FontPlatformData and doesn't touch the
> actual Windows platform font object. Then, UniscribeController only inspects
> the Windows platform font object to perform shaping and placement.
> 
> At drawing time, we set the font size using the FontPlatformData's size:
> CGContextSetFontSize(cgContext, platformData.size());
> 
> I still need to figure out how the simple text path works, and how OS X's
> complex path works.

The simple text codepath works because it's based entirely on Font::platformWidthForGlyph(), and that function uses the font size from the FontPlatformData.

[Myles C. Maxfield]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (In reply to comment #30)
> > > > > Also fast/text/small-caps-web-font.html
> > > > 
> > > > Looks like the tests in css1/ and css2.1 should just be rebaselined.
> > > > However, fast/text/small-caps-web-font.html looks like a real regression.
> > > 
> > > This patch didn't cause the bad rendering of
> > > fast/text/small-caps-web-font.html; instead, it simply uncovered an existing
> > > problem with the complex text codepath & small caps on Windows.
> > 
> > This problem is because, for web fonts, Font::platformCreateScaledFont()
> > only modifies the size inside the FontPlatformData and doesn't touch the
> > actual Windows platform font object. Then, UniscribeController only inspects
> > the Windows platform font object to perform shaping and placement.
> > 
> > At drawing time, we set the font size using the FontPlatformData's size:
> > CGContextSetFontSize(cgContext, platformData.size());
> > 
> > I still need to figure out how the simple text path works, and how OS X's
> > complex path works.
> 
> The simple text codepath works because it's based entirely on
> Font::platformWidthForGlyph(), and that function uses the font size from the
> FontPlatformData.

OS X works because we modify the OS X platform font object to incorporate the scale.

[Myles C. Maxfield]

(In reply to comment #36)
> (In reply to comment #35)
> > (In reply to comment #34)
> > > (In reply to comment #32)
> > > > (In reply to comment #31)
> > > > > (In reply to comment #30)
> > > > > > Also fast/text/small-caps-web-font.html
> > > > > 
> > > > > Looks like the tests in css1/ and css2.1 should just be rebaselined.
> > > > > However, fast/text/small-caps-web-font.html looks like a real regression.
> > > > 
> > > > This patch didn't cause the bad rendering of
> > > > fast/text/small-caps-web-font.html; instead, it simply uncovered an existing
> > > > problem with the complex text codepath & small caps on Windows.
> > > 
> > > This problem is because, for web fonts, Font::platformCreateScaledFont()
> > > only modifies the size inside the FontPlatformData and doesn't touch the
> > > actual Windows platform font object. Then, UniscribeController only inspects
> > > the Windows platform font object to perform shaping and placement.
> > > 
> > > At drawing time, we set the font size using the FontPlatformData's size:
> > > CGContextSetFontSize(cgContext, platformData.size());
> > > 
> > > I still need to figure out how the simple text path works, and how OS X's
> > > complex path works.
> > 
> > The simple text codepath works because it's based entirely on
> > Font::platformWidthForGlyph(), and that function uses the font size from the
> > FontPlatformData.
> 
> OS X works because we modify the OS X platform font object to incorporate
> the scale.

Even in old OS X versions, shaping can only occur with a CoreText font, and we bake in the FontPlatformData's size into the CoreText font when creating it.

[Myles C. Maxfield]

(In reply to comment #37)
> (In reply to comment #36)
> > (In reply to comment #35)
> > > (In reply to comment #34)
> > > > (In reply to comment #32)
> > > > > (In reply to comment #31)
> > > > > > (In reply to comment #30)
> > > > > > > Also fast/text/small-caps-web-font.html
> > > > > > 
> > > > > > Looks like the tests in css1/ and css2.1 should just be rebaselined.
> > > > > > However, fast/text/small-caps-web-font.html looks like a real regression.
> > > > > 
> > > > > This patch didn't cause the bad rendering of
> > > > > fast/text/small-caps-web-font.html; instead, it simply uncovered an existing
> > > > > problem with the complex text codepath & small caps on Windows.
> > > > 
> > > > This problem is because, for web fonts, Font::platformCreateScaledFont()
> > > > only modifies the size inside the FontPlatformData and doesn't touch the
> > > > actual Windows platform font object. Then, UniscribeController only inspects
> > > > the Windows platform font object to perform shaping and placement.
> > > > 
> > > > At drawing time, we set the font size using the FontPlatformData's size:
> > > > CGContextSetFontSize(cgContext, platformData.size());
> > > > 
> > > > I still need to figure out how the simple text path works, and how OS X's
> > > > complex path works.
> > > 
> > > The simple text codepath works because it's based entirely on
> > > Font::platformWidthForGlyph(), and that function uses the font size from the
> > > FontPlatformData.
> > 
> > OS X works because we modify the OS X platform font object to incorporate
> > the scale.
> 
> Even in old OS X versions, shaping can only occur with a CoreText font, and
> we bake in the FontPlatformData's size into the CoreText font when creating
> it.

I'm not sure small caps ever worked in the complex text codepath on Windows.

[Myles C. Maxfield]

Created attachment 266400 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 266475 [details]
Patch for committing

[Myles C. Maxfield]

Committed r192992: <http://trac.webkit.org/changeset/192992>

[yisibl]

https://bugs.webkit.org/show_bug.cgi?id=12530