REGRESSION (r222670 and r222732): RTL truncated text may not be drawn
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Oct 2017 17:04:51 +0000 (17:04 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Oct 2017 17:04:51 +0000 (17:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=178278
<rdar://problem/34982818>

Reviewed by Darin Adler.

Source/WebCore:

Revert r222732 and partially revert r222670. The underlying font rendering machinery implements
text truncation by taking a TextRun object that represents all of the text in the line fragment
and a subrange of the glyphs to render from this fragment. Only the glyphs in this subrange are
drawn and they are drawn in the same position they would be in had the entire line fragment been
drawn. Following r222670 InlineTextBox applies the truncation to the TextRun in InlineTextBox::text().
Together with r222732, which assumed that the number of glyphs to draw is equal to the length of
the TextRun, a truncated text run would be drawn at the wrong position on screen and could give
the impression that the text is not drawn. Instead InlineTextBox::text() should always return
the text for the entire line fragment without considering truncation and when calling TextPainter::paint()
we need to pass the truncated length of the line fragment.

Test: fast/text/ellipsis-text-rtl.html

* rendering/InlineTextBox.cpp:
(WebCore::InlineTextBox::paint): Compute the truncated length (number of glyphs) and pass this
to TextPainter::paint()
(WebCore::InlineTextBox::text const): Do not apply truncation to the text run. Truncation is
implemented by telling the underlying font rendering machinery to paint the subrange of the
text run that represents the non-truncated (visible) text.
* rendering/InlineTextBox.h:
* rendering/SimpleLineLayoutFunctions.cpp:
(WebCore::SimpleLineLayout::paintFlow): Pass the entire length of the text run as we did prior
to r222732.
* rendering/TextPainter.cpp:
(WebCore::TextPainter::paint): Take a length that represents the number of glyphs to draw from
the text run as we use to take prior to r222732.
* rendering/TextPainter.h:

LayoutTests:

Add a test to ensure that we draw right-to-left truncated text correctly.

* fast/text/ellipsis-text-rtl-expected.html: Added.
* fast/text/ellipsis-text-rtl.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@223552 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/text/ellipsis-text-rtl-expected.html [new file with mode: 0644]
LayoutTests/fast/text/ellipsis-text-rtl.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/InlineTextBox.cpp
Source/WebCore/rendering/InlineTextBox.h
Source/WebCore/rendering/SimpleLineLayoutFunctions.cpp
Source/WebCore/rendering/TextPainter.cpp
Source/WebCore/rendering/TextPainter.h

index 44110e9..5078fe1 100644 (file)
@@ -1,3 +1,16 @@
+2017-10-17  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r222670 and r222732): RTL truncated text may not be drawn
+        https://bugs.webkit.org/show_bug.cgi?id=178278
+        <rdar://problem/34982818>
+
+        Reviewed by Darin Adler.
+
+        Add a test to ensure that we draw right-to-left truncated text correctly.
+
+        * fast/text/ellipsis-text-rtl-expected.html: Added.
+        * fast/text/ellipsis-text-rtl.html: Added.
+
 2017-10-10  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] __proto__ getter should be fast
diff --git a/LayoutTests/fast/text/ellipsis-text-rtl-expected.html b/LayoutTests/fast/text/ellipsis-text-rtl-expected.html
new file mode 100644 (file)
index 0000000..cd7a7df
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+@font-face {
+    font-family: "Ahem";
+    src: url("../../resources/Ahem.ttf");
+}
+
+#expected {
+    font: 20px/1 "Ahem";
+    overflow: hidden;
+}
+</style>
+</head>
+<body>
+<div id="expected">ab</div>
+</body>
+</html>
diff --git a/LayoutTests/fast/text/ellipsis-text-rtl.html b/LayoutTests/fast/text/ellipsis-text-rtl.html
new file mode 100644 (file)
index 0000000..0c9a2ba
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+@font-face {
+    font-family: "Ahem";
+    src: url("../../resources/Ahem.ttf");
+}
+
+#test {
+    font: 20px/1 "Ahem";
+    direction: rtl;
+    unicode-bidi: bidi-override;
+    text-overflow: ellipsis;
+    overflow: hidden;
+    white-space: nowrap;
+    width: 40px;
+}
+</style>
+</head>
+<body>
+<div id="test">abcXYZ</div>
+</body>
+</html>
index 24c8152..abd7129 100644 (file)
@@ -1,3 +1,39 @@
+2017-10-17  Daniel Bates  <dabates@apple.com>
+
+        REGRESSION (r222670 and r222732): RTL truncated text may not be drawn
+        https://bugs.webkit.org/show_bug.cgi?id=178278
+        <rdar://problem/34982818>
+
+        Reviewed by Darin Adler.
+
+        Revert r222732 and partially revert r222670. The underlying font rendering machinery implements
+        text truncation by taking a TextRun object that represents all of the text in the line fragment
+        and a subrange of the glyphs to render from this fragment. Only the glyphs in this subrange are
+        drawn and they are drawn in the same position they would be in had the entire line fragment been
+        drawn. Following r222670 InlineTextBox applies the truncation to the TextRun in InlineTextBox::text().
+        Together with r222732, which assumed that the number of glyphs to draw is equal to the length of
+        the TextRun, a truncated text run would be drawn at the wrong position on screen and could give
+        the impression that the text is not drawn. Instead InlineTextBox::text() should always return
+        the text for the entire line fragment without considering truncation and when calling TextPainter::paint()
+        we need to pass the truncated length of the line fragment.
+
+        Test: fast/text/ellipsis-text-rtl.html
+
+        * rendering/InlineTextBox.cpp:
+        (WebCore::InlineTextBox::paint): Compute the truncated length (number of glyphs) and pass this
+        to TextPainter::paint()
+        (WebCore::InlineTextBox::text const): Do not apply truncation to the text run. Truncation is
+        implemented by telling the underlying font rendering machinery to paint the subrange of the
+        text run that represents the non-truncated (visible) text.
+        * rendering/InlineTextBox.h:
+        * rendering/SimpleLineLayoutFunctions.cpp:
+        (WebCore::SimpleLineLayout::paintFlow): Pass the entire length of the text run as we did prior
+        to r222732.
+        * rendering/TextPainter.cpp:
+        (WebCore::TextPainter::paint): Take a length that represents the number of glyphs to draw from
+        the text run as we use to take prior to r222732.
+        * rendering/TextPainter.h:
+
 2017-10-17  Zalan Bujtas  <zalan@apple.com>
 
         [FrameView::layout cleanup] Move text auto sizing logic to a separate function
index 2cf2f04..af8b81a 100644 (file)
@@ -491,6 +491,8 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
     auto text = this->text();
     TextRun textRun = createTextRun(text);
     unsigned length = textRun.length();
+    if (m_truncation != cNoTruncation)
+        length = m_truncation;
 
     unsigned selectionStart = 0;
     unsigned selectionEnd = 0;
@@ -549,7 +551,7 @@ void InlineTextBox::paint(PaintInfo& paintInfo, const LayoutPoint& paintOffset,
         if (currentEnd < length)
             textPainter.paintRange(textRun, boxRect, textOrigin, currentEnd, length);
     } else
-        textPainter.paint(textRun, boxRect, textOrigin, selectionStart, selectionEnd, paintSelectedTextOnly, paintSelectedTextSeparately, paintNonSelectedTextOnly);
+        textPainter.paint(textRun, length, boxRect, textOrigin, selectionStart, selectionEnd, paintSelectedTextOnly, paintSelectedTextSeparately, paintNonSelectedTextOnly);
 
     // Paint decorations
     TextDecoration textDecorations = lineStyle.textDecorationsInEffect();
@@ -1077,8 +1079,6 @@ TextRun InlineTextBox::createTextRun(String& string) const
 
 String InlineTextBox::text(bool ignoreCombinedText, bool ignoreHyphen) const
 {
-    if (UNLIKELY(m_truncation == cFullTruncation))
-        return emptyString();
     if (auto* combinedText = this->combinedText()) {
         if (ignoreCombinedText)
             return renderer().text()->substring(m_start, m_len);
@@ -1089,7 +1089,7 @@ String InlineTextBox::text(bool ignoreCombinedText, bool ignoreHyphen) const
             return renderer().text()->substring(m_start, m_len);
         return makeString(StringView(renderer().text()).substring(m_start, m_len), lineStyle().hyphenString());
     }
-    return renderer().text()->substring(m_start, m_truncation != cNoTruncation ? m_truncation : m_len);
+    return renderer().text()->substring(m_start, m_len);
 }
 
 inline const RenderCombineText* InlineTextBox::combinedText() const
index 506367a..ba55d3e 100644 (file)
@@ -167,7 +167,7 @@ private:
     const RenderCombineText* combinedText() const;
     const FontCascade& lineFont() const;
 
-    String text(bool ignoreCombinedText = false, bool ignoreHyphen = false) const; // The text to render.
+    String text(bool ignoreCombinedText = false, bool ignoreHyphen = false) const; // The effective text for the run.
     TextRun createTextRun(String&) const;
 
     ExpansionBehavior expansionBehavior() const;
index a8943b1..06b61cc 100644 (file)
@@ -120,7 +120,7 @@ void paintFlow(const RenderBlockFlow& flow, const Layout& layout, PaintInfo& pai
         TextRun textRun(run.hasHyphen() ? textWithHyphen : run.text(), 0, run.expansion(), run.expansionBehavior());
         textRun.setTabSize(!style.collapseWhiteSpace(), style.tabSize());
         FloatPoint textOrigin = FloatPoint(rect.x() + paintOffset.x(), roundToDevicePixel(run.baselinePosition() + paintOffset.y(), deviceScaleFactor));
-        textPainter.paint(textRun, rect, textOrigin);
+        textPainter.paint(textRun, textRun.length(), rect, textOrigin);
         if (textDecorationPainter) {
             textDecorationPainter->setWidth(rect.width());
             textDecorationPainter->paintTextDecoration(textRun, textOrigin, rect.location() + paintOffset);
index 8dc378f..50a7b42 100644 (file)
@@ -190,11 +190,11 @@ void TextPainter::paintRange(const TextRun& textRun, const FloatRect& boxRect, c
     paintTextAndEmphasisMarksIfNeeded(textRun, boxRect, textOrigin, start, end, m_style, m_shadow);
 }
     
-void TextPainter::paint(const TextRun& textRun, const FloatRect& boxRect, const FloatPoint& textOrigin, unsigned selectionStart, unsigned selectionEnd,
+void TextPainter::paint(const TextRun& textRun, unsigned length, const FloatRect& boxRect, const FloatPoint& textOrigin, unsigned selectionStart, unsigned selectionEnd,
     bool paintSelectedTextOnly, bool paintSelectedTextSeparately, bool paintNonSelectedTextOnly)
 {
     ASSERT(m_font);
-    unsigned length = textRun.length();
+    ASSERT(length <= textRun.length());
     if (!paintSelectedTextOnly) {
         // For stroked painting, we have to change the text drawing mode. It's probably dangerous to leave that mutated as a side
         // effect, so only when we know we're stroking, do a save/restore.
index c74d9ea..6dc104b 100644 (file)
@@ -62,7 +62,7 @@ public:
     void setEmphasisMark(const AtomicString& mark, float offset, const RenderCombineText*);
 
     void paintRange(const TextRun&, const FloatRect& boxRect, const FloatPoint& textOrigin, unsigned start, unsigned end);
-    void paint(const TextRun&, const FloatRect& boxRect, const FloatPoint& textOrigin, unsigned selectionStart = 0, unsigned selectionEnd = 0, bool paintSelectedTextOnly = false, bool paintSelectedTextSeparately = false, bool paintNonSelectedTextOnly = false);
+    void paint(const TextRun&, unsigned length, const FloatRect& boxRect, const FloatPoint& textOrigin, unsigned selectionStart = 0, unsigned selectionEnd = 0, bool paintSelectedTextOnly = false, bool paintSelectedTextSeparately = false, bool paintNonSelectedTextOnly = false);
 
 private:
     void paintTextOrEmphasisMarks(const FontCascade&, const TextRun&, const AtomicString& emphasisMark, float emphasisMarkOffset,