Changeset 276380 in webkit for trunk/Source/WebCore/css/CSSCounterStyleRule.cpp

Timestamp:

Apr 21, 2021, 12:07:50 PM (4 years ago)

Author:

[email protected]

Message:

[css-counter-styles] Parse @counter-style descriptors
​https://bugs.webkit.org/show_bug.cgi?id=224718

Patch by Tyler Wilcock <Tyler Wilcock> on 2021-04-21
Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Parsing for all @counter-style descriptors is implemented with this
patch, so mark more tests passing.

You'll notice that some @counter-style descriptors implemented in this
patch did not gain any passing tests (e.g. pad, negative). In all
of these cases, the expected results contain a <string> value, and we
fail only because we incorrectly don't serialize these <string> values
with quotes. I have manually confirmed in all cases that these values
are properly parsed, so it's just the serialization that's incorrect.

These <string> values serialize without quotes because WebKit's representation
of custom identifiers is not a separate type, but instead overloaded onto the
CSS_STRING type. This means that during serialization time, WebKit must guess
whether it is actually serializing a string (and include quotes if so), or if
it's serializing a custom ident (leaving off quotes if so).

Relevant code snippet:

​https://github.com/WebKit/WebKit/blob/36caeec07975bd5f47db8ac6b749c2787230a461/Source/WebCore/css/CSSMarkup.cpp#L153#L161

Relevant changelog snippet from David Hyatt, 2016-12-07:

We also overload CSS_STRING primitive value type and have it act as both a string
and a custom identifier. This is lame, since the parser should have made two different
types of objects instead, but since our parser doesn't do that yet, I added a serializeAsStringOrCustomIdent
that preserves our old behavior of "quote the string only if needed." In this case what
that really meant was "Try to guess that we were originally a custom ident and leave off
quotes if so." This function will go away once we properly create CSSStringValues and
CSSCustomIdentValues instead of turning the latter into strings.

• web-platform-tests/css/css-counter-styles/counter-style-fallback-expected.txt:
• web-platform-tests/css/css-counter-styles/counter-style-prefix-suffix-syntax-expected.txt:
• web-platform-tests/css/css-counter-styles/counter-style-range-syntax-expected.txt:
• web-platform-tests/css/css-counter-styles/counter-style-speak-as-syntax-expected.txt:
• web-platform-tests/css/css-counter-styles/counter-style-system-syntax-expected.txt:

Source/WebCore:

Implement parsing and CSSCounterStyleRule IDL interface for @counter-style descriptors.
See spec for full details on all descriptors:

​https://drafts.csswg.org/css-counter-styles-3/#the-counter-style-rule

Test: webexposed/counter-style-image-symbols-not-exposed.html and WPTs

• css/CSSComputedStyleDeclaration.cpp:

(WebCore::ComputedStyleExtractor::valueForPropertyInStyle):
Return nullptr for new @counter-style descriptor properties.

• css/CSSCounterStyleRule.cpp:

(WebCore::toCounterStyleSystemEnum):
(WebCore::symbolsValidForSystem):
(WebCore::StyleRuleCounterStyle::newValueInvalidOrEqual const):
(WebCore::CSSCounterStyleRule::cssText const):
(WebCore::CSSCounterStyleRule::setName):
(WebCore::CSSCounterStyleRule::setterInternal):
(WebCore::CSSCounterStyleRule::setSystem):
(WebCore::CSSCounterStyleRule::setNegative):
(WebCore::CSSCounterStyleRule::setPrefix):
(WebCore::CSSCounterStyleRule::setSuffix):
(WebCore::CSSCounterStyleRule::setRange):
(WebCore::CSSCounterStyleRule::setPad):
(WebCore::CSSCounterStyleRule::setFallback):
(WebCore::CSSCounterStyleRule::setSymbols):
(WebCore::CSSCounterStyleRule::setAdditiveSymbols):
(WebCore::CSSCounterStyleRule::setSpeakAs):
Implement setters and tangential functionality required by setters.

• css/CSSCounterStyleRule.h:

Replace FIXME with actual descriptor getter and setter
implementations.

• css/CSSProperties.json:

Add @counter-style descriptor properties.

• css/CSSValueKeywords.in:

Add new values required for system and speak-as
@counter-style descriptor properties.

• css/parser/CSSPropertyParser.cpp:

(WebCore::consumeCounterStyleSystem):
(WebCore::consumeCounterStyleSymbol):
(WebCore::consumeCounterStyleNegative):
(WebCore::consumeCounterStyleRangeBound):
(WebCore::consumeCounterStyleRange):
(WebCore::consumeCounterStylePad):
(WebCore::consumeCounterStyleSymbols):
(WebCore::consumeCounterStyleAdditiveSymbols):
(WebCore::consumeCounterStyleSpeakAs):
(WebCore::CSSPropertyParser::parseCounterStyleDescriptor):
Parse @counter-style descriptors.

Tools:

• DumpRenderTree/TestOptions.cpp:

(WTR::TestOptions::defaults):
Fix typo (missing 's'). CSSCounterStyleAtRulesEnabled, not
CSSCounterStyleAtRuleEnabled.

LayoutTests:

Add test ensuring <image> @counter-style symbol values cannot be
parsed when the counterStyleAtRuleImageSymbolsEnabled feature flag
is disabled.

---

This test is skipped on Windows because I haven't been able to get the
required feature flags (CSSCounterStyleAtRulesEnabled and
CSSCounterStyleAtRuleImageSymbolsEnabled) to work properly for that
port.

The code hidden behind these flags is all in the CSS parser, which is not
unique to Windows, so I think we can be confident that if the test passes
on all other platforms, that the behavior is correct on Windows too.

One attempt at implementing the necessary Windows-specific flag functionality is here:

​https://bugs.webkit.org/attachment.cgi?id=426371&action=edit

Which failed to compile[1] with this error:

C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp(834,51): error C2039: 'setCSSCounterStyleAtRulesEnabled': is not a member of 'IWebPreferencesPrivate7' [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.vcxproj]
C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp(835,62): error C2039: 'setCSSCounterStyleAtRuleImageSymbolsEnabled': is not a member of 'IWebPreferencesPrivate7' [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Tools\DumpRenderTree\DumpRenderTreeLib.vcxproj]

Those methods are present in IWebPreferencesPrivate7.idl, and implemented similarly to other
flags in other places (e.g. win/WebPreferences.{h, cpp}, win/WebPreferenceKeysPrivate.h).
I can't reproduce this compilation error on my Windows machine.

I then tried removing the lines that caused the above compilation failure.
Those setters are called in DumpRenderTree::enableExperimentalFeatures, so in
lieu of enabling these flags there I could enable the flag I need via test header.

That patch is: ​https://bugs.webkit.org/attachment.cgi?id=426509&action=edit

This results in successful compilation, but causes lots (all?) of the
layout tests to fail[2] with a stacktrace that looks like:

00 00000065738fdf00 00007ffc3e9e3113 WebKit!WebPreferences::speechRecognitionEnabled(int * enabled = 0x00007ffc`3eae0f50)+0x29 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebKitLegacy\win\WebPreferences.cpp @ 2617]
01 00000065738fdf30 00007ffc3e9e3cc0 DumpRenderTreeLib!resetWebPreferencesToConsistentValues(struct IWebPreferences * preferences = 0x00000205`e2f204b0)+0x63 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp @ 847]
02 00000065738fdfa0 00007ffc3e9e4171 DumpRenderTreeLib!resetWebViewToConsistentStateBeforeTesting(class WTR::TestOptions * options = 0x00000065`738fea60)+0x2e0 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp @ 1054]
03 00000065738fe050 00007ffc3e9e67d3 DumpRenderTreeLib!runTest(class std::basic_string<char,std::char_traits<char>,std::allocator<char> > * inputLine = <Value unavailable error>)+0x2f1 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp @ 1239]
04 00000065738feca0 00007ff789952f30 DumpRenderTreeLib!main(int argc = <Value unavailable error>, char argv = <Value unavailable error>)+0x5d3 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\DumpRenderTree\win\DumpRenderTree.cpp @ 1676]
05 00000065738ff5b0 00007ff789953884 DumpRenderTree!main(int argc = 0n2, char argv = 0x00000205`e2e74b80)+0x880 [C:\cygwin\home\buildbot\worker\Windows-EWS\build\Tools\win\DLLLauncher\DLLLauncherMain.cpp @ 232]

I haven't done much digging into why this happens, and cannot reproduce it on my Windows machine.

[1]: ​https://ews-build.webkit.org/#/builders/10/builds/86747
[2]: ​https://ews-build.webkit.org/#/builders/10/builds/86897

• platform/win/TestExpectations: Skip newly added test on Windows.
• webexposed/counter-style-image-symbols-not-exposed-expected.txt: Added.
• webexposed/counter-style-image-symbols-not-exposed.html: Added.

File:

• trunk/Source/WebCore/css/CSSCounterStyleRule.cpp (modified) (4 diffs)

trunk/Source/WebCore/css/CSSCounterStyleRule.cpp

@@ -27 +27 @@
 #include "CSSCounterStyleRule.h"
 
+#include "CSSPropertyParser.h"
+#include "CSSStyleSheet.h"
+#include "CSSTokenizer.h"
+#include "Pair.h"
+#include <wtf/text/StringBuilder.h>
+
 namespace WebCore {
     
@@ -41 +47 @@
 }
 
+static CounterStyleSystem toCounterStyleSystemEnum(RefPtr<CSSValue> system)
+{
+    if (!system || !system->isPrimitiveValue())
+        return CounterStyleSystem::Symbolic;
+
+    auto& primitiveSystemValue = downcast<CSSPrimitiveValue>(*system);
+    ASSERT(primitiveSystemValue.isValueID() || primitiveSystemValue.isPair());
+    CSSValueID systemKeyword = CSSValueInvalid;
+    if (primitiveSystemValue.isValueID())
+        systemKeyword = primitiveSystemValue.valueID();
+    else if (auto* pair = primitiveSystemValue.pairValue()) {
+        // This value must be `fixed` or `extends`, both of which can or must have an additional component.
+        auto firstValue = pair->first();
+        ASSERT(firstValue && firstValue->isValueID());
+        if (firstValue)
+            systemKeyword = firstValue->valueID();
+    }
+
+    switch (systemKeyword) {
+    case CSSValueCyclic:
+        return CounterStyleSystem::Cyclic;
+    case CSSValueFixed:
+        return CounterStyleSystem::Fixed;
+    case CSSValueSymbolic:
+        return CounterStyleSystem::Symbolic;
+    case CSSValueAlphabetic:
+        return CounterStyleSystem::Alphabetic;
+    case CSSValueNumeric:
+        return CounterStyleSystem::Numeric;
+    case CSSValueAdditive:
+        return CounterStyleSystem::Additive;
+    case CSSValueExtends:
+        return CounterStyleSystem::Extends;
+    default:
+        ASSERT_NOT_REACHED();
+        return CounterStyleSystem::Symbolic;
+    }
+}
+
+static bool symbolsValidForSystem(CounterStyleSystem system, RefPtr<CSSValue> symbols, RefPtr<CSSValue> additiveSymbols)
+{
+    switch (system) {
+    case CounterStyleSystem::Cyclic:
+    case CounterStyleSystem::Fixed:
+    case CounterStyleSystem::Symbolic:
+        return symbols && symbols->isValueList() && downcast<CSSValueList>(*symbols).length();
+    case CounterStyleSystem::Alphabetic:
+    case CounterStyleSystem::Numeric:
+        return symbols && symbols->isValueList() && downcast<CSSValueList>(*symbols).length() >= 2u;
+    case CounterStyleSystem::Additive:
+        return additiveSymbols && additiveSymbols->isValueList() && downcast<CSSValueList>(*additiveSymbols).length();
+    case CounterStyleSystem::Extends:
+        return !symbols && !additiveSymbols;
+    default:
+        ASSERT_NOT_REACHED();
+        return false;
+    }
+}
+
+bool StyleRuleCounterStyle::newValueInvalidOrEqual(CSSPropertyID propertyID, const RefPtr<CSSValue> newValue) const
+{
+    auto currentValue = m_properties->getPropertyCSSValue(propertyID);
+    if (compareCSSValuePtr(currentValue, newValue))
+        return true;
+
+    RefPtr<CSSValue> symbols;
+    RefPtr<CSSValue> additiveSymbols;
+    switch (propertyID) {
+    case CSSPropertySystem:
+        // If the attribute being set is `system`, and the new value would change the algorithm used, do nothing
+        // and abort these steps.
+        // (It's okay to change an aspect of the algorithm, like the first symbol value of a `fixed` system.)
+        return toCounterStyleSystemEnum(currentValue) != toCounterStyleSystemEnum(newValue);
+    case CSSPropertySymbols:
+        symbols = newValue;
+        additiveSymbols = m_properties->getPropertyCSSValue(CSSPropertyAdditiveSymbols);
+        break;
+    case CSSPropertyAdditiveSymbols:
+        symbols = m_properties->getPropertyCSSValue(CSSPropertySymbols);
+        additiveSymbols = newValue;
+        break;
+    default:
+        return false;
+    }
+    auto system = m_properties->getPropertyCSSValue(CSSPropertySystem);
+    return symbolsValidForSystem(toCounterStyleSystemEnum(system), symbols, additiveSymbols);
+}
+
 StyleRuleCounterStyle::~StyleRuleCounterStyle() = default;
 
@@ -65 +159 @@
 String CSSCounterStyleRule::cssText() const
 {
-    // FIXME: Implement this function when we parse @counter-style descriptors.
-    return emptyString();
+    String systemText = system();
+    const char* systemPrefix = systemText.isEmpty() ? "" : " system: ";
+    const char* systemSuffix = systemText.isEmpty() ? "" : ";";
+
+    String symbolsText = symbols();
+    const char* symbolsPrefix = symbolsText.isEmpty() ? "" : " symbols: ";
+    const char* symbolsSuffix = symbolsText.isEmpty() ? "" : ";";
+
+    String additiveSymbolsText = additiveSymbols();
+    const char* additiveSymbolsPrefix = additiveSymbolsText.isEmpty() ? "" : " additive-symbols: ";
+    const char* additiveSymbolsSuffix = additiveSymbolsText.isEmpty() ? "" : ";";
+
+    String negativeText = negative();
+    const char* negativePrefix = negativeText.isEmpty() ? "" : " negative: ";
+    const char* negativeSuffix = negativeText.isEmpty() ? "" : ";";
+
+    String prefixText = prefix();
+    const char* prefixTextPrefix = prefixText.isEmpty() ? "" : " prefix: ";
+    const char* prefixTextSuffix = prefixText.isEmpty() ? "" : ";";
+
+    String suffixText = suffix();
+    const char* suffixTextPrefix = suffixText.isEmpty() ? "" : " suffix: ";
+    const char* suffixTextSuffix = suffixText.isEmpty() ? "" : ";";
+
+    String padText = pad();
+    const char* padPrefix = padText.isEmpty() ? "" : " pad: ";
+    const char* padSuffix = padText.isEmpty() ? "" : ";";
+
+    String rangeText = range();
+    const char* rangePrefix = rangeText.isEmpty() ? "" : " range: ";
+    const char* rangeSuffix = rangeText.isEmpty() ? "" : ";";
+
+    String fallbackText = fallback();
+    const char* fallbackPrefix = fallbackText.isEmpty() ? "" : " fallback: ";
+    const char* fallbackSuffix = fallbackText.isEmpty() ? "" : ";";
+
+    String speakAsText = speakAs();
+    const char* speakAsPrefix = speakAsText.isEmpty() ? "" : " speak-as: ";
+    const char* speakAsSuffix = speakAsText.isEmpty() ? "" : ";";
+
+    return makeString("@counter-style ", name(), " {", systemPrefix, systemText, systemSuffix, symbolsPrefix, symbolsText, symbolsSuffix, additiveSymbolsPrefix, additiveSymbolsText, additiveSymbolsSuffix, negativePrefix, negativeText, negativeSuffix, prefixTextPrefix, prefixText, prefixTextSuffix, suffixTextPrefix, suffixText, suffixTextSuffix, padPrefix, padText, padSuffix, rangePrefix, rangeText, rangeSuffix, fallbackPrefix, fallbackText, fallbackSuffix, speakAsPrefix, speakAsText, speakAsSuffix, " }");
 }
 
@@ -74 +207 @@
 }
 
+// https://drafts.csswg.org/css-counter-styles-3/#dom-csscounterstylerule-name
+void CSSCounterStyleRule::setName(const String& text)
+{
+    auto tokens = CSSTokenizer(text).tokenRange();
+    auto name = CSSPropertyParserHelpers::consumeCounterStyleNameInPrelude(tokens);
+    if (name.isNull() || name == m_counterStyleRule->name())
+        return;
+
+    CSSStyleSheet::RuleMutationScope mutationScope(this);
+    m_counterStyleRule->setName(name);
+}
+
+void CSSCounterStyleRule::setterInternal(CSSPropertyID propertyID, const String& valueText)
+{
+    auto tokens = CSSTokenizer(valueText).tokenRange();
+    auto newValue = CSSPropertyParser::parseCounterStyleDescriptor(propertyID, tokens, parserContext());
+    if (m_counterStyleRule->newValueInvalidOrEqual(propertyID, newValue))
+        return;
+
+    CSSStyleSheet::RuleMutationScope mutationScope(this);
+    m_counterStyleRule->mutableProperties().setProperty(propertyID, WTFMove(newValue));
+}
+
+void CSSCounterStyleRule::setSystem(const String& text)
+{
+    setterInternal(CSSPropertySystem, text);
+}
+
+void CSSCounterStyleRule::setNegative(const String& text)
+{
+    setterInternal(CSSPropertyNegative, text);
+}
+
+void CSSCounterStyleRule::setPrefix(const String& text)
+{
+    setterInternal(CSSPropertyPrefix, text);
+}
+
+void CSSCounterStyleRule::setSuffix(const String& text)
+{
+    setterInternal(CSSPropertySuffix, text);
+}
+
+void CSSCounterStyleRule::setRange(const String& text)
+{
+    setterInternal(CSSPropertyRange, text);
+}
+
+void CSSCounterStyleRule::setPad(const String& text)
+{
+    setterInternal(CSSPropertyPad, text);
+}
+
+void CSSCounterStyleRule::setFallback(const String& text)
+{
+    setterInternal(CSSPropertyFallback, text);
+}
+
+void CSSCounterStyleRule::setSymbols(const String& text)
+{
+    setterInternal(CSSPropertySymbols, text);
+}
+
+void CSSCounterStyleRule::setAdditiveSymbols(const String& text)
+{
+    setterInternal(CSSPropertyAdditiveSymbols, text);
+}
+
+void CSSCounterStyleRule::setSpeakAs(const String& text)
+{
+    setterInternal(CSSPropertySpeakAs, text);
+}
+
 } // namespace WebCore