TreeWalker.previousSibling() / nextSibling() does not behave according to the specif...

https://bugs.webkit.org/show_bug.cgi?id=149493

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Rebaseline existing W3C DOM test now that more checks are passing.

* web-platform-tests/dom/traversal/TreeWalker-expected.txt:

Source/WebCore:

TreeWalker.previousSibling()  / nextSibling() does not behave according
to the specification:
- https://dom.spec.whatwg.org/#dom-treewalker-nextsibling
- https://dom.spec.whatwg.org/#dom-treewalker-previoussibling
- https://dom.spec.whatwg.org/#concept-traverse-siblings

In particular, the previous code would fail to update 'node' variable
in the case acceptNode() returned FILTER_REJECT. This patch fixes this
and refactors the function to match more closely the text of the spec
and avoid code duplication.

No new tests, already covered by existing test.

* dom/TreeWalker.cpp:
(WebCore::TreeWalker::traverseSiblings):
(WebCore::TreeWalker::previousSibling):
(WebCore::TreeWalker::nextSibling):
(WebCore::TreeWalker::previousNode): Deleted.
* dom/TreeWalker.h:

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

diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
index b336896514d01f5cc9fd703258eac8400bf83bf9..59413b2136da5285326e88c793cd0607e25d86b9 100644 (file)
--- a/LayoutTests/imported/w3c/ChangeLog
+++ b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,14 @@
+2015-09-23  Chris Dumez  <cdumez@apple.com>
+
+        TreeWalker.previousSibling()  / nextSibling() does not behave according to the specification
+        https://bugs.webkit.org/show_bug.cgi?id=149493
+
+        Reviewed by Darin Adler.
+
+        Rebaseline existing W3C DOM test now that more checks are passing.
+
+        * web-platform-tests/dom/traversal/TreeWalker-expected.txt:
+
 2015-09-23  Chris Dumez  <cdumez@apple.com>
 
         Range's setStartBefore() / setStartAfter() / setEndBefore() / setEndAfter() do not match the specification

diff --git a/LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-expected.txt
index 655784db5bb28b8110723f9c543da6f9beec9078..f26aae84e314add726f2d46fcc401ec241dd609c 100644 (file)
--- a/LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-expected.txt
+++ b/LayoutTests/imported/w3c/web-platform-tests/dom/traversal/TreeWalker-expected.txt
@@ -146,7 +146,7 @@ PASS document.createTreeWalker(document, 0, (function(node) { return node.nodeNa
 PASS document.createTreeWalker(document, 0xFFFFFFFF, null) 
 PASS document.createTreeWalker(document, 0xFFFFFFFF, (function(node) { return true })) 
 PASS document.createTreeWalker(document, 0xFFFFFFFF, (function(node) { return false })) 
-FAIL document.createTreeWalker(document, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) assert_equals: .nextSibling() expected Text node "TreeWalker tests" but got null
+PASS document.createTreeWalker(document, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) 
 PASS document.createTreeWalker(document, NodeFilter.SHOW_ELEMENT, null) 
 PASS document.createTreeWalker(document, NodeFilter.SHOW_ELEMENT, (function(node) { return true })) 
 PASS document.createTreeWalker(document, NodeFilter.SHOW_ELEMENT, (function(node) { return false })) 
@@ -166,7 +166,7 @@ PASS document.createTreeWalker(detachedDiv, 0, (function(node) { return node.nod
 PASS document.createTreeWalker(detachedDiv, 0xFFFFFFFF, null) 
 PASS document.createTreeWalker(detachedDiv, 0xFFFFFFFF, (function(node) { return true })) 
 PASS document.createTreeWalker(detachedDiv, 0xFFFFFFFF, (function(node) { return false })) 
-FAIL document.createTreeWalker(detachedDiv, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) assert_equals: .nextSibling() expected Text node "Wxyzabcd" but got null
+PASS document.createTreeWalker(detachedDiv, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) 
 PASS document.createTreeWalker(detachedDiv, NodeFilter.SHOW_ELEMENT, null) 
 PASS document.createTreeWalker(detachedDiv, NodeFilter.SHOW_ELEMENT, (function(node) { return true })) 
 PASS document.createTreeWalker(detachedDiv, NodeFilter.SHOW_ELEMENT, (function(node) { return false })) 
@@ -186,7 +186,7 @@ PASS document.createTreeWalker(foreignDoc, 0, (function(node) { return node.node
 PASS document.createTreeWalker(foreignDoc, 0xFFFFFFFF, null) 
 PASS document.createTreeWalker(foreignDoc, 0xFFFFFFFF, (function(node) { return true })) 
 PASS document.createTreeWalker(foreignDoc, 0xFFFFFFFF, (function(node) { return false })) 
-FAIL document.createTreeWalker(foreignDoc, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) assert_equals: .nextSibling() expected Text node "Efghijkl" but got Comment node <!--"Commenter" and "commentator" mean different things.  I'v...-->
+PASS document.createTreeWalker(foreignDoc, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) 
 PASS document.createTreeWalker(foreignDoc, NodeFilter.SHOW_ELEMENT, null) 
 PASS document.createTreeWalker(foreignDoc, NodeFilter.SHOW_ELEMENT, (function(node) { return true })) 
 PASS document.createTreeWalker(foreignDoc, NodeFilter.SHOW_ELEMENT, (function(node) { return false })) 
@@ -226,7 +226,7 @@ PASS document.createTreeWalker(xmlDoc, 0, (function(node) { return node.nodeName
 PASS document.createTreeWalker(xmlDoc, 0xFFFFFFFF, null) 
 PASS document.createTreeWalker(xmlDoc, 0xFFFFFFFF, (function(node) { return true })) 
 PASS document.createTreeWalker(xmlDoc, 0xFFFFFFFF, (function(node) { return false })) 
-FAIL document.createTreeWalker(xmlDoc, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) assert_equals: .nextSibling() expected Text node "do re mi fa so la ti" but got Comment node <!--I maliciously created a comment that will break incautiou...-->
+PASS document.createTreeWalker(xmlDoc, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) 
 PASS document.createTreeWalker(xmlDoc, NodeFilter.SHOW_ELEMENT, null) 
 PASS document.createTreeWalker(xmlDoc, NodeFilter.SHOW_ELEMENT, (function(node) { return true })) 
 PASS document.createTreeWalker(xmlDoc, NodeFilter.SHOW_ELEMENT, (function(node) { return false })) 
@@ -506,8 +506,7 @@ PASS document.createTreeWalker(testDiv, 0, (function(node) { return node.nodeNam
 PASS document.createTreeWalker(testDiv, 0xFFFFFFFF, null) 
 PASS document.createTreeWalker(testDiv, 0xFFFFFFFF, (function(node) { return true })) 
 PASS document.createTreeWalker(testDiv, 0xFFFFFFFF, (function(node) { return false })) 
-FAIL document.createTreeWalker(testDiv, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) assert_equals: .nextSibling() expected Text node "Ijklmnop
-" but got Comment node <!--Alphabet soup?-->
+PASS document.createTreeWalker(testDiv, 0xFFFFFFFF, (function(node) { return node.nodeName[0] == '#' })) 
 PASS document.createTreeWalker(testDiv, NodeFilter.SHOW_ELEMENT, null) 
 PASS document.createTreeWalker(testDiv, NodeFilter.SHOW_ELEMENT, (function(node) { return true })) 
 PASS document.createTreeWalker(testDiv, NodeFilter.SHOW_ELEMENT, (function(node) { return false })) 

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index ea416b5c441492a639ca8f1c9b7cadbcb9e5c961..6f5352ebf7e7d073f660e35822df153e1f99fc56 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,30 @@
+2015-09-23  Chris Dumez  <cdumez@apple.com>
+
+        TreeWalker.previousSibling()  / nextSibling() does not behave according to the specification
+        https://bugs.webkit.org/show_bug.cgi?id=149493
+
+        Reviewed by Darin Adler.
+
+        TreeWalker.previousSibling()  / nextSibling() does not behave according
+        to the specification:
+        - https://dom.spec.whatwg.org/#dom-treewalker-nextsibling
+        - https://dom.spec.whatwg.org/#dom-treewalker-previoussibling
+        - https://dom.spec.whatwg.org/#concept-traverse-siblings
+
+        In particular, the previous code would fail to update 'node' variable
+        in the case acceptNode() returned FILTER_REJECT. This patch fixes this
+        and refactors the function to match more closely the text of the spec
+        and avoid code duplication.
+
+        No new tests, already covered by existing test.
+
+        * dom/TreeWalker.cpp:
+        (WebCore::TreeWalker::traverseSiblings):
+        (WebCore::TreeWalker::previousSibling):
+        (WebCore::TreeWalker::nextSibling):
+        (WebCore::TreeWalker::previousNode): Deleted.
+        * dom/TreeWalker.h:
+
 2015-09-23  Alex Christensen  <achristensen@webkit.org>
 
         Fix CMake build.

diff --git a/Source/WebCore/dom/TreeWalker.cpp b/Source/WebCore/dom/TreeWalker.cpp
index 877482aff47a3c5434af7532e6dc053d1513cd1d..ce7f1c189944bae6a982f7b4d2dc4613b6a1ae22 100644 (file)
--- a/Source/WebCore/dom/TreeWalker.cpp
+++ b/Source/WebCore/dom/TreeWalker.cpp
@@ -130,29 +130,24 @@ Node* TreeWalker::lastChild()
     return nullptr;
 }
 
-Node* TreeWalker::previousSibling()
+template<TreeWalker::SiblingTraversalType type> Node* TreeWalker::traverseSiblings()
 {
     RefPtr<Node> node = m_current;
     if (node == root())
         return nullptr;
-    while (1) {
-        for (RefPtr<Node> sibling = node->previousSibling(); sibling; ) {
+
+    auto isNext = type == SiblingTraversalType::Next;
+    while (true) {
+        for (RefPtr<Node> sibling = isNext ? node->nextSibling() : node->previousSibling(); sibling; ) {
             short acceptNodeResult = acceptNode(sibling.get());
-            switch (acceptNodeResult) {
-                case NodeFilter::FILTER_ACCEPT:
-                    m_current = sibling.release();
-                    return m_current.get();
-                case NodeFilter::FILTER_SKIP:
-                    if (sibling->lastChild()) {
-                        sibling = sibling->lastChild();
-                        node = sibling;
-                        continue;
-                    }
-                    break;
-                case NodeFilter::FILTER_REJECT:
-                    break;
+            if (acceptNodeResult == NodeFilter::FILTER_ACCEPT) {
+                m_current = WTF::move(sibling);
+                return m_current.get();
             }
-            sibling = sibling->previousSibling();
+            node = sibling;
+            sibling = isNext ? sibling->firstChild() : sibling->lastChild();
+            if (acceptNodeResult == NodeFilter::FILTER_REJECT || !sibling)
+                sibling = isNext ? node->nextSibling() : node->previousSibling();
         }
         node = node->parentNode();
         if (!node || node == root())
@@ -163,37 +158,14 @@ Node* TreeWalker::previousSibling()
     }
 }
 
+Node* TreeWalker::previousSibling()
+{
+    return traverseSiblings<SiblingTraversalType::Previous>();
+}
+
 Node* TreeWalker::nextSibling()
 {
-    RefPtr<Node> node = m_current;
-    if (node == root())
-        return nullptr;
-    while (1) {
-        for (RefPtr<Node> sibling = node->nextSibling(); sibling; ) {
-            short acceptNodeResult = acceptNode(sibling.get());
-            switch (acceptNodeResult) {
-                case NodeFilter::FILTER_ACCEPT:
-                    m_current = sibling.release();
-                    return m_current.get();
-                case NodeFilter::FILTER_SKIP:
-                    if (sibling->firstChild()) {
-                        sibling = sibling->firstChild();
-                        node = sibling;
-                        continue;
-                    }
-                    break;
-                case NodeFilter::FILTER_REJECT:
-                    break;
-            }
-            sibling = sibling->nextSibling();
-        }
-        node = node->parentNode();
-        if (!node || node == root())
-            return nullptr;
-        short acceptNodeResult = acceptNode(node.get());
-        if (acceptNodeResult == NodeFilter::FILTER_ACCEPT)
-            return nullptr;
-    }
+    return traverseSiblings<SiblingTraversalType::Next>();
 }
 
 Node* TreeWalker::previousNode()

diff --git a/Source/WebCore/dom/TreeWalker.h b/Source/WebCore/dom/TreeWalker.h
index 228f23d4746a2159bb50bf6aa62788b73364fb9f..795eed133ac4202beda396f81d46c3cd99577c2b 100644 (file)
--- a/Source/WebCore/dom/TreeWalker.h
+++ b/Source/WebCore/dom/TreeWalker.h
@@ -55,6 +55,8 @@ namespace WebCore {
 
     private:
         TreeWalker(PassRefPtr<Node>, unsigned long whatToShow, RefPtr<NodeFilter>&&);
+        enum class SiblingTraversalType { Previous, Next };
+        template<SiblingTraversalType> Node* traverseSiblings();
         
         Node* setCurrent(PassRefPtr<Node>);