Skip to content

Commit

Permalink
Use base::ValuesEquivalent for custom property style queries
Browse files Browse the repository at this point in the history
We previously compared the tokens from the tokenizated string instead.

Add tests for matching and serializing original string in style()
queries.

Bug: 1302630
Change-Id: I2768b930407f65c979b320597276e0d5d01a1b93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4342894
Reviewed-by: Steinar H Gunderson <sesse@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1118626}
  • Loading branch information
Rune Lillesveen authored and Chromium LUCI CQ committed Mar 17, 2023
1 parent fbbd3ab commit 1233312
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 56 deletions.
55 changes: 1 addition & 54 deletions third_party/blink/renderer/core/css/media_query_evaluator.cc
Expand Up @@ -1532,56 +1532,6 @@ KleeneValue MediaQueryEvaluator::EvalFeature(
return result ? KleeneValue::kTrue : KleeneValue::kFalse;
}

namespace {

void ConsumeWhitespace(base::span<CSSParserToken>::const_iterator& iterator,
const base::span<CSSParserToken>::const_iterator& end) {
while (iterator != end && (*iterator).GetType() == kWhitespaceToken) {
iterator++;
}
}

void ConsumeWhitespaceReverse(
base::span<CSSParserToken>::const_iterator& iterator,
const base::span<CSSParserToken>::const_iterator& start) {
while (iterator != start && (*(iterator - 1)).GetType() == kWhitespaceToken) {
iterator--;
}
}

bool TokensEqualIgnoringLeadingAndTrailingSpaces(
const CSSVariableData* value1,
const CSSVariableData* value2) {
if (value1 == value2) {
return true;
}
if (!value1 || !value2) {
return false;
}

CSSTokenizer tokenizer1(value1->OriginalText());
CSSTokenizer tokenizer2(value2->OriginalText());
auto tokens1vec = tokenizer1.TokenizeToEOF();
auto tokens2vec = tokenizer2.TokenizeToEOF();

const base::span<CSSParserToken> tokens1 = tokens1vec;
const base::span<CSSParserToken> tokens2 = tokens2vec;

base::span<CSSParserToken>::const_iterator tokens1_start = tokens1.begin();
base::span<CSSParserToken>::const_iterator tokens1_end = tokens1.end();
base::span<CSSParserToken>::const_iterator tokens2_start = tokens2.begin();
base::span<CSSParserToken>::const_iterator tokens2_end = tokens2.end();

ConsumeWhitespace(tokens1_start, tokens1_end);
ConsumeWhitespaceReverse(tokens1_end, tokens1_start);
ConsumeWhitespace(tokens2_start, tokens2_end);
ConsumeWhitespaceReverse(tokens2_end, tokens2_start);

return std::equal(tokens1_start, tokens1_end, tokens2_start, tokens2_end);
}

} // namespace

KleeneValue MediaQueryEvaluator::EvalStyleFeature(
const MediaQueryFeatureExpNode& feature,
MediaQueryResultFlags* result_flags) const {
Expand Down Expand Up @@ -1620,10 +1570,7 @@ KleeneValue MediaQueryEvaluator::EvalStyleFeature(
CSSVariableData* computed =
container->ComputedStyleRef().GetVariableData(property_name);

// TODO(crbug.com/1220144): Compare the two CSSVariableData using
// base::ValuesEquivalent when we correctly strip leading and trailing
// whitespaces for custom property values.
if (TokensEqualIgnoringLeadingAndTrailingSpaces(computed, query_computed)) {
if (base::ValuesEquivalent(computed, query_computed)) {
return KleeneValue::kTrue;
}
return KleeneValue::kFalse;
Expand Down
Expand Up @@ -12,11 +12,13 @@
@container STyle(--foo) { }
@container style( ( --FOO: BAR) OR ( prop: val ) ) { }
@container style (--foo: bar) { }
@container style(--foo: bar baz) { }
@container style(--foo:2.100 ) { }
</style>
<script>
setup(() => {
assert_implements_container_queries();
assert_equals(testSheet.sheet.cssRules.length, 6);
assert_equals(testSheet.sheet.cssRules.length, 8);
});

const tests = [
Expand All @@ -25,7 +27,9 @@
["style(--foo: )", "Empty declaration value"],
["STyle(--foo)", "Missing declaration value"],
["style((--FOO: BAR) or ( prop: val ))", "Unknown CSS property after 'or'"],
["style (--foo: bar)", "Not a style function with space before '('"]
["style (--foo: bar)", "Not a style function with space before '('"],
["style(--foo: bar baz)", "Spaces preserved in custom property value"],
["style(--foo: 2.100)", "Original string number in custom property value"]
].map((e, i) => [testSheet.sheet.cssRules[i], ...e]);

tests.forEach((t) => {
Expand Down
Expand Up @@ -365,3 +365,48 @@
assert_equals(getComputedStyle(document.querySelector("#reg-initial-keyword")).color, green);
}, "Match registered <length> custom property with initial value via initial keyword.");
</script>

<style>
#original-text {
--number: 100.00;
--spaces: a b;
}
@container style(--number: 100.00) {
#original-text-number {
color: green;
}
}
@container style(--number: 100.0) {
#original-text-number {
color: red;
}
}
@container style(--number: 100) {
#original-text-number {
color: red;
}
}
@container style(--spaces: a b) {
#original-text-spaces {
color: green;
}
}
@container style(--number: a b) {
#original-text-spaces {
color: red;
}
}
</style>
<div id="original-text">
<div id="original-text-number"></div>
<div id="original-text-spaces"></div>
</div>
<script>
test(() => {
assert_equals(getComputedStyle(document.querySelector("#original-text-number")).color, green);
}, "Should only match exact string for numbers in non-registered custom properties");

test(() => {
assert_equals(getComputedStyle(document.querySelector("#original-text-spaces")).color, green);
}, "Spaces should not collapse in non-registered custom properties");
</script>

0 comments on commit 1233312

Please sign in to comment.