Skip to content
Permalink
Browse files
[@Property] Inline style should not validate registered custom proper…
…ty values

https://bugs.webkit.org/show_bug.cgi?id=249763
<rdar://problem/103629699>

Reviewed by Tim Nguyen.

Validation is supposed to happen at computed value time only.

https://drafts.css-houdini.org/css-properties-values-api/#parsing-custom-properties

* LayoutTests/css-custom-properties-api/inline-expected.txt:
* LayoutTests/css-custom-properties-api/inline.html:

Update this test to the correct behavior.

* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/registered-property-cssom-expected.txt:
* Source/WebCore/animation/KeyframeEffect.cpp:
(WebCore::processIterableKeyframes):
(WebCore::processPropertyIndexedKeyframes):
* Source/WebCore/animation/WebAnimation.cpp:
(WebCore::WebAnimation::commitStyles):
* Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:
(WebCore::PropertySetCSSStyleDeclaration::setProperty):
* Source/WebCore/css/StyleProperties.cpp:
(WebCore::MutableStyleProperties::setCustomProperty):

Stop validating registered properties against the syntax.

* Source/WebCore/css/StyleProperties.h:
* Source/WebCore/css/parser/CSSPropertyParser.cpp:
(WebCore::CSSPropertyParser::consumeCustomPropertyValueWithSyntax):
(WebCore::CSSPropertyParser::canParseTypedCustomPropertyValue): Deleted.
* Source/WebCore/css/parser/CSSPropertyParser.h:
* Source/WebCore/dom/StyledElement.cpp:
(WebCore::StyledElement::setInlineStyleCustomProperty):

Canonical link: https://commits.webkit.org/258240@main
  • Loading branch information
anttijk committed Dec 22, 2022
1 parent dced9d4 commit e78366ee24607dc13c6b13633c8a1ddf390df061
Show file tree
Hide file tree
Showing 11 changed files with 22 additions and 58 deletions.
@@ -4,12 +4,12 @@ test
PASS CSSOM setters function as expected for unregistered properties
PASS CSS.registerProperty
PASS Formerly valid values are still readable from inline styles but are computed as the unset value
PASS Values not matching the registered type can't be set
PASS Values not matching the registered type can be set.
PASS Values can be removed from inline styles
PASS Stylesheets can be modified by CSSOM
PASS Valid values can be set on inline styles
FAIL Var values are accepted assert_equals: expected "calc(var(--b) + 10px)" but got "inherit"
FAIL Var values are accepted without validation assert_equals: expected "calc(var(--b) + 15px)" but got "inherit"
FAIL Var values are accepted without validation, even when it is obvious they will not parse assert_equals: expected "calc(var(--b) 15px)" but got "inherit"
FAIL Var values are accepted without validation, even when it is obvious they will not parse (typed) assert_equals: expected "calc(var(--registered) 15px)" but got "inherit"
PASS Var values are accepted
PASS Var values are accepted without validation
PASS Var values are accepted without validation, even when it is obvious they will not parse
PASS Var values are accepted without validation, even when it is obvious they will not parse (typed)

@@ -47,9 +47,11 @@
test(function() {
inlineStyle.setProperty('--a', 'hi');
inlineStyle.setProperty('--b', 'hi');
assert_equals(inlineStyle.getPropertyValue('--a'), 'hello');
assert_equals(inlineStyle.getPropertyValue('--a'), 'hi');
assert_equals(inlineStyle.getPropertyValue('--b'), 'hi');
}, "Values not matching the registered type can't be set");
assert_equals(computedStyle.getPropertyValue('--a'), '0px');
assert_equals(computedStyle.getPropertyValue('--b'), 'hi');
}, "Values not matching the registered type can be set.");

test(function() {
inlineStyle.removeProperty('--a');
@@ -61,8 +63,8 @@
}, "Values can be removed from inline styles");

test(function() {
sheetStyle.setProperty('--a', 'banana'); // Invalid, no change
assert_equals(computedStyle.getPropertyValue('--a'), '160px');
sheetStyle.setProperty('--a', 'banana'); // Invalid, treated as initial value.
assert_equals(computedStyle.getPropertyValue('--a'), '0px');
sheetStyle.setProperty('--a', '20px');
assert_equals(computedStyle.getPropertyValue('--a'), '20px');
sheetStyle.setProperty('--a', 'initial');
@@ -2,8 +2,8 @@
FAIL CSSOM setters function as expected for unregistered properties assert_equals: expected " 10px" but got "10px"
PASS CSS.registerProperty
FAIL Formerly valid values are still readable from inline styles but are computed as the unset value assert_equals: expected "5" but got ""
FAIL Values not matching the registered type can still be set assert_equals: expected "hi" but got ""
PASS Values not matching the registered type can still be set
PASS Values can be removed from inline styles
FAIL Stylesheets can be modified by CSSOM assert_equals: expected "0px" but got "10px"
PASS Stylesheets can be modified by CSSOM
PASS Valid values can be set on inline styles

@@ -346,7 +346,7 @@ static inline ExceptionOr<void> processIterableKeyframes(JSGlobalObject& lexical
auto stringValue = propertyAndValue.values[0];
if (cssPropertyId == CSSPropertyCustom) {
auto customProperty = propertyAndValue.customProperty;
if (keyframeOutput.style->setCustomProperty(&document, customProperty, stringValue, false, parserContext))
if (keyframeOutput.style->setCustomProperty(customProperty, stringValue, false, parserContext))
keyframeOutput.customStyleStrings.set(customProperty, stringValue);
} else if (keyframeOutput.style->setProperty(cssPropertyId, stringValue, false, parserContext))
keyframeOutput.styleStrings.set(cssPropertyId, stringValue);
@@ -388,7 +388,7 @@ static inline ExceptionOr<void> processPropertyIndexedKeyframes(JSGlobalObject&
// 2. Add the property-value pair, property name → v, to k.
if (propertyName == CSSPropertyCustom) {
auto customProperty = m.customProperty;
if (k.style->setCustomProperty(&document, customProperty, v, false, parserContext))
if (k.style->setCustomProperty(customProperty, v, false, parserContext))
k.customStyleStrings.set(customProperty, v);
} else if (k.style->setProperty(propertyName, v, false, parserContext))
k.styleStrings.set(propertyName, v);
@@ -1592,7 +1592,7 @@ ExceptionOr<void> WebAnimation::commitStyles()
},
[&] (const AtomString& customProperty) {
if (auto cssValue = computedStyleExtractor.customPropertyValue(customProperty))
return inlineStyle->setCustomProperty(&styledElement.document(), customProperty, cssValue->cssText(), false, { styledElement.document() });
return inlineStyle->setCustomProperty(customProperty, cssValue->cssText(), false, { styledElement.document() });
return false;
}
);
@@ -260,16 +260,9 @@ ExceptionOr<void> PropertySetCSSStyleDeclaration::setProperty(const String& prop
return { };

bool changed;
if (UNLIKELY(propertyID == CSSPropertyCustom)) {
Document* document = nullptr;

if (parentElement())
document = &parentElement()->document();
else if (parentStyleSheet())
document = parentStyleSheet()->ownerDocument();

changed = m_propertySet->setCustomProperty(document, propertyName, value, important, cssParserContext());
} else
if (UNLIKELY(propertyID == CSSPropertyCustom))
changed = m_propertySet->setCustomProperty(propertyName, value, important, cssParserContext());
else
changed = m_propertySet->setProperty(propertyID, value, important, cssParserContext());

didMutate(changed ? PropertyChanged : NoChanges);
@@ -34,13 +34,11 @@
#include "CSSPendingSubstitutionValue.h"
#include "CSSPrimitiveValue.h"
#include "CSSPropertyParser.h"
#include "CSSRegisteredCustomProperty.h"
#include "CSSTokenizer.h"
#include "CSSValueKeywords.h"
#include "CSSValueList.h"
#include "CSSValuePool.h"
#include "Color.h"
#include "CustomPropertyRegistry.h"
#include "Document.h"
#include "FontSelectionValueInlines.h"
#include "PropertySetCSSStyleDeclaration.h"
@@ -1279,7 +1277,7 @@ bool MutableStyleProperties::setProperty(CSSPropertyID propertyID, const String&
return setProperty(propertyID, value, important, parserContext, didFailParsing);
}

bool MutableStyleProperties::setCustomProperty(const Document* document, const String& propertyName, const String& value, bool important, CSSParserContext parserContext)
bool MutableStyleProperties::setCustomProperty(const String& propertyName, const String& value, bool important, CSSParserContext parserContext)
{
// Setting the value to an empty string just removes the property in both IE and Gecko.
// Setting it to null seems to produce less consistent results, but we treat it just the same.
@@ -1289,13 +1287,6 @@ bool MutableStyleProperties::setCustomProperty(const Document* document, const S
parserContext.mode = cssParserMode();

auto propertyNameAtom = AtomString { propertyName };
auto* registered = document ? document->customPropertyRegistry().get(propertyNameAtom) : nullptr;

auto& syntax = registered ? registered->syntax : CSSCustomPropertySyntax::universal();

CSSTokenizer tokenizer(value);
if (!CSSPropertyParser::canParseTypedCustomPropertyValue(syntax, tokenizer.tokenRange(), parserContext))
return false;

// When replacing an existing property value, this moves the property to the end of the list.
// Firefox preserves the position, and MSIE moves the property to the beginning.
@@ -293,7 +293,7 @@ class MutableStyleProperties final : public StyleProperties {
Vector<CSSProperty, 4> m_propertyVector;

// Methods for querying and altering CSS custom properties.
bool setCustomProperty(const Document*, const String& propertyName, const String& value, bool important, CSSParserContext);
bool setCustomProperty(const String& propertyName, const String& value, bool important, CSSParserContext);
bool removeCustomProperty(const String& propertyName, String* returnText = nullptr);

private:
@@ -256,12 +256,6 @@ RefPtr<CSSValue> CSSPropertyParser::parseSingleValue(CSSPropertyID property, con
return value;
}

bool CSSPropertyParser::canParseTypedCustomPropertyValue(const CSSCustomPropertySyntax& syntax, const CSSParserTokenRange& tokens, const CSSParserContext& context)
{
CSSPropertyParser parser(tokens, context, nullptr);
return parser.canParseTypedCustomPropertyValue(syntax);
}

RefPtr<CSSCustomPropertyValue> CSSPropertyParser::parseTypedCustomPropertyValue(const AtomString& name, const CSSCustomPropertySyntax& syntax, const CSSParserTokenRange& tokens, Style::BuilderState& builderState, const CSSParserContext& context)
{
CSSPropertyParser parser(tokens, context, nullptr, false);
@@ -429,20 +423,6 @@ std::pair<RefPtr<CSSValue>, CSSCustomPropertySyntax::Type> CSSPropertyParser::co
return { nullptr, CSSCustomPropertySyntax::Type::Unknown };
}

bool CSSPropertyParser::canParseTypedCustomPropertyValue(const CSSCustomPropertySyntax& syntax)
{
if (syntax.isUniversal())
return true;

m_range.consumeWhitespace();

if (maybeConsumeCSSWideKeyword(m_range))
return true;

auto [value, syntaxType] = consumeCustomPropertyValueWithSyntax(syntax);
return value;
}

void CSSPropertyParser::collectParsedCustomPropertyValueDependencies(const CSSCustomPropertySyntax& syntax, bool isInitial, HashSet<CSSPropertyID>& dependencies)
{
if (syntax.isUniversal())
@@ -50,7 +50,6 @@ class CSSPropertyParser {
// Parses a non-shorthand CSS property
static RefPtr<CSSValue> parseSingleValue(CSSPropertyID, const CSSParserTokenRange&, const CSSParserContext&);

static bool canParseTypedCustomPropertyValue(const CSSCustomPropertySyntax&, const CSSParserTokenRange&, const CSSParserContext&);
static RefPtr<CSSCustomPropertyValue> parseTypedCustomPropertyInitialValue(const AtomString&, const CSSCustomPropertySyntax&, CSSParserTokenRange, Style::BuilderState&, const CSSParserContext&);
static RefPtr<CSSCustomPropertyValue> parseTypedCustomPropertyValue(const AtomString& name, const CSSCustomPropertySyntax&, const CSSParserTokenRange&, Style::BuilderState&, const CSSParserContext&);
static void collectParsedCustomPropertyValueDependencies(const CSSCustomPropertySyntax&, bool isRoot, HashSet<CSSPropertyID>& dependencies, const CSSParserTokenRange&, const CSSParserContext&);
@@ -66,7 +65,6 @@ class CSSPropertyParser {
RefPtr<CSSValue> parseSingleValue(CSSPropertyID, CSSPropertyID = CSSPropertyInvalid);

std::pair<RefPtr<CSSValue>, CSSCustomPropertySyntax::Type> consumeCustomPropertyValueWithSyntax(const CSSCustomPropertySyntax&);
bool canParseTypedCustomPropertyValue(const CSSCustomPropertySyntax&);
RefPtr<CSSCustomPropertyValue> parseTypedCustomPropertyValue(const AtomString& name, const CSSCustomPropertySyntax&, Style::BuilderState&);
void collectParsedCustomPropertyValueDependencies(const CSSCustomPropertySyntax&, bool isInitial, HashSet<CSSPropertyID>& dependencies);

@@ -229,7 +229,7 @@ bool StyledElement::setInlineStyleProperty(CSSPropertyID propertyID, const Strin

bool StyledElement::setInlineStyleCustomProperty(const AtomString& property, const String& value, bool important)
{
bool changes = ensureMutableInlineStyle().setCustomProperty(&document(), property.string(), value, important, CSSParserContext(document()));
bool changes = ensureMutableInlineStyle().setCustomProperty(property.string(), value, important, CSSParserContext(document()));
if (changes)
inlineStyleChanged();
return changes;

0 comments on commit e78366e

Please sign in to comment.