- Revision
- 215117
- Author
- mmaxfi...@apple.com
- Date
- 2017-04-07 14:37:57 -0700 (Fri, 07 Apr 2017)
Log Message
REGRESSION(r211382): Complex text with justification erroneously overflows containers
https://bugs.webkit.org/show_bug.cgi?id=170399
<rdar://problem/31442008>
Reviewed by Simon Fraser.
Source/WebCore:
When we perform justification, we adjust glyphs' advances to add extra space between words.
ComplexTextController maintains an invariant where m_totalWidth is equal to the sum of these
advances. However, in RTL text, inserting extra justification space to the left of a glyph
would break that invariant, and would increase the advances of two glyphs instead of just
one. Then, when we go to draw the text, the sum of the advances is wider than m_totalWidth,
which means the glyphs would be drawn outside of their container.
This regressed in r211382 simply because of an oversight and because there were no tests for
this codepath.
Test: ComplexTextControllerTest.TotalWidthWithJustification
* platform/graphics/ComplexTextController.cpp:
(WebCore::ComplexTextController::adjustGlyphsAndAdvances):
* rendering/InlineBox.h:
(WebCore::InlineBox::InlineBox):
Tools:
Check for the invariant that the sum of the advances is equal to m_totalWidth.
* TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:
(TestWebKitAPI::TEST_F):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (215116 => 215117)
--- trunk/Source/WebCore/ChangeLog 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Source/WebCore/ChangeLog 2017-04-07 21:37:57 UTC (rev 215117)
@@ -1,3 +1,28 @@
+2017-04-07 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ REGRESSION(r211382): Complex text with justification erroneously overflows containers
+ https://bugs.webkit.org/show_bug.cgi?id=170399
+ <rdar://problem/31442008>
+
+ Reviewed by Simon Fraser.
+
+ When we perform justification, we adjust glyphs' advances to add extra space between words.
+ ComplexTextController maintains an invariant where m_totalWidth is equal to the sum of these
+ advances. However, in RTL text, inserting extra justification space to the left of a glyph
+ would break that invariant, and would increase the advances of two glyphs instead of just
+ one. Then, when we go to draw the text, the sum of the advances is wider than m_totalWidth,
+ which means the glyphs would be drawn outside of their container.
+
+ This regressed in r211382 simply because of an oversight and because there were no tests for
+ this codepath.
+
+ Test: ComplexTextControllerTest.TotalWidthWithJustification
+
+ * platform/graphics/ComplexTextController.cpp:
+ (WebCore::ComplexTextController::adjustGlyphsAndAdvances):
+ * rendering/InlineBox.h:
+ (WebCore::InlineBox::InlineBox):
+
2017-04-07 Chris Dumez <cdu...@apple.com>
Throttle / Align DOM Timers in cross-origin iframes to 30fps
Modified: trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp (215116 => 215117)
--- trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp 2017-04-07 21:37:57 UTC (rev 215117)
@@ -700,7 +700,6 @@
void ComplexTextController::adjustGlyphsAndAdvances()
{
bool afterExpansion = (m_run.expansionBehavior() & LeadingExpansionMask) == ForbidLeadingExpansion;
- float widthSinceLastCommit = 0;
size_t runCount = m_complexTextRuns.size();
bool hasExtraSpacing = (m_font.letterSpacing() || m_font.wordSpacing() || m_expansion) && !m_run.spacingDisabled();
bool runForcesLeadingExpansion = (m_run.expansionBehavior() & LeadingExpansionMask) == ForceLeadingExpansion;
@@ -742,7 +741,7 @@
FloatSize advance = treatAsSpace ? FloatSize(spaceWidth, advances[i].height()) : advances[i];
if (ch == '\t' && m_run.allowTabs())
- advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalWidth + widthSinceLastCommit));
+ advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalWidth));
else if (FontCascade::treatAsZeroWidthSpace(ch) && !treatAsSpace) {
advance.setWidth(0);
glyph = font.spaceGlyph();
@@ -785,17 +784,22 @@
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.expand(m_expansionPerOpportunity, 0);
if (expandLeft) {
+ m_expansion -= m_expansionPerOpportunity;
// Increase previous width
- if (m_adjustedBaseAdvances.isEmpty())
+ if (m_adjustedBaseAdvances.isEmpty()) {
+ advance.expand(m_expansionPerOpportunity, 0);
complexTextRun.growInitialAdvanceHorizontally(m_expansionPerOpportunity);
- else
+ } else {
m_adjustedBaseAdvances.last().expand(m_expansionPerOpportunity, 0);
+ m_totalWidth += m_expansionPerOpportunity;
+ }
}
- if (expandRight)
+ if (expandRight) {
+ m_expansion -= m_expansionPerOpportunity;
+ advance.expand(m_expansionPerOpportunity, 0);
afterExpansion = true;
+ }
} else
afterExpansion = false;
@@ -806,7 +810,7 @@
afterExpansion = false;
}
- widthSinceLastCommit += advance.width();
+ m_totalWidth += advance.width();
// FIXME: Combining marks should receive a text emphasis mark if they are combine with a space.
if (m_forTextEmphasis && (!FontCascade::canReceiveTextEmphasis(ch) || (U_GET_GC_MASK(ch) & U_GC_M_MASK)))
@@ -834,8 +838,6 @@
if (!isMonotonic)
complexTextRun.setIsNonMonotonic();
}
-
- m_totalWidth += widthSinceLastCommit;
}
// Missing glyphs run constructor. Core Text will not generate a run of missing glyphs, instead falling back on
Modified: trunk/Source/WebCore/rendering/InlineBox.h (215116 => 215117)
--- trunk/Source/WebCore/rendering/InlineBox.h 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Source/WebCore/rendering/InlineBox.h 2017-04-07 21:37:57 UTC (rev 215117)
@@ -280,16 +280,16 @@
float expansion() const { return m_expansion; }
private:
- InlineBox* m_next; // The next element on the same line as us.
- InlineBox* m_prev; // The previous element on the same line as us.
+ InlineBox* m_next { nullptr }; // The next element on the same line as us.
+ InlineBox* m_prev { nullptr }; // The previous element on the same line as us.
- InlineFlowBox* m_parent; // The box that contains us.
+ InlineFlowBox* m_parent { nullptr }; // The box that contains us.
RenderObject& m_renderer;
public:
FloatPoint m_topLeft;
- float m_logicalWidth;
+ float m_logicalWidth { 0 };
#define ADD_BOOLEAN_BITFIELD(name, Name) \
private:\
@@ -368,21 +368,12 @@
#undef ADD_BOOLEAN_BITFIELD
private:
- float m_expansion;
+ float m_expansion { 0 };
InlineBoxBitfields m_bitfields;
protected:
explicit InlineBox(RenderObject& renderer)
- : m_next(nullptr)
- , m_prev(nullptr)
- , m_parent(nullptr)
- , m_renderer(renderer)
- , m_logicalWidth(0)
- , m_expansion(0)
-#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
- , m_deletionSentinel(deletionSentinelNotDeletedValue)
- , m_hasBadParent(false)
-#endif
+ : m_renderer(renderer)
{
}
@@ -393,12 +384,7 @@
, m_renderer(renderer)
, m_topLeft(topLeft)
, m_logicalWidth(logicalWidth)
- , m_expansion(0)
, m_bitfields(firstLine, constructed, dirty, extracted, isHorizontal)
-#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
- , m_deletionSentinel(deletionSentinelNotDeletedValue)
- , m_hasBadParent(false)
-#endif
{
}
@@ -427,10 +413,10 @@
#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
private:
- static const unsigned deletionSentinelNotDeletedValue = 0xF0F0F0F0U;
- static const unsigned deletionSentinelDeletedValue = 0xF0DEADF0U;
- unsigned m_deletionSentinel;
- bool m_hasBadParent;
+ static constexpr unsigned deletionSentinelNotDeletedValue = 0xF0F0F0F0U;
+ static constexpr unsigned deletionSentinelDeletedValue = 0xF0DEADF0U;
+ unsigned m_deletionSentinel { deletionSentinelNotDeletedValue };
+ bool m_hasBadParent { false };
#endif
};
Modified: trunk/Tools/ChangeLog (215116 => 215117)
--- trunk/Tools/ChangeLog 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Tools/ChangeLog 2017-04-07 21:37:57 UTC (rev 215117)
@@ -1,3 +1,16 @@
+2017-04-07 Myles C. Maxfield <mmaxfi...@apple.com>
+
+ REGRESSION(r211382): Complex text with justification erroneously overflows containers
+ https://bugs.webkit.org/show_bug.cgi?id=170399
+ <rdar://problem/31442008>
+
+ Reviewed by Simon Fraser.
+
+ Check for the invariant that the sum of the advances is equal to m_totalWidth.
+
+ * TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp:
+ (TestWebKitAPI::TEST_F):
+
2017-04-07 Ryan Haddad <ryanhad...@apple.com>
[ios-simulator] API test WebKit2.WKWebProcessPlugInRangeHandle timing out
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp (215116 => 215117)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp 2017-04-07 21:37:57 UTC (rev 215117)
@@ -355,4 +355,37 @@
EXPECT_NEAR(glyphBuffer.advanceAt(3).height(), 256 - 64, 0.0001);
}
+TEST_F(ComplexTextControllerTest, TotalWidthWithJustification)
+{
+ FontCascadeDescription description;
+ description.setOneFamily("Times");
+ description.setComputedSize(80);
+ FontCascade font(description);
+ font.update();
+
+ Vector<FloatSize> advances = { FloatSize(1, 0), FloatSize(2, 0), FloatSize(4, 0), FloatSize(8, 0), FloatSize(16, 0) };
+#if USE_LAYOUT_SPECIFIC_ADVANCES
+ Vector<FloatPoint> origins = { FloatPoint(), FloatPoint(), FloatPoint(), FloatPoint(), FloatPoint() };
+#else
+ Vector<FloatPoint> origins = { };
+#endif
+
+ FloatSize initialAdvance = FloatSize();
+
+ UChar characters[] = { 0x644, ' ', 0x644, ' ', 0x644 };
+ size_t charactersLength = WTF_ARRAY_LENGTH(characters);
+ TextRun textRun(StringView(characters, charactersLength), 0, 14, DefaultExpansion, RTL);
+ auto run = ComplexTextController::ComplexTextRun::create(advances, origins, { 5, 6, 7, 8, 9 }, { 4, 3, 2, 1, 0 }, initialAdvance, font.primaryFont(), characters, 0, charactersLength, 0, 5, false);
+ Vector<Ref<ComplexTextController::ComplexTextRun>> runs;
+ runs.append(WTFMove(run));
+ ComplexTextController controller(font, textRun, runs);
+
+ EXPECT_NEAR(controller.totalWidth(), 1 + 20 + 7 + 4 + 20 + 7 + 16, 0.0001);
+ GlyphBuffer glyphBuffer;
+ EXPECT_NEAR(controller.runWidthSoFar(), 0, 0.0001);
+ controller.advance(5, &glyphBuffer);
+ EXPECT_EQ(glyphBuffer.size(), 5U);
+ EXPECT_NEAR(glyphBuffer.advanceAt(0).width() + glyphBuffer.advanceAt(1).width() + glyphBuffer.advanceAt(2).width() + glyphBuffer.advanceAt(3).width() + glyphBuffer.advanceAt(4).width(), controller.totalWidth(), 0.0001);
}
+
+}
