Changeset 154384 in webkit


Ignore:
Timestamp:
Aug 21, 2013 6:43:00 AM (11 years ago)
Author:
allan.jensen@digia.com
Message:

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.

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.
Location:
trunk
Files:
3 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r154383 r154384  
     12013-08-21  Allan Sandfeld Jensen  <allan.jensen@digia.com>
     2
     3        Font’s fast code path doesn’t handle partial runs correctly when kerning or ligatures are enabled
     4        https://bugs.webkit.org/show_bug.cgi?id=100050
     5
     6        Reviewed by Antti Koivisto.
     7
     8        * fast/text/resources/PTS55F-webfont.ttf: Added.
     9        * fast/text/partial-textruns-expected.html: Added.
     10        * fast/text/partial-textruns.html: Added.
     11
    1122013-08-20  Antonio Gomes  <a1.gomes@sisa.samsung.com>
    213

  • trunk/Source/WebCore/ChangeLog

    r154383 r154384  
     12013-08-21  Allan Sandfeld Jensen  <allan.jensen@digia.com>
     2
     3        Font’s fast code path doesn’t handle partial runs correctly when kerning or ligatures are enabled
     4        https://bugs.webkit.org/show_bug.cgi?id=100050
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Always let WidthIterator iterate over an entire TextRun to avoid problems
     9        with pixel rounding or shaping on partial runs.
     10
     11        This fix is necessary for Qt because the complex font-path can not disable
     12        shaping, leading to the complex path painting slighly different from the
     13        fast path, which messes up selection painting.
     14
     15        No change in functionality, no new tests.
     16
     17        * platform/graphics/Font.cpp:
     18        (WebCore::Font::drawText):
     19        (WebCore::Font::drawEmphasisMarks):
     20        (WebCore::Font::selectionRectForText):
     21        (WebCore::Font::offsetForPosition):
     22        * platform/graphics/FontFastPath.cpp:
     23        (WebCore::Font::getGlyphsAndAdvancesForSimpleText):
     24        (WebCore::Font::selectionRectForSimpleText):
     25        (WebCore::Font::offsetForPositionForSimpleText):
     26        * platform/graphics/GlyphBuffer.h:
     27        (WebCore::GlyphBuffer::add):
     28        (GlyphBuffer):
     29        * platform/graphics/WidthIterator.cpp:
     30        (WebCore::WidthIterator::advanceInternal):
     31        * platform/graphics/WidthIterator.h:
     32        (WidthIterator): Removed now unused advanceOneCharacter method.
     33
    1342013-08-20  Antonio Gomes  <a1.gomes@sisa.samsung.com>
    235

  • trunk/Source/WebCore/platform/graphics/Font.cpp

    r152653 r154384  
    265265    to = (to == -1 ? run.length() : to);
    266266
    267     CodePath codePathToUse = codePath(run);
    268     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
    269     if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
    270         codePathToUse = Complex;
    271 
    272     if (codePathToUse != Complex)
     267    if (codePath(run) != Complex)
    273268        return drawSimpleText(context, run, point, from, to);
    274269
    … …  
    284279        to = run.length();
    285280
    286     CodePath codePathToUse = codePath(run);
    287     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
    288     if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
    289         codePathToUse = Complex;
    290 
    291     if (codePathToUse != Complex)
     281    if (codePath(run) != Complex)
    292282        drawEmphasisMarksForSimpleText(context, run, mark, point, from, to);
    293283    else
    … …  
    361351    to = (to == -1 ? run.length() : to);
    362352
    363     CodePath codePathToUse = codePath(run);
    364     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
    365     if (codePathToUse != Complex && typesettingFeatures() && (from || to != run.length()) && !run.renderingContext())
    366         codePathToUse = Complex;
    367 
    368     if (codePathToUse != Complex)
     353    if (codePath(run) != Complex)
    369354        return selectionRectForSimpleText(run, point, h, from, to);
    370355
    … …  
    374359int Font::offsetForPosition(const TextRun& run, float x, bool includePartialGlyphs) const
    375360{
    376     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
    377     if (codePath(run) != Complex && !typesettingFeatures())
     361    if (codePath(run) != Complex)
    378362        return offsetForPositionForSimpleText(run, x, includePartialGlyphs);
    379363

  • trunk/Source/WebCore/platform/graphics/FontFastPath.cpp

    r150956 r154384  
    130130float Font::getGlyphsAndAdvancesForSimpleText(const TextRun& run, int from, int to, GlyphBuffer& glyphBuffer, ForTextEmphasisOrNot forTextEmphasis) const
    131131{
    132     float initialAdvance;
    133 
    134132    WidthIterator it(this, run, 0, false, forTextEmphasis);
    135     // FIXME: Using separate glyph buffers for the prefix and the suffix is incorrect when kerning or
    136     // ligatures are enabled.
    137133    GlyphBuffer localGlyphBuffer;
    138     it.advance(from, &localGlyphBuffer);
    139     float beforeWidth = it.m_runWidthSoFar;
    140     it.advance(to, &glyphBuffer);
    141 
    142     if (glyphBuffer.isEmpty())
    143         return 0;
    144 
    145     float afterWidth = it.m_runWidthSoFar;
     134    it.advance(run.length(), &localGlyphBuffer);
     135
     136    if (localGlyphBuffer.isEmpty())
     137        return 0;
     138
     139    float totalWidth = it.m_runWidthSoFar;
     140    float beforeWidth = 0;
     141    int glyphPos = 0;
     142    for (; glyphPos < localGlyphBuffer.size() && it.m_characterIndex[glyphPos] < from; ++glyphPos)
     143        beforeWidth += localGlyphBuffer.advanceAt(glyphPos).width();
     144    int glyphFrom = glyphPos;
     145
     146    float afterWidth = totalWidth;
     147    glyphPos = localGlyphBuffer.size() - 1;
     148    for (; glyphPos >= glyphFrom && it.m_characterIndex[glyphPos] >= to; --glyphPos)
     149        afterWidth -= localGlyphBuffer.advanceAt(glyphPos).width();
     150    int glyphTo = glyphPos + 1;
     151
     152    glyphBuffer.add(&localGlyphBuffer, glyphFrom, glyphTo - glyphFrom);
    146153
    147154    if (run.rtl()) {
    148         float finalRoundingWidth = it.m_finalRoundingWidth;
    149         it.advance(run.length(), &localGlyphBuffer);
    150         initialAdvance = finalRoundingWidth + it.m_runWidthSoFar - afterWidth;
    151     } else
    152         initialAdvance = beforeWidth;
    153 
    154     if (run.rtl())
    155155        glyphBuffer.reverse(0, glyphBuffer.size());
    156 
    157     return initialAdvance;
     156        return totalWidth - afterWidth;
     157    }
     158
     159    return beforeWidth;
    158160}
    159161
    … …  
    297299    GlyphBuffer glyphBuffer;
    298300    WidthIterator it(this, run);
    299     it.advance(from, &glyphBuffer);
    300     float beforeWidth = it.m_runWidthSoFar;
    301     it.advance(to, &glyphBuffer);
    302     float afterWidth = it.m_runWidthSoFar;
     301    it.advance(run.length(), &glyphBuffer);
     302
     303    float totalWidth = it.m_runWidthSoFar;
     304    float beforeWidth = 0;
     305    int glyphPos = 0;
     306    for (; glyphPos < glyphBuffer.size() && it.m_characterIndex[glyphPos] < from; ++glyphPos)
     307        beforeWidth += glyphBuffer.advanceAt(glyphPos).width();
     308    int glyphFrom = glyphPos;
     309
     310    float afterWidth = totalWidth;
     311    glyphPos = glyphBuffer.size() - 1;
     312    for (; glyphPos >= glyphFrom && it.m_characterIndex[glyphPos] >= to; --glyphPos)
     313        afterWidth -= glyphBuffer.advanceAt(glyphPos).width();
    303314
    304315    // Using roundf() rather than ceilf() for the right edge as a compromise to ensure correct caret positioning.
    305316    if (run.rtl()) {
    306         it.advance(run.length(), &glyphBuffer);
    307         float totalWidth = it.m_runWidthSoFar;
    308317        return FloatRect(floorf(point.x() + totalWidth - afterWidth), point.y(), roundf(point.x() + totalWidth - beforeWidth) - floorf(point.x() + totalWidth - afterWidth), h);
    309318    }
    … …  
    314323int Font::offsetForPositionForSimpleText(const TextRun& run, float x, bool includePartialGlyphs) const
    315324{
    316     float delta = x;
    317 
     325    GlyphBuffer glyphBuffer;
    318326    WidthIterator it(this, run);
    319     GlyphBuffer localGlyphBuffer;
    320     unsigned offset;
     327    it.advance(run.length(), &glyphBuffer);
     328
     329    int characterOffset = 0;
    321330    if (run.rtl()) {
    322         delta -= floatWidthForSimpleText(run);
    323         while (1) {
    324             offset = it.m_currentCharacter;
    325             float w;
    326             if (!it.advanceOneCharacter(w, localGlyphBuffer))
     331        float currentX = it.m_runWidthSoFar;
     332        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
     333            if (glyphPosition == glyphBuffer.size()) {
     334                characterOffset = run.length();
    327335                break;
    328             delta += w;
     336            }
     337            characterOffset = it.m_characterIndex[glyphPosition];
     338            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
    329339            if (includePartialGlyphs) {
    330                 if (delta - w / 2 >= 0)
     340                if (currentX - glyphWidth / 2.0f <= x)
    331341                    break;
    332342            } else {
    333                 if (delta >= 0)
     343                if (currentX - glyphWidth <= x)
    334344                    break;
    335345            }
     346            currentX -= glyphWidth;
    336347        }
    337348    } else {
    338         while (1) {
    339             offset = it.m_currentCharacter;
    340             float w;
    341             if (!it.advanceOneCharacter(w, localGlyphBuffer))
     349        float currentX = 0;
     350        for (int glyphPosition = 0; glyphPosition <= glyphBuffer.size(); ++glyphPosition) {
     351            if (glyphPosition == glyphBuffer.size()) {
     352                characterOffset = run.length();
    342353                break;
    343             delta -= w;
     354            }
     355            characterOffset = it.m_characterIndex[glyphPosition];
     356            float glyphWidth = glyphBuffer.advanceAt(glyphPosition).width();
    344357            if (includePartialGlyphs) {
    345                 if (delta + w / 2 <= 0)
     358                if (currentX + glyphWidth / 2.0f >= x)
    346359                    break;
    347360            } else {
    348                 if (delta <= 0)
     361                if (currentX + glyphWidth >= x)
    349362                    break;
    350363            }
     364            currentX += glyphWidth;
    351365        }
    352366    }
    353367
    354     return offset;
    355 }
    356 
    357 }
     368    return characterOffset;
     369}
     370
     371} // namespace WebCore

  • trunk/Source/WebCore/platform/graphics/GlyphBuffer.h

    r151610 r154384  
    140140    }
    141141
     142    void add(const GlyphBuffer* glyphBuffer, int from, int len)
     143    {
     144        m_glyphs.append(glyphBuffer->glyphs(from), len);
     145        m_advances.append(glyphBuffer->advances(from), len);
     146        m_fontData.append(glyphBuffer->m_fontData.data() + from, len);
     147#if PLATFORM(WIN)
     148        m_offsets.append(glyphBuffer->m_offsets.data() + from, len);
     149#endif
     150    }
     151
    142152    void add(Glyph glyph, const SimpleFontData* font, float width, const FloatSize* offset = 0)
    143153    {

  • trunk/Source/WebCore/platform/graphics/WidthIterator.cpp

    r148956 r154384  
    163163    while (textIterator.consume(character, clusterLength)) {
    164164        unsigned advanceLength = clusterLength;
    165         const GlyphData& glyphData = glyphDataForCharacter(character, rtl, textIterator.currentCharacter(), advanceLength);
     165        int currentCharacterIndex = textIterator.currentCharacter();
     166        const GlyphData& glyphData = glyphDataForCharacter(character, rtl, currentCharacterIndex, advanceLength);
    166167        Glyph glyph = glyphData.glyph;
    167168        const SimpleFontData* fontData = glyphData.fontData;
    … …  
    231232                                else
    232233                                    glyphBuffer->add(fontData->spaceGlyph(), fontData, expansionAtThisOpportunity);
     234                                m_characterIndex.append(currentCharacterIndex);
    233235                            } else
    234236                                glyphBuffer->expandLastAdvance(expansionAtThisOpportunity);
    … …  
    297299        }
    298300
    299         if (glyphBuffer)
     301        if (glyphBuffer) {
    300302            glyphBuffer->add(glyph, fontData, (rtl ? oldWidth + lastRoundingWidth : width));
     303            m_characterIndex.append(currentCharacterIndex);
     304        }
    301305
    302306        lastRoundingWidth = width - oldWidth;
    … …  
    338342}
    339343
    340 bool WidthIterator::advanceOneCharacter(float& width, GlyphBuffer& glyphBuffer)
    341 {
    342     int oldSize = glyphBuffer.size();
    343     advance(m_currentCharacter + 1, &glyphBuffer);
    344     float w = 0;
    345     for (int i = oldSize; i < glyphBuffer.size(); ++i)
    346         w += glyphBuffer.advanceAt(i).width();
    347     width = w;
    348     return glyphBuffer.size() > oldSize;
    349 }
    350 
    351 }
     344}

  • trunk/Source/WebCore/platform/graphics/WidthIterator.h

    r154186 r154384  
    4444
    4545    unsigned advance(int to, GlyphBuffer*);
    46     bool advanceOneCharacter(float& width, GlyphBuffer&);
    4746
    4847    float maxGlyphBoundingBoxY() const { ASSERT(m_accountForGlyphBounds); return m_maxGlyphBoundingBoxY; }
    … …  
    8483    bool m_isAfterExpansion;
    8584    float m_finalRoundingWidth;
     85    Vector<int> m_characterIndex;
    8686
    8787#if ENABLE(SVG_FONTS)
Note: See TracChangeset for help on using the changeset viewer.