Revert r154384 and r154674 as they broke selection rect computations for text with...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Feb 2014 23:07:29 +0000 (23:07 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Feb 2014 23:07:29 +0000 (23:07 +0000)
Source/WebCore:

* 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:
* platform/graphics/WidthIterator.cpp:
(WebCore::WidthIterator::WidthIterator):
(WebCore::WidthIterator::advanceInternal):
(WebCore::WidthIterator::advanceOneCharacter):
* platform/graphics/WidthIterator.h:

LayoutTests:

* fast/text/partial-textruns-expected.html: Removed.
* fast/text/partial-textruns.html: Removed.
* fast/text/resources/PTS55F-webfont.ttf: Removed.

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

LayoutTests/ChangeLog
LayoutTests/fast/text/partial-textruns-expected.html [deleted file]
LayoutTests/fast/text/partial-textruns.html [deleted file]
LayoutTests/fast/text/resources/PTS55F-webfont.ttf [deleted file]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Font.cpp
Source/WebCore/platform/graphics/FontFastPath.cpp
Source/WebCore/platform/graphics/GlyphBuffer.h
Source/WebCore/platform/graphics/WidthIterator.cpp
Source/WebCore/platform/graphics/WidthIterator.h

index f44c283c9acac5b58a36feb5fbffbda53a5f51af..880185b83a654e85d34f5267b65a3d9b898d6d8d 100644 (file)
@@ -1,3 +1,11 @@
+2014-02-07  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Revert r154384 and r154674 as they broke selection rect computations for text with ligatures.
+
+        * fast/text/partial-textruns-expected.html: Removed.
+        * fast/text/partial-textruns.html: Removed.
+        * fast/text/resources/PTS55F-webfont.ttf: Removed.
+
 2014-02-07  Brendan Long  <b.long@cablelabs.com>
 
         Update TextTrack API to current spec
diff --git a/LayoutTests/fast/text/partial-textruns-expected.html b/LayoutTests/fast/text/partial-textruns-expected.html
deleted file mode 100644 (file)
index 063db4b..0000000
+++ /dev/null
@@ -1,37 +0,0 @@
-<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>
\ No newline at end of file
diff --git a/LayoutTests/fast/text/partial-textruns.html b/LayoutTests/fast/text/partial-textruns.html
deleted file mode 100644 (file)
index f118f0b..0000000
+++ /dev/null
@@ -1,35 +0,0 @@
-<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>
\ No newline at end of file
diff --git a/LayoutTests/fast/text/resources/PTS55F-webfont.ttf b/LayoutTests/fast/text/resources/PTS55F-webfont.ttf
deleted file mode 100644 (file)
index c530f69..0000000
Binary files a/LayoutTests/fast/text/resources/PTS55F-webfont.ttf and /dev/null differ
index 41b9cb7c36ef22e92bbe229d862c63062777cef1..e06bb15a00e312943bec8bd9c637023c676e2da5 100644 (file)
@@ -1,3 +1,23 @@
+2014-02-07  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Revert r154384 and r154674 as they broke selection rect computations for text with ligatures.
+
+        * 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:
+        * platform/graphics/WidthIterator.cpp:
+        (WebCore::WidthIterator::WidthIterator):
+        (WebCore::WidthIterator::advanceInternal):
+        (WebCore::WidthIterator::advanceOneCharacter):
+        * platform/graphics/WidthIterator.h:
+
 2014-02-07  Benjamin Poulain  <bpoulain@apple.com>
 
         [WK2] Add support for text document viewport configuration
index 456f26c0d383a3b9c20fa466deb9d1aeb385ab65..624694bd7c87414cef82bd68fdf2dce8bbf4c132 100644 (file)
@@ -334,7 +334,12 @@ float Font::drawText(GraphicsContext* context, const TextRun& run, const FloatPo
 
     to = (to == -1 ? run.length() : to);
 
-    if (codePath(run) != Complex)
+    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)
         return drawSimpleText(context, run, point, from, to);
 
     return drawComplexText(context, run, point, from, to);
@@ -348,7 +353,12 @@ void Font::drawEmphasisMarks(GraphicsContext* context, const TextRun& run, const
     if (to < 0)
         to = run.length();
 
-    if (codePath(run) != Complex)
+    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)
         drawEmphasisMarksForSimpleText(context, run, mark, point, from, to);
     else
         drawEmphasisMarksForComplexText(context, run, mark, point, from, to);
@@ -420,7 +430,12 @@ FloatRect Font::selectionRectForText(const TextRun& run, const FloatPoint& point
 {
     to = (to == -1 ? run.length() : to);
 
-    if (codePath(run) != Complex)
+    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)
         return selectionRectForSimpleText(run, point, h, from, to);
 
     return selectionRectForComplexText(run, point, h, from, to);
@@ -428,7 +443,8 @@ FloatRect Font::selectionRectForText(const TextRun& run, const FloatPoint& point
 
 int Font::offsetForPosition(const TextRun& run, float x, bool includePartialGlyphs) const
 {
-    if (codePath(run) != Complex)
+    // 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())
         return offsetForPositionForSimpleText(run, x, includePartialGlyphs);
 
     return offsetForPositionForComplexText(run, x, includePartialGlyphs);
index 20a281de95067590b01a0b4ad258db9589a92d8b..d985c30d96a424fe96bfa53d7f7a2b4a738114f7 100644 (file)
@@ -127,34 +127,32 @@ 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(run.length(), &localGlyphBuffer);
+    it.advance(from, &localGlyphBuffer);
+    float beforeWidth = it.m_runWidthSoFar;
+    it.advance(to, &glyphBuffer);
 
-    if (localGlyphBuffer.isEmpty())
+    if (glyphBuffer.isEmpty())
         return 0;
 
-    float totalWidth = it.m_runWidthSoFar;
-    float beforeWidth = 0;
-    int glyphPos = 0;
-    for (; glyphPos < localGlyphBuffer.size() && it.m_characterIndexOfGlyph[glyphPos] < from; ++glyphPos)
-        beforeWidth += localGlyphBuffer.advanceAt(glyphPos).width();
-    int glyphFrom = glyphPos;
-
-    float afterWidth = totalWidth;
-    glyphPos = localGlyphBuffer.size() - 1;
-    for (; glyphPos >= glyphFrom && it.m_characterIndexOfGlyph[glyphPos] >= to; --glyphPos)
-        afterWidth -= localGlyphBuffer.advanceAt(glyphPos).width();
-    int glyphTo = glyphPos + 1;
-
-    glyphBuffer.add(&localGlyphBuffer, glyphFrom, glyphTo - glyphFrom);
+    float afterWidth = it.m_runWidthSoFar;
 
     if (run.rtl()) {
+        float finalRoundingWidth = it.m_finalRoundingWidth;
+        it.advance(run.length(), &localGlyphBuffer);
+        initialAdvance = finalRoundingWidth + it.m_runWidthSoFar - afterWidth;
+    } else
+        initialAdvance = beforeWidth;
+
+    if (run.rtl())
         glyphBuffer.reverse(0, glyphBuffer.size());
-        return totalWidth - afterWidth;
-    }
 
-    return beforeWidth;
+    return initialAdvance;
 }
 
 float Font::drawSimpleText(GraphicsContext* context, const TextRun& run, const FloatPoint& point, int from, int to) const
@@ -301,22 +299,15 @@ FloatRect Font::selectionRectForSimpleText(const TextRun& run, const FloatPoint&
 {
     GlyphBuffer glyphBuffer;
     WidthIterator it(this, run);
-    it.advance(run.length(), &glyphBuffer);
-
-    float totalWidth = it.m_runWidthSoFar;
-    float beforeWidth = 0;
-    int glyphPos = 0;
-    for (; glyphPos < glyphBuffer.size() && it.m_characterIndexOfGlyph[glyphPos] < from; ++glyphPos)
-        beforeWidth += glyphBuffer.advanceAt(glyphPos).width();
-    int glyphFrom = glyphPos;
-
-    float afterWidth = totalWidth;
-    glyphPos = glyphBuffer.size() - 1;
-    for (; glyphPos >= glyphFrom && it.m_characterIndexOfGlyph[glyphPos] >= to; --glyphPos)
-        afterWidth -= glyphBuffer.advanceAt(glyphPos).width();
+    it.advance(from, &glyphBuffer);
+    float beforeWidth = it.m_runWidthSoFar;
+    it.advance(to, &glyphBuffer);
+    float afterWidth = it.m_runWidthSoFar;
 
     // 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);
     }
 
@@ -325,50 +316,45 @@ FloatRect Font::selectionRectForSimpleText(const TextRun& run, const FloatPoint&
 
 int Font::offsetForPositionForSimpleText(const TextRun& run, float x, bool includePartialGlyphs) const
 {
-    GlyphBuffer glyphBuffer;
-    WidthIterator it(this, run);
-    it.advance(run.length(), &glyphBuffer);
+    float delta = x;
 
-    int characterOffset = 0;
+    WidthIterator it(this, run);
+    GlyphBuffer localGlyphBuffer;
+    unsigned offset;
     if (run.rtl()) {
-        float currentX = it.m_runWidthSoFar;
-        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
-            if (glyphPosition == glyphBuffer.size()) {
-                characterOffset = run.length();
+        delta -= floatWidthForSimpleText(run);
+        while (1) {
+            offset = it.m_currentCharacter;
+            float w;
+            if (!it.advanceOneCharacter(w, localGlyphBuffer))
                 break;
-            }
-            characterOffset = it.m_characterIndexOfGlyph[glyphPosition];
-            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
+            delta += w;
             if (includePartialGlyphs) {
-                if (currentX - glyphWidth / 2.0f <= x)
+                if (delta - w / 2 >= 0)
                     break;
             } else {
-                if (currentX - glyphWidth <= x)
+                if (delta >= 0)
                     break;
             }
-            currentX -= glyphWidth;
         }
     } else {
-        float currentX = 0;
-        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
-            if (glyphPosition == glyphBuffer.size()) {
-                characterOffset = run.length();
+        while (1) {
+            offset = it.m_currentCharacter;
+            float w;
+            if (!it.advanceOneCharacter(w, localGlyphBuffer))
                 break;
-            }
-            characterOffset = it.m_characterIndexOfGlyph[glyphPosition];
-            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
+            delta -= w;
             if (includePartialGlyphs) {
-                if (currentX + glyphWidth / 2.0f >= x)
+                if (delta + w / 2 <= 0)
                     break;
             } else {
-                if (currentX + glyphWidth >= x)
+                if (delta <= 0)
                     break;
             }
-            currentX += glyphWidth;
         }
     }
 
-    return characterOffset;
+    return offset;
 }
 
-} // namespace WebCore
+}
index d72634cfd5b3eeee65b5b436c1a28648e7efca5b..e338456923daae80ccfd466eb6cb18eaa2251880 100644 (file)
@@ -122,16 +122,6 @@ public:
 #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);
index 15b0d8a4ea23fd5f9cdfa03a80e3f58adca8c45a..d97cf7a0e8ef1987926d143009ebf14064e07997 100644 (file)
@@ -66,8 +66,6 @@ WidthIterator::WidthIterator(const Font* font, const TextRun& run, HashSet<const
         else
             m_expansionPerOpportunity = m_expansion / expansionOpportunityCount;
     }
-    // Character-index will end up the same or slightly shorter than m_run, so if we reserve that much it will never need to resize.
-    m_characterIndexOfGlyph.reserveInitialCapacity(m_run.length());
 }
 
 GlyphData WidthIterator::glyphDataForCharacter(UChar32 character, bool mirror, int currentCharacter, unsigned& advanceLength)
@@ -174,8 +172,7 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
     CharactersTreatedAsSpace charactersTreatedAsSpace;
     while (textIterator.consume(character, clusterLength)) {
         unsigned advanceLength = clusterLength;
-        int currentCharacterIndex = textIterator.currentCharacter();
-        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, currentCharacterIndex, advanceLength);
+        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, textIterator.currentCharacter(), advanceLength);
         Glyph glyph = glyphData.glyph;
         const SimpleFontData* fontData = glyphData.fontData;
 
@@ -241,7 +238,6 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
                                     glyphBuffer->add(fontData->zeroWidthSpaceGlyph(), fontData, m_expansionPerOpportunity);
                                 else
                                     glyphBuffer->add(fontData->spaceGlyph(), fontData, expansionAtThisOpportunity);
-                                m_characterIndexOfGlyph.append(currentCharacterIndex);
                             } else
                                 glyphBuffer->expandLastAdvance(expansionAtThisOpportunity);
                         }
@@ -308,10 +304,8 @@ inline unsigned WidthIterator::advanceInternal(TextIterator& textIterator, Glyph
                 widthSinceLastRounding += width;
         }
 
-        if (glyphBuffer) {
+        if (glyphBuffer)
             glyphBuffer->add(glyph, fontData, (rtl ? oldWidth + lastRoundingWidth : width));
-            m_characterIndexOfGlyph.append(currentCharacterIndex);
-        }
 
         lastRoundingWidth = width - oldWidth;
 
@@ -351,4 +345,15 @@ 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;
+}
+
 }
index 9a39d3b54959c58b97f214b7a87418040e703daf..cd06d7c4dcc4120a66a3d04487ae88fdca1398b6 100644 (file)
@@ -42,6 +42,7 @@ public:
     WidthIterator(const Font*, const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, bool accountForGlyphBounds = false, bool forTextEmphasis = false);
 
     unsigned advance(int to, GlyphBuffer*);
+    bool advanceOneCharacter(float& width, GlyphBuffer&);
 
     float maxGlyphBoundingBoxY() const { ASSERT(m_accountForGlyphBounds); return m_maxGlyphBoundingBoxY; }
     float minGlyphBoundingBoxY() const { ASSERT(m_accountForGlyphBounds); return m_minGlyphBoundingBoxY; }
@@ -79,8 +80,6 @@ public:
     float m_expansionPerOpportunity;
     bool m_isAfterExpansion;
     float m_finalRoundingWidth;
-    // An inline capacity of 10 catches around 2/3 of the cases. To catch 90% we would need 32.
-    Vector<int, 10> m_characterIndexOfGlyph;
 
 #if ENABLE(SVG_FONTS)
     String m_lastGlyphName;