Summary: | Unify font-variant-* with font-variant shorthand | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||
Component: | Text | Assignee: | 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
Myles C. Maxfield
2015-10-02 16:54:58 PDT
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. 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. 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() (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(). (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. 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 .... 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. (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? (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. Created attachment 266037 [details]
WIP
It's not obvious from inspection how font-variant: all works. The spec says that font-variant: small-caps in an @font-face declaration "does not affect font selection" Created attachment 266046 [details]
Parsing test
Created attachment 266067 [details]
Patch
Lots of editing failures. 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. 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 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. 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 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. 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
Created attachment 266072 [details]
Patch
(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. (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 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" Committed r192819: <http://trac.webkit.org/changeset/192819> Re-opened since this is blocked by bug 151681 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> Also fast/text/small-caps-web-font.html (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. (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. (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. (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. (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. (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. (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. (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. Created attachment 266400 [details]
Patch for committing
Created attachment 266475 [details]
Patch for committing
Committed r192992: <http://trac.webkit.org/changeset/192992> |