Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[@property] Fallback that does not match syntax should make var() invalid #8116

Merged
merged 1 commit into from Jan 1, 2023
Merged

[@property] Fallback that does not match syntax should make var() invalid #8116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...form-tests/css/css-properties-values-api/var-reference-registered-properties-expected.txt
View file
Open in desktop
Expand Up @@ -10,7 +10,7 @@ FAIL Values are absolutized when substituting into properties with universal syn
PASS Valid fallback does not invalidate var()-reference [<length>, 10px]
PASS Valid fallback does not invalidate var()-reference [<length> | <color>, red]
PASS Valid fallback does not invalidate var()-reference [<length> | none, none]
FAIL Invalid fallback invalidates var()-reference [<length>, red] assert_equals: expected "" but got "40px"
FAIL Invalid fallback invalidates var()-reference [<length> | none, nolength] assert_equals: expected "" but got "40px"
FAIL Invalid fallback invalidates var()-reference [<length>, var(--novar)] assert_equals: expected "" but got "40px"
PASS Invalid fallback invalidates var()-reference [<length>, red]
PASS Invalid fallback invalidates var()-reference [<length> | none, nolength]
PASS Invalid fallback invalidates var()-reference [<length>, var(--novar)]

58 changes: 40 additions & 18 deletions Source/WebCore/css/CSSVariableReferenceValue.cpp
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@
#include "config.h"
#include "CSSVariableReferenceValue.h"

#include "CSSPropertyParser.h"
#include "CSSRegisteredCustomProperty.h"
#include "CSSVariableData.h"
#include "ConstantPropertyMap.h"
Expand Down Expand Up @@ -69,38 +70,55 @@ String CSSVariableReferenceValue::customCSSText() const
return m_stringValue;
}

bool CSSVariableReferenceValue::resolveVariableFallback(CSSParserTokenRange range, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
auto CSSVariableReferenceValue::resolveVariableFallback(const AtomString& variableName, CSSParserTokenRange range, Style::BuilderState& builderState) const -> std::pair<FallbackResult, Vector<CSSParserToken>>
{
ASSERT(range.atEnd() || range.peek().type() == CommaToken);

if (range.atEnd())
return false;
ASSERT(range.peek().type() == CommaToken);
return { FallbackResult::None, { } };

range.consumeIncludingWhitespace();
return resolveTokenRange(range, tokens, builderState);

auto tokens = resolveTokenRange(range, builderState);

auto* registered = builderState.document().customPropertyRegistry().get(variableName);
if (!registered || registered->syntax.isUniversal()) {
if (!tokens)
return { FallbackResult::None, { } };

return { FallbackResult::Valid, WTFMove(*tokens) };
}

// https://drafts.css-houdini.org/css-properties-values-api/#fallbacks-in-var-references
// The fallback value must match the syntax definition of the custom property being referenced,
// otherwise the declaration is invalid at computed-value time
if (!tokens || !CSSPropertyParser::isValidCustomPropertyValueForSyntax(registered->syntax, *tokens, m_context))
return { FallbackResult::Invalid, { } };

return { FallbackResult::Valid, WTFMove(*tokens) };
}

bool CSSVariableReferenceValue::resolveVariableReference(CSSParserTokenRange range, CSSValueID functionId, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
bool CSSVariableReferenceValue::resolveVariableReference(CSSParserTokenRange range, CSSValueID functionId, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState) const
{
ASSERT(functionId == CSSValueVar || functionId == CSSValueEnv);

auto& style = builderState.style();

range.consumeWhitespace();
ASSERT(range.peek().type() == IdentToken);
auto variableName = range.consumeIncludingWhitespace().value().toAtomString();
ASSERT(range.atEnd() || (range.peek().type() == CommaToken));

// Apply this variable first, in case it is still unresolved
builderState.builder().applyCustomProperty(variableName);

// Apply fallback to detect cycles
Vector<CSSParserToken> fallbackTokens;
bool fallbackSuccess = resolveVariableFallback(CSSParserTokenRange(range), fallbackTokens, builderState);
// Fallback has to be resolved even when not used to detect cycles and invalid syntax.
auto [fallbackResult, fallbackTokens] = resolveVariableFallback(variableName, range, builderState);
if (fallbackResult == FallbackResult::Invalid)
return false;

auto* property = [&]() -> const CSSCustomPropertyValue* {
if (functionId == CSSValueEnv)
return builderState.document().constantProperties().values().get(variableName);

auto* customProperty = style.getCustomProperty(variableName);
auto* customProperty = builderState.style().getCustomProperty(variableName);
if (customProperty)
return customProperty;

Expand All @@ -112,7 +130,7 @@ bool CSSVariableReferenceValue::resolveVariableReference(CSSParserTokenRange ran
if (fallbackTokens.size() > maxSubstitutionTokens)
return false;

if (fallbackSuccess) {
if (fallbackResult == FallbackResult::Valid) {
tokens.appendVector(fallbackTokens);
return true;
}
Expand All @@ -127,8 +145,9 @@ bool CSSVariableReferenceValue::resolveVariableReference(CSSParserTokenRange ran
return true;
}

bool CSSVariableReferenceValue::resolveTokenRange(CSSParserTokenRange range, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
std::optional<Vector<CSSParserToken>> CSSVariableReferenceValue::resolveTokenRange(CSSParserTokenRange range, Style::BuilderState& builderState) const
{
Vector<CSSParserToken> tokens;
bool success = true;
while (!range.atEnd()) {
auto functionId = range.peek().functionId();
Expand All @@ -139,16 +158,19 @@ bool CSSVariableReferenceValue::resolveTokenRange(CSSParserTokenRange range, Vec
}
tokens.append(range.consume());
}
return success;
if (!success)
return { };

return tokens;
}

RefPtr<CSSVariableData> CSSVariableReferenceValue::resolveVariableReferences(Style::BuilderState& builderState) const
{
Vector<CSSParserToken> resolvedTokens;
if (!resolveTokenRange(m_data->tokenRange(), resolvedTokens, builderState))
auto resolvedTokens = resolveTokenRange(m_data->tokenRange(), builderState);
if (!resolvedTokens)
return nullptr;

return CSSVariableData::create(resolvedTokens);
return CSSVariableData::create(*resolvedTokens);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems unfortunate we have to copy here. Can we add a CSSVariableData::create() overload that takes an r-value and move the resolved tokens in? Or does the processing done in CSSVariableData's constructor make this a meaning less optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something to do separately. I did notice it and wondered about the same things you did.

}

} // namespace WebCore
7 changes: 4 additions & 3 deletions Source/WebCore/css/CSSVariableReferenceValue.h
View file
Open in desktop
Expand Up @@ -61,9 +61,10 @@ class CSSVariableReferenceValue : public CSSValue {
private:
explicit CSSVariableReferenceValue(Ref<CSSVariableData>&&, const CSSParserContext&);

static bool resolveTokenRange(CSSParserTokenRange, Vector<CSSParserToken>&, Style::BuilderState&);
static bool resolveVariableReference(CSSParserTokenRange, CSSValueID, Vector<CSSParserToken>&, Style::BuilderState&);
static bool resolveVariableFallback(CSSParserTokenRange, Vector<CSSParserToken>&, Style::BuilderState&);
std::optional<Vector<CSSParserToken>> resolveTokenRange(CSSParserTokenRange, Style::BuilderState&) const;
bool resolveVariableReference(CSSParserTokenRange, CSSValueID, Vector<CSSParserToken>&, Style::BuilderState&) const;
enum class FallbackResult : uint8_t { None, Valid, Invalid };
std::pair<FallbackResult, Vector<CSSParserToken>> resolveVariableFallback(const AtomString& variableName, CSSParserTokenRange, Style::BuilderState&) const;

Ref<CSSVariableData> m_data;
mutable String m_stringValue;
Expand Down
9 changes: 9 additions & 0 deletions Source/WebCore/css/parser/CSSPropertyParser.cpp
View file
Open in desktop
Expand Up @@ -285,6 +285,15 @@ void CSSPropertyParser::collectParsedCustomPropertyValueDependencies(const CSSCu
parser.collectParsedCustomPropertyValueDependencies(syntax, isRoot, dependencies);
}

bool CSSPropertyParser::isValidCustomPropertyValueForSyntax(const CSSCustomPropertySyntax& syntax, CSSParserTokenRange tokens, const CSSParserContext& context)
{
if (syntax.isUniversal())
return true;

CSSPropertyParser parser { tokens, context, nullptr };
return !!parser.consumeCustomPropertyValueWithSyntax(syntax).first;
}

bool CSSPropertyParser::parseValueStart(CSSPropertyID propertyID, bool important)
{
if (consumeCSSWideKeyword(propertyID, important))
Expand Down
1 change: 1 addition & 0 deletions Source/WebCore/css/parser/CSSPropertyParser.h
View file
Open in desktop
Expand Up @@ -53,6 +53,7 @@ class CSSPropertyParser {
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&);
static bool isValidCustomPropertyValueForSyntax(const CSSCustomPropertySyntax&, CSSParserTokenRange, const CSSParserContext&);

static RefPtr<CSSValue> parseCounterStyleDescriptor(CSSPropertyID, CSSParserTokenRange&, const CSSParserContext&);

Expand Down