Skip to content

GitLab

T
tangled
Commit 42eb4f28 authored by Emilio Cobos Álvarez's avatar Emilio Cobos Álvarez Committed by Commit Bot
Browse files
Options

Don't skip invalid functions when looking for var references. 

This matches the spec, WebKit and Firefox.

From https://drafts.csswg.org/css-variables/#using-variables:

> If a property contains one or more var() functions, and those functions are
> syntactically valid, the entire property’s grammar must be assumed to be
> valid at parse time. It is only syntax-checked at computed-value time, after
> var() functions have been substituted.

Bug: 921152
Change-Id: Ie46268ffae4e01f457f379c674e0cf1a7ccea354
Reviewed-on: https://chromium-review.googlesource.com/c/1446653
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Reviewed-by: default avatarEric Willigers <ericwilligers@chromium.org>
Cr-Commit-Position: refs/heads/master@{#628023}
parent e20e1aa5
Hide whitespace changes
Inline Side-by-side
Showing with 14 additions and 118 deletions
third_party/blink/renderer/core/css/parser/css_variable_parser.cc
View file @ 42eb4f28
......@@ -13,12 +13,10 @@ namespace blink {
namespace {
bool IsValidVariableReference(CSSParserTokenRange, bool);
bool IsValidEnvVariableReference(CSSParserTokenRange, bool);
bool IsValidVariableReference(CSSParserTokenRange);
bool IsValidEnvVariableReference(CSSParserTokenRange);
bool ClassifyBlock(CSSParserTokenRange range,
bool& has_references,
bool skip_variables) {
bool ClassifyBlock(CSSParserTokenRange range, bool& has_references) {
size_t block_stack_size = 0;
while (!range.AtEnd()) {
......@@ -31,13 +29,12 @@ bool ClassifyBlock(CSSParserTokenRange range,
// and used as fallbacks.
switch (token.FunctionId()) {
case CSSValueVar:
if (!IsValidVariableReference(range.ConsumeBlock(), skip_variables))
if (!IsValidVariableReference(range.ConsumeBlock()))
return false; // Invalid reference.
has_references = true;
continue;
case CSSValueEnv:
if (!IsValidEnvVariableReference(range.ConsumeBlock(),
skip_variables))
if (!IsValidEnvVariableReference(range.ConsumeBlock()))
return false; // Invalid reference.
has_references = true;
continue;
......@@ -48,12 +45,6 @@ bool ClassifyBlock(CSSParserTokenRange range,
const CSSParserToken& token = range.Consume();
if (token.GetBlockType() == CSSParserToken::kBlockStart) {
// If we are an invalid function then we should skip over any variables
// this function contains.
if (token.GetType() == CSSParserTokenType::kFunctionToken &&
token.FunctionId() == CSSValueInvalid) {
skip_variables = true;
}
++block_stack_size;
} else if (token.GetBlockType() == CSSParserToken::kBlockEnd) {
--block_stack_size;
......@@ -82,10 +73,8 @@ bool ClassifyBlock(CSSParserTokenRange range,
return true;
}
bool IsValidVariableReference(CSSParserTokenRange range, bool skip_variables) {
bool IsValidVariableReference(CSSParserTokenRange range) {
range.ConsumeWhitespace();
if (skip_variables)
return false;
if (!CSSVariableParser::IsValidVariableName(
range.ConsumeIncludingWhitespace()))
return false;
......@@ -98,14 +87,11 @@ bool IsValidVariableReference(CSSParserTokenRange range, bool skip_variables) {
return false;
bool has_references = false;
return ClassifyBlock(range, has_references, skip_variables);
return ClassifyBlock(range, has_references);
}
bool IsValidEnvVariableReference(CSSParserTokenRange range,
bool skip_variables) {
bool IsValidEnvVariableReference(CSSParserTokenRange range) {
range.ConsumeWhitespace();
if (skip_variables)
return false;
if (range.ConsumeIncludingWhitespace().GetType() !=
CSSParserTokenType::kIdentToken)
return false;
......@@ -118,7 +104,7 @@ bool IsValidEnvVariableReference(CSSParserTokenRange range,
return false;
bool has_references = false;
return ClassifyBlock(range, has_references, skip_variables);
return ClassifyBlock(range, has_references);
}
CSSValueID ClassifyVariableRange(CSSParserTokenRange range,
......@@ -133,7 +119,7 @@ CSSValueID ClassifyVariableRange(CSSParserTokenRange range,
return id;
}
if (ClassifyBlock(range, has_references, false /* skip_variables */))
if (ClassifyBlock(range, has_references))
return CSSValueInternalVariableValue;
return CSSValueInvalid;
}
......
third_party/blink/web_tests/css-parser/variables-invalid-functions.html deleted 100644 → 0
View file @ e20e1aa5
<!DOCTYPE html>
<html>
<head>
<link rel="help" href="https://drafts.csswg.org/css-env-1/">
<title>Test env() and var() throw syntax errors if used with invalid functions</title>
<script src="../resources/testharness.js"></script>
<script src="../resources/testharnessreport.js"></script>
<style>
div {
--a: 0px;
margin-left: 10px;
}
</style>
</head>
<body>
<script>
// This value is expected if the syntax is valid.
const workingValue = "0px";
// This value is expected if the syntax is invalid.
const pageDefaultValue = "10px";
// This value is expected if the calc() syntax is valid.
const workingCalcValue = "20px";
const testCases = [
{ style: "", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: env(safe-area-inset-left)", expectedPropertyValue: workingValue },
// min and max() are not supported.
{ style: "margin-left: min(env(safe-area-inset-left))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(safe-area-inset-left), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(safe-area-inset-left, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(safe-area-inset-left, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(test))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(test), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(test, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(env(test, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(safe-area-inset-left))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(safe-area-inset-left), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(safe-area-inset-left, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(safe-area-inset-left, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(test))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(test), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(test, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(env(test, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
// calc() should work.
{ style: "margin-left: calc(env(safe-area-inset-left) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(env(safe-area-inset-left, 0px) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(env(safe-area-inset-left, 0) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(env(test) + 20px)", expectedPropertyValue: "0px" },
{ style: "margin-left: calc(env(test, 1px) + 20px)", expectedPropertyValue: "21px" },
// min and max() are not supported.
{ style: "margin-left: min(var(--a))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--a), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--a, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--a, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--b))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--b), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--b, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: min(var(--b, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--a))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--a), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--a, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--a, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--b))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--b), 1px)", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--b, 1px))", expectedPropertyValue: pageDefaultValue },
{ style: "margin-left: max(var(--b, 1px), 1px)", expectedPropertyValue: pageDefaultValue },
// calc() should work.
{ style: "margin-left: calc(var(--a) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(var(--a, 0px) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(var(--a, 0) + 20px)", expectedPropertyValue: workingCalcValue },
{ style: "margin-left: calc(var(--b) + 20px)", expectedPropertyValue: "0px" },
{ style: "margin-left: calc(var(--b, 1px) + 20px)", expectedPropertyValue: "21px" },
];
testCases.forEach((testcase) => {
test(() => {
const elem = document.createElement("div");
const style = window.getComputedStyle(elem);
document.body.appendChild(elem);
elem.style.cssText = testcase.style;
assert_equals(style.getPropertyValue("margin-left"), testcase.expectedPropertyValue);
}, testcase.style + " " + testcase.expectedPropertyValue);
});
</script>
</body>
</html>
third_party/blink/web_tests/external/wpt/css/css-env/supports-script.tentative.html
View file @ 42eb4f28
......@@ -11,6 +11,8 @@
test(() => {
assert_true(CSS.supports("background: env(test)"));
assert_true(CSS.supports("background", "env(test)"));
assert_true(CSS.supports("background", "env(test, 10px)"));
assert_true(CSS.supports("background", "foobar(env(test))"));
assert_false(CSS.supports("background", "env()"));
assert_false(CSS.supports("background", "env(test,)"));
});
......
third_party/blink/web_tests/external/wpt/css/cssom/CSS.html
View file @ 42eb4f28
......@@ -26,6 +26,8 @@
assert_equals(CSS.supports("color: red"), true, "CSS.supports: Single-argument form allows for declarations without enclosing parentheses");
assert_equals(CSS.supports("(color: red) and (color: blue)"), true, "CSS.supports: Complex conditions allowed");
assert_equals(CSS.supports("not (foobar)"), true, "CSS.supports: general_enclosed still parses");
assert_equals(CSS.supports("color: something-pointless var(--foo)"), true, "Variable references always parse");
assert_equals(CSS.supports("color: something-pointless(var(--foo))"), true, "Variable references in an unknown function always parse");
}, "CSS.supports, one argument form");
test(function () {
// https://drafts.csswg.org/css-conditional/#dom-css-supports
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment