Compile the :root pseudo class and fix a related issue with :nth-child()
authorbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Apr 2014 06:26:20 +0000 (06:26 +0000)
committerbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Apr 2014 06:26:20 +0000 (06:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=131926

Reviewed by Andreas Kling.

Source/WebCore:
Add the :root pseudo class. This is another trivial selector, we just need to compare
the element pointer with the documentElement.

I discovered some issues with :nth-child(n) through the layout tests for ":root".
When the pseudo class nth-child could match anything, no code was generated. That decision
was taken when generating the fragments.

The specification of :nth-child() has two tests: the parent test and the counter test.
Since some fragments would not generate any code for :nth-child(n), they would succeed on the root,
which is incorrect since the root should fail the parent test.

This was fixed by moving the filtering of non-counting :nth-child() after we generate the parent
check.
We still don't generate any counter test unless required.

Test: fast/selectors/nth-child-on-root.html

* cssjit/SelectorCompiler.cpp:
(WebCore::SelectorCompiler::addPseudoClassType):
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementMatching):
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthChild):
(WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsRoot):
* dom/Document.h:
(WebCore::Document::documentElementMemoryOffset):

LayoutTests:
Add more test coverage that would have caught the bug with :nth-child(n).

* fast/selectors/nth-child-on-root-expected.txt: Added.
* fast/selectors/nth-child-on-root.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@167585 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/selectors/nth-child-on-root-expected.txt [new file with mode: 0644]
LayoutTests/fast/selectors/nth-child-on-root.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/cssjit/SelectorCompiler.cpp
Source/WebCore/dom/Document.h

index 9365093..a17dcc0 100644 (file)
@@ -1,5 +1,17 @@
 2014-04-20  Benjamin Poulain  <benjamin@webkit.org>
 
+        Compile the :root pseudo class and fix a related issue with :nth-child()
+        https://bugs.webkit.org/show_bug.cgi?id=131926
+
+        Reviewed by Andreas Kling.
+
+        Add more test coverage that would have caught the bug with :nth-child(n).
+
+        * fast/selectors/nth-child-on-root-expected.txt: Added.
+        * fast/selectors/nth-child-on-root.html: Added.
+
+2014-04-20  Benjamin Poulain  <benjamin@webkit.org>
+
         Add Element.matches, the standard name for webkitMatchesSelector
         https://bugs.webkit.org/show_bug.cgi?id=131922
 
diff --git a/LayoutTests/fast/selectors/nth-child-on-root-expected.txt b/LayoutTests/fast/selectors/nth-child-on-root-expected.txt
new file mode 100644 (file)
index 0000000..91ff81e
--- /dev/null
@@ -0,0 +1,20 @@
+Verify the nth-child() pseudo class matcher always test for the parent element. Some :nth-child pseudo selectors can skip counting the siblings, but they should never skip the parent check.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.querySelectorAll("html:nth-child(1)").length is 0
+PASS document.querySelectorAll("html:nth-child(n)").length is 0
+PASS document.querySelectorAll("html:nth-child(n+1)").length is 0
+PASS document.querySelectorAll(":root:nth-child(1)").length is 0
+PASS document.querySelectorAll(":root:nth-child(n)").length is 0
+PASS document.querySelectorAll(":root:nth-child(n+1)").length is 0
+PASS getComputedStyle(document.documentElement).backgroundColor is "rgb(255, 255, 255)"
+PASS document.querySelectorAll("svg:root").length is 0
+PASS document.querySelectorAll("svg:nth-child(1)").length is 1
+PASS document.querySelectorAll("svg:nth-child(n)").length is 1
+PASS getComputedStyle(document.querySelector("svg")).backgroundColor is "rgb(1, 2, 3)"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/selectors/nth-child-on-root.html b/LayoutTests/fast/selectors/nth-child-on-root.html
new file mode 100644 (file)
index 0000000..7c9d698
--- /dev/null
@@ -0,0 +1,60 @@
+<!doctype html>
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<style>
+html {
+    background-color:white;
+}
+html:nth-child(1) {
+    background-color:rgb(1, 2, 3);
+}
+html:nth-child(n) {
+    background-color:rgb(1, 2, 3);
+}
+html:nth-child(n+1) {
+    background-color:rgb(1, 2, 3);
+}
+
+:root:nth-child(1) {
+    background-color:rgb(1, 2, 3);
+}
+:root:nth-child(n) {
+    background-color:rgb(1, 2, 3);
+}
+:root:nth-child(n+1) {
+    background-color:rgb(1, 2, 3);
+}
+
+svg:nth-child(n) {
+    background-color:rgb(1, 2, 3);
+}
+svg:root {
+    background-color:rgb(4, 5, 6);
+}
+</style>
+</head>
+<body>
+    <div style="display:none">
+        <svg><g></g></svg>
+    </div>
+</body>
+<script>
+description('Verify the nth-child() pseudo class matcher always test for the parent element. Some :nth-child pseudo selectors can skip counting the siblings, but they should never skip the parent check.');
+
+shouldBe('document.querySelectorAll("html:nth-child(1)").length', '0');
+shouldBe('document.querySelectorAll("html:nth-child(n)").length', '0');
+shouldBe('document.querySelectorAll("html:nth-child(n+1)").length', '0');
+shouldBe('document.querySelectorAll(":root:nth-child(1)").length', '0');
+shouldBe('document.querySelectorAll(":root:nth-child(n)").length', '0');
+shouldBe('document.querySelectorAll(":root:nth-child(n+1)").length', '0');
+shouldBeEqualToString('getComputedStyle(document.documentElement).backgroundColor', 'rgb(255, 255, 255)');
+
+// This svg document is not the root, ":root" should not match anything, nth-child should work.
+shouldBe('document.querySelectorAll("svg:root").length', '0');
+shouldBe('document.querySelectorAll("svg:nth-child(1)").length', '1');
+shouldBe('document.querySelectorAll("svg:nth-child(n)").length', '1');
+shouldBeEqualToString('getComputedStyle(document.querySelector("svg")).backgroundColor', 'rgb(1, 2, 3)');
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</html>
index 37a7592..db99926 100644 (file)
@@ -1,5 +1,37 @@
 2014-04-20  Benjamin Poulain  <benjamin@webkit.org>
 
+        Compile the :root pseudo class and fix a related issue with :nth-child()
+        https://bugs.webkit.org/show_bug.cgi?id=131926
+
+        Reviewed by Andreas Kling.
+
+        Add the :root pseudo class. This is another trivial selector, we just need to compare
+        the element pointer with the documentElement.
+
+        I discovered some issues with :nth-child(n) through the layout tests for ":root".
+        When the pseudo class nth-child could match anything, no code was generated. That decision
+        was taken when generating the fragments.
+
+        The specification of :nth-child() has two tests: the parent test and the counter test.
+        Since some fragments would not generate any code for :nth-child(n), they would succeed on the root,
+        which is incorrect since the root should fail the parent test.
+
+        This was fixed by moving the filtering of non-counting :nth-child() after we generate the parent
+        check.
+        We still don't generate any counter test unless required.
+
+        Test: fast/selectors/nth-child-on-root.html
+
+        * cssjit/SelectorCompiler.cpp:
+        (WebCore::SelectorCompiler::addPseudoClassType):
+        (WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementMatching):
+        (WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsNthChild):
+        (WebCore::SelectorCompiler::SelectorCodeGenerator::generateElementIsRoot):
+        * dom/Document.h:
+        (WebCore::Document::documentElementMemoryOffset):
+
+2014-04-20  Benjamin Poulain  <benjamin@webkit.org>
+
         Add Element.matches, the standard name for webkitMatchesSelector
         https://bugs.webkit.org/show_bug.cgi?id=131922
 
index c21e606..fe8d88d 100644 (file)
@@ -190,6 +190,7 @@ private:
     void generateElementHasClasses(Assembler::JumpList& failureCases, const LocalRegister& elementDataAddress, const Vector<const AtomicStringImpl*>& classNames);
     void generateElementIsLink(Assembler::JumpList& failureCases);
     void generateElementIsNthChild(Assembler::JumpList& failureCases, const SelectorFragment&);
+    void generateElementIsRoot(Assembler::JumpList& failureCases);
     void generateElementIsTarget(Assembler::JumpList& failureCases);
 
     // Helpers.
@@ -319,6 +320,7 @@ static inline FunctionType addPseudoClassType(const CSSSelector& selector, Selec
         return FunctionType::SimpleSelectorChecker;
 
     case CSSSelector::PseudoClassLink:
+    case CSSSelector::PseudoClassRoot:
     case CSSSelector::PseudoClassTarget:
         fragment.pseudoClasses.add(type);
         return FunctionType::SimpleSelectorChecker;
@@ -343,10 +345,6 @@ static inline FunctionType addPseudoClassType(const CSSSelector& selector, Selec
             if (a <= 0 && b < 1)
                 return FunctionType::CannotMatchAnything;
 
-            // Anything modulo 1 is zero. Unless b restrict the range, this does not filter anything out.
-            if (a == 1 && (!b || (b == 1)))
-                return FunctionType::SimpleSelectorChecker;
-
             fragment.nthChildfilters.append(std::pair<int, int>(a, b));
             if (selectorContext == SelectorContext::QuerySelector)
                 return FunctionType::SimpleSelectorChecker;
@@ -1168,6 +1166,9 @@ void SelectorCodeGenerator::generateElementMatching(Assembler::JumpList& failure
     if (fragment.pseudoClasses.contains(CSSSelector::PseudoClassLink))
         generateElementIsLink(failureCases);
 
+    if (fragment.pseudoClasses.contains(CSSSelector::PseudoClassRoot))
+        generateElementIsRoot(failureCases);
+
     if (fragment.pseudoClasses.contains(CSSSelector::PseudoClassTarget))
         generateElementIsTarget(failureCases);
 
@@ -1942,6 +1943,20 @@ void SelectorCodeGenerator::generateElementIsNthChild(Assembler::JumpList& failu
     Assembler::RegisterID parentElement = m_registerAllocator.allocateRegister();
     generateWalkToParentElement(failureCases, parentElement);
 
+    Vector<std::pair<int, int>> validSubsetFilters;
+    validSubsetFilters.reserveInitialCapacity(fragment.nthChildfilters.size());
+    for (const auto& slot : fragment.nthChildfilters) {
+        int a = slot.first;
+        int b = slot.second;
+
+        // Anything modulo 1 is zero. Unless b restricts the range, this does not filter anything out.
+        if (a == 1 && (!b || (b == 1)))
+            continue;
+        validSubsetFilters.uncheckedAppend(slot);
+    }
+    if (validSubsetFilters.isEmpty())
+        return;
+
     // Setup the counter at 1.
     LocalRegister elementCounter(m_registerAllocator);
     m_assembler.move(Assembler::TrustedImm32(1), elementCounter);
@@ -2018,7 +2033,7 @@ void SelectorCodeGenerator::generateElementIsNthChild(Assembler::JumpList& failu
     }
 
     // Test every the nth-child filter.
-    for (const auto& slot : fragment.nthChildfilters) {
+    for (const auto& slot : validSubsetFilters) {
         int a = slot.first;
         int b = slot.second;
 
@@ -2043,6 +2058,13 @@ void SelectorCodeGenerator::generateElementIsNthChild(Assembler::JumpList& failu
     }
 }
 
+void SelectorCodeGenerator::generateElementIsRoot(Assembler::JumpList& failureCases)
+{
+    LocalRegister document(m_registerAllocator);
+    getDocument(m_assembler, elementAddressRegister, document);
+    failureCases.append(m_assembler.branchPtr(Assembler::NotEqual, Assembler::Address(document, Document::documentElementMemoryOffset()), elementAddressRegister));
+}
+
 void SelectorCodeGenerator::generateElementIsTarget(Assembler::JumpList& failureCases)
 {
     LocalRegister document(m_registerAllocator);
index 8719df2..22f035b 100644 (file)
@@ -406,6 +406,7 @@ public:
     {
         return m_documentElement.get();
     }
+    static ptrdiff_t documentElementMemoryOffset() { return OBJECT_OFFSETOF(Document, m_documentElement); }
 
     Element* activeElement();
     bool hasFocus() const;