<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[215117] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/215117">215117</a></dd>
<dt>Author</dt> <dd>mmaxfield@apple.com</dd>
<dt>Date</dt> <dd>2017-04-07 14:37:57 -0700 (Fri, 07 Apr 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>REGRESSION(<a href="http://trac.webkit.org/projects/webkit/changeset/211382">r211382</a>): 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 <a href="http://trac.webkit.org/projects/webkit/changeset/211382">r211382</a> 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):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreplatformgraphicsComplexTextControllercpp">trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp</a></li>
<li><a href="#trunkSourceWebCorerenderingInlineBoxh">trunk/Source/WebCore/rendering/InlineBox.h</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsWebCoreComplexTextControllercpp">trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (215116 => 215117)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,28 @@
</span><ins>+2017-04-07 Myles C. Maxfield <mmaxfield@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):
+
</ins><span class="cx"> 2017-04-07 Chris Dumez <cdumez@apple.com>
</span><span class="cx">
</span><span class="cx"> Throttle / Align DOM Timers in cross-origin iframes to 30fps
</span></span></pre></div>
<a id="trunkSourceWebCoreplatformgraphicsComplexTextControllercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp (215116 => 215117)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -700,7 +700,6 @@
</span><span class="cx"> void ComplexTextController::adjustGlyphsAndAdvances()
</span><span class="cx"> {
</span><span class="cx"> bool afterExpansion = (m_run.expansionBehavior() & LeadingExpansionMask) == ForbidLeadingExpansion;
</span><del>- float widthSinceLastCommit = 0;
</del><span class="cx"> size_t runCount = m_complexTextRuns.size();
</span><span class="cx"> bool hasExtraSpacing = (m_font.letterSpacing() || m_font.wordSpacing() || m_expansion) && !m_run.spacingDisabled();
</span><span class="cx"> bool runForcesLeadingExpansion = (m_run.expansionBehavior() & LeadingExpansionMask) == ForceLeadingExpansion;
</span><span class="lines">@@ -742,7 +741,7 @@
</span><span class="cx"> FloatSize advance = treatAsSpace ? FloatSize(spaceWidth, advances[i].height()) : advances[i];
</span><span class="cx">
</span><span class="cx"> if (ch == '\t' && m_run.allowTabs())
</span><del>- advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalWidth + widthSinceLastCommit));
</del><ins>+ advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalWidth));
</ins><span class="cx"> else if (FontCascade::treatAsZeroWidthSpace(ch) && !treatAsSpace) {
</span><span class="cx"> advance.setWidth(0);
</span><span class="cx"> glyph = font.spaceGlyph();
</span><span class="lines">@@ -785,17 +784,22 @@
</span><span class="cx"> if (m_expansion) {
</span><span class="cx"> bool expandLeft, expandRight;
</span><span class="cx"> std::tie(expandLeft, expandRight) = expansionLocation(ideograph, treatAsSpace, m_run.ltr(), afterExpansion, forbidLeadingExpansion, forbidTrailingExpansion, forceLeadingExpansion, forceTrailingExpansion);
</span><del>- m_expansion -= m_expansionPerOpportunity;
- advance.expand(m_expansionPerOpportunity, 0);
</del><span class="cx"> if (expandLeft) {
</span><ins>+ m_expansion -= m_expansionPerOpportunity;
</ins><span class="cx"> // Increase previous width
</span><del>- if (m_adjustedBaseAdvances.isEmpty())
</del><ins>+ if (m_adjustedBaseAdvances.isEmpty()) {
+ advance.expand(m_expansionPerOpportunity, 0);
</ins><span class="cx"> complexTextRun.growInitialAdvanceHorizontally(m_expansionPerOpportunity);
</span><del>- else
</del><ins>+ } else {
</ins><span class="cx"> m_adjustedBaseAdvances.last().expand(m_expansionPerOpportunity, 0);
</span><ins>+ m_totalWidth += m_expansionPerOpportunity;
+ }
</ins><span class="cx"> }
</span><del>- if (expandRight)
</del><ins>+ if (expandRight) {
+ m_expansion -= m_expansionPerOpportunity;
+ advance.expand(m_expansionPerOpportunity, 0);
</ins><span class="cx"> afterExpansion = true;
</span><ins>+ }
</ins><span class="cx"> } else
</span><span class="cx"> afterExpansion = false;
</span><span class="cx">
</span><span class="lines">@@ -806,7 +810,7 @@
</span><span class="cx"> afterExpansion = false;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- widthSinceLastCommit += advance.width();
</del><ins>+ m_totalWidth += advance.width();
</ins><span class="cx">
</span><span class="cx"> // FIXME: Combining marks should receive a text emphasis mark if they are combine with a space.
</span><span class="cx"> if (m_forTextEmphasis && (!FontCascade::canReceiveTextEmphasis(ch) || (U_GET_GC_MASK(ch) & U_GC_M_MASK)))
</span><span class="lines">@@ -834,8 +838,6 @@
</span><span class="cx"> if (!isMonotonic)
</span><span class="cx"> complexTextRun.setIsNonMonotonic();
</span><span class="cx"> }
</span><del>-
- m_totalWidth += widthSinceLastCommit;
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Missing glyphs run constructor. Core Text will not generate a run of missing glyphs, instead falling back on
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingInlineBoxh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/InlineBox.h (215116 => 215117)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -280,16 +280,16 @@
</span><span class="cx"> float expansion() const { return m_expansion; }
</span><span class="cx">
</span><span class="cx"> private:
</span><del>- InlineBox* m_next; // The next element on the same line as us.
- InlineBox* m_prev; // The previous element on the same line as us.
</del><ins>+ 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.
</ins><span class="cx">
</span><del>- InlineFlowBox* m_parent; // The box that contains us.
</del><ins>+ InlineFlowBox* m_parent { nullptr }; // The box that contains us.
</ins><span class="cx">
</span><span class="cx"> RenderObject& m_renderer;
</span><span class="cx">
</span><span class="cx"> public:
</span><span class="cx"> FloatPoint m_topLeft;
</span><del>- float m_logicalWidth;
</del><ins>+ float m_logicalWidth { 0 };
</ins><span class="cx">
</span><span class="cx"> #define ADD_BOOLEAN_BITFIELD(name, Name) \
</span><span class="cx"> private:\
</span><span class="lines">@@ -368,21 +368,12 @@
</span><span class="cx"> #undef ADD_BOOLEAN_BITFIELD
</span><span class="cx">
</span><span class="cx"> private:
</span><del>- float m_expansion;
</del><ins>+ float m_expansion { 0 };
</ins><span class="cx"> InlineBoxBitfields m_bitfields;
</span><span class="cx">
</span><span class="cx"> protected:
</span><span class="cx"> explicit InlineBox(RenderObject& renderer)
</span><del>- : 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
</del><ins>+ : m_renderer(renderer)
</ins><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -393,12 +384,7 @@
</span><span class="cx"> , m_renderer(renderer)
</span><span class="cx"> , m_topLeft(topLeft)
</span><span class="cx"> , m_logicalWidth(logicalWidth)
</span><del>- , m_expansion(0)
</del><span class="cx"> , m_bitfields(firstLine, constructed, dirty, extracted, isHorizontal)
</span><del>-#if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
- , m_deletionSentinel(deletionSentinelNotDeletedValue)
- , m_hasBadParent(false)
-#endif
</del><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -427,10 +413,10 @@
</span><span class="cx">
</span><span class="cx"> #if !ASSERT_WITH_SECURITY_IMPLICATION_DISABLED
</span><span class="cx"> private:
</span><del>- static const unsigned deletionSentinelNotDeletedValue = 0xF0F0F0F0U;
- static const unsigned deletionSentinelDeletedValue = 0xF0DEADF0U;
- unsigned m_deletionSentinel;
- bool m_hasBadParent;
</del><ins>+ static constexpr unsigned deletionSentinelNotDeletedValue = 0xF0F0F0F0U;
+ static constexpr unsigned deletionSentinelDeletedValue = 0xF0DEADF0U;
+ unsigned m_deletionSentinel { deletionSentinelNotDeletedValue };
+ bool m_hasBadParent { false };
</ins><span class="cx"> #endif
</span><span class="cx"> };
</span><span class="cx">
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (215116 => 215117)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog 2017-04-07 20:49:49 UTC (rev 215116)
+++ trunk/Tools/ChangeLog 2017-04-07 21:37:57 UTC (rev 215117)
</span><span class="lines">@@ -1,3 +1,16 @@
</span><ins>+2017-04-07 Myles C. Maxfield <mmaxfield@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):
+
</ins><span class="cx"> 2017-04-07 Ryan Haddad <ryanhaddad@apple.com>
</span><span class="cx">
</span><span class="cx"> [ios-simulator] API test WebKit2.WKWebProcessPlugInRangeHandle timing out
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWebCoreComplexTextControllercpp"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/ComplexTextController.cpp (215116 => 215117)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -355,4 +355,37 @@
</span><span class="cx"> EXPECT_NEAR(glyphBuffer.advanceAt(3).height(), 256 - 64, 0.0001);
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+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);
</ins><span class="cx"> }
</span><ins>+
+}
</ins></span></pre>
</div>
</div>
</body>
</html>