Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Correct serialization of Selectors 
https://bugs.webkit.org/show_bug.cgi?id=184604
rdar://97092572

Reviewed by Antti Koivisto and Tim Nguyen.

We did not use serializeIdentifier for qualified name prefixes and
local names. E.g., x:x was not serialized as x\:x. This was previously
attempted, but it would do the wrong thing for * and thus backed out.
So now we do not escape when prefix or local name is *.

The only edge case remaining is whether the * and \* inputs should mean
the same thing. w3c/csswg-drafts#8911 will
eventually address this.

Meanwhile this seems like a solid improvement.

Also synchronize web-platform-tests/css/cssom up to upstream commit
57c5006 for real this time.
264934@main missed a couple changes.

* LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssimportrule-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssimportrule.html:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/selectorSerialize-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/selectorSerialize.html:
* LayoutTests/imported/w3c/web-platform-tests/css/cssom/xml-stylesheet-pi-in-doctype.xhtml:
* Source/WebCore/css/CSSSelector.cpp:
(WebCore::serializeIdentifierOrStar):
(WebCore::CSSSelector::selectorText const):

Canonical link: https://commits.webkit.org/264980@main
  • Loading branch information
@annevk
annevk committed Jun 8, 2023
1 parent dc79ccb commit 892935d
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 17 deletions.
2 changes: 2 additions & 0 deletions LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssimportrule-expected.txt
View file
Expand Up @@ -8,4 +8,6 @@ PASS Values of CSSImportRule attributes
PASS CSSImportRule : MediaList mediaText attribute should be updated due to [PutForwards]
PASS CSSStyleDeclaration cssText attribute should be updated due to [PutForwards]
PASS StyleSheet : MediaList mediaText attribute should be updated due to [PutForwards]
FAIL Existence and writability of CSSImportRule supportsText attribute assert_idl_attribute: property "supportsText" not found in prototype chain
FAIL Value of CSSImportRule supportsText attribute assert_equals: expected (string) "(display: flex) or (display: block)" but got (undefined) undefined

18 changes: 16 additions & 2 deletions LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssimportrule.html
View file
Expand Up @@ -15,27 +15,31 @@
@import url("support/a-green.css");
@import url("support/a-green.css") screen;
@import url("support/a-green.css") all;
@import url("support/a-green") supports((display: flex) or (display: block));
@page { background-color: red; }
</style>
</head>
<body>
<div id="log"></div>

<script type="text/javascript">
var styleSheet, ruleList, rule, ruleWithMedia, ruleWithMediaAll;
var styleSheet, ruleList, rule, ruleWithMedia, ruleWithMediaAll, ruleWithSupports;
setup(function() {
styleSheet = document.getElementById("styleElement").sheet;
ruleList = styleSheet.cssRules;
rule = ruleList[0];
ruleWithMedia = ruleList[1];
ruleWithMediaAll = ruleList[2];
ruleWithSupports = ruleList[3];
});

test(function() {
assert_true(rule instanceof CSSRule);
assert_true(rule instanceof CSSImportRule);
assert_true(ruleWithMedia instanceof CSSRule);
assert_true(ruleWithMedia instanceof CSSImportRule);
assert_true(ruleWithSupports instanceof CSSRule);
assert_true(ruleWithSupports instanceof CSSImportRule);
}, "CSSRule and CSSImportRule types");

test(function() {
Expand Down Expand Up @@ -66,6 +70,7 @@
assert_equals(rule.cssText, '@import url("support/a-green.css");');
assert_equals(ruleWithMedia.cssText, '@import url("support/a-green.css") screen;');
assert_equals(ruleWithMediaAll.cssText, '@import url("support/a-green.css") all;');
assert_equals(ruleWithSupports.cssText, '@import url("support/a-green") supports((display: flex) or (display: block));');
assert_equals(rule.parentRule, null);
assert_true(rule.parentStyleSheet instanceof CSSStyleSheet);
}, "Values of CSSRule attributes");
Expand Down Expand Up @@ -94,7 +99,7 @@
}, "CSSImportRule : MediaList mediaText attribute should be updated due to [PutForwards]");

test(function() {
var ruleWithPage = ruleList[3];
var ruleWithPage = ruleList[4];
ruleWithPage.style = "margin-top: 10px;"
assert_equals(ruleWithPage.style.cssText, "margin-top: 10px;");
}, "CSSStyleDeclaration cssText attribute should be updated due to [PutForwards]");
Expand All @@ -103,6 +108,15 @@
styleSheet.media = "screen";
assert_equals(styleSheet.media.mediaText, "screen");
}, "StyleSheet : MediaList mediaText attribute should be updated due to [PutForwards]");

test(function() {
assert_idl_attribute(ruleWithSupports, "supportsText");
assert_readonly(ruleWithSupports, "supportsText");
}, "Existence and writability of CSSImportRule supportsText attribute");

test(function() {
assert_equals(ruleWithSupports.supportsText, "(display: flex) or (display: block)");
}, "Value of CSSImportRule supportsText attribute");
</script>
</body>
</html>
17 changes: 11 additions & 6 deletions LayoutTests/imported/w3c/web-platform-tests/css/cssom/selectorSerialize-expected.txt
View file
Expand Up @@ -11,10 +11,15 @@ PASS single pseudo (simple) selector "nth-last-child" which accepts arguments in
PASS single pseudo (simple) selector "nth-of-child" which accepts arguments in the sequence of simple selectors that is not a universal selector
PASS single pseudo (simple) selector ":nth-last-of-type" which accepts arguments in the sequence of simple selectors that is not a universal selector
PASS single pseudo (simple) selector ":not" which accepts arguments in the sequence of simple selectors that is not a universal selector
FAIL escaped character in attribute name assert_equals: expected "[ns\\:foo]" but got "[ns:foo]"
FAIL escaped character as code point in attribute name assert_equals: expected "[\\30 zonk]" but got "[0zonk]"
FAIL escaped character (@) in attribute name assert_equals: expected "[\\@]" but got "[@]"
FAIL escaped character in attribute name with any namespace assert_equals: expected "[*|ns\\:foo]" but got "[*|ns:foo]"
FAIL escaped character in attribute prefix assert_equals: expected "[ns\\:odd|foo]" but got "[ns:odd|foo]"
FAIL escaped character in both attribute prefix and name assert_equals: expected "[ns\\:odd|odd\\:name]" but got "[ns:odd|odd:name]"
PASS escaped character in attribute name
PASS escaped character as code point in attribute name
PASS escaped character (@) in attribute name
PASS escaped character in attribute name with any namespace
PASS escaped character in attribute prefix
PASS escaped character in both attribute prefix and name
PASS escaped character (\) in element name
PASS escaped character (\) in element name with any namespace without default
PASS escaped character (\) in element name with any namespace with default
PASS escaped character (\) in element name with no namespace
PASS escaped character (*) in element prefix

22 changes: 21 additions & 1 deletion LayoutTests/imported/w3c/web-platform-tests/css/cssom/selectorSerialize.html
View file
Expand Up @@ -97,7 +97,7 @@
assert_selector_serializes_to(' :not( :hover ) ', ':not(:hover)');
}, 'single pseudo (simple) selector ":not" which accepts arguments in the sequence of simple selectors that is not a universal selector')

var escaped_ns_rule = "@namespace ns\\:odd url(ns);";
const escaped_ns_rule = "@namespace ns\\:odd url(ns);";
test(function() {
assert_selector_serializes_to("[ns\\:foo]", "[ns\\:foo]");
}, "escaped character in attribute name");
Expand All @@ -116,6 +116,26 @@
test(function() {
assert_selector_serializes_to(escaped_ns_rule + "[ns\\:odd|odd\\:name]", "[ns\\:odd|odd\\:name]");
}, "escaped character in both attribute prefix and name");

test(() => {
assert_selector_serializes_to("\\\\", "\\\\");
}, "escaped character (\\) in element name");
test(() => {
assert_selector_serializes_to("*|\\\\", "\\\\");
}, "escaped character (\\) in element name with any namespace without default");
test(() => {
assert_selector_serializes_to("@namespace 'blah'; *|\\\\", "*|\\\\");
}, "escaped character (\\) in element name with any namespace with default");
test(() => {
assert_selector_serializes_to("|\\\\", "|\\\\");
}, "escaped character (\\) in element name with no namespace");

const element_escaped_ns_rule = "@namespace x\\* 'blah';";
test(() => {
assert_selector_serializes_to(element_escaped_ns_rule + "x\\*|test", "x\\*|test");
}, "escaped character (*) in element prefix");

// TODO: https://github.com/w3c/csswg-drafts/issues/8911
</script>
</body>
</html>
2 changes: 1 addition & 1 deletion LayoutTests/imported/w3c/web-platform-tests/css/cssom/xml-stylesheet-pi-in-doctype.xhtml
View file
Expand Up @@ -2,7 +2,7 @@
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>xml-stylesheet processing instruction in doctype internal subset</title>
<link rel="help" href="https://w3c.github.io/csswg-drafts/cssom/#prolog"/>
<link rel="help" href="https://drafts.csswg.org/cssom/#prolog"/>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
</head>
Expand Down
24 changes: 17 additions & 7 deletions Source/WebCore/css/CSSSelector.cpp
View file
Expand Up @@ -407,11 +407,19 @@ String CSSSelector::selectorText(StringView separator, StringView rightSide) con
{
StringBuilder builder;

if (match() == CSSSelector::Tag && !m_tagIsForNamespaceRule) {
if (tagQName().prefix().isNull())
builder.append(tagQName().localName());
auto serializeIdentifierOrStar = [&] (const AtomString& identifier) {
if (identifier == starAtom())
builder.append('*');
else
builder.append(tagQName().prefix().string(), '|', tagQName().localName());
serializeIdentifier(identifier, builder);
};

if (match() == CSSSelector::Tag && !m_tagIsForNamespaceRule) {
if (auto& prefix = tagQName().prefix(); !prefix.isNull()) {
serializeIdentifierOrStar(prefix);
builder.append('|');
}
serializeIdentifierOrStar(tagQName().localName());
}

const CSSSelector* cs = this;
Expand Down Expand Up @@ -780,9 +788,11 @@ String CSSSelector::selectorText(StringView separator, StringView rightSide) con
}
} else if (cs->isAttributeSelector()) {
builder.append('[');
if (auto& prefix = cs->attribute().prefix(); !prefix.isEmpty())
builder.append(prefix, '|');
builder.append(cs->attribute().localName());
if (auto& prefix = cs->attribute().prefix(); !prefix.isEmpty()) {
serializeIdentifierOrStar(prefix);
builder.append('|');
}
serializeIdentifierOrStar(cs->attribute().localName());
switch (cs->match()) {
case CSSSelector::Exact:
builder.append('=');
Expand Down

0 comments on commit 892935d

Please sign in to comment.