⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 281389 in webkit


Ignore:
Timestamp:
Aug 21, 2021, 10:41:27 PM (5 years ago)
Author:
mmaxfield@apple.com
Message:

overwriteCodePoint() in createAndFillGlyphPage() is wrong
​https://bugs.webkit.org/show_bug.cgi?id=215643
<rdar://problem/67430461>

Reviewed by Alan Bujtas.

Source/WebCore:

The CSS spec says that certain characters have to be invisible. Previously, we were doing this
by just treating these characters as if they were the zero width space character. However, this
is wrong for 2 reasons:

  1. If the primary font supports the desired character but doesn't support the zero width space

character, we'll fallback past it and trigger a download for secondary fonts until
we find one that supports the ZWS character (which the page didn't even ask for!)

  1. Shaping is Turing-complete, and expects that the correct glyphIDs are fed into the shaper.

We can't just arbitrarily swap out glyphIDs before shaping just because we feel like it.

Instead, the solution is to handle these invisible characters after font fallback, and after
shaping. This patch does it just like we handle adding letter-spacing, word-spacing, and
justification after shaping - by iterating over the glyphs output by the shaper and seeing if
any need to be adjusted.

This patch also includes a fix for the situation where the glyph buffer might be left in an
inconsistent state, where the array of glyphs is shorter than the array of characters -
this new codepath added in this patch revealed this existing problem, and fixes it here too.

Test: fast/text/zwj-ligature.html

  • platform/graphics/Font.cpp:

(WebCore::createAndFillGlyphPage):
(WebCore::overrideControlCharacters): Deleted.

  • platform/graphics/GlyphBuffer.h:

(WebCore::GlyphBuffer::glyphAt const):
(WebCore::GlyphBuffer::deleteGlyphWithoutAffectingSize):

  • platform/graphics/WidthIterator.cpp:

(WebCore::WidthIterator::applyCSSVisibilityRules):
(WebCore::WidthIterator::advance):

  • platform/graphics/WidthIterator.h:

Source/WTF:

  • wtf/unicode/CharacterNames.h:

LayoutTests:

  • fast/text/resources/Ahem-GSUB-ligatures.ttf: Added.
  • fast/text/zwj-ligature-expected.html: Added.
  • fast/text/zwj-ligature.html: Added.
  • platform/mac/fast/text/format-control-expected.txt: Progression! A new ligature is formed that wasn't formed before.

Also, this ligature is formed in both Chrome and Firefox, so we match them now.

  • platform/win/TestExpectations:
Location:
trunk
Files:
3 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r281387 r281389  
     12021-08-21  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        overwriteCodePoint() in createAndFillGlyphPage() is wrong
     4        https://bugs.webkit.org/show_bug.cgi?id=215643
     5        <rdar://problem/67430461>
     6
     7        Reviewed by Alan Bujtas.
     8
     9        * fast/text/resources/Ahem-GSUB-ligatures.ttf: Added.
     10        * fast/text/zwj-ligature-expected.html: Added.
     11        * fast/text/zwj-ligature.html: Added.
     12        * platform/mac/fast/text/format-control-expected.txt: Progression! A new ligature is formed that wasn't formed before.
     13              Also, this ligature is formed in both Chrome and Firefox, so we match them now.
     14        * platform/win/TestExpectations:
     15
    1162021-08-21  Alan Bujtas  <zalan@apple.com>
    217

  • trunk/LayoutTests/platform/mac/fast/text/format-control-expected.txt

    r177774 r281389  
    1515            text run at (0,0) width 290: "f\x{200C}i f\x{200C}l f\x{200C}f f\x{200C}f\x{200C}i f\x{200C}f\x{200C}l f\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}l f\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C} \x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C} \x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C} \x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}\x{200C}i"
    1616        RenderBlock {P} at (0,172) size 784x46
    17           RenderText {#text} at (0,0) size 290x46
    18             text run at (0,0) width 290: "f\x{200D}i f\x{200D}l f\x{200D}f f\x{200D}f\x{200D}i f\x{200D}f\x{200D}l f\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}l f\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}i"
     17          RenderText {#text} at (0,0) size 282x46
     18            text run at (0,0) width 282: "f\x{200D}i f\x{200D}l f\x{200D}f f\x{200D}f\x{200D}i f\x{200D}f\x{200D}l f\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}l f\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D} \x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}\x{200D}i"
    1919      RenderBlock {DIV} at (0,316) size 784x221
    2020        RenderBlock {P} at (0,0) size 784x47

  • trunk/LayoutTests/platform/win/TestExpectations

    r281385 r281389  
    46904690
    46914691webkit.org/b/229379 fast/visual-viewport/resize-event-fired-window-resized.html [ Skip ]
     4692
     4693webkit.org/b/215318 fast/text/zwj-ligature.html [ ImageOnlyFailure ]

  • trunk/Source/WTF/ChangeLog

    r281384 r281389  
     12021-08-21  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        overwriteCodePoint() in createAndFillGlyphPage() is wrong
     4        https://bugs.webkit.org/show_bug.cgi?id=215643
     5        <rdar://problem/67430461>
     6
     7        Reviewed by Alan Bujtas.
     8
     9        * wtf/unicode/CharacterNames.h:
     10
    1112021-08-21  Sihui Liu  <sihui_liu@apple.com>
    212

  • trunk/Source/WTF/wtf/unicode/CharacterNames.h

    r279165 r281389  
    5252constexpr UChar carriageReturn = 0x000D;
    5353constexpr UChar combiningEnclosingKeycap = 0x20E3;
     54constexpr UChar deleteCharacter = 0x007F;
    5455constexpr UChar ethiopicPrefaceColon = 0x1366;
    5556constexpr UChar ethiopicWordspace = 0x1361;
    … …  
    7778constexpr UChar newlineCharacter = 0x000A;
    7879constexpr UChar noBreakSpace = 0x00A0;
     80constexpr UChar nullCharacter = 0x0;
    7981constexpr UChar objectReplacementCharacter = 0xFFFC;
    8082constexpr UChar optionKey = 0x2325;
    … …  
    127129using WTF::Unicode::carriageReturn;
    128130using WTF::Unicode::combiningEnclosingKeycap;
     131using WTF::Unicode::deleteCharacter;
    129132using WTF::Unicode::ethiopicPrefaceColon;
    130133using WTF::Unicode::ethiopicWordspace;
    … …  
    152155using WTF::Unicode::newlineCharacter;
    153156using WTF::Unicode::noBreakSpace;
     157using WTF::Unicode::nullCharacter;
    154158using WTF::Unicode::objectReplacementCharacter;
    155159using WTF::Unicode::popDirectionalFormatting;

  • trunk/Source/WebCore/ChangeLog

    r281388 r281389  
     12021-08-21  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        overwriteCodePoint() in createAndFillGlyphPage() is wrong
     4        https://bugs.webkit.org/show_bug.cgi?id=215643
     5        <rdar://problem/67430461>
     6
     7        Reviewed by Alan Bujtas.
     8
     9        The CSS spec says that certain characters have to be invisible. Previously, we were doing this
     10        by just treating these characters as if they were the zero width space character. However, this
     11        is wrong for 2 reasons:
     12
     13        1. If the primary font supports the desired character but doesn't support the zero width space
     14               character, we'll fallback past it and trigger a download for secondary fonts until
     15               we find one that supports the ZWS character (which the page didn't even ask for!)
     16        2. Shaping is Turing-complete, and expects that the correct glyphIDs are fed into the shaper.
     17               We can't just arbitrarily swap out glyphIDs before shaping just because we feel like it.
     18
     19        Instead, the solution is to handle these invisible characters after font fallback, and after
     20        shaping. This patch does it just like we handle adding letter-spacing, word-spacing, and
     21        justification after shaping - by iterating over the glyphs output by the shaper and seeing if
     22        any need to be adjusted.
     23
     24        This patch also includes a fix for the situation where the glyph buffer might be left in an
     25        inconsistent state, where the array of glyphs is shorter than the array of characters -
     26        this new codepath added in this patch revealed this existing problem, and fixes it here too.
     27
     28        Test: fast/text/zwj-ligature.html
     29
     30        * platform/graphics/Font.cpp:
     31        (WebCore::createAndFillGlyphPage):
     32        (WebCore::overrideControlCharacters): Deleted.
     33        * platform/graphics/GlyphBuffer.h:
     34        (WebCore::GlyphBuffer::glyphAt const):
     35        (WebCore::GlyphBuffer::deleteGlyphWithoutAffectingSize):
     36        * platform/graphics/WidthIterator.cpp:
     37        (WebCore::WidthIterator::applyCSSVisibilityRules):
     38        (WebCore::WidthIterator::advance):
     39        * platform/graphics/WidthIterator.h:
     40
    1412021-08-21  Alan Bujtas  <zalan@apple.com>
    242

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

    r281294 r281389  
    307307}
    308308
    309 #if !USE(FREETYPE)
     309#if PLATFORM(WIN)
    310310static void overrideControlCharacters(Vector<UChar>& buffer, unsigned start, unsigned end)
    311311{
    … …  
    365365    Vector<UChar> buffer(glyphPageSize * 2 + 2);
    366366    unsigned bufferLength;
    367     // Fill in a buffer with the entire "page" of characters that we want to look up glyphs for.
    368367    if (U_IS_BMP(start)) {
    369368        bufferLength = glyphPageSize;
    … …  
    371370            buffer[i] = start + i;
    372371
    373 #if !USE(FREETYPE)
     372#if PLATFORM(WIN)
     373        // FIXME: https://bugs.webkit.org/show_bug.cgi?id=215318 Delete this and use https://bugs.webkit.org/show_bug.cgi?id=215643 on Windows.
    374374        overrideControlCharacters(buffer, start, start + glyphPageSize);
    375375#endif

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

    r281294 r281389  
    7272        return *m_fonts[index];
    7373    }
    74     Glyph glyphAt(unsigned index) const { return m_glyphs[index]; }
     74    GlyphBufferGlyph glyphAt(unsigned index) const { return m_glyphs[index]; }
    7575    GlyphBufferAdvance advanceAt(unsigned index) const { return m_advances[index]; }
    7676    GlyphBufferOrigin originAt(unsigned index) const { return m_origins[index]; }
    … …  
    116116        m_origins.remove(location, length);
    117117        m_offsetsInString.remove(location, length);
     118    }
     119
     120    void deleteGlyphWithoutAffectingSize(unsigned index)
     121    {
     122        // GlyphID 0xFFFF is the "deleted glyph" and is supposed to be invisible when rendered.
     123        static const constexpr GlyphBufferGlyph deletedGlyph = 0xFFFF;
     124        m_glyphs[index] = deletedGlyph;
     125        m_advances[index] = makeGlyphBufferAdvance();
    118126    }
    119127

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

    r281378 r281389  
    516516}
    517517
     518void WidthIterator::applyCSSVisibilityRules(GlyphBuffer& glyphBuffer, unsigned glyphBufferStartIndex)
     519{
     520    Vector<unsigned> glyphsIndicesToBeDeleted;
     521
     522    float yPosition = height(glyphBuffer.initialAdvance());
     523    for (unsigned i = glyphBufferStartIndex; i < glyphBuffer.size(); yPosition += height(glyphBuffer.advanceAt(i)), ++i) {
     524        auto stringOffset = glyphBuffer.checkedStringOffsetAt(i, m_run.length());
     525        if (!stringOffset)
     526            continue;
     527        auto characterResponsibleForThisGlyph = m_run[stringOffset.value()];
     528
     529        switch (characterResponsibleForThisGlyph) {
     530        case newlineCharacter:
     531        case carriageReturn:
     532        case tabCharacter:
     533        case noBreakSpace:
     534            ASSERT(glyphBuffer.fonts(i)[0]);
     535            // FIXME: Is this actually necessary? If the font specifically has a glyph for NBSP, I don't see a reason not to use it.
     536            glyphBuffer.glyphs(i)[0] = glyphBuffer.fonts(i)[0]->spaceGlyph();
     537            continue;
     538        }
     539
     540        // FIXME: "Control characters (Unicode category Cc)—other than tabs (U+0009), line feeds (U+000A), carriage returns (U+000D) and sequences that form a segment break—must be rendered as a visible glyph"
     541        // https://www.w3.org/TR/css-text-3/#white-space-processing
     542
     543        if ((characterResponsibleForThisGlyph >= nullCharacter && characterResponsibleForThisGlyph < space)
     544            || (characterResponsibleForThisGlyph >= deleteCharacter && characterResponsibleForThisGlyph < noBreakSpace)) {
     545            m_runWidthSoFar -= width(glyphBuffer.advanceAt(i));
     546            glyphBuffer.deleteGlyphWithoutAffectingSize(i);
     547            continue;
     548        }
     549
     550        switch (characterResponsibleForThisGlyph) {
     551        case softHyphen:
     552        case leftToRightMark:
     553        case rightToLeftMark:
     554        case leftToRightEmbed:
     555        case rightToLeftEmbed:
     556        case leftToRightOverride:
     557        case rightToLeftOverride:
     558        case leftToRightIsolate:
     559        case rightToLeftIsolate:
     560        case zeroWidthNonJoiner:
     561        case zeroWidthJoiner:
     562        case popDirectionalFormatting:
     563        case popDirectionalIsolate:
     564        case firstStrongIsolate:
     565        case objectReplacementCharacter:
     566        case zeroWidthNoBreakSpace:
     567            m_runWidthSoFar -= width(glyphBuffer.advanceAt(i));
     568            glyphBuffer.deleteGlyphWithoutAffectingSize(i);
     569            continue;
     570        }
     571    }
     572
     573   
     574}
     575
    518576void WidthIterator::finalize(GlyphBuffer& buffer)
    519577{
    … …  
    560618    if (hasExtraSpacing() || m_containsTabs || m_run.horizontalGlyphStretch() != 1)
    561619        applyExtraSpacingAfterShaping(glyphBuffer, characterStartIndex, glyphBufferStartIndex, offset, startingRunWidth);
     620
     621    applyCSSVisibilityRules(glyphBuffer, glyphBufferStartIndex);
    562622}
    563623

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

    r281378 r281389  
    7474    bool hasExtraSpacing() const;
    7575    void applyExtraSpacingAfterShaping(GlyphBuffer&, unsigned characterStartIndex, unsigned glyphBufferStartIndex, unsigned characterDestinationIndex, float startingRunWidth);
     76    void applyCSSVisibilityRules(GlyphBuffer&, unsigned glyphBufferStartIndex);
     77
    7678    struct AdditionalWidth {
    7779        float left;
Note: See TracChangeset for help on using the changeset viewer.