Bug 149104 - Node.baseURI should not return null for detached nodes
Summary: Node.baseURI should not return null for detached nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://dom.spec.whatwg.org/#dom-node...
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2015-09-13 10:35 PDT by Chris Dumez
Modified: 2015-09-16 10:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.98 KB, patch)
2015-09-13 11:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (608.19 KB, application/zip)
2015-09-13 12:08 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (688.57 KB, application/zip)
2015-09-13 12:15 PDT, Build Bot
no flags Details
Patch (13.41 KB, patch)
2015-09-13 13:23 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-09-13 10:35:01 PDT
Node.baseURI should not return null for detached nodes. It should return the node document's base URL. The node document is set when the node is created so it is valid even if the node is detached:
https://dom.spec.whatwg.org/#dom-node-baseuri

WebKit currently traverses the parents to find the base URL, which only works if the node is attached. Also, WebKit takes into account the xml:base attribute when computing the baseURI.

Both Chrome and Firefox already dropped support for xml:base so I think we should as well:
https://code.google.com/p/chromium/issues/detail?id=341854
https://bugzilla.mozilla.org/show_bug.cgi?id=903372

Firefox complies with the specification. Chrome's baseURI still only works for attached Nodes as their implementation still traverses the DOM tree, despite dropping support for xml:base.
Comment 1 Chris Dumez 2015-09-13 10:35:15 PDT
Relevant W3C bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20976
Comment 2 Chris Dumez 2015-09-13 10:35:47 PDT
rdar://problem/22559535
Comment 3 Chris Dumez 2015-09-13 11:26:07 PDT
Created attachment 261083 [details]
Patch
Comment 4 Sam Weinig 2015-09-13 11:36:45 PDT
Comment on attachment 261083 [details]
Patch

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

> LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03-expected.txt:3
> +Message	nodegetbaseuri03: assertNull failed, actual file:///Users/chris/WebKit/OpenSource/LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03.xhtml

I don't think we want to check-in something with your user specific path in it.
Comment 5 Chris Dumez 2015-09-13 11:55:39 PDT
Comment on attachment 261083 [details]
Patch

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

>> LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03-expected.txt:3
>> +Message	nodegetbaseuri03: assertNull failed, actual file:///Users/chris/WebKit/OpenSource/LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03.xhtml
> 
> I don't think we want to check-in something with your user specific path in it.

Interesting :) sorry about that, I guess I will have to tweak the test.
Comment 6 Build Bot 2015-09-13 12:08:39 PDT
Comment on attachment 261083 [details]
Patch

Attachment 261083 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/168031

New failing tests:
dom/xhtml/level3/core/nodegetbaseuri03.xhtml
Comment 7 Build Bot 2015-09-13 12:08:43 PDT
Created attachment 261085 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Build Bot 2015-09-13 12:15:13 PDT
Comment on attachment 261083 [details]
Patch

Attachment 261083 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/168035

New failing tests:
dom/xhtml/level3/core/nodegetbaseuri03.xhtml
Comment 9 Build Bot 2015-09-13 12:15:17 PDT
Created attachment 261086 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 10 Chris Dumez 2015-09-13 13:23:02 PDT
Created attachment 261089 [details]
Patch
Comment 11 WebKit Commit Bot 2015-09-13 20:05:01 PDT
Comment on attachment 261089 [details]
Patch

Clearing flags on attachment: 261089

Committed r189677: <http://trac.webkit.org/changeset/189677>
Comment 12 WebKit Commit Bot 2015-09-13 20:05:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Darin Adler 2015-09-16 10:25:51 PDT
Comment on attachment 261089 [details]
Patch

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

> LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03.js:102
> -      assertNull("nodegetbaseuri03",baseURI);
> +      assertTrue("nodegetbaseuri03", baseURI == null);

Why change this if it’s going to fail anyway? We should consider just removing the test entirely if we’re going to make changes like these. Or changing the test to expect correct behavior.
Comment 14 Chris Dumez 2015-09-16 10:29:27 PDT
(In reply to comment #13)
> Comment on attachment 261089 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261089&action=review
> 
> > LayoutTests/dom/xhtml/level3/core/nodegetbaseuri03.js:102
> > -      assertNull("nodegetbaseuri03",baseURI);
> > +      assertTrue("nodegetbaseuri03", baseURI == null);
> 
> Why change this if it’s going to fail anyway? We should consider just
> removing the test entirely if we’re going to make changes like these. Or
> changing the test to expect correct behavior.

These reason I had to change it is because when it failed, it was printing out an absolute path and therefore would give different results on the bots. I otherwise try not to edit imported compliance tests as I think it is our policy.

I however agree with you that we could simply drop some of the old DOM compliance tests that are now outdated. I'll do that from now on and drop this one in a follow-up patch.