- Revision
- 188377
- Author
- mmaxfi...@apple.com
- Date
- 2015-08-12 22:36:21 -0700 (Wed, 12 Aug 2015)
Log Message
[Cocoa] [CJK-configured device] System font has vertical punctuation
https://bugs.webkit.org/show_bug.cgi?id=147964
<rdar://problem/22256660>
Reviewed by Dean Jackson.
Source/WebCore:
GlyphPage::fill() has multiple code paths to accomplish its goal. It uses the shouldUseCoreText() helper
function to determine which one of the paths should be taken. However, not all of the code paths in
GlyphPage::fill() are able of handling all situations. Indeed, the CoreText code paths in GlyphPage::fill()
are only able to handle the situations which shouldUseCoreText() returns true for. This happens in the
following cases:
1. If the font is a composite font
2. If the font is used for text-combine
3. If the font has vertical glyphs
In r187693, I added one more case to this list: If the font is the system font. However, I failed to add
the necessary support to GlyphPage::fill() for this case. Becasue of this, we just happened to fall into
the case of vertical fonts (just by coincidence), which causes us to use
CTFontGetVerticalGlyphsForCharacters() instead of CTFontGetGlyphsForCharacters().
The solution is to adopt the same behavior we were using before r187693. Back then, we were using
CGFontGetGlyphsForUnichars(), which always returned horizontal glyphs. We should simply adopt this same
behavior, except in the Core Text case. Therefore, this patch is just a simple check to see if we are
using the system font when determining which Core Text function to use.
Test: fast/text/system-font-punctuation.html
* platform/graphics/FontDescription.h:
(WebCore::FontDescription::setWidthVariant):
* platform/graphics/FontPlatformData.h:
(WebCore::FontPlatformData::isForTextCombine):
* platform/graphics/mac/GlyphPageMac.cpp:
(WebCore::shouldUseCoreText):
(WebCore::GlyphPage::fill):
* rendering/RenderCombineText.cpp:
(WebCore::RenderCombineText::combineText):
LayoutTests:
Make sure punctuation isn't vertical.
* fast/text/system-font-punctuation.html: Added.
* platform/ios-simulator/fast/text/system-font-punctuation-expected.txt: Added
* platform/mac/fast/text/system-font-punctuation-expected.txt: Added
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (188376 => 188377)
--- trunk/LayoutTests/ChangeLog 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/LayoutTests/ChangeLog 2015-08-13 05:36:21 UTC (rev 188377)
@@ -1,3 +1,17 @@
+2015-08-12 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ [Cocoa] [CJK-configured device] System font has vertical punctuation
+ https://bugs.webkit.org/show_bug.cgi?id=147964
+ <rdar://problem/22256660>
+
+ Reviewed by Dean Jackson.
+
+ Make sure punctuation isn't vertical.
+
+ * fast/text/system-font-punctuation.html: Added.
+ * platform/ios-simulator/fast/text/system-font-punctuation-expected.txt: Added
+ * platform/mac/fast/text/system-font-punctuation-expected.txt: Added
+
2015-08-12 Alexey Proskuryakov <a...@apple.com>
Removing an expectation for a long fixed bug.
Added: trunk/LayoutTests/fast/text/system-font-punctuation.html (0 => 188377)
--- trunk/LayoutTests/fast/text/system-font-punctuation.html (rev 0)
+++ trunk/LayoutTests/fast/text/system-font-punctuation.html 2015-08-13 05:36:21 UTC (rev 188377)
@@ -0,0 +1,7 @@
+<!DOCTYPE>
+<html>
+<body>
+This test makes sure punctuation laid out with the system font does not use vertical glyphs. The test passes if the semicolon below looks like a regular horizontal semicolon (;) and is not sideways.
+<div style="font: 50px UICTFontTextStyleBody;">;</div>
+</body>
+</html>
Added: trunk/LayoutTests/platform/ios-simulator/fast/text/system-font-punctuation-expected.txt (0 => 188377)
--- trunk/LayoutTests/platform/ios-simulator/fast/text/system-font-punctuation-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/ios-simulator/fast/text/system-font-punctuation-expected.txt 2015-08-13 05:36:21 UTC (rev 188377)
@@ -0,0 +1,12 @@
+layer at (0,0) size 800x600
+ RenderView at (0,0) size 800x600
+layer at (0,0) size 800x600
+ RenderBlock {HTML} at (0,0) size 800x600
+ RenderBody {BODY} at (8,8) size 784x584
+ RenderBlock (anonymous) at (0,0) size 784x40
+ RenderText {#text} at (0,0) size 720x39
+ text run at (0,0) width 720: "This test makes sure punctuation laid out with the system font does not use vertical glyphs. The test passes if the"
+ text run at (0,20) width 527: "semicolon below looks like a regular horizontal semicolon (;) and is not sideways."
+ RenderBlock {DIV} at (0,40) size 784x62
+ RenderText {#text} at (0,0) size 14x61
+ text run at (0,0) width 14: ";"
Modified: trunk/Source/WebCore/ChangeLog (188376 => 188377)
--- trunk/Source/WebCore/ChangeLog 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/Source/WebCore/ChangeLog 2015-08-13 05:36:21 UTC (rev 188377)
@@ -1,3 +1,43 @@
+2015-08-12 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ [Cocoa] [CJK-configured device] System font has vertical punctuation
+ https://bugs.webkit.org/show_bug.cgi?id=147964
+ <rdar://problem/22256660>
+
+ Reviewed by Dean Jackson.
+
+ GlyphPage::fill() has multiple code paths to accomplish its goal. It uses the shouldUseCoreText() helper
+ function to determine which one of the paths should be taken. However, not all of the code paths in
+ GlyphPage::fill() are able of handling all situations. Indeed, the CoreText code paths in GlyphPage::fill()
+ are only able to handle the situations which shouldUseCoreText() returns true for. This happens in the
+ following cases:
+
+ 1. If the font is a composite font
+ 2. If the font is used for text-combine
+ 3. If the font has vertical glyphs
+
+ In r187693, I added one more case to this list: If the font is the system font. However, I failed to add
+ the necessary support to GlyphPage::fill() for this case. Becasue of this, we just happened to fall into
+ the case of vertical fonts (just by coincidence), which causes us to use
+ CTFontGetVerticalGlyphsForCharacters() instead of CTFontGetGlyphsForCharacters().
+
+ The solution is to adopt the same behavior we were using before r187693. Back then, we were using
+ CGFontGetGlyphsForUnichars(), which always returned horizontal glyphs. We should simply adopt this same
+ behavior, except in the Core Text case. Therefore, this patch is just a simple check to see if we are
+ using the system font when determining which Core Text function to use.
+
+ Test: fast/text/system-font-punctuation.html
+
+ * platform/graphics/FontDescription.h:
+ (WebCore::FontDescription::setWidthVariant):
+ * platform/graphics/FontPlatformData.h:
+ (WebCore::FontPlatformData::isForTextCombine):
+ * platform/graphics/mac/GlyphPageMac.cpp:
+ (WebCore::shouldUseCoreText):
+ (WebCore::GlyphPage::fill):
+ * rendering/RenderCombineText.cpp:
+ (WebCore::RenderCombineText::combineText):
+
2015-08-12 Jinyoung Hur <hur....@navercorp.com>
[WinCairo] Turn on WOFF font
Modified: trunk/Source/WebCore/platform/graphics/FontDescription.h (188376 => 188377)
--- trunk/Source/WebCore/platform/graphics/FontDescription.h 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/Source/WebCore/platform/graphics/FontDescription.h 2015-08-13 05:36:21 UTC (rev 188377)
@@ -124,7 +124,7 @@
void setIsSpecifiedFont(bool isSpecifiedFont) { m_isSpecifiedFont = isSpecifiedFont; }
void setOrientation(FontOrientation orientation) { m_orientation = orientation; }
void setNonCJKGlyphOrientation(NonCJKGlyphOrientation orientation) { m_nonCJKGlyphOrientation = orientation; }
- void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; }
+ void setWidthVariant(FontWidthVariant widthVariant) { m_widthVariant = widthVariant; } // Make sure new callers of this sync with FontPlatformData::isForTextCombine()!
void setLocale(const AtomicString&);
void setFeatureSettings(PassRefPtr<FontFeatureSettings> settings) { m_featureSettings = settings; }
void setFontSynthesis(FontSynthesis fontSynthesis) { m_fontSynthesis = fontSynthesis; }
Modified: trunk/Source/WebCore/platform/graphics/FontPlatformData.h (188376 => 188377)
--- trunk/Source/WebCore/platform/graphics/FontPlatformData.h 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/Source/WebCore/platform/graphics/FontPlatformData.h 2015-08-13 05:36:21 UTC (rev 188377)
@@ -137,6 +137,7 @@
bool isCompositeFontReference() const { return m_isCompositeFontReference; }
FontOrientation orientation() const { return m_orientation; }
FontWidthVariant widthVariant() const { return m_widthVariant; }
+ bool isForTextCombine() const { return widthVariant() != RegularWidth; } // Keep in sync with callers of FontDescription::setWidthVariant().
void setOrientation(FontOrientation orientation) { m_orientation = orientation; }
void setSyntheticOblique(bool syntheticOblique) { m_syntheticOblique = syntheticOblique; }
Modified: trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp (188376 => 188377)
--- trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/Source/WebCore/platform/graphics/mac/GlyphPageMac.cpp 2015-08-13 05:36:21 UTC (rev 188377)
@@ -42,9 +42,12 @@
static bool shouldUseCoreText(const UChar* buffer, unsigned bufferLength, const Font* fontData)
{
+ // This needs to be kept in sync with GlyphPage::fill(). Currently, the CoreText paths are not able to handle
+ // every situtation. Returning true from this function in a new situation will require you to explicitly add
+ // handling for that situation in the CoreText paths of GlyphPage::fill().
if (fontData->platformData().isCompositeFontReference() || fontData->isSystemFont())
return true;
- if (fontData->platformData().widthVariant() != RegularWidth || fontData->hasVerticalGlyphs()) {
+ if (fontData->platformData().isForTextCombine() || fontData->hasVerticalGlyphs()) {
// Ideographs don't have a vertical variant or width variants.
for (unsigned i = 0; i < bufferLength; ++i) {
if (!FontCascade::isCJKIdeograph(buffer[i]))
@@ -88,10 +91,12 @@
}
}
} else if (!fontData->platformData().isCompositeFontReference()) {
- if (fontData->platformData().widthVariant() == RegularWidth)
+ // Because we know the implementation of shouldUseCoreText(), if the font isn't for text combine and it isn't a system font,
+ // we know it must have vertical glyphs.
+ if (fontData->platformData().isForTextCombine() || fontData->isSystemFont())
+ CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
+ else
CTFontGetVerticalGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
- else
- CTFontGetGlyphsForCharacters(fontData->platformData().ctFont(), buffer, glyphs.data(), bufferLength);
// When buffer consists of surrogate pairs, CTFontGetVerticalGlyphsForCharacters and CTFontGetGlyphsForCharacters
// place the glyphs at indices corresponding to the first character of each pair.
ASSERT(!(bufferLength % length) && (bufferLength / length == 1 || bufferLength / length == 2));
Modified: trunk/Source/WebCore/rendering/RenderCombineText.cpp (188376 => 188377)
--- trunk/Source/WebCore/rendering/RenderCombineText.cpp 2015-08-13 05:30:21 UTC (rev 188376)
+++ trunk/Source/WebCore/rendering/RenderCombineText.cpp 2015-08-13 05:36:21 UTC (rev 188377)
@@ -117,7 +117,7 @@
// Need to try compressed glyphs.
static const FontWidthVariant widthVariants[] = { HalfWidth, ThirdWidth, QuarterWidth };
for (size_t i = 0 ; i < WTF_ARRAY_LENGTH(widthVariants) ; ++i) {
- description.setWidthVariant(widthVariants[i]);
+ description.setWidthVariant(widthVariants[i]); // When modifying this, make sure to keep it in sync with FontPlatformData::isForTextCombine()!
FontCascade compressedFont(description, style().fontCascade().letterSpacing(), style().fontCascade().wordSpacing());
compressedFont.update(fontSelector);
