Log Message
Correct spacing regression on inter-element complex path shaping on some fonts https://bugs.webkit.org/show_bug.cgi?id=166013
Reviewed by Simon Fraser.
Source/WebCore:
This patch brings the implementation of ComplexTextController in-line with the
design at https://trac.webkit.org/wiki/ComplexTextController. Previously,
ComplexTextController had a few problems:
- The total width computed by ComplexTextController didn't match the width if
you iterated over the entire string and added up the advances
- FontCascade::getGlyphsAndAdvancesForComplexText() tried to compensate for
the above by construing the concepts of paint advances as distinct from layout
advances
- Initial advances were considered part of layout sometimes and part of painting
other times, depending on which function reports the information
- For RTL runs, the wrong origin was added to the initial advance, and the origin
should have been subtracted instead of added
This patch exhaustively updates every function in ComplexTextController to be
consistent with the design linked to above. This design solves all of these
problems.
Tests: ComplexTextControllerTest.InitialAdvanceWithLeftRunInRTL
ComplexTextControllerTest.InitialAdvanceInRTL
ComplexTextControllerTest.InitialAdvanceWithLeftRunInLTR
ComplexTextControllerTest.InitialAdvanceInLTR
ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
ComplexTextControllerTest.LeadingExpansion
ComplexTextControllerTest.VerticalAdvances
* platform/graphics/GlyphBuffer.h:
(WebCore::GlyphBuffer::setLeadingExpansion): Deleted. No longer necessary.
(WebCore::GlyphBuffer::leadingExpansion): Deleted. Ditto.
* platform/graphics/cocoa/FontCascadeCocoa.mm:
(WebCore::FontCascade::adjustSelectionRectForComplexText): Removed use of
unnecessary leadingExpansion().
(WebCore::FontCascade::getGlyphsAndAdvancesForComplexText): This function needs
to compute paint advances, which means that it can't base this information off
of layout advances. This function uses the trick mentioned at the end of the
above link to compute the paint offset of an arbitrary glyph in the middle of
an RTL run.
* platform/graphics/mac/ComplexTextController.cpp:
(WebCore::ComplexTextController::computeExpansionOpportunity): Refactored for
testing.
(WebCore::ComplexTextController::ComplexTextController): Ditto.
(WebCore::ComplexTextController::finishConstruction): Ditto.
(WebCore::ComplexTextController::offsetForPosition): This function operates on
layout advances, and the initial layout advance is already added into the
m_adjustedBaseAdvances vector by adjustGlyphsAndAdvances(). Therefore, there is
no need to add it again here.
(WebCore::ComplexTextController::advance): This function had completely busted
logic about the relationship between initial advances and the first origin in
each run. Because of the fortunate choice of only representing layout advances
in m_adjustedBaseAdvances, this entire block can be removed and the raw paint
initial advance can be reported to the GlyphBuffer. Later in the function, we
have to update the logic about how to compute a paint advance given a layout
advance and some origins. In particular, there are two tricky pieces here: 1.
The layout advance for the first glyph is equal to (initial advance - first
origin + first Core Text advance, so computing the paint offset must cancel out
the initial layout offset, and 2. the last paint advance in a run must actually
end up at the position of the first glyph in the next run, so the next run's
initial advance must be queried.
(WebCore::ComplexTextController::adjustGlyphsAndAdvances): Previously, we
represented an initial advance of a successive run by just adding it to the
previous run's last advance. However, this is incompatible with the new model
presented in the link above, so we remove this section. We also have to add in
the logic that the layout advance for the first glyph is equal to the formula
presented above.
* platform/graphics/mac/ComplexTextController.h:
(WebCore::ComplexTextController::ComplexTextRun::initialAdvance): Adjust comment
to reflect reality.
(WebCore::ComplexTextController::leadingExpansion): Deleted.
Tools:
Unskip existing tests and make some new tests:
- Testing complex text with no origins
- Testing initial expansions
- Testing the sign of vertical advances
* TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:
(TestWebKitAPI::TEST_F):
Modified Paths
- trunk/Source/WebCore/ChangeLog
- trunk/Source/WebCore/platform/graphics/GlyphBuffer.h
- trunk/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm
- trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp
- trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.h
- trunk/Tools/ChangeLog
- trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp
Diff
Modified: trunk/Source/WebCore/ChangeLog (211381 => 211382)
--- trunk/Source/WebCore/ChangeLog 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Source/WebCore/ChangeLog 2017-01-30 20:43:14 UTC (rev 211382)
@@ -1,3 +1,78 @@
+2017-01-30 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ Correct spacing regression on inter-element complex path shaping on some fonts
+ https://bugs.webkit.org/show_bug.cgi?id=166013
+
+ Reviewed by Simon Fraser.
+
+ This patch brings the implementation of ComplexTextController in-line with the
+ design at https://trac.webkit.org/wiki/ComplexTextController. Previously,
+ ComplexTextController had a few problems:
+ - The total width computed by ComplexTextController didn't match the width if
+ you iterated over the entire string and added up the advances
+ - FontCascade::getGlyphsAndAdvancesForComplexText() tried to compensate for
+ the above by construing the concepts of paint advances as distinct from layout
+ advances
+ - Initial advances were considered part of layout sometimes and part of painting
+ other times, depending on which function reports the information
+ - For RTL runs, the wrong origin was added to the initial advance, and the origin
+ should have been subtracted instead of added
+
+ This patch exhaustively updates every function in ComplexTextController to be
+ consistent with the design linked to above. This design solves all of these
+ problems.
+
+ Tests: ComplexTextControllerTest.InitialAdvanceWithLeftRunInRTL
+ ComplexTextControllerTest.InitialAdvanceInRTL
+ ComplexTextControllerTest.InitialAdvanceWithLeftRunInLTR
+ ComplexTextControllerTest.InitialAdvanceInLTR
+ ComplexTextControllerTest.InitialAdvanceInRTLNoOrigins
+ ComplexTextControllerTest.LeadingExpansion
+ ComplexTextControllerTest.VerticalAdvances
+
+ * platform/graphics/GlyphBuffer.h:
+ (WebCore::GlyphBuffer::setLeadingExpansion): Deleted. No longer necessary.
+ (WebCore::GlyphBuffer::leadingExpansion): Deleted. Ditto.
+ * platform/graphics/cocoa/FontCascadeCocoa.mm:
+ (WebCore::FontCascade::adjustSelectionRectForComplexText): Removed use of
+ unnecessary leadingExpansion().
+ (WebCore::FontCascade::getGlyphsAndAdvancesForComplexText): This function needs
+ to compute paint advances, which means that it can't base this information off
+ of layout advances. This function uses the trick mentioned at the end of the
+ above link to compute the paint offset of an arbitrary glyph in the middle of
+ an RTL run.
+ * platform/graphics/mac/ComplexTextController.cpp:
+ (WebCore::ComplexTextController::computeExpansionOpportunity): Refactored for
+ testing.
+ (WebCore::ComplexTextController::ComplexTextController): Ditto.
+ (WebCore::ComplexTextController::finishConstruction): Ditto.
+ (WebCore::ComplexTextController::offsetForPosition): This function operates on
+ layout advances, and the initial layout advance is already added into the
+ m_adjustedBaseAdvances vector by adjustGlyphsAndAdvances(). Therefore, there is
+ no need to add it again here.
+ (WebCore::ComplexTextController::advance): This function had completely busted
+ logic about the relationship between initial advances and the first origin in
+ each run. Because of the fortunate choice of only representing layout advances
+ in m_adjustedBaseAdvances, this entire block can be removed and the raw paint
+ initial advance can be reported to the GlyphBuffer. Later in the function, we
+ have to update the logic about how to compute a paint advance given a layout
+ advance and some origins. In particular, there are two tricky pieces here: 1.
+ The layout advance for the first glyph is equal to (initial advance - first
+ origin + first Core Text advance, so computing the paint offset must cancel out
+ the initial layout offset, and 2. the last paint advance in a run must actually
+ end up at the position of the first glyph in the next run, so the next run's
+ initial advance must be queried.
+ (WebCore::ComplexTextController::adjustGlyphsAndAdvances): Previously, we
+ represented an initial advance of a successive run by just adding it to the
+ previous run's last advance. However, this is incompatible with the new model
+ presented in the link above, so we remove this section. We also have to add in
+ the logic that the layout advance for the first glyph is equal to the formula
+ presented above.
+ * platform/graphics/mac/ComplexTextController.h:
+ (WebCore::ComplexTextController::ComplexTextRun::initialAdvance): Adjust comment
+ to reflect reality.
+ (WebCore::ComplexTextController::leadingExpansion): Deleted.
+
2017-01-30 Simon Fraser <simon.fra...@apple.com>
Fixed elements should not rubber-band in WK2, nor remain at negative offsets
Modified: trunk/Source/WebCore/platform/graphics/GlyphBuffer.h (211381 => 211382)
--- trunk/Source/WebCore/platform/graphics/GlyphBuffer.h 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Source/WebCore/platform/graphics/GlyphBuffer.h 2017-01-30 20:43:14 UTC (rev 211382)
@@ -102,9 +102,6 @@
void setInitialAdvance(GlyphBufferAdvance initialAdvance) { m_initialAdvance = initialAdvance; }
const GlyphBufferAdvance& initialAdvance() const { return m_initialAdvance; }
-
- void setLeadingExpansion(float leadingExpansion) { m_leadingExpansion = leadingExpansion; }
- float leadingExpansion() const { return m_leadingExpansion; }
Glyph glyphAt(unsigned index) const
{
@@ -248,7 +245,6 @@
#if PLATFORM(WIN)
Vector<FloatSize, 2048> m_offsets;
#endif
- float m_leadingExpansion;
};
}
Modified: trunk/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm (211381 => 211382)
--- trunk/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Source/WebCore/platform/graphics/cocoa/FontCascadeCocoa.mm 2017-01-30 20:43:14 UTC (rev 211382)
@@ -497,7 +497,7 @@
float afterWidth = controller.runWidthSoFar();
if (run.rtl())
- selectionRect.move(controller.totalWidth() - afterWidth + controller.leadingExpansion(), 0);
+ selectionRect.move(controller.totalWidth() - afterWidth, 0);
else
selectionRect.move(beforeWidth, 0);
selectionRect.setWidth(LayoutUnit::fromFloatCeil(afterWidth - beforeWidth));
@@ -508,20 +508,25 @@
float initialAdvance;
ComplexTextController controller(*this, run, false, 0, forTextEmphasis);
- controller.advance(from);
- float beforeWidth = controller.runWidthSoFar();
+ GlyphBuffer dummyGlyphBuffer;
+ controller.advance(from, &dummyGlyphBuffer);
controller.advance(to, &glyphBuffer);
if (glyphBuffer.isEmpty())
return 0;
- float afterWidth = controller.runWidthSoFar();
-
if (run.rtl()) {
- initialAdvance = controller.totalWidth() - afterWidth + controller.leadingExpansion();
+ // Exploit the fact that the sum of the paint advances is equal to
+ // the sum of the layout advances.
+ initialAdvance = controller.totalWidth();
+ for (unsigned i = 0; i < glyphBuffer.size(); ++i)
+ initialAdvance -= glyphBuffer.advanceAt(i).width();
glyphBuffer.reverse(0, glyphBuffer.size());
- } else
- initialAdvance = beforeWidth;
+ } else {
+ initialAdvance = dummyGlyphBuffer.initialAdvance().width();
+ for (unsigned i = 0; i < dummyGlyphBuffer.size(); ++i)
+ initialAdvance += dummyGlyphBuffer.advanceAt(i).width();
+ }
return initialAdvance;
}
Modified: trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp (211381 => 211382)
--- trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp 2017-01-30 20:43:14 UTC (rev 211382)
@@ -107,19 +107,12 @@
return layout.width(from, len, fallbackFonts);
}
-ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)
- : m_fallbackFonts(fallbackFonts)
- , m_font(font)
- , m_run(run)
- , m_end(run.length())
- , m_expansion(run.expansion())
- , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
- , m_forTextEmphasis(forTextEmphasis)
+void ComplexTextController::computeExpansionOpportunity()
{
if (!m_expansion)
m_expansionPerOpportunity = 0;
else {
- unsigned expansionOpportunityCount = FontCascade::expansionOpportunityCount(m_run.text(), m_run.ltr() ? LTR : RTL, run.expansionBehavior()).first;
+ unsigned expansionOpportunityCount = FontCascade::expansionOpportunityCount(m_run.text(), m_run.ltr() ? LTR : RTL, m_run.expansionBehavior()).first;
if (!expansionOpportunityCount)
m_expansionPerOpportunity = 0;
@@ -126,7 +119,19 @@
else
m_expansionPerOpportunity = m_expansion / expansionOpportunityCount;
}
+}
+ComplexTextController::ComplexTextController(const FontCascade& font, const TextRun& run, bool mayUseNaturalWritingDirection, HashSet<const Font*>* fallbackFonts, bool forTextEmphasis)
+ : m_fallbackFonts(fallbackFonts)
+ , m_font(font)
+ , m_run(run)
+ , m_end(run.length())
+ , m_expansion(run.expansion())
+ , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
+ , m_forTextEmphasis(forTextEmphasis)
+{
+ computeExpansionOpportunity();
+
collectComplexTextRuns();
finishConstruction();
@@ -136,7 +141,10 @@
: m_font(font)
, m_run(run)
, m_end(run.length())
+ , m_expansion(run.expansion())
{
+ computeExpansionOpportunity();
+
for (auto& run : runs)
m_complexTextRuns.append(run.ptr());
@@ -157,8 +165,6 @@
glyphCountSoFar += m_complexTextRuns[i]->glyphCount();
}
}
-
- m_runWidthSoFar = m_leadingExpansion;
}
unsigned ComplexTextController::offsetForPosition(float h, bool includePartialGlyphs)
@@ -166,7 +172,6 @@
if (h >= m_totalWidth)
return m_run.ltr() ? m_end : 0;
- h -= m_leadingExpansion;
if (h < 0)
return m_run.ltr() ? 0 : m_end;
@@ -180,8 +185,6 @@
for (unsigned j = 0; j < complexTextRun.glyphCount(); ++j) {
size_t index = offsetIntoAdjustedGlyphs + j;
CGFloat adjustedAdvance = m_adjustedBaseAdvances[index].width;
- if (!index)
- adjustedAdvance += complexTextRun.initialAdvance().width;
if (x < adjustedAdvance) {
CFIndex hitGlyphStart = complexTextRun.indexAt(j);
CFIndex hitGlyphEnd;
@@ -561,7 +564,7 @@
offset = m_end;
if (offset <= m_currentCharacter) {
- m_runWidthSoFar = m_leadingExpansion;
+ m_runWidthSoFar = 0;
m_numGlyphsSoFar = 0;
m_currentRun = 0;
m_glyphInCurrentRun = 0;
@@ -572,14 +575,14 @@
size_t runCount = m_complexTextRuns.size();
- unsigned leftmostGlyph = 0;
- unsigned currentRunIndex = indexOfCurrentRun(leftmostGlyph);
+ unsigned indexOfLeftmostGlyphInCurrentRun = 0; // Relative to the beginning of ComplexTextController.
+ unsigned currentRunIndex = indexOfCurrentRun(indexOfLeftmostGlyphInCurrentRun);
while (m_currentRun < runCount) {
const ComplexTextRun& complexTextRun = *m_complexTextRuns[currentRunIndex];
bool ltr = complexTextRun.isLTR();
size_t glyphCount = complexTextRun.glyphCount();
- unsigned g = ltr ? m_glyphInCurrentRun : glyphCount - 1 - m_glyphInCurrentRun;
- unsigned k = leftmostGlyph + g;
+ unsigned glyphIndexIntoCurrentRun = ltr ? m_glyphInCurrentRun : glyphCount - 1 - m_glyphInCurrentRun;
+ unsigned glyphIndexIntoComplexTextController = indexOfLeftmostGlyphInCurrentRun + glyphIndexIntoCurrentRun;
if (fallbackFonts && &complexTextRun.font() != &m_font.primaryFont())
fallbackFonts->add(&complexTextRun.font());
@@ -586,43 +589,45 @@
// We must store the initial advance for the first glyph we are going to draw.
// When leftmostGlyph is 0, it represents the first glyph to draw, taking into
// account the text direction.
- if (glyphBuffer && !leftmostGlyph) {
- CGSize initialAdvance = complexTextRun.initialAdvance();
-#if USE_LAYOUT_SPECIFIC_ADVANCES
- unsigned index = ltr ? 0 : m_adjustedBaseAdvances.size() - 1;
- initialAdvance.width += glyphOrigin(index).x;
- initialAdvance.height += glyphOrigin(index).y;
-#endif
- glyphBuffer->setInitialAdvance(initialAdvance);
+ if (!indexOfLeftmostGlyphInCurrentRun && glyphBuffer)
+ glyphBuffer->setInitialAdvance(complexTextRun.initialAdvance());
- glyphBuffer->setLeadingExpansion(m_leadingExpansion);
- }
-
while (m_glyphInCurrentRun < glyphCount) {
- unsigned glyphStartOffset = complexTextRun.indexAt(g);
+ unsigned glyphStartOffset = complexTextRun.indexAt(glyphIndexIntoCurrentRun);
unsigned glyphEndOffset;
if (complexTextRun.isMonotonic()) {
if (ltr)
- glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(g + 1 < glyphCount ? complexTextRun.indexAt(g + 1) : complexTextRun.indexEnd()));
+ glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(glyphIndexIntoCurrentRun + 1 < glyphCount ? complexTextRun.indexAt(glyphIndexIntoCurrentRun + 1) : complexTextRun.indexEnd()));
else
- glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(g > 0 ? complexTextRun.indexAt(g - 1) : complexTextRun.indexEnd()));
+ glyphEndOffset = std::max<unsigned>(glyphStartOffset, static_cast<unsigned>(glyphIndexIntoCurrentRun > 0 ? complexTextRun.indexAt(glyphIndexIntoCurrentRun - 1) : complexTextRun.indexEnd()));
} else
- glyphEndOffset = complexTextRun.endOffsetAt(g);
+ glyphEndOffset = complexTextRun.endOffsetAt(glyphIndexIntoCurrentRun);
- CGSize adjustedBaseAdvance = m_adjustedBaseAdvances[k];
+ CGSize adjustedBaseAdvance = m_adjustedBaseAdvances[glyphIndexIntoComplexTextController];
if (glyphStartOffset + complexTextRun.stringLocation() >= m_currentCharacter)
return;
if (glyphBuffer && !m_characterInCurrentGlyph) {
+ auto currentGlyphOrigin = glyphOrigin(glyphIndexIntoComplexTextController);
GlyphBufferAdvance paintAdvance = adjustedBaseAdvance;
-#if USE_LAYOUT_SPECIFIC_ADVANCES
- if (k + 1 < m_adjustedBaseAdvances.size()) {
- paintAdvance.setWidth(paintAdvance.width() + glyphOrigin(k + 1).x - glyphOrigin(k).x);
- paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(k + 1).y + glyphOrigin(k).y);
+ if (!glyphIndexIntoCurrentRun) {
+ // The first layout advance of every run includes the "initial layout advance." However, here, we need
+ // paint advances, so subtract it out before transforming the layout advance into a paint advance.
+ paintAdvance.setWidth(paintAdvance.width() - (complexTextRun.initialAdvance().width - currentGlyphOrigin.x));
+ paintAdvance.setHeight(paintAdvance.height() - (complexTextRun.initialAdvance().height - currentGlyphOrigin.y));
}
-#endif
- glyphBuffer->add(m_adjustedGlyphs[k], &complexTextRun.font(), paintAdvance, complexTextRun.indexAt(m_glyphInCurrentRun));
+ paintAdvance.setWidth(paintAdvance.width() + glyphOrigin(glyphIndexIntoComplexTextController + 1).x - currentGlyphOrigin.x);
+ paintAdvance.setHeight(paintAdvance.height() + glyphOrigin(glyphIndexIntoComplexTextController + 1).y - currentGlyphOrigin.y);
+ if (glyphIndexIntoCurrentRun == glyphCount - 1 && currentRunIndex + 1 < runCount) {
+ // Our paint advance points to the end of the run. However, the next run may have an
+ // initial advance, and our paint advance needs to point to the location of the next
+ // glyph. So, we need to add in the next run's initial advance.
+ paintAdvance.setWidth(paintAdvance.width() - glyphOrigin(glyphIndexIntoComplexTextController + 1).x + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().width);
+ paintAdvance.setHeight(paintAdvance.height() - glyphOrigin(glyphIndexIntoComplexTextController + 1).y + m_complexTextRuns[currentRunIndex + 1]->initialAdvance().height);
+ }
+ paintAdvance.setHeight(-paintAdvance.height()); // Increasing y points down
+ glyphBuffer->add(m_adjustedGlyphs[glyphIndexIntoComplexTextController], &complexTextRun.font(), paintAdvance, complexTextRun.indexAt(m_glyphInCurrentRun));
}
unsigned oldCharacterInCurrentGlyph = m_characterInCurrentGlyph;
@@ -636,14 +641,14 @@
m_glyphInCurrentRun++;
m_characterInCurrentGlyph = 0;
if (ltr) {
- g++;
- k++;
+ glyphIndexIntoCurrentRun++;
+ glyphIndexIntoComplexTextController++;
} else {
- g--;
- k--;
+ glyphIndexIntoCurrentRun--;
+ glyphIndexIntoComplexTextController--;
}
}
- currentRunIndex = incrementCurrentRun(leftmostGlyph);
+ currentRunIndex = incrementCurrentRun(indexOfLeftmostGlyphInCurrentRun);
m_glyphInCurrentRun = 0;
}
}
@@ -690,16 +695,6 @@
unsigned glyphCount = complexTextRun.glyphCount();
const Font& font = complexTextRun.font();
- // Represent the initial advance for a text run by adjusting the advance
- // of the last glyph of the previous text run in the glyph buffer.
- if (!complexTextRun.glyphOrigins() && runIndex && m_adjustedBaseAdvances.size()) {
- CGSize previousAdvance = m_adjustedBaseAdvances.last();
- previousAdvance.width += complexTextRun.initialAdvance().width;
- previousAdvance.height -= complexTextRun.initialAdvance().height;
- m_adjustedBaseAdvances[m_adjustedBaseAdvances.size() - 1] = previousAdvance;
- }
- widthSinceLastCommit += complexTextRun.initialAdvance().width;
-
if (!complexTextRun.isLTR())
m_isLTROnly = false;
@@ -743,6 +738,15 @@
glyph = font.spaceGlyph();
}
+ if (!i) {
+ advance.width += complexTextRun.initialAdvance().width;
+ advance.height += complexTextRun.initialAdvance().height;
+ if (auto* origins = complexTextRun.glyphOrigins()) {
+ advance.width -= origins[0].x;
+ advance.height -= origins[0].y;
+ }
+ }
+
advance.width += font.syntheticBoldOffset();
if (hasExtraSpacing) {
@@ -774,20 +778,17 @@
if (m_expansion) {
bool expandLeft, expandRight;
std::tie(expandLeft, expandRight) = expansionLocation(ideograph, treatAsSpace, m_run.ltr(), afterExpansion, forbidLeadingExpansion, forbidTrailingExpansion, forceLeadingExpansion, forceTrailingExpansion);
+ m_expansion -= m_expansionPerOpportunity;
+ advance.width += m_expansionPerOpportunity;
if (expandLeft) {
// Increase previous width
- m_expansion -= m_expansionPerOpportunity;
- m_totalWidth += m_expansionPerOpportunity;
if (m_adjustedBaseAdvances.isEmpty())
- m_leadingExpansion = m_expansionPerOpportunity;
+ complexTextRun.growInitialAdvanceHorizontally(m_expansionPerOpportunity);
else
m_adjustedBaseAdvances.last().width += m_expansionPerOpportunity;
}
- if (expandRight) {
- m_expansion -= m_expansionPerOpportunity;
- advance.width += m_expansionPerOpportunity;
+ if (expandRight)
afterExpansion = true;
- }
} else
afterExpansion = false;
@@ -804,9 +805,7 @@
if (m_forTextEmphasis && (!FontCascade::canReceiveTextEmphasis(ch) || (U_GET_GC_MASK(ch) & U_GC_M_MASK)))
glyph = 0;
- advance.height *= -1;
m_adjustedBaseAdvances.append(advance);
-#if USE_LAYOUT_SPECIFIC_ADVANCES
if (auto* origins = complexTextRun.glyphOrigins()) {
ASSERT(m_glyphOrigins.size() < m_adjustedBaseAdvances.size());
m_glyphOrigins.grow(m_adjustedBaseAdvances.size());
@@ -813,7 +812,6 @@
m_glyphOrigins[m_glyphOrigins.size() - 1] = origins[i];
ASSERT(m_glyphOrigins.size() == m_adjustedBaseAdvances.size());
}
-#endif
m_adjustedGlyphs.append(glyph);
FloatRect glyphBounds = font.boundsForGlyph(glyph);
Modified: trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.h (211381 => 211382)
--- trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.h 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Source/WebCore/platform/graphics/mac/ComplexTextController.h 2017-01-30 20:43:14 UTC (rev 211382)
@@ -51,8 +51,7 @@
enum GlyphIterationStyle { IncludePartialGlyphs, ByWholeGlyphs };
-// ComplexTextController is responsible for rendering and measuring glyphs for
-// complex scripts on macOS and iOS.
+// See https://trac.webkit.org/wiki/ComplexTextController for more information about ComplexTextController.
class ComplexTextController {
WTF_MAKE_FAST_ALLOCATED;
public:
@@ -77,8 +76,6 @@
float minGlyphBoundingBoxY() const { return m_minGlyphBoundingBoxY; }
float maxGlyphBoundingBoxY() const { return m_maxGlyphBoundingBoxY; }
- float leadingExpansion() const { return m_leadingExpansion; }
-
class ComplexTextRun : public RefCounted<ComplexTextRun> {
public:
static Ref<ComplexTextRun> create(CTRunRef ctRun, const Font& font, const UChar* characters, unsigned stringLocation, size_t stringLength, CFRange runRange)
@@ -108,15 +105,33 @@
const CGGlyph* glyphs() const { return m_glyphs; }
/*
- * X (Paint glyph position) X (Paint glyph position) X (Paint glyph position)
- * 7 7 7
- * / / /
- * / (Glyph origin) / (Glyph origin) / (Glyph origin)
- * / / /
- * / / /
- * X-----------------------X--------------------------X--------------------------X---->...
- * (text position ^) (initial advance) (base advance) (base advance)
+ * This is the format of the information CoreText gives us about each run:
+ *
+ * ----->X (Paint glyph position) X (Paint glyph position) X (Paint glyph position)
+ * / 7 7 7
+ * / / / /
+ * (Initial advance) / / (Glyph origin) / (Glyph origin) / (Glyph origin)
+ * ------------------- / / /
+ * / / / /
+ * X X--------------------------X--------------------------X--------------------------X
+ * (text position ^) (base advance) (base advance) (base advance)
+ *
+ *
+ *
+ *
+ *
+ * And here is the output we transform this into (for each run):
+ *
+ * ----->X------------------------->X------------------------->X
+ * / (Paint advance) (Paint advance) \
+ * / \
+ * (Initial advance) / \ (Paint advance)
+ * ------------------- ----------------
+ * / \
+ * X--------------------------------------------------X--------------------------X--------------------------X
+ * (text position ^) (layout advance) (layout advance) (layout advance)
*/
+ void growInitialAdvanceHorizontally(CGFloat delta) { m_initialAdvance.width += delta; }
CGSize initialAdvance() const { return m_initialAdvance; }
const CGSize* baseAdvances() const { return m_baseAdvances; }
const CGPoint* glyphOrigins() const { return m_glyphOrigins.size() == glyphCount() ? m_glyphOrigins.data() : nullptr; }
@@ -149,6 +164,7 @@
bool m_isMonotonic { true };
};
private:
+ void computeExpansionOpportunity();
void finishConstruction();
static unsigned stringBegin(const ComplexTextRun& run) { return run.stringLocation() + run.indexBegin(); }
@@ -206,7 +222,6 @@
unsigned m_characterInCurrentGlyph { 0 };
float m_expansion { 0 };
float m_expansionPerOpportunity { 0 };
- float m_leadingExpansion { 0 };
float m_minGlyphBoundingBoxX { std::numeric_limits<float>::max() };
float m_maxGlyphBoundingBoxX { std::numeric_limits<float>::min() };
Modified: trunk/Tools/ChangeLog (211381 => 211382)
--- trunk/Tools/ChangeLog 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Tools/ChangeLog 2017-01-30 20:43:14 UTC (rev 211382)
@@ -1,3 +1,18 @@
+2017-01-30 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ Correct spacing regression on inter-element complex path shaping on some fonts
+ https://bugs.webkit.org/show_bug.cgi?id=166013
+
+ Reviewed by Simon Fraser.
+
+ Unskip existing tests and make some new tests:
+ - Testing complex text with no origins
+ - Testing initial expansions
+ - Testing the sign of vertical advances
+
+ * TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:
+ (TestWebKitAPI::TEST_F):
+
2017-01-30 Carlos Alberto Lopez Perez <clo...@igalia.com>
[GTK][EFL] Avoid using a thin directory to create the built product on the archive-built-product step.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp (211381 => 211382)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp 2017-01-30 20:08:29 UTC (rev 211381)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp 2017-01-30 20:43:14 UTC (rev 211382)
@@ -45,7 +45,7 @@
}
};
-TEST_F(ComplexTextControllerTest, DISABLED_InitialAdvanceWithLeftRunInRTL)
+TEST_F(ComplexTextControllerTest, InitialAdvanceWithLeftRunInRTL)
{
FontCascadeDescription description;
description.setOneFamily("Times");
@@ -79,6 +79,7 @@
totalWidth += advances[i].width;
EXPECT_NEAR(controller.totalWidth(), spaceWidth + totalWidth, 0.0001);
GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(0, &glyphBuffer);
EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(1, &glyphBuffer);
@@ -96,7 +97,7 @@
EXPECT_NEAR(glyphBuffer.advanceAt(5).width(), spaceWidth + initialAdvance.width, 0.0001);
}
-TEST_F(ComplexTextControllerTest, DISABLED_InitialAdvanceInRTL)
+TEST_F(ComplexTextControllerTest, InitialAdvanceInRTL)
{
FontCascadeDescription description;
description.setOneFamily("Times");
@@ -127,6 +128,7 @@
totalWidth += advances[i].width;
EXPECT_NEAR(controller.totalWidth(), totalWidth, 0.0001);
GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(0, &glyphBuffer);
EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(1, &glyphBuffer);
@@ -141,9 +143,10 @@
EXPECT_NEAR(glyphBuffer.advanceAt(2).width(), advances[2].width, 0.0001);
EXPECT_NEAR(glyphBuffer.advanceAt(3).width(), advances[1].width, 0.0001);
EXPECT_NEAR(glyphBuffer.advanceAt(4).width(), -initialAdvance.width, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(4).height(), initialAdvance.height, 0.0001);
}
-TEST_F(ComplexTextControllerTest, DISABLED_InitialAdvanceWithLeftRunInLTR)
+TEST_F(ComplexTextControllerTest, InitialAdvanceWithLeftRunInLTR)
{
FontCascadeDescription description;
description.setOneFamily("LucidaGrande");
@@ -174,6 +177,7 @@
EXPECT_NEAR(controller.totalWidth(), spaceWidth + 76.347656 + initialAdvance.width, 0.0001);
GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(0, &glyphBuffer);
EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(1, &glyphBuffer);
@@ -190,7 +194,7 @@
EXPECT_NEAR(glyphBuffer.advanceAt(2).width(), 23.281250, 0.0001);
}
-TEST_F(ComplexTextControllerTest, DISABLED_InitialAdvanceInLTR)
+TEST_F(ComplexTextControllerTest, InitialAdvanceInLTR)
{
FontCascadeDescription description;
description.setOneFamily("LucidaGrande");
@@ -218,6 +222,7 @@
EXPECT_NEAR(controller.totalWidth(), 76.347656 + initialAdvance.width, 0.0001);
GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(0, &glyphBuffer);
EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
controller.advance(1, &glyphBuffer);
@@ -231,4 +236,124 @@
EXPECT_NEAR(glyphBuffer.advanceAt(1).width(), 23.281250, 0.0001);
}
+TEST_F(ComplexTextControllerTest, InitialAdvanceInRTLNoOrigins)
+{
+ FontCascadeDescription description;
+ description.setOneFamily("Times");
+ description.setComputedSize(48);
+ FontCascade font(description);
+ font.update();
+
+ CGSize initialAdvance = CGSizeMake(4.33996383363472, 12.368896925859);
+
+ UChar characters[] = { 0x633, 0x20, 0x627, 0x650 };
+ size_t charactersLength = WTF_ARRAY_LENGTH(characters);
+ TextRun textRun(StringView(characters, charactersLength));
+ auto run1 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(-4.33996383363472, -12.368896925859), CGSizeMake(14.0397830018083, 0) }, { }, { 884, 240 }, { 3, 2 }, initialAdvance, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(2, 2), false);
+ auto run2 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(12.0, 0) }, { }, { 3 }, { 1 }, CGSizeZero, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(1, 1), false);
+ auto run3 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(43.8119349005425, 0) }, { }, { 276 }, { 0 }, CGSizeZero, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(0, 1), false);
+ Vector<Ref<ComplexTextController::ComplexTextRun>> runs;
+ runs.append(WTFMove(run1));
+ runs.append(WTFMove(run2));
+ runs.append(WTFMove(run3));
+ ComplexTextController controller(font, textRun, runs);
+
+ CGFloat totalWidth = 14.0397830018083 + 12.0 + 43.8119349005425;
+ EXPECT_NEAR(controller.totalWidth(), totalWidth, 0.0001);
+ GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(0, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(1, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 43.8119349005425, 0.0001);
+ controller.advance(2, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 43.8119349005425 + 12.0, 0.0001);
+ controller.advance(3, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), totalWidth, 0.0001);
+ controller.advance(4, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), totalWidth, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().width(), initialAdvance.width, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().height(), initialAdvance.height, 0.0001);
+ EXPECT_EQ(glyphBuffer.size(), 4U);
+ EXPECT_NEAR(glyphBuffer.advanceAt(0).width(), 43.8119349005425, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(1).width(), 12.0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(2).width(), 14.0397830018083, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(3).width(), -4.33996383363472, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(3).height(), 12.368896925859, 0.0001);
}
+
+TEST_F(ComplexTextControllerTest, LeadingExpansion)
+{
+ FontCascadeDescription description;
+ description.setOneFamily("Times");
+ description.setComputedSize(48);
+ FontCascade font(description);
+ font.update();
+
+ UChar characters[] = { 'a' };
+ size_t charactersLength = WTF_ARRAY_LENGTH(characters);
+ TextRun textRun(StringView(characters, charactersLength), 0, 100, ForceLeadingExpansion);
+ auto run = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(24, 0) }, { }, { 16 }, { 0 }, CGSizeZero, font.primaryFont(), characters, 0, charactersLength, CFRangeMake(0, 1), true);
+ Vector<Ref<ComplexTextController::ComplexTextRun>> runs;
+ runs.append(WTFMove(run));
+ ComplexTextController controller(font, textRun, runs);
+
+ CGFloat totalWidth = 100 + 24;
+ EXPECT_NEAR(controller.totalWidth(), totalWidth, 0.0001);
+ GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(0, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(1, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), totalWidth, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().width(), 100, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().height(), 0, 0.0001);
+ EXPECT_EQ(glyphBuffer.size(), 1U);
+ EXPECT_NEAR(glyphBuffer.advanceAt(0).width(), 24, 0.0001);
+}
+
+TEST_F(ComplexTextControllerTest, VerticalAdvances)
+{
+ FontCascadeDescription description;
+ description.setOneFamily("Times");
+ description.setComputedSize(48);
+ FontCascade font(description);
+ font.update();
+
+ UChar characters[] = { 'a', 'b', 'c', 'd' };
+ size_t charactersLength = WTF_ARRAY_LENGTH(characters);
+ TextRun textRun(StringView(characters, charactersLength));
+ auto run1 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(0, 1), CGSizeMake(0, 2) }, { CGPointMake(0, 4), CGPointMake(0, 8) }, { 16, 17 }, { 0, 1 }, CGSizeMake(0, 16), font.primaryFont(), characters, 0, charactersLength, CFRangeMake(0, 2), true);
+ auto run2 = ComplexTextController::ComplexTextRun::createForTesting({ CGSizeMake(0, 32), CGSizeMake(0, 64) }, { CGPointMake(0, 128), CGPointMake(0, 256) }, { 18, 19 }, { 2, 3 }, CGSizeMake(0, 512), font.primaryFont(), characters, 0, charactersLength, CFRangeMake(2, 2), true);
+ Vector<Ref<ComplexTextController::ComplexTextRun>> runs;
+ runs.append(WTFMove(run1));
+ runs.append(WTFMove(run2));
+ ComplexTextController controller(font, textRun, runs);
+
+ EXPECT_NEAR(controller.totalWidth(), 0, 0.0001);
+ GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(0, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(1, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(2, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(3, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(4, &glyphBuffer);
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().width(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.initialAdvance().height(), 16, 0.0001);
+ EXPECT_EQ(glyphBuffer.size(), 4U);
+ EXPECT_NEAR(glyphBuffer.advanceAt(0).width(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(0).height(), 4 - 1 -8, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(1).width(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(1).height(), 8 - 2 - 512, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(2).width(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(2).height(), 128 - 32 - 256, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(3).width(), 0, 0.0001);
+ EXPECT_NEAR(glyphBuffer.advanceAt(3).height(), 256 - 64, 0.0001);
+}
+
+}
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
