Changeset 283752 in webkit


Ignore:
Timestamp:
Oct 7, 2021 4:05:49 PM (10 months ago)
Author:
mmaxfield@apple.com
Message:

Stop parsing font palette things that we can't implement
​https://bugs.webkit.org/show_bug.cgi?id=230799
<rdar://problem/83537772>

Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

  • web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html:

Source/WebCore:

We can't implement the <string> values in base-palette and override-colors. It's better
not to parse them, so this behavior can at least be feature detected by JS reading the
serialized value that was parsed.

I'm investigating changing the spec to disallow these unimplementable things in
​https://github.com/w3c/csswg-drafts/issues/6627.

For base-palette, we were representing the absence of the descriptor by making the internal
representation hold a null string. Now that we're removing the string value from the internal
representation, this is now being represented by using an std::optional instead.

Test: imported/w3c/web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html

  • css/CSSFontPaletteValuesRule.cpp:

(WebCore::CSSFontPaletteValuesRule::basePalette const):
(WebCore::CSSFontPaletteValuesRule::overrideColors const):
(WebCore::CSSFontPaletteValuesRule::cssText const):

  • css/StyleRule.cpp:

(WebCore::StyleRuleFontPaletteValues::StyleRuleFontPaletteValues):

  • css/StyleRule.h:
  • css/parser/CSSParserImpl.cpp:

(WebCore::CSSParserImpl::consumeFontPaletteValuesRule):

  • css/parser/CSSPropertyParser.cpp:

(WebCore::consumeBasePaletteDescriptor):
(WebCore::consumeOverrideColorsDescriptor):

  • platform/graphics/FontPaletteValues.h:

(WebCore::FontPaletteIndex::operator bool const):
(WebCore::FontPaletteIndex::operator== const):
(WebCore::add):
(WebCore::FontPaletteValues::FontPaletteValues):
(WebCore::FontPaletteValues::basePalette const):

  • platform/graphics/cocoa/FontCacheCoreText.cpp:

(WebCore::addAttributesForCustomFontPalettes):

Location:
trunk
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/imported/w3c/ChangeLog

    r283742 r283752  
     12021-10-07  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Stop parsing font palette things that we can't implement
     4        https://bugs.webkit.org/show_bug.cgi?id=230799
     5        <rdar://problem/83537772>
     6
     7        Reviewed by Simon Fraser.
     8
     9        * web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html:
     10
    1112021-10-07  Aditya Keerthi  <akeerthi@apple.com>
    212

  • trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html

    r283539 r283752  
    145145    assert_not_equals(text.indexOf("base-palette: 2;"), -1);
    146146    assert_equals(text.indexOf("override-colors: \"a\""), -1);
    147     assert_equals(text.indexOf("override-colors: 3"), -1);
    148     assert_not_equals(text.indexOf("override-colors: \"b\""), -1);
     147    assert_not_equals(text.indexOf("override-colors: 3"), -1);
     148    assert_equals(text.indexOf("override-colors: \"b\""), -1);
    149149});
    150150
    … …  
    154154    assert_equals(rule.fontFamily, "bar");
    155155    assert_equals(rule.basePalette, "2");
    156     assert_equals(rule.overrideColors, "\"b\" rgb(17, 34, 51)");
     156    assert_equals(rule.overrideColors, "3 rgb(17, 34, 51)");
    157157});
    158158
    … …  
    160160    let text = rules[3].cssText;
    161161    assert_equals(text.indexOf("base-palette: \"foo\";"), -1);
    162     assert_equals(text.indexOf("base-palette: 1"), -1);
    163     assert_not_equals(text.indexOf("base-palette: \"bar\";"), -1);
     162    assert_not_equals(text.indexOf("base-palette: 1"), -1);
     163    assert_equals(text.indexOf("base-palette: \"bar\";"), -1);
    164164    assert_equals(text.indexOf("override-colors: 3"), -1);
    165165    assert_equals(text.indexOf("override-colors: \"baz\""), -1);
    … …  
    171171    assert_equals(rule.name, "--D");
    172172    assert_equals(rule.fontFamily, "");
    173     assert_equals(rule.basePalette, "bar");
     173    assert_equals(rule.basePalette, "1");
    174174    assert_equals(rule.overrideColors.indexOf("),"), -1);
    175175    assert_equals(rule.overrideColors.indexOf("4 "), 0);

  • trunk/Source/WebCore/ChangeLog

    r283747 r283752  
     12021-10-07  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        Stop parsing font palette things that we can't implement
     4        https://bugs.webkit.org/show_bug.cgi?id=230799
     5        <rdar://problem/83537772>
     6
     7        Reviewed by Simon Fraser.
     8
     9        We can't implement the <string> values in base-palette and override-colors. It's better
     10        not to parse them, so this behavior can at least be feature detected by JS reading the
     11        serialized value that was parsed.
     12
     13        I'm investigating changing the spec to disallow these unimplementable things in
     14        https://github.com/w3c/csswg-drafts/issues/6627.
     15
     16        For base-palette, we were representing the absence of the descriptor by making the internal
     17        representation hold a null string. Now that we're removing the string value from the internal
     18        representation, this is now being represented by using an std::optional instead.
     19
     20        Test: imported/w3c/web-platform-tests/css/css-fonts/parsing/font-palette-values-valid.html
     21
     22        * css/CSSFontPaletteValuesRule.cpp:
     23        (WebCore::CSSFontPaletteValuesRule::basePalette const):
     24        (WebCore::CSSFontPaletteValuesRule::overrideColors const):
     25        (WebCore::CSSFontPaletteValuesRule::cssText const):
     26        * css/StyleRule.cpp:
     27        (WebCore::StyleRuleFontPaletteValues::StyleRuleFontPaletteValues):
     28        * css/StyleRule.h:
     29        * css/parser/CSSParserImpl.cpp:
     30        (WebCore::CSSParserImpl::consumeFontPaletteValuesRule):
     31        * css/parser/CSSPropertyParser.cpp:
     32        (WebCore::consumeBasePaletteDescriptor):
     33        (WebCore::consumeOverrideColorsDescriptor):
     34        * platform/graphics/FontPaletteValues.h:
     35        (WebCore::FontPaletteIndex::operator bool const):
     36        (WebCore::FontPaletteIndex::operator== const):
     37        (WebCore::add):
     38        (WebCore::FontPaletteValues::FontPaletteValues):
     39        (WebCore::FontPaletteValues::basePalette const):
     40        * platform/graphics/cocoa/FontCacheCoreText.cpp:
     41        (WebCore::addAttributesForCustomFontPalettes):
     42
    1432021-10-07  Tim Horton  <timothy_horton@apple.com>
    244

  • trunk/Source/WebCore/css/CSSFontPaletteValuesRule.cpp

    r283736 r283752  
    5959String CSSFontPaletteValuesRule::basePalette() const
    6060{
    61     switch (m_fontPaletteValuesRule->basePalette().type) {
     61    if (!m_fontPaletteValuesRule->basePalette())
     62        return StringImpl::empty();
     63
     64    switch (m_fontPaletteValuesRule->basePalette()->type) {
    6265    case FontPaletteIndex::Type::Light:
    6366        return "light"_s;
    … …  
    6568        return "dark"_s;
    6669    case FontPaletteIndex::Type::Integer:
    67         return makeString(m_fontPaletteValuesRule->basePalette().integer);
    68     case FontPaletteIndex::Type::String:
    69         if (!m_fontPaletteValuesRule->basePalette().string.isNull())
    70             return m_fontPaletteValuesRule->basePalette().string;
    71         return StringImpl::empty();
     70        return makeString(m_fontPaletteValuesRule->basePalette()->integer);
    7271    }
    7372    RELEASE_ASSERT_NOT_REACHED();
    … …  
    8180            result.append(", ");
    8281        const auto& item = m_fontPaletteValuesRule->overrideColors()[i];
    83         WTF::switchOn(item.first, [&] (const AtomString& string) {
    84             result.append(serializeString(string));
    85         }, [&] (int64_t index) {
    86             result.append(index);
    87         });
    88         result.append(' ', serializationForCSS(item.second));
     82        result.append(item.first, ' ', serializationForCSS(item.second));
    8983    }
    9084    return result.toString();
    … …  
    9892        builder.append("font-family: ", m_fontPaletteValuesRule->fontFamily(), "; ");
    9993
    100     switch (m_fontPaletteValuesRule->basePalette().type) {
    101     case FontPaletteIndex::Type::Light:
    102         builder.append("base-palette: light; ");
    103         break;
    104     case FontPaletteIndex::Type::Dark:
    105         builder.append("base-palette: dark; ");
    106         break;
    107     case FontPaletteIndex::Type::Integer:
    108         builder.append("base-palette: ", m_fontPaletteValuesRule->basePalette().integer, "; ");
    109         break;
    110     case FontPaletteIndex::Type::String:
    111         if (!m_fontPaletteValuesRule->basePalette().string.isNull())
    112             builder.append("base-palette: ", serializeString(m_fontPaletteValuesRule->basePalette().string), "; ");
    113         break;
     94    if (m_fontPaletteValuesRule->basePalette()) {
     95        switch (m_fontPaletteValuesRule->basePalette()->type) {
     96        case FontPaletteIndex::Type::Light:
     97            builder.append("base-palette: light; ");
     98            break;
     99        case FontPaletteIndex::Type::Dark:
     100            builder.append("base-palette: dark; ");
     101            break;
     102        case FontPaletteIndex::Type::Integer:
     103            builder.append("base-palette: ", m_fontPaletteValuesRule->basePalette()->integer, "; ");
     104            break;
     105        }
    114106    }
    115107
    … …  
    119111            if (i)
    120112                builder.append(',');
    121             builder.append(' ');
    122             WTF::switchOn(m_fontPaletteValuesRule->overrideColors()[i].first, [&] (const AtomString& name) {
    123                 builder.append(serializeString(name.string()));
    124             }, [&] (unsigned index) {
    125                 builder.append(index);
    126             });
    127             builder.append(' ', serializationForCSS(m_fontPaletteValuesRule->overrideColors()[i].second));
     113            builder.append(' ', m_fontPaletteValuesRule->overrideColors()[i].first, ' ', serializationForCSS(m_fontPaletteValuesRule->overrideColors()[i].second));
    128114        }
    129115        builder.append("; ");

  • trunk/Source/WebCore/css/StyleRule.cpp

    r283398 r283752  
    331331}
    332332
    333 StyleRuleFontPaletteValues::StyleRuleFontPaletteValues(const AtomString& name, const AtomString& fontFamily, const FontPaletteIndex& basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors)
     333StyleRuleFontPaletteValues::StyleRuleFontPaletteValues(const AtomString& name, const AtomString& fontFamily, std::optional<FontPaletteIndex> basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors)
    334334    : StyleRuleBase(StyleRuleType::FontPaletteValues)
    335335    , m_name(name)

  • trunk/Source/WebCore/css/StyleRule.h

    r283398 r283752  
    160160class StyleRuleFontPaletteValues final : public StyleRuleBase {
    161161public:
    162     static Ref<StyleRuleFontPaletteValues> create(const AtomString& name, const AtomString& fontFamily, const FontPaletteIndex& basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors)
     162    static Ref<StyleRuleFontPaletteValues> create(const AtomString& name, const AtomString& fontFamily, std::optional<FontPaletteIndex> basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors)
    163163    {
    164164        return adoptRef(*new StyleRuleFontPaletteValues(name, fontFamily, basePalette, WTFMove(overrideColors)));
    … …  
    182182    }
    183183
    184     const FontPaletteIndex& basePalette() const
     184    std::optional<FontPaletteIndex> basePalette() const
    185185    {
    186186        return m_fontPaletteValues.basePalette();
    … …  
    195195
    196196private:
    197     StyleRuleFontPaletteValues(const AtomString& name, const AtomString& fontFamily, const FontPaletteIndex& basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors);
     197    StyleRuleFontPaletteValues(const AtomString& name, const AtomString& fontFamily, std::optional<FontPaletteIndex> basePalette, Vector<FontPaletteValues::OverriddenColor>&& overrideColors);
    198198    StyleRuleFontPaletteValues(const StyleRuleFontPaletteValues&);
    199199

  • trunk/Source/WebCore/css/parser/CSSParserImpl.cpp

    r283540 r283752  
    681681        fontFamily = downcast<CSSPrimitiveValue>(*fontFamilyValue).fontFamily().familyName;
    682682
    683     FontPaletteIndex basePalette;
     683    std::optional<FontPaletteIndex> basePalette;
    684684    if (auto basePaletteValue = properties->getPropertyCSSValue(CSSPropertyBasePalette)) {
    685685        const auto& primitiveValue = downcast<CSSPrimitiveValue>(*basePaletteValue);
    686         if (primitiveValue.isString())
    687             basePalette = FontPaletteIndex(primitiveValue.stringValue());
    688         else if (primitiveValue.isNumber())
     686        if (primitiveValue.isNumber())
    689687            basePalette = FontPaletteIndex(primitiveValue.value<unsigned>());
    690688        else if (primitiveValue.valueID() == CSSValueLight)
    … …  
    698696        const auto& list = downcast<CSSValueList>(*overrideColorsValue);
    699697        for (const auto& item : list) {
    700             FontPaletteValues::PaletteColorIndex key(nullAtom());
    701698            const auto& pair = downcast<CSSFontPaletteValuesOverrideColorsValue>(item.get());
    702             if (pair.key().isString())
    703                 key = pair.key().stringValue();
    704             else if (pair.key().isNumber())
    705                 key = pair.key().value<unsigned>();
    706             else
     699            if (!pair.key().isNumber())
    707700                continue;
     701            unsigned key = pair.key().value<unsigned>();
    708702            Color color = pair.color().isRGBColor() ? pair.color().color() : StyleColor::colorFromKeyword(pair.color().valueID(), { });
    709703            overrideColors.append(std::make_pair(key, color));
    … …  
    711705    }
    712706
    713     return StyleRuleFontPaletteValues::create(name->stringValue(), fontFamily, basePalette, WTFMove(overrideColors));
     707    return StyleRuleFontPaletteValues::create(name->stringValue(), fontFamily, WTFMove(basePalette), WTFMove(overrideColors));
    714708}
    715709

  • trunk/Source/WebCore/css/parser/CSSPropertyParser.cpp

    r283742 r283752  
    48564856    if (auto result = consumeIdent<CSSValueLight, CSSValueDark>(range))
    48574857        return result;
    4858     if (auto result = consumeString(range))
    4859         return result;
    48604858    return consumeInteger(range, 0);
    48614859}
    … …  
    48654863    RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
    48664864    do {
    4867         RefPtr<CSSPrimitiveValue> key;
    4868         if (range.peek().type() == StringToken)
    4869             key = consumeString(range);
    4870         else
    4871             key = consumeInteger(range, 0);
     4865        auto key = consumeInteger(range, 0);
    48724866        if (!key)
    48734867            return nullptr;

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

    r283398 r283752  
    5252    }
    5353
    54     FontPaletteIndex(const AtomString& string)
    55         : type(Type::String)
    56         , string(string)
    57     {
    58     }
    59 
    6054    operator bool() const
    6155    {
    62         return type != Type::String || !string.isNull();
     56        return type != Type::Integer || integer;
    6357    }
    6458
    … …  
    6963        if (type == Type::Integer)
    7064            return integer == other.integer;
    71         if (type == Type::String)
    72             return string == other.string;
    7365        return true;
    7466    }
    … …  
    8274        Light,
    8375        Dark,
    84         Integer,
    85         String
    86     } type { Type::String };
     76        Integer
     77    };
     78    Type type { Type::Integer };
    8779
    8880    unsigned integer { 0 };
    89     AtomString string;
    9081};
    9182
    … …  
    9586    if (paletteIndex.type == FontPaletteIndex::Type::Integer)
    9687        add(hasher, paletteIndex.integer);
    97     else if (paletteIndex.type == FontPaletteIndex::Type::String)
    98         add(hasher, paletteIndex.string);
    9988}
    10089
    10190class FontPaletteValues {
    10291public:
    103     using PaletteColorIndex = Variant<AtomString, unsigned>;
    104     using OverriddenColor = std::pair<PaletteColorIndex, Color>;
     92    using OverriddenColor = std::pair<unsigned, Color>;
    10593
    10694    FontPaletteValues() = default;
    10795
    108     FontPaletteValues(const FontPaletteIndex& basePalette, Vector<OverriddenColor>&& overrideColors)
     96    FontPaletteValues(std::optional<FontPaletteIndex> basePalette, Vector<OverriddenColor>&& overrideColors)
    10997        : m_basePalette(basePalette)
    11098        , m_overrideColors(WTFMove(overrideColors))
    … …  
    112100    }
    113101
    114     const FontPaletteIndex& basePalette() const
     102    std::optional<FontPaletteIndex> basePalette() const
    115103    {
    116104        return m_basePalette;
    … …  
    138126
    139127private:
    140     FontPaletteIndex m_basePalette;
     128    std::optional<FontPaletteIndex> m_basePalette;
    141129    Vector<OverriddenColor> m_overrideColors;
    142130};

  • trunk/Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp

    r283536 r283752  
    458458}
    459459
    460 static void addAttributesForCustomFontPalettes(CFMutableDictionaryRef attributes, const FontPaletteIndex& basePalette, const Vector<FontPaletteValues::OverriddenColor>& overrideColors)
    461 {
    462     switch (basePalette.type) {
    463     case FontPaletteIndex::Type::Light:
    464         addLightPalette(attributes);
    465         break;
    466     case FontPaletteIndex::Type::Dark:
    467         addDarkPalette(attributes);
    468         break;
    469     case FontPaletteIndex::Type::Integer: {
    470         int64_t rawIndex = basePalette.integer; // There is no kCFNumberUIntType.
    471         auto number = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &rawIndex));
    472         CFDictionaryAddValue(attributes, kCTFontPaletteAttribute, number.get());
    473         break;
    474     }
    475     case FontPaletteIndex::Type::String:
    476         // This is unimplementable in Core Text.
    477         break;
     460static void addAttributesForCustomFontPalettes(CFMutableDictionaryRef attributes, std::optional<FontPaletteIndex> basePalette, const Vector<FontPaletteValues::OverriddenColor>& overrideColors)
     461{
     462    if (basePalette) {
     463        switch (basePalette->type) {
     464        case FontPaletteIndex::Type::Light:
     465            addLightPalette(attributes);
     466            break;
     467        case FontPaletteIndex::Type::Dark:
     468            addDarkPalette(attributes);
     469            break;
     470        case FontPaletteIndex::Type::Integer: {
     471            int64_t rawIndex = basePalette->integer; // There is no kCFNumberUIntType.
     472            auto number = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &rawIndex));
     473            CFDictionaryAddValue(attributes, kCTFontPaletteAttribute, number.get());
     474            break;
     475        }
     476        }
    478477    }
    479478
    … …  
    481480        auto overrideDictionary = adoptCF(CFDictionaryCreateMutable(kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
    482481        for (const auto& pair : overrideColors) {
    483             const auto& paletteColorIndex = pair.first;
    484482            const auto& color = pair.second;
    485             WTF::switchOn(paletteColorIndex, [] (const AtomString&) {
    486                 // This is unimplementable in Core Text.
    487             }, [&] (unsigned index) {
    488                 int64_t rawIndex = index; // There is no kCFNumberUIntType.
    489                 auto number = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &rawIndex));
    490                 auto colorObject = cachedCGColor(color);
    491                 CFDictionaryAddValue(overrideDictionary.get(), number.get(), colorObject);
    492             });
     483            int64_t rawIndex = pair.first; // There is no kCFNumberUIntType.
     484            auto number = adoptCF(CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt64Type, &rawIndex));
     485            auto colorObject = cachedCGColor(color);
     486            CFDictionaryAddValue(overrideDictionary.get(), number.get(), colorObject);
    493487        }
    494488        if (CFDictionaryGetCount(overrideDictionary.get()))
Note: See TracChangeset for help on using the changeset viewer.