Running the Acid3 test in debug builds reveals an assertion failure: ASSERTION FAILED: !stayWithin || !n->nextSibling() || n->nextSibling()->isDescendantOf(stayWithin) (/Users/weinig/Code/webkit/OpenSource/WebCore/dom/Node.cpp:490 WebCore::Node* WebCore::Node::traverseNextNode(const WebCore::Node*) const) This stems from the TreeWalker class being able to set the currentNode to an arbitrary node but not checking that is within it's root node when using the traverseNextNode/traversePreviousNode functions.
Created attachment 18291 [details] patch
Comment on attachment 18291 [details] patch Looks great!
Landed in r29203.
Comment on attachment 18291 [details] patch This is wrong. If you read the TreeWalker documentation, you'll see that it has well defined behavior when used on nodes outside the original subtree. The code correctly implemented most of those rules before -- this patch broke it! I have a patch that fixes it. The test case expects behavior that is incorrect, so I need to change it around.
Here is the relevant quote from the specification: "All the TreeWalker's navigation methods operate in terms of the context of the currentNode at the time they are invoked, no matter what has happened to, or around, that node since the last time the TreeWalker was accessed. This remains true even if the currentNode is moved out of its original subtree." And: "If we instead insert the currentNode into the <subtree/> element, like so: [...] we have moved the currentNode out from under the TreeWalker's root node. This does not invalidate the TreeWalker; it may still be used to navigate relative to the currentNode." The real fix here would be to remove the assertions, and I have done this in my new patch.
The patch in bug 3492 addresses this.
I realized after landing that this was not correct (but only now thought that the right corse of action would have been to roll it out). A fix I made is in bug 16759, but is probably not necessary with Darin's patch.