Bug 36427 – XPathEvaluator returns document root for "/" when the context node is in a detached subtree (unlike Firefox)

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

XPathEvaluator returns invalid root node for "/" expression when context node is an element without a parent node. 

For example:

var xpe = new XPathEvaluator....
...
xpe.evaluate("/", document.body ... // returns window.document in Webkit. Firefox returns context node.

[Alexey Proskuryakov]

> xpe.evaluate("/", document.body ... // returns window.document in Webkit.

The description doesn't match test case - in the latter, it's a detached subtree of elements created with document.createElement:


  var fset = window.fset = document.createElement('fieldset');
  fset.appendChild( document.createElement('legend') );
  var form = document.createElement('form');
  form.appendChild( document.createElement('input') );
  fset.appendChild( form );

  // get fieldset
  assert( evaluateXPath(fset,'/')[0] == fset, 'Trying to get fieldset element.' ); // fails for webkit
  assert( evaluateXPath(fset,'/form')[0] == form, 'Tring to get form element' );


Anyway, the XPath spec <http://www.w3.org/TR/xpath/> says "/ selects the document root (which is always the parent of the document element)". Why do you think that this is a bug in WebKit?

[Chris Evans]

Copying in some discussion where we conclude it's a reasonable idea to follow Firefox:

---
Thanks for the references. I did spend some time poking around trying to find a clear reference for what to do with detached subtrees, but didn't find one. That's why matching Firefox seemed like the best option. It's also more in-line with internal expectations of WebKit code, which is very handy in terms of not being an intrusive patch.

To go into more detail, the relevant sentence in http://www.w3.org/TR/xpath/ seems to have changed since Alexey quoted it in the older bug. It is now, section "2 Location Paths": "A / by itself selects the root node of the document containing the context node."

And "contains" is best defined by http://www.w3.org/TR/domcore/#dom-node-contains. Putting it together, we have well defined behavior when the context node is attached to the document but not when it is detached.

TL;DR: IMHO, a careful reading leaves us free to follow Firefox. It'll be nice to have two major rendering engines behaving the same way.
---

---
Actually, the spec hasn't changed since 1999, and it still has that specific example saying that Firefox is wrong.

"/ selects the document root (which is always the parent of the document element)"

It's of vague interest that XPath 2 actually changes the meaning of leading slash "to begin the path at the root node of the tree that contains the context node". However, that's an entirely separate incompatible spec that's not relevant to the Web.

> And "contains" is best defined by http://www.w3.org/TR/domcore/#dom-node-contains

XPath authors most certainly didn't envision referencing the DOM4 train wreck in 1999 :)

I'm ambivalent on whether we match Firefox or the spec here. This seems to be a case that's only happening in browsers, so XPath spec authors likely haven't really thought of it, and it does seem kind of logical to start from tree root.
---

[Chris Evans]

Created attachment 158459 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158459 [details]
Patch

Clearing flags on attachment: 158459

Committed r125631: <http://trac.webkit.org/changeset/125631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 158459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158459&action=review

> Source/WebCore/ChangeLog:10
> +        The consensus seems to be that the XPath spec is ambiguous for the case of detached nodes, and that using the fragment root is more intuitive than the document root for the case of detached nodes.
> +        For example, http://www.w3.org/TR/xpath/ section 2 "Location Paths" is only clear for attached nodes: "A / by itself selects the root node of the document containing the context node. If it is followed by a relative location path, then the location path selects the set of nodes that would be selected by the relative location path relative to the root node of the document containing the context node."

I'd be happier if the description was more direct. What we're doing is an intentional spec violation, there is no ambiguity.

> Source/WebCore/xml/XPathPath.cpp:98
> -    // For absolute location paths, the context node is ignored - the
> -    // document's root node is used instead.
> +    // For absolute location paths, the context node is ignored. The
> +    // document's root node is used for attached nodes, otherwise the root
> +    // node of the detached subtree is used.

This was a bad comment, and is still a bad one. Re-stating what the (pretty obvious) code does is not helpful.

Saying why we do this might be helpful, since we don't follow the spec.

> LayoutTests/fast/xpath/xpath-detached-nodes-expected.txt:8
> +PASS result.numberValue is 1
> +PASS result.numberValue is 0
> +PASS result.numberValue is 1
> +PASS result.numberValue is 0
> +PASS result.numberValue is NaN

It's generally desirable to make test output a little more self-descriptive. With a long list of similar subtests, it's hard to even find which one failed.

Putting the whole evaluate statement into shouldBe would be perfectly fine here, in my opinion.

> LayoutTests/fast/xpath/xpath-detached-nodes.html:13
> +  if (window.testRunner)
> +    testRunner.dumpAsText();

This is unnecessary, js-test-pre.js does that for you.

> LayoutTests/fast/xpath/xpath-detached-nodes.html:15
> +  frag = document.createDocumentFragment();

I think that it would be important to test "/" path for fragments.

[Chris Evans]

I'll fix these things.

I'm most concerned about the comment in the code being correct. Alexey, would you mind re-iterating why you think it's a willful spec violation so that we're all on the same page? I can then try and document it clearly.

[Alexey Proskuryakov]

I'm primarily going from this example in the spec:

"/ selects the document root (which is always the parent of the document element)"

[Chris Evans]

What's your reading on the definition of "document element"?

Section 5.1 seems closest to defining that:
---
The root node is the root of the tree. A root node does not occur except as the root of the tree. The element node for the document element is a child of the root node.
---

[Alexey Proskuryakov]

My understanding is that this is terminology from XML Infoset <http://www.w3.org/TR/xml-infoset/>, which gives it a 1-to-1 relationship with XML's document element.

Anyway, I'm not sure why you are asking about document element. The example says that / selects "document root", not "tree root" or "document element".

But this discussion made me realize that we didn't check or test "//" behavior. It's just a shortcut, so it should have changed its behavior in detached trees in this patch, too.

[Chris Evans]

Created attachment 160069 [details]
Patch

Action Alexey's comments.

[Alexey Proskuryakov]

Comment on attachment 160069 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160069&action=review

Thank you!

> Source/WebCore/xml/XPathPath.cpp:101
> +    // "/ selects the document root (which is always the parent of the
> +    // document element)"
> +    // "A / by itself selects the root node of the document containing the
> +    // context node."

We don't need to make lines this short, it's normal to have much longer lines in WebKit.

> Source/WebCore/xml/XPathPath.cpp:104
> +    // This is for compatability with Firefox, and also seems like a more

Typo: should be "compatibility".

[Chris Evans]

Created attachment 160492 [details]
Patch

Patch for landing.

[WebKit Review Bot]

Comment on attachment 160492 [details]
Patch

Clearing flags on attachment: 160492

Committed r126621: <http://trac.webkit.org/changeset/126621>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/11313005>