Font’s fast code path doesn’t handle partial runs correctly when kern…

…ing or ligatures are enabled https://bugs.webkit.org/show_bug.cgi?id=100050 Reviewed by Antti Koivisto. Source/WebCore: Always let WidthIterator iterate over an entire TextRun to avoid problems with pixel rounding or shaping on partial runs. This fix is necessary for Qt because the complex font-path can not disable shaping, leading to the complex path painting slighly different from the fast path, which messes up selection painting. No change in functionality, no new tests. * platform/graphics/Font.cpp: (WebCore::Font::drawText): (WebCore::Font::drawEmphasisMarks): (WebCore::Font::selectionRectForText): (WebCore::Font::offsetForPosition): * platform/graphics/FontFastPath.cpp: (WebCore::Font::getGlyphsAndAdvancesForSimpleText): (WebCore::Font::selectionRectForSimpleText): (WebCore::Font::offsetForPositionForSimpleText): * platform/graphics/GlyphBuffer.h: (WebCore::GlyphBuffer::add): (GlyphBuffer): * platform/graphics/WidthIterator.cpp: (WebCore::WidthIterator::advanceInternal): * platform/graphics/WidthIterator.h: (WidthIterator): Removed now unused advanceOneCharacter method. LayoutTests: * fast/text/resources/PTS55F-webfont.ttf: Added. * fast/text/partial-textruns-expected.html: Added. * fast/text/partial-textruns.html: Added. Canonical link: https://commits.webkit.org/138048@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@154384 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Aug 21, 2013 · 7979e07 · 7979e07
1 parent f44cd3d
commit 7979e07
Show file tree

Hide file tree

Showing 10 changed files with 195 additions and 78 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,14 @@
+2013-08-21  Allan Sandfeld Jensen  <allan.jensen@digia.com>
+
+        Font’s fast code path doesn’t handle partial runs correctly when kerning or ligatures are enabled 
+        https://bugs.webkit.org/show_bug.cgi?id=100050
+
+        Reviewed by Antti Koivisto.
+
+        * fast/text/resources/PTS55F-webfont.ttf: Added.
+        * fast/text/partial-textruns-expected.html: Added.
+        * fast/text/partial-textruns.html: Added.
+
 2013-08-20  Antonio Gomes  <a1.gomes@sisa.samsung.com>
 
         Harden RenderBox::canBeScrolledAndHasScrollableArea logic

diff --git a/LayoutTests/fast/text/partial-textruns-expected.html b/LayoutTests/fast/text/partial-textruns-expected.html
@@ -0,0 +1,37 @@
+<html>
+<head>
+<style>
+    @font-face {
+        font-family: PTSans;
+        src: url('resources/PTS55F-webfont.ttf') format("truetype");
+        font-weight: normal;
+        font-style: normal;
+    }
+    #sandbox { position: absolute; width: 400px; }
+    article { 
+        width: 400px;
+        font-family: PTSans;
+        font-size: 24px;
+        -webkit-font-kerning: normal;
+    }
+</style>
+</head>
+<body>
+<div id=sandbox>
+<article>
+This test is meant <span id=selection>to test if font metrics change when partially rendered. To test select multiple lines with the begin</span> or end line being only partially selected. The test succedes if none of the letters in the partially selected lines shift compared to when they are not selected or the line fully selected.
+</article>
+</div>
+
+<script>
+    var selection = window.getSelection();
+    if (selection.rangeCount > 0)
+        selection.removeAllRanges();
+    var selectionNode = document.getElementById("selection");
+    var range = document.createRange();
+    range.selectNode(selectionNode);
+    selection.addRange(range);
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/fast/text/partial-textruns.html b/LayoutTests/fast/text/partial-textruns.html
@@ -0,0 +1,35 @@
+<html>
+<head>
+<style>
+    @font-face {
+        font-family: PTSans;
+        src: url('resources/PTS55F-webfont.ttf') format("truetype");
+        font-weight: normal;
+        font-style: normal;
+    }
+    #sandbox { position: absolute; width: 400px; }
+    article { 
+        width: 400px;
+        font-family: PTSans;
+        font-size: 24px;
+        -webkit-font-kerning: normal;
+    }
+</style>
+</head>
+<body>
+<div id=sandbox>
+<article id=article>This test is meant to test if font metrics change when partially rendered. To test select multiple lines with the begin or end line being only partially selected. The test succedes if none of the letters in the partially selected lines shift compared to when they are not selected or the line fully selected.</article>
+</div>
+<script>
+    var selection = window.getSelection();
+    if (selection.rangeCount > 0)
+        selection.removeAllRanges();
+    var article = document.getElementById("article");
+    var range = document.createRange();
+    range.setStart(article.firstChild, 19);
+    range.setEnd(article.firstChild, 119);
+    selection.addRange(range);
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/fast/text/resources/PTS55F-webfont.ttf b/LayoutTests/fast/text/resources/PTS55F-webfont.ttf
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,36 @@
+2013-08-21  Allan Sandfeld Jensen  <allan.jensen@digia.com>
+
+        Font’s fast code path doesn’t handle partial runs correctly when kerning or ligatures are enabled 
+        https://bugs.webkit.org/show_bug.cgi?id=100050
+
+        Reviewed by Antti Koivisto.
+
+        Always let WidthIterator iterate over an entire TextRun to avoid problems
+        with pixel rounding or shaping on partial runs.
+
+        This fix is necessary for Qt because the complex font-path can not disable
+        shaping, leading to the complex path painting slighly different from the
+        fast path, which messes up selection painting.
+
+        No change in functionality, no new tests.
+
+        * platform/graphics/Font.cpp:
+        (WebCore::Font::drawText):
+        (WebCore::Font::drawEmphasisMarks):
+        (WebCore::Font::selectionRectForText):
+        (WebCore::Font::offsetForPosition):
+        * platform/graphics/FontFastPath.cpp:
+        (WebCore::Font::getGlyphsAndAdvancesForSimpleText):
+        (WebCore::Font::selectionRectForSimpleText):
+        (WebCore::Font::offsetForPositionForSimpleText):
+        * platform/graphics/GlyphBuffer.h:
+        (WebCore::GlyphBuffer::add):
+        (GlyphBuffer):
+        * platform/graphics/WidthIterator.cpp:
+        (WebCore::WidthIterator::advanceInternal):
+        * platform/graphics/WidthIterator.h:
+        (WidthIterator): Removed now unused advanceOneCharacter method.
+
 2013-08-20  Antonio Gomes  <a1.gomes@sisa.samsung.com>
 
         Harden RenderBox::canBeScrolledAndHasScrollableArea logic

diff --git a/Source/WebCore/platform/graphics/Font.cpp b/Source/WebCore/platform/graphics/Font.cpp
@@ -264,12 +264,7 @@ void Font::drawText(GraphicsContext* context, const TextRun& run, const FloatPoi
 
     to = (to == -1 ? run.length() : to);
 
-    CodePath codePathToUse = codePath(run);
-    // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
-    if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
-        codePathToUse = Complex;
-
-    if (codePathToUse != Complex)
+    if (codePath(run) != Complex)
         return drawSimpleText(context, run, point, from, to);
 
     return drawComplexText(context, run, point, from, to);
@@ -283,12 +278,7 @@ void Font::drawEmphasisMarks(GraphicsContext* context, const TextRun& run, const
     if (to < 0)
         to = run.length();
 
-    CodePath codePathToUse = codePath(run);
-    // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
-    if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
-        codePathToUse = Complex;
-
-    if (codePathToUse != Complex)
+    if (codePath(run) != Complex)
         drawEmphasisMarksForSimpleText(context, run, mark, point, from, to);
     else
         drawEmphasisMarksForComplexText(context, run, mark, point, from, to);
@@ -360,21 +350,15 @@ FloatRect Font::selectionRectForText(const TextRun& run, const FloatPoint& point
 {
     to = (to == -1 ? run.length() : to);
 
-    CodePath codePathToUse = codePath(run);
-    // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
-    if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
-        codePathToUse = Complex;
-
-    if (codePathToUse != Complex)
+    if (codePath(run) != Complex)
         return selectionRectForSimpleText(run, point, h, from, to);
 
     return selectionRectForComplexText(run, point, h, from, to);
 }
 
 int Font::offsetForPosition(const TextRun& run, float x, bool includePartialGlyphs) const
 {
-    // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
-    if (codePath(run) != Complex && !typesettingFeatures())
+    if (codePath(run) != Complex)
         return offsetForPositionForSimpleText(run, x, includePartialGlyphs);
 
     return offsetForPositionForComplexText(run, x, includePartialGlyphs);

diff --git a/Source/WebCore/platform/graphics/FontFastPath.cpp b/Source/WebCore/platform/graphics/FontFastPath.cpp
@@ -129,32 +129,34 @@ int Font::emphasisMarkHeight(const AtomicString& mark) const
 
 float Font::getGlyphsAndAdvancesForSimpleText(const TextRun& run, int from, int to, GlyphBuffer& glyphBuffer, ForTextEmphasisOrNot forTextEmphasis) const
 {
-    float initialAdvance;
-
     WidthIterator it(this, run, 0, false, forTextEmphasis);
-    // FIXME: Using separate glyph buffers for the prefix and the suffix is incorrect when kerning or
-    // ligatures are enabled.
     GlyphBuffer localGlyphBuffer;
-    it.advance(from, &localGlyphBuffer);
-    float beforeWidth = it.m_runWidthSoFar;
-    it.advance(to, &glyphBuffer);
+    it.advance(run.length(), &localGlyphBuffer);
 
-    if (glyphBuffer.isEmpty())
+    if (localGlyphBuffer.isEmpty())
         return 0;
 
-    float afterWidth = it.m_runWidthSoFar;
+    float totalWidth = it.m_runWidthSoFar;
+    float beforeWidth = 0;
+    int glyphPos = 0;
+    for (; glyphPos < localGlyphBuffer.size() && it.m_characterIndex[glyphPos] < from; ++glyphPos)
+        beforeWidth += localGlyphBuffer.advanceAt(glyphPos).width();
+    int glyphFrom = glyphPos;
 
-    if (run.rtl()) {
-        float finalRoundingWidth = it.m_finalRoundingWidth;
-        it.advance(run.length(), &localGlyphBuffer);
-        initialAdvance = finalRoundingWidth + it.m_runWidthSoFar - afterWidth;
-    } else
-        initialAdvance = beforeWidth;
+    float afterWidth = totalWidth;
+    glyphPos = localGlyphBuffer.size() - 1;
+    for (; glyphPos >= glyphFrom && it.m_characterIndex[glyphPos] >= to; --glyphPos)
+        afterWidth -= localGlyphBuffer.advanceAt(glyphPos).width();
+    int glyphTo = glyphPos + 1;
+
+    glyphBuffer.add(&localGlyphBuffer, glyphFrom, glyphTo - glyphFrom);
 
-    if (run.rtl())
+    if (run.rtl()) {
         glyphBuffer.reverse(0, glyphBuffer.size());
+        return totalWidth - afterWidth;
+    }
 
-    return initialAdvance;
+    return beforeWidth;
 }
 
 void Font::drawSimpleText(GraphicsContext* context, const TextRun& run, const FloatPoint& point, int from, int to) const
@@ -296,15 +298,22 @@ FloatRect Font::selectionRectForSimpleText(const TextRun& run, const FloatPoint&
 {
     GlyphBuffer glyphBuffer;
     WidthIterator it(this, run);
-    it.advance(from, &glyphBuffer);
-    float beforeWidth = it.m_runWidthSoFar;
-    it.advance(to, &glyphBuffer);
-    float afterWidth = it.m_runWidthSoFar;
+    it.advance(run.length(), &glyphBuffer);
+
+    float totalWidth = it.m_runWidthSoFar;
+    float beforeWidth = 0;
+    int glyphPos = 0;
+    for (; glyphPos < glyphBuffer.size() && it.m_characterIndex[glyphPos] < from; ++glyphPos)
+        beforeWidth += glyphBuffer.advanceAt(glyphPos).width();
+    int glyphFrom = glyphPos;
+
+    float afterWidth = totalWidth;
+    glyphPos = glyphBuffer.size() - 1;
+    for (; glyphPos >= glyphFrom && it.m_characterIndex[glyphPos] >= to; --glyphPos)
+        afterWidth -= glyphBuffer.advanceAt(glyphPos).width();
 
     // Using roundf() rather than ceilf() for the right edge as a compromise to ensure correct caret positioning.
     if (run.rtl()) {
-        it.advance(run.length(), &glyphBuffer);
-        float totalWidth = it.m_runWidthSoFar;
         return FloatRect(floorf(point.x() + totalWidth - afterWidth), point.y(), roundf(point.x() + totalWidth - beforeWidth) - floorf(point.x() + totalWidth - afterWidth), h);
     }
 
@@ -313,45 +322,50 @@ FloatRect Font::selectionRectForSimpleText(const TextRun& run, const FloatPoint&
 
 int Font::offsetForPositionForSimpleText(const TextRun& run, float x, bool includePartialGlyphs) const
 {
-    float delta = x;
-
+    GlyphBuffer glyphBuffer;
     WidthIterator it(this, run);
-    GlyphBuffer localGlyphBuffer;
-    unsigned offset;
+    it.advance(run.length(), &glyphBuffer);
+
+    int characterOffset = 0;
     if (run.rtl()) {
-        delta -= floatWidthForSimpleText(run);
-        while (1) {
-            offset = it.m_currentCharacter;
-            float w;
-            if (!it.advanceOneCharacter(w, localGlyphBuffer))
+        float currentX = it.m_runWidthSoFar;
+        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
+            if (glyphPosition == glyphBuffer.size()) {
+                characterOffset = run.length();
                 break;
-            delta += w;
+            }
+            characterOffset = it.m_characterIndex[glyphPosition];
+            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
             if (includePartialGlyphs) {
-                if (delta - w / 2 >= 0)
+                if (currentX - glyphWidth / 2.0f <= x)
                     break;
             } else {
-                if (delta >= 0)
+                if (currentX - glyphWidth <= x)
                     break;
             }
+            currentX -= glyphWidth;
         }
     } else {
-        while (1) {
-            offset = it.m_currentCharacter;
-            float w;
-            if (!it.advanceOneCharacter(w, localGlyphBuffer))
+        float currentX = 0;
+        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
+            if (glyphPosition == glyphBuffer.size()) {
+                characterOffset = run.length();
                 break;
-            delta -= w;
+            }
+            characterOffset = it.m_characterIndex[glyphPosition];
+            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
             if (includePartialGlyphs) {
-                if (delta + w / 2 <= 0)
+                if (currentX + glyphWidth / 2.0f >= x)
                     break;
             } else {
-                if (delta <= 0)
+                if (currentX + glyphWidth >= x)
                     break;
             }
+            currentX += glyphWidth;
         }
     }
 
-    return offset;
+    return characterOffset;
 }
 
-}
+} // namespace WebCore
diff --git a/Source/WebCore/platform/graphics/GlyphBuffer.h b/Source/WebCore/platform/graphics/GlyphBuffer.h
@@ -139,6 +139,16 @@ class GlyphBuffer {
 #endif
     }
 
+    void add(const GlyphBuffer* glyphBuffer, int from, int len)
+    {
+        m_glyphs.append(glyphBuffer->glyphs(from), len);
+        m_advances.append(glyphBuffer->advances(from), len);
+        m_fontData.append(glyphBuffer->m_fontData.data() + from, len);
+#if PLATFORM(WIN)
+        m_offsets.append(glyphBuffer->m_offsets.data() + from, len);
+#endif
+    }
+
     void add(Glyph glyph, const SimpleFontData* font, float width, const FloatSize* offset = 0)
     {
         m_fontData.append(font);

diff --git a/Source/WebCore/platform/graphics/WidthIterator.cpp b/Source/WebCore/platform/graphics/WidthIterator.cpp
@@ -162,7 +162,8 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
     CharactersTreatedAsSpace charactersTreatedAsSpace;
     while (textIterator.consume(character, clusterLength)) {
         unsigned advanceLength = clusterLength;
-        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, textIterator.currentCharacter(), advanceLength);
+        int currentCharacterIndex = textIterator.currentCharacter();
+        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, currentCharacterIndex, advanceLength);
         Glyph glyph = glyphData.glyph;
         const SimpleFontData* fontData = glyphData.fontData;
 
@@ -230,6 +231,7 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
                                     glyphBuffer->add(fontData->zeroWidthSpaceGlyph(), fontData, m_expansionPerOpportunity);
                                 else
                                     glyphBuffer->add(fontData->spaceGlyph(), fontData, expansionAtThisOpportunity);
+                                m_characterIndex.append(currentCharacterIndex);
                             } else
                                 glyphBuffer->expandLastAdvance(expansionAtThisOpportunity);
                         }
@@ -296,8 +298,10 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
                 widthSinceLastRounding += width;
         }
 
-        if (glyphBuffer)
+        if (glyphBuffer) {
             glyphBuffer->add(glyph, fontData, (rtl ? oldWidth + lastRoundingWidth : width));
+            m_characterIndex.append(currentCharacterIndex);
+        }
 
         lastRoundingWidth = width - oldWidth;
 
@@ -337,15 +341,4 @@ unsigned WidthIterator::advance(int offset, GlyphBuffer* glyphBuffer)
     return advanceInternal(textIterator, glyphBuffer);
 }
 
-bool WidthIterator::advanceOneCharacter(float& width, GlyphBuffer& glyphBuffer)
-{
-    int oldSize = glyphBuffer.size();
-    advance(m_currentCharacter + 1, &glyphBuffer);
-    float w = 0;
-    for (int i = oldSize; i < glyphBuffer.size(); ++i)
-        w += glyphBuffer.advanceAt(i).width();
-    width = w;
-    return glyphBuffer.size() > oldSize;
-}
-
 }