Bug 149773

Summary: Unify font-variant-* with font-variant shorthand
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: 50167214, buildbot, commit-queue, darin, dino, hyatt, jdaggett, jonlee, rniwa, ryanhaddad, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 151698, 151537, 151681    
Bug Blocks: 149779    
Attachments:
Description Flags
WIP
none
Parsing test
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews112 for mac-yosemite
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Patch
darin: review+
Patch for committing
none
Patch for committing none

Description Myles C. Maxfield 2015-10-02 16:54:58 PDT
Currently we have split-brain behavior.
Comment 1 Radar WebKit Bug Importer 2015-10-02 17:23:29 PDT
<rdar://problem/22959952>
Comment 2 Myles C. Maxfield 2015-11-12 15:49:05 PST
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.
Comment 3 Myles C. Maxfield 2015-11-19 18:17:03 PST
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.
Comment 4 Myles C. Maxfield 2015-11-21 18:43:52 PST
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()
Comment 5 Myles C. Maxfield 2015-11-21 18:48:30 PST
(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().
Comment 6 Myles C. Maxfield 2015-11-21 18:54:57 PST
(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.
Comment 7 Myles C. Maxfield 2015-11-21 19:14:34 PST
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 ....
Comment 8 Myles C. Maxfield 2015-11-21 19:36:31 PST
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.
Comment 9 Myles C. Maxfield 2015-11-21 19:38:37 PST
(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?
Comment 10 Myles C. Maxfield 2015-11-21 19:39:56 PST
(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.
Comment 11 Myles C. Maxfield 2015-11-21 19:59:36 PST
Created attachment 266037 [details]
WIP
Comment 12 Myles C. Maxfield 2015-11-21 20:11:21 PST
It's not obvious from inspection how font-variant: all works.
Comment 13 Myles C. Maxfield 2015-11-21 20:25:45 PST
The spec says that font-variant: small-caps in an @font-face declaration "does not affect font selection"
Comment 14 Myles C. Maxfield 2015-11-21 22:30:42 PST
Created attachment 266046 [details]
Parsing test
Comment 15 Myles C. Maxfield 2015-11-22 13:28:13 PST
Created attachment 266067 [details]
Patch
Comment 16 Myles C. Maxfield 2015-11-22 13:33:40 PST
Lots of editing failures.
Comment 17 Build Bot 2015-11-22 14:11:08 PST
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.
Comment 18 Build Bot 2015-11-22 14:11:10 PST
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
Comment 19 Build Bot 2015-11-22 14:12:06 PST
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.
Comment 20 Build Bot 2015-11-22 14:12:10 PST
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
Comment 21 Build Bot 2015-11-22 14:15:05 PST
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.
Comment 22 Build Bot 2015-11-22 14:15:08 PST
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
Comment 23 Myles C. Maxfield 2015-11-22 15:59:21 PST
Created attachment 266072 [details]
Patch
Comment 24 John Daggett 2015-11-24 20:05:24 PST
(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.
Comment 25 Myles C. Maxfield 2015-11-25 11:21:08 PST
(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
Comment 26 Darin Adler 2015-11-30 09:36:34 PST
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"
Comment 27 Myles C. Maxfield 2015-11-30 13:20:44 PST
Committed r192819: <http://trac.webkit.org/changeset/192819>
Comment 28 WebKit Commit Bot 2015-11-30 16:06:34 PST
Re-opened since this is blocked by bug 151681
Comment 29 Ryan Haddad 2015-11-30 16:07:43 PST
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>
Comment 30 Myles C. Maxfield 2015-11-30 19:03:07 PST
Also fast/text/small-caps-web-font.html
Comment 31 Myles C. Maxfield 2015-11-30 21:24:00 PST
(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.
Comment 32 Myles C. Maxfield 2015-11-30 22:33:50 PST
(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.
Comment 33 Myles C. Maxfield 2015-11-30 22:58:40 PST
(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.
Comment 34 Myles C. Maxfield 2015-11-30 23:07:48 PST
(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.
Comment 35 Myles C. Maxfield 2015-11-30 23:11:04 PST
(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.
Comment 36 Myles C. Maxfield 2015-11-30 23:18:44 PST
(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.
Comment 37 Myles C. Maxfield 2015-11-30 23:20:40 PST
(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.
Comment 38 Myles C. Maxfield 2015-11-30 23:24:38 PST
(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.
Comment 39 Myles C. Maxfield 2015-12-01 15:04:25 PST
Created attachment 266400 [details]
Patch for committing
Comment 40 Myles C. Maxfield 2015-12-02 14:58:07 PST
Created attachment 266475 [details]
Patch for committing
Comment 41 Myles C. Maxfield 2015-12-02 20:08:56 PST
Committed r192992: <http://trac.webkit.org/changeset/192992>