[CSS Parser] Fix font-synthesis and text-decoration-skip parsing
authorhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 16:57:30 +0000 (16:57 +0000)
committerhyatt@apple.com <hyatt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 16:57:30 +0000 (16:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164736

Reviewed by Dean Jackson.

Source/WebCore:

Fix the properties to not allow duplicate values, to reject when
garbage values are included, to require that none be a singleton,
and to preserve the declaration order of the properties.

* css/StyleBuilderConverter.h:
(WebCore::StyleBuilderConverter::convertTextDecorationSkip):
* css/parser/CSSParser.cpp:
(WebCore::CSSParser::parseFontSynthesis):
(WebCore::CSSParser::parseTextDecorationSkip):
* css/parser/CSSPropertyParser.cpp:
(WebCore::consumeFontSynthesis):
(WebCore::consumeTextDecorationSkip):
(WebCore::CSSPropertyParser::parseSingleValue):

LayoutTests:

* fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip-expected.txt:
* fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html:
* fast/css3-text/font-synthesis-parse-expected.txt:
* fast/css3-text/font-synthesis-parse.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip-expected.txt
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html
LayoutTests/fast/css3-text/font-synthesis-parse-expected.txt
LayoutTests/fast/css3-text/font-synthesis-parse.html
Source/WebCore/ChangeLog
Source/WebCore/css/StyleBuilderConverter.h
Source/WebCore/css/parser/CSSParser.cpp
Source/WebCore/css/parser/CSSPropertyParser.cpp

index 8b10bc9..b67ce7d 100644 (file)
@@ -1,3 +1,15 @@
+2016-11-14  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Fix font-synthesis and text-decoration-skip parsing
+        https://bugs.webkit.org/show_bug.cgi?id=164736
+
+        Reviewed by Dean Jackson.
+
+        * fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip-expected.txt:
+        * fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html:
+        * fast/css3-text/font-synthesis-parse-expected.txt:
+        * fast/css3-text/font-synthesis-parse.html:
+
 2016-11-15  Daniel Bates  <dabates@apple.com>
 
         Disallow loads using HTTP 0.9 at the ResourceHandle/NetworkDataTask level
index 7b24441..1449c27 100644 (file)
@@ -11,52 +11,42 @@ PASS declaration.length is 1
 PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
 PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 0
 PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "ink"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "ink"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 1
 PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
 PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "objects"
-PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "objects"
+PASS declaration.length is 0
+PASS computedStyle.getPropertyCSSValue('-webkit-text-decoration-skip').cssText is "auto"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 1
 PASS declaration.getPropertyValue('-webkit-text-decoration-skip') is "none"
index 4d3943a..7d30a50 100644 (file)
     testInkIsValid(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: ink ink ink ink ink; }", 0);
-    testInkIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: garbage; }", 0);
     testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: garbage ink; }", 0);
-    testInkIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: ink garbage; }", 0);
-    testInkIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
     
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: garbage ink garbage; }", 0);
-    testInkIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
     
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: ink garbage ink; }", 0);
-    testInkIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
     
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: objects; }", 0);
     testObjectsIsValid(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: objects objects objects objects objects; }", 0);
-    testObjectsIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: garbage objects; }", 0);
-    testObjectsIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: objects garbage; }", 0);
-    testObjectsIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: garbage objects garbage; }", 0);
-    testObjectsIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: objects garbage objects; }", 0);
-    testObjectsIsValid(stylesheet, target);
+    testInvalidRule(stylesheet, target);
 
     stylesheet.insertRule(".p { -webkit-text-decoration-skip: none; }", 0);
     testNoneIsValid(stylesheet, target);
index 04b6b26..9bf63f0 100644 (file)
@@ -16,19 +16,11 @@ PASS declaration.getPropertyValue('font-synthesis') is "style"
 PASS computedStyle.getPropertyCSSValue('font-synthesis').cssText is "style"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 1
-PASS declaration.getPropertyValue('font-synthesis') is "style"
-PASS computedStyle.getPropertyCSSValue('font-synthesis').cssText is "style"
-PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
 PASS declaration.getPropertyValue('font-synthesis') is "weight style"
 PASS computedStyle.getPropertyCSSValue('font-synthesis').cssText is "style weight"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 1
-PASS declaration.getPropertyValue('font-synthesis') is "weight style"
-PASS computedStyle.getPropertyCSSValue('font-synthesis').cssText is "style weight"
-PASS cssRule.type is cssRule.STYLE_RULE
-PASS declaration.length is 1
-PASS declaration.getPropertyValue('font-synthesis') is "weight style"
+PASS declaration.getPropertyValue('font-synthesis') is "style weight"
 PASS computedStyle.getPropertyCSSValue('font-synthesis').cssText is "style weight"
 PASS cssRule.type is cssRule.STYLE_RULE
 PASS declaration.length is 0
index 7b39ce1..de9ca6c 100644 (file)
@@ -67,7 +67,7 @@
       shouldBe("cssRule.type", "cssRule.STYLE_RULE");
       declaration = cssRule.style;
       shouldBe("declaration.length", "1");
-      shouldBeEqualToString("declaration.getPropertyValue('font-synthesis')", "weight style");
+      shouldBeEqualToString("declaration.getPropertyValue('font-synthesis')", "style weight");
       computedStyle = window.getComputedStyle(target, null);
       shouldBeEqualToString("computedStyle.getPropertyCSSValue('font-synthesis').cssText", "style weight");
       stylesheet.deleteRule(0);
     stylesheet.insertRule("#p { font-synthesis: weight }", 0);
     testWeight(stylesheet, target);
 
-    stylesheet.insertRule("#p { font-synthesis: style style }", 0);
-    testStyle(stylesheet, target);
-
     stylesheet.insertRule("#p { font-synthesis: style }", 0);
     testStyle(stylesheet, target);
 
     stylesheet.insertRule("#p { font-synthesis: weight style }", 0);
     testWeightAndStyle(stylesheet, target);
 
-    stylesheet.insertRule("#p { font-synthesis: style weight weight}", 0);
-    testStyleAndWeight(stylesheet, target);
-
     stylesheet.insertRule("#p { font-synthesis: style weight }", 0);
     testStyleAndWeight(stylesheet, target);
 
index 201c25d..8cd976b 100644 (file)
@@ -1,3 +1,24 @@
+2016-11-14  Dave Hyatt  <hyatt@apple.com>
+
+        [CSS Parser] Fix font-synthesis and text-decoration-skip parsing
+        https://bugs.webkit.org/show_bug.cgi?id=164736
+
+        Reviewed by Dean Jackson.
+
+        Fix the properties to not allow duplicate values, to reject when
+        garbage values are included, to require that none be a singleton,
+        and to preserve the declaration order of the properties.
+
+        * css/StyleBuilderConverter.h:
+        (WebCore::StyleBuilderConverter::convertTextDecorationSkip):
+        * css/parser/CSSParser.cpp:
+        (WebCore::CSSParser::parseFontSynthesis):
+        (WebCore::CSSParser::parseTextDecorationSkip):
+        * css/parser/CSSPropertyParser.cpp:
+        (WebCore::consumeFontSynthesis):
+        (WebCore::consumeTextDecorationSkip):
+        (WebCore::CSSPropertyParser::parseSingleValue):
+
 2016-11-15  Daniel Bates  <dabates@apple.com>
 
         Disallow loads using HTTP 0.9 at the ResourceHandle/NetworkDataTask level
index 856287b..ace5d86 100644 (file)
@@ -750,7 +750,7 @@ inline TextDecorationSkip StyleBuilderConverter::convertTextDecorationSkip(Style
     if (is<CSSPrimitiveValue>(value))
         return valueToDecorationSkip(downcast<CSSPrimitiveValue>(value));
 
-    TextDecorationSkip skip = RenderStyle::initialTextDecorationSkip();
+    TextDecorationSkip skip = TextDecorationSkipNone;
     for (auto& currentValue : downcast<CSSValueList>(value))
         skip |= valueToDecorationSkip(downcast<CSSPrimitiveValue>(currentValue.get()));
     return skip;
index d7fdbfb..2a00c32 100644 (file)
@@ -7083,7 +7083,6 @@ bool CSSParser::parseFontWeight(bool important)
 
 bool CSSParser::parseFontSynthesis(bool important)
 {
-    // none | [ weight || style ]
     CSSParserValue* value = m_valueList->current();
     if (value && value->id == CSSValueNone) {
         addProperty(CSSPropertyFontSynthesis, CSSValuePool::singleton().createIdentifierValue(CSSValueNone), important);
@@ -7091,29 +7090,30 @@ bool CSSParser::parseFontSynthesis(bool important)
         return true;
     }
 
-    bool encounteredWeight = false;
-    bool encounteredStyle = false;
-    while (value) {
+    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
+
+    do {
         switch (value->id) {
         case CSSValueWeight:
-            encounteredWeight = true;
-            break;
-        case CSSValueStyle:
-            encounteredStyle = true;
+        case CSSValueStyle: {
+            auto singleValue = CSSValuePool::singleton().createIdentifierValue(value->id);
+            if (list->hasValue(singleValue.ptr()))
+                return false;
+            list->append(WTFMove(singleValue));
             break;
+        }
         default:
             return false;
         }
-        value = m_valueList->next();
-    }
-
-    auto list = CSSValueList::createSpaceSeparated();
-    if (encounteredWeight)
-        list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueWeight));
-    if (encounteredStyle)
-        list->append(CSSValuePool::singleton().createIdentifierValue(CSSValueStyle));
-    addProperty(CSSPropertyFontSynthesis, WTFMove(list), important);
+    } while ((value = m_valueList->next()));
+    
+    if (!list->length())
+        return false;
+    
+    addProperty(CSSPropertyFontSynthesis, list.releaseNonNull(), important);
+    m_valueList->next();
     return true;
+
 }
 
 bool CSSParser::parseFontFaceSrcURI(CSSValueList& valueList)
@@ -10411,19 +10411,36 @@ bool CSSParser::parseTextDecorationSkip(bool important)
     // The text-decoration-skip property has syntax "none | [ objects || spaces || ink || edges || box-decoration ]".
     // However, only 'none' and 'ink' are implemented yet, so we will parse syntax "none | ink" for now.
     CSSParserValue* value = m_valueList->current();
+    if (value && value->id == CSSValueNone) {
+        addProperty(CSSPropertyWebkitTextDecorationSkip, CSSValuePool::singleton().createIdentifierValue(CSSValueNone), important);
+        m_valueList->next();
+        return true;
+    }
+    
+    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
+
     do {
         switch (value->id) {
-        case CSSValueNone:
         case CSSValueAuto:
         case CSSValueInk:
-        case CSSValueObjects:
-            addProperty(CSSPropertyWebkitTextDecorationSkip, CSSValuePool::singleton().createIdentifierValue(value->id), important);
-            return true;
-        default:
+        case CSSValueObjects: {
+            auto singleValue = CSSValuePool::singleton().createIdentifierValue(value->id);
+            if (list->hasValue(singleValue.ptr()))
+                return false;
+            list->append(WTFMove(singleValue));
             break;
         }
+        default:
+            return false;
+        }
     } while ((value = m_valueList->next()));
-    return false;
+    
+    if (!list->length())
+        return false;
+    
+    addProperty(CSSPropertyWebkitTextDecorationSkip, list.releaseNonNull(), important);
+    m_valueList->next();
+    return true;
 }
 
 bool CSSParser::parseTextUnderlinePosition(bool important)
index 781b542..5c2d24a 100644 (file)
@@ -790,30 +790,23 @@ static RefPtr<CSSValueList> consumeFontFamily(CSSParserTokenRange& range)
 static RefPtr<CSSValue> consumeFontSynthesis(CSSParserTokenRange& range)
 {
     // none | [ weight || style ]
-    if (range.peek().id() == CSSValueNone)
+    CSSValueID id = range.peek().id();
+    if (id == CSSValueNone)
         return consumeIdent(range);
     
-    RefPtr<CSSValue> weight;
-    RefPtr<CSSValue> style;
-    
-    // FIXME: We allow weight and style to occur multiple times because the
-    // old parser did, and layout tests specifically check for it. It seems
-    // wrong though.
-    do {
-        if (range.peek().id() == CSSValueWeight)
-            weight = consumeIdent(range);
-        else if (range.peek().id() == CSSValueStyle)
-            style = consumeIdent(range);
-        else
+    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
+    while (true) {
+        auto ident = consumeIdent<CSSValueWeight, CSSValueStyle>(range);
+        if (!ident)
+            break;
+        if (list->hasValue(ident.get()))
             return nullptr;
-    } while (!range.atEnd());
+        list->append(ident.releaseNonNull());
+    }
     
-    auto list = CSSValueList::createSpaceSeparated();
-    if (weight)
-        list->append(weight.releaseNonNull());
-    if (style)
-        list->append(style.releaseNonNull());
-    return WTFMove(list);
+    if (!list->length())
+        return nullptr;
+    return list;
 }
 
 static RefPtr<CSSValue> consumeLetterSpacing(CSSParserTokenRange& range, CSSParserMode cssParserMode)
@@ -1418,6 +1411,27 @@ static RefPtr<CSSValue> consumeTextDecorationLine(CSSParserTokenRange& range)
     return list;
 }
 
+static RefPtr<CSSValue> consumeTextDecorationSkip(CSSParserTokenRange& range)
+{
+    CSSValueID id = range.peek().id();
+    if (id == CSSValueNone)
+        return consumeIdent(range);
+
+    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
+    while (true) {
+        auto ident = consumeIdent<CSSValueAuto, CSSValueInk, CSSValueObjects>(range);
+        if (!ident)
+            break;
+        if (list->hasValue(ident.get()))
+            return nullptr;
+        list->append(ident.releaseNonNull());
+    }
+
+    if (!list->length())
+        return nullptr;
+    return list;
+}
+
 static RefPtr<CSSValue> consumeTextEmphasisStyle(CSSParserTokenRange& range)
 {
     CSSValueID id = range.peek().id();
@@ -3509,6 +3523,8 @@ RefPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID property, CSS
         return consumePositiveInteger(m_range);
     case CSSPropertyWebkitTextDecorationColor:
         return consumeColor(m_range, m_context.mode);
+    case CSSPropertyWebkitTextDecorationSkip:
+        return consumeTextDecorationSkip(m_range);
     case CSSPropertyWebkitTextStrokeWidth:
         return consumeTextStrokeWidth(m_range, m_context.mode);
     case CSSPropertyWebkitTextFillColor: