Changeset 289609 in webkit


Ignore:
Timestamp:
Feb 11, 2022 12:40:49 AM (5 months ago)
Author:
mmaxfield@apple.com
Message:

Tab characters and ch units do not obey synthetic bold width adjustments correctly
​https://bugs.webkit.org/show_bug.cgi?id=236172

Reviewed by Alan Bujtas.

Source/WebCore:

It turns out we have a lot of places where code wants to know the synthetic-bold-expanded
width of characters. One place is 'tab-width: <integer>' and another is the 'ch' unit.
However, WidthIterator and ComplexTextController don't want the synthetic-bold-expanded
widths, because they explicitly apply synthetic bold after shaping.

This patch adds a 2-value enum argument to the Font::widthForGlyph() function, so callers
can pass in which behavior they want. The function has a default value to include the
synthetic bold expansion.

I then audited every call site of this function, and passed in the correct enum value.
Doing this led me to discover two bugs where the wrong behavior was being used, and this
patch fixes them and adds tests for them.

Tests: fast/text/ch-unit-synthetic-bold.html

fast/text/tab-width-synthetic-bold-complex.html
fast/text/tab-width-synthetic-bold.html

  • layout/formattingContexts/inline/text/TextUtil.cpp:

(WebCore::Layout::fallbackFontsForRunWithIterator):

  • platform/graphics/ComplexTextController.cpp:

(WebCore::ComplexTextController::adjustGlyphsAndAdvances):
(WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun):

  • platform/graphics/Font.cpp:

(WebCore::Font::platformGlyphInit):

  • platform/graphics/Font.h:

(WebCore::Font::spaceWidth const):
(WebCore::Font::widthForGlyph const):

  • platform/graphics/FontCascade.cpp:

(WebCore::FontCascade::widthForSimpleText const):

  • platform/graphics/FontCascade.h:

(WebCore::FontCascade::tabWidth const):

  • platform/graphics/WidthIterator.cpp:

(WebCore::WidthIterator::advanceInternal):
(WebCore::WidthIterator::calculateAdditionalWidth const):

LayoutTests:

3 new tests. Also, the changes in ignored-properties-001-expected.txt are a revert of the rebaseline in
r281687. That rebaseline was wrong, so this undoes it.

  • fast/text/ch-unit-synthetic-bold-expected.html: Added.
  • fast/text/ch-unit-synthetic-bold.html: Added.
  • fast/text/tab-width-synthetic-bold-complex-expected.html: Added.
  • fast/text/tab-width-synthetic-bold-complex.html: Added.
  • fast/text/tab-width-synthetic-bold-expected.html: Added.
  • fast/text/tab-width-synthetic-bold.html: Added.
  • platform/ios/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
  • platform/mac-catalina/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
  • platform/mac-mojave/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
  • platform/mac/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
Location:
trunk
Files:
6 added
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r289607 r289609  
     12022-02-11  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Tab characters and ch units do not obey synthetic bold width adjustments correctly
     4        https://bugs.webkit.org/show_bug.cgi?id=236172
     5
     6        Reviewed by Alan Bujtas.
     7
     8        3 new tests. Also, the changes in ignored-properties-001-expected.txt are a revert of the rebaseline in
     9        r281687. That rebaseline was wrong, so this undoes it.
     10
     11        * fast/text/ch-unit-synthetic-bold-expected.html: Added.
     12        * fast/text/ch-unit-synthetic-bold.html: Added.
     13        * fast/text/tab-width-synthetic-bold-complex-expected.html: Added.
     14        * fast/text/tab-width-synthetic-bold-complex.html: Added.
     15        * fast/text/tab-width-synthetic-bold-expected.html: Added.
     16        * fast/text/tab-width-synthetic-bold.html: Added.
     17        * platform/ios/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
     18        * platform/mac-catalina/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
     19        * platform/mac-mojave/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
     20        * platform/mac/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt:
     21
    1222022-02-11  Diego Pino Garcia  <dpino@igalia.com>
    223

  • trunk/LayoutTests/platform/gtk/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt

    r267658 r289609  
    2525PASS menclose layout is not affected by width: 100px !important; height: 200px !important;
    2626PASS merror preferred width calculation is not affected by writing-mode: vertical-rl;
    27 FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 8 +/- 1 but got 0
     27FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 9 +/- 1 but got 0
    2828PASS merror preferred width calculation is not affected by white-space: normal;
    2929PASS merror layout is not affected by white-space: normal;

  • trunk/LayoutTests/platform/ios/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt

    r281687 r289609  
    2525PASS menclose layout is not affected by width: 100px !important; height: 200px !important;
    2626PASS merror preferred width calculation is not affected by writing-mode: vertical-rl;
    27 FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 8.03125 +/- 1 but got 0
     27FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 9.03125 +/- 1 but got 0
    2828PASS merror preferred width calculation is not affected by white-space: normal;
    2929PASS merror layout is not affected by white-space: normal;

  • trunk/LayoutTests/platform/mac-catalina/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt

    r281687 r289609  
    2525PASS menclose layout is not affected by width: 100px !important; height: 200px !important;
    2626PASS merror preferred width calculation is not affected by writing-mode: vertical-rl;
    27 FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 8.03125 +/- 1 but got 0
     27FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 9.03125 +/- 1 but got 0
    2828PASS merror preferred width calculation is not affected by white-space: normal;
    2929PASS merror layout is not affected by white-space: normal;

  • trunk/LayoutTests/platform/mac-mojave/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt

    r281687 r289609  
    2525PASS menclose layout is not affected by width: 100px !important; height: 200px !important;
    2626PASS merror preferred width calculation is not affected by writing-mode: vertical-rl;
    27 FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 8.03125 +/- 1 but got 0
     27FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 9.03125 +/- 1 but got 0
    2828PASS merror preferred width calculation is not affected by white-space: normal;
    2929PASS merror layout is not affected by white-space: normal;

  • trunk/LayoutTests/platform/mac/imported/w3c/web-platform-tests/mathml/relations/css-styling/ignored-properties-001-expected.txt

    r281687 r289609  
    2525PASS menclose layout is not affected by width: 100px !important; height: 200px !important;
    2626PASS merror preferred width calculation is not affected by writing-mode: vertical-rl;
    27 FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 8.03125 +/- 1 but got 0
     27FAIL merror layout is not affected by writing-mode: vertical-rl; assert_approx_equals: inline position (child 1) expected 9.03125 +/- 1 but got 0
    2828PASS merror preferred width calculation is not affected by white-space: normal;
    2929PASS merror layout is not affected by white-space: normal;

  • trunk/Source/WebCore/ChangeLog

    r289606 r289609  
     12022-02-11  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Tab characters and ch units do not obey synthetic bold width adjustments correctly
     4        https://bugs.webkit.org/show_bug.cgi?id=236172
     5
     6        Reviewed by Alan Bujtas.
     7
     8        It turns out we have a lot of places where code wants to know the synthetic-bold-expanded
     9        width of characters. One place is 'tab-width: <integer>' and another is the 'ch' unit.
     10        However, WidthIterator and ComplexTextController don't want the synthetic-bold-expanded
     11        widths, because they explicitly apply synthetic bold after shaping.
     12
     13        This patch adds a 2-value enum argument to the Font::widthForGlyph() function, so callers
     14        can pass in which behavior they want. The function has a default value to include the
     15        synthetic bold expansion.
     16
     17        I then audited every call site of this function, and passed in the correct enum value.
     18        Doing this led me to discover two bugs where the wrong behavior was being used, and this
     19        patch fixes them and adds tests for them.
     20
     21        Tests: fast/text/ch-unit-synthetic-bold.html
     22               fast/text/tab-width-synthetic-bold-complex.html
     23               fast/text/tab-width-synthetic-bold.html
     24
     25        * layout/formattingContexts/inline/text/TextUtil.cpp:
     26        (WebCore::Layout::fallbackFontsForRunWithIterator):
     27        * platform/graphics/ComplexTextController.cpp:
     28        (WebCore::ComplexTextController::adjustGlyphsAndAdvances):
     29        (WebCore::ComplexTextController::ComplexTextRun::ComplexTextRun):
     30        * platform/graphics/Font.cpp:
     31        (WebCore::Font::platformGlyphInit):
     32        * platform/graphics/Font.h:
     33        (WebCore::Font::spaceWidth const):
     34        (WebCore::Font::widthForGlyph const):
     35        * platform/graphics/FontCascade.cpp:
     36        (WebCore::FontCascade::widthForSimpleText const):
     37        * platform/graphics/FontCascade.h:
     38        (WebCore::FontCascade::tabWidth const):
     39        * platform/graphics/WidthIterator.cpp:
     40        (WebCore::WidthIterator::advanceInternal):
     41        (WebCore::WidthIterator::calculateAdditionalWidth const):
     42
    1432022-02-11  Nikolas Zimmermann  <nzimmermann@igalia.com>
    244

  • trunk/Source/WebCore/layout/formattingContexts/inline/text/TextUtil.cpp

    r289007 r289609  
    122122            if (glyphData.glyph && glyphData.font && glyphData.font != &primaryFont) {
    123123                auto isNonSpacingMark = U_MASK(u_charType(character)) & U_GC_MN_MASK;
    124                 if (isNonSpacingMark || glyphData.font->widthForGlyph(glyphData.glyph))
     124                // If we include the synthetic bold expansion, then even zero-width glyphs will have their fonts added.
     125                if (isNonSpacingMark || glyphData.font->widthForGlyph(glyphData.glyph, Font::SyntheticBoldInclusion::Exclude))
    125126                    fallbackFonts.add(glyphData.font);
    126127            }

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

    r288783 r289609  
    694694        const FloatSize* advances = complexTextRun.baseAdvances();
    695695
    696         float spaceWidth = font.spaceWidth();
     696        // Lower in this function, synthetic bold is blanket-applied to everything, so no need to double-apply it here.
     697        float spaceWidth = font.spaceWidth(Font::SyntheticBoldInclusion::Exclude);
    697698        const UChar* cp = complexTextRun.characters();
    698699        FloatPoint glyphOrigin;
    … …  
    715716            FloatSize advance = treatAsSpace ? FloatSize(spaceWidth, advances[i].height()) : advances[i];
    716717
    717             if (ch == '\t' && m_run.allowTabs())
    718                 advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalAdvance.width()));
     718            if (ch == tabCharacter && m_run.allowTabs())
     719                advance.setWidth(m_font.tabWidth(font, m_run.tabSize(), m_run.xPos() + m_totalAdvance.width(), Font::SyntheticBoldInclusion::Exclude));
    719720            else if (FontCascade::treatAsZeroWidthSpace(ch) && !treatAsSpace) {
    720721                advance.setWidth(0);
    … …  
    845846    // Synthesize a run of missing glyphs.
    846847    m_glyphs.fill(0, m_glyphCount);
    847     m_baseAdvances.fill(FloatSize(m_font.widthForGlyph(0), 0), m_glyphCount);
     848    // Synthetic bold will be handled later in adjustGlyphsAndAdvances().
     849    m_baseAdvances.fill(FloatSize(m_font.widthForGlyph(0, Font::SyntheticBoldInclusion::Exclude), 0), m_glyphCount);
    848850}
    849851

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

    r289151 r289609  
    166166        m_fontMetrics.setIdeogramWidth(platformData().size());
    167167
    168     m_spaceWidth = widthForGlyph(m_spaceGlyph);
     168    m_spaceWidth = widthForGlyph(m_spaceGlyph, SyntheticBoldInclusion::Exclude); // spaceWidth() handles adding in the synthetic bold.
    169169    auto amountToAdjustLineGap = std::min(m_fontMetrics.floatLineGap(), 0.0f);
    170170    m_fontMetrics.setLineGap(m_fontMetrics.floatLineGap() - amountToAdjustLineGap);
    171171    m_fontMetrics.setLineSpacing(m_fontMetrics.floatLineSpacing() - amountToAdjustLineGap);
    172172    determinePitch();
    173     // Nasty hack to determine if we should round or ceil space widths.
    174     // If the font is monospace or fake monospace we ceil to ensure that
    175     // every character and the space are the same width. Otherwise we round.
    176     m_adjustedSpaceWidth = m_treatAsFixedPitch ? ceilf(m_spaceWidth) : roundf(m_spaceWidth);
    177173}
    178174

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

    r288783 r289609  
    139139    FloatRect boundsForGlyph(Glyph) const;
    140140
    141     // If you're calling this, you need to care about synthetic bold.
    142     // This function returns the width before adding in the synthetic bold offset.
    143     // This is because these widths are fed as an input to shaping, which needs to consume the true widths.
    144     // Then, the synthetic bold offset is applied after shaping.
    145     // If you're not going through WidthIterator or ComplexTextController (and thus potentially executing shaping yourself),
    146     // then you need to decide whether or not you need to add syntheticBoldOffset() to the result of your widthForGlyph() call.
    147     float widthForGlyph(Glyph) const;
     141    // Should the result of this function include the results of synthetic bold?
     142    enum class SyntheticBoldInclusion {
     143        Incorporate,
     144        Exclude
     145    };
     146    float widthForGlyph(Glyph, SyntheticBoldInclusion = SyntheticBoldInclusion::Incorporate) const;
    148147
    149148    const Path& pathForGlyph(Glyph) const; // Don't store the result of this! The hash map is free to rehash at any point, leaving this reference dangling.
    150     FloatRect platformBoundsForGlyph(Glyph) const;
    151     float platformWidthForGlyph(Glyph) const;
    152     Path platformPathForGlyph(Glyph) const;
    153 
    154     // If you're calling this, you need to care about synthetic bold.
    155     // See the comment just above widthForGlyph().
    156     float spaceWidth() const { return m_spaceWidth; }
     149
     150    float spaceWidth(SyntheticBoldInclusion SyntheticBoldInclusion = SyntheticBoldInclusion::Incorporate) const
     151    {
     152        return m_spaceWidth + (SyntheticBoldInclusion == SyntheticBoldInclusion::Incorporate ? syntheticBoldOffset() : 0);
     153    }
    157154
    158155    float syntheticBoldOffset() const { return m_syntheticBoldOffset; }
    … …  
    236233    float widthForGDIGlyph(Glyph) const;
    237234#endif
     235
     236    FloatRect platformBoundsForGlyph(Glyph) const;
     237    float platformWidthForGlyph(Glyph) const;
     238    Path platformPathForGlyph(Glyph) const;
    238239
    239240#if PLATFORM(COCOA)
    … …  
    325326
    326327    float m_spaceWidth { 0 };
    327     float m_adjustedSpaceWidth { 0 };
    328328
    329329    float m_syntheticBoldOffset { 0 };
    … …  
    370370}
    371371
    372 ALWAYS_INLINE float Font::widthForGlyph(Glyph glyph) const
     372ALWAYS_INLINE float Font::widthForGlyph(Glyph glyph, SyntheticBoldInclusion SyntheticBoldInclusion) const
    373373{
    374374    // The optimization of returning 0 for the zero-width-space glyph is incorrect for the LastResort font,
    … …  
    381381    float width = m_glyphToWidthMap.metricsForGlyph(glyph);
    382382    if (width != cGlyphSizeUnknown)
    383         return width;
     383        return width + (SyntheticBoldInclusion == SyntheticBoldInclusion::Incorporate ? syntheticBoldOffset() : 0);
    384384
    385385#if ENABLE(OPENTYPE_VERTICAL)
    … …  
    391391
    392392    m_glyphToWidthMap.setMetricsForGlyph(glyph, width);
    393     return width;
     393    return width + (SyntheticBoldInclusion == SyntheticBoldInclusion::Incorporate ? syntheticBoldOffset() : 0);
    394394}
    395395

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

    r289101 r289609  
    317317    for (unsigned i = 0; i < text.length(); ++i) {
    318318        auto glyph = glyphDataForCharacter(text[i], false).glyph;
     319        ASSERT(!font.syntheticBoldOffset()); // This function should only be called when RenderText::computeCanUseSimplifiedTextMeasuring() returns true, and that function requires no synthetic bold.
    319320        auto glyphWidth = font.widthForGlyph(glyph);
    320321        beforeWidth += glyphWidth;

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

    r289007 r289609  
    169169
    170170    const FontMetrics& metricsOfPrimaryFont() const { return primaryFont().fontMetrics(); }
    171     float tabWidth(const Font&, const TabSize&, float) const;
    172     float tabWidth(const TabSize& tabSize, float position) const { return tabWidth(primaryFont(), tabSize, position); }
     171    float tabWidth(const Font&, const TabSize&, float, Font::SyntheticBoldInclusion) const;
    173172    bool hasValidAverageCharWidth() const;
    174173    bool fastAverageCharWidthIfAvailable(float &width) const; // returns true on success
    … …  
    374373}
    375374
    376 inline float FontCascade::tabWidth(const Font& font, const TabSize& tabSize, float position) const
     375inline float FontCascade::tabWidth(const Font& font, const TabSize& tabSize, float position, Font::SyntheticBoldInclusion syntheticBoldInclusion) const
    377376{
    378377    float baseTabWidth = tabSize.widthInPixels(font.spaceWidth());
     378    float result = 0;
    379379    if (!baseTabWidth)
    380         return letterSpacing();
    381     float tabDeltaWidth = baseTabWidth - fmodf(position, baseTabWidth);
    382     return (tabDeltaWidth < font.spaceWidth() / 2) ? baseTabWidth : tabDeltaWidth;
    383 }
    384 
    385 }
     380        result = letterSpacing();
     381    else {
     382        float tabDeltaWidth = baseTabWidth - fmodf(position, baseTabWidth);
     383        result = (tabDeltaWidth < font.spaceWidth() / 2) ? baseTabWidth : tabDeltaWidth;
     384    }
     385    // If our caller passes in SyntheticBoldInclusion::Exclude, that means they're going to apply synthetic bold themselves later.
     386    // However, regardless of that, the space characters that are fed into the width calculation need to have their correct width, including the synthetic bold.
     387    // So, we've already got synthetic bold applied, so if we're supposed to exclude it, we need to subtract it out here.
     388    return result - (syntheticBoldInclusion == Font::SyntheticBoldInclusion::Exclude ? font.syntheticBoldOffset() : 0);
     389}
     390
     391}

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

    r288564 r289609  
    307307
    308308        previousWidth = width;
    309         width = font.widthForGlyph(glyph);
     309        width = font.widthForGlyph(glyph, Font::SyntheticBoldInclusion::Exclude); // We apply synthetic bold after shaping, in applyCSSVisibilityRules().
    310310
    311311        if (&font != lastFontData)
    … …  
    315315
    316316        if (FontCascade::treatAsSpace(character))
    317             charactersTreatedAsSpace.constructAndAppend(currentCharacterIndex, character == space, previousWidth, character == tabCharacter ? width : font.spaceWidth());
     317            charactersTreatedAsSpace.constructAndAppend(currentCharacterIndex, character == space, previousWidth, character == tabCharacter ? width : font.spaceWidth(Font::SyntheticBoldInclusion::Exclude));
    318318
    319319        if (m_accountForGlyphBounds) {
    … …  
    355355    if (character == tabCharacter && m_run.allowTabs()) {
    356356        auto& font = glyphBuffer.fontAt(trailingGlyphIndex);
    357         auto newWidth = m_font.tabWidth(font, m_run.tabSize(), position);
     357        // Synthetic bold will be handled in applyCSSVisibilityRules() later.
     358        auto newWidth = m_font.tabWidth(font, m_run.tabSize(), position, Font::SyntheticBoldInclusion::Exclude);
    358359        auto currentWidth = width(glyphBuffer.advanceAt(trailingGlyphIndex));
    359360        rightAdditionalWidth += newWidth - currentWidth;
Note: See TracChangeset for help on using the changeset viewer.