Fix namespaces of elements created in XML documents #14095

nox · 2016-11-06T14:19:12Z

Step 6 of the spec is not correctly implemented, and elements created in XML documents also get the HTML namespace where they shouldn't.

Files:

components/script/dom/document.rs
tests/wpt/metadata/dom/nodes/Node-properties.html.ini
tests/wpt/web-platform-tests/dom/nodes/Node-properties.html

highfive · 2016-11-06T14:19:13Z

Please make a comment here if you intend to work on this issue. Thank you!

nox · 2016-11-06T14:19:38Z

Blocks #2185.

zimurgh · 2016-11-06T15:05:33Z

I would like to take this one!

nox · 2016-11-06T15:06:04Z

Sure!

linclark · 2016-11-21T14:30:03Z

I saw this listed as potentially open on https://starters.servo.org/. If it is available, I'd like to give it a shot.

jdm · 2016-11-21T14:36:06Z

That sounds fine to me. @OldManMike, if you'd like to work on something else let us know and we'll find another issue for you.

zimurgh · 2016-11-26T15:36:34Z

Sure, I'd be willing to pass this one off to @linclark!

linclark · 2016-11-29T01:49:01Z

Thanks! I posted my work-in-progress. I ran into two issues, which I explain in the PR.

[WIP] Fix namespaces of elements created in XML documents This is a work in progress. Without this PR, all elements are created using the HTML namespace, even when createElement is called in an XML document. To fix this, I check `self.is_html_document` and assign the namespace based on that. However, there were two problems with this approach: 1. It seems some documents that I would think are HTML documents aren't currently handled as such. See the condition I added in the second commit. 2. The last test in `Document-constructor.html` fails. I'm not sure whether the test is correct, though. I tried running something similar in Firefox DevEdition (replacing `assert_equals` with `console.log`) and it looks to me like that test would fail in Firefox as well. ``` var doc = new Document(); var a = doc.createElement("a"); a.href = "http://example.org/?\u00E4"; console.log(a.href, "http://example.org/?%C3%A4"); ``` Result: ``` "http://example.org/?ä" "http://example.org/?%C3%A4" ``` ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14095 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because there are already tests which were being ignored.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14401)

jdm · 2017-01-27T19:28:21Z

#14401 has a WIP solution, and there are some review comments that need to be addressed.

enizor · 2017-02-06T12:51:33Z

I would like to work on this issue!

jdm · 2017-02-06T14:30:09Z

Please do! Ask questions if anything is unclear.

enizor · 2017-02-07T21:16:42Z

I used @linclark's changes to understand the unpassed tests and her thoughts about modifying the doctype when XML documents are created.

The tests are failing on

PASS expected FAIL xmlElement.namespaceURI
PASS expected FAIL detachedXmlElement.namespaceURI

I think that these tests are supposed to pass, and the expected: FAIL was added for some reason in #13210 (some form of acknowledgement? ). Deleting tests/wpt/metadata/dom/nodes/Node-properties.html.ini would do the trick in that case. What are your thoughts on this ?

Another question : Steps 3 and 6 of the spec (about the is attribute) aren't implemented. Is there a reason for this? Is it the matter of another issue?

jdm · 2017-02-07T21:23:01Z

Yes, the contents of the ini files reflect the status of the test on the master branch of Servo. PASS expected FAIL means that the test now passes correctly. Deleting the ini file is the correct thing to do.

jdm · 2017-02-07T21:24:08Z

The is member is specific to Custom Elements which we don't support yet.

jdm · 2017-02-23T06:52:00Z

@enizor Have you made any progress with this?

enizor · 2017-02-23T10:27:28Z

I have a lot of trouble understanding the origin of the failed test pointed out in the PR.

I looked for other failed tests and found Document-createElement.html.

I changed the test as only HTML documents have the HTML namespace now.

assert_equals(elt.namespaceURI,
-                        desc == "XML document" ? null : HTMLNS, "namespaceURI")
+                        desc == "HTML document" ? HTMLNS : null, "namespaceURI")

All the tests results from Document-createElement.html are then expected.

I will take another look in the Document-constructor.html fail this week-end

jdm · 2017-03-04T04:39:23Z

@Ms2ger You're more familiar with the namespace stuff than me; could you answer the previous comment?

enizor · 2017-03-05T21:08:30Z

Nevermind my previous comment, I realised that I misunderstood the HTML namespace usage. No changes for Document-createElement are needed.

jdm · 2017-03-21T14:53:32Z

@enizor How is it going? Still working on this?

enizor · 2017-03-27T15:27:47Z

I still don't understand the test fail. I had some trouble running the tests (mach test is dependant of your last mach build, and not the current branch, right?) as the compilation is quite slow on my laptop. I think I should give up on this issue and try some stuff on my side, to make sure I understand more about the project and its tools. Then I may be able to tackle issues with a better efficiency!

jdm · 2017-03-27T15:40:07Z

Yeah, ./mach test relies on the previous completed build. I'll mark this as unassigned given you previous comment.

cu1t · 2017-04-11T14:34:22Z

Hi everyone! I'm new to both rust and servo, but would like to try working on this issue as it is currently not assigned to anyone.

jdm · 2017-04-11T22:52:01Z

Go ahead! Please ask questions if anything is unclear!

cu1t · 2017-04-12T14:02:40Z

A bit unsure how to proceed here. As mentioned above and in @linclark's pull request, required changes result in Document-constructor.html test to fail:

var doc = new Document();
var a = doc.createElement("a");
a.href = "http://example.org/?\u00E4";
console.log(a.href, "http://example.org/?%C3%A4");

I'm not very familiar with the specs, but after giving them some look I tend to think that it is test at fault here. Created document will have contentType set to "application/xml", self.is_html_document will also be false -> empty namespace for createElement. So during element creation this should be treated just as XML. I don't see why this should encode URL then? I'm not 100% on this though, so should I perhaps raise issue at https://github.com/w3c/web-platform-tests?

Fix namespaces of elements created in XML documents Correctly implement following step of [Dom Document Spec](https://dom.spec.whatwg.org/#dom-document-createelement): > Let namespace be the HTML namespace, if the context object is an HTML document or context object’s content type is "application/xhtml+xml", and null otherwise. Note, this will make following test in `tests/wpt/web-platform-tests/dom/nodes/Document-constructor.html` to fail, so related .ini file added to mark it as such: ``` test(function() { var doc = new Document(); var a = doc.createElement("a"); // In UTF-8: 0xC3 0xA4 a.href = "http://example.org/?\u00E4"; assert_equals(a.href, "http://example.org/?%C3%A4"); }, "new Document(): URL parsing") ``` I'm not very familiar with specs, but from quick look at it, I'm doubtfull that it is valid in the first place. This is an "application/xml" document, so I don't see why it should encode a.href. Firefox also fails that. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14095 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because because there are already tests which were being ignored    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16472)

nox added A-content/dom Interacting with the DOM from web content E-less-complex Straightforward. Recommended for a new contributor. labels Nov 6, 2016

nox added the C-assigned There is someone working on resolving the issue label Nov 6, 2016

linclark mentioned this issue Nov 29, 2016

[WIP] Fix namespaces of elements created in XML documents #14401

Closed

5 tasks

jdm added the C-has-open-pr There is a PR open that resolves the issue label Jan 5, 2017

jdm removed C-assigned There is someone working on resolving the issue C-has-open-pr There is a PR open that resolves the issue labels Jan 27, 2017

jdm added the C-assigned There is someone working on resolving the issue label Feb 6, 2017

jdm removed the C-assigned There is someone working on resolving the issue label Mar 27, 2017

jdm added the C-assigned There is someone working on resolving the issue label Apr 11, 2017

cu1t mentioned this issue Apr 15, 2017

Fix namespaces of elements created in XML documents #16472

Merged

5 tasks

bors-servo closed this as completed in #16472 Apr 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix namespaces of elements created in XML documents #14095

Fix namespaces of elements created in XML documents #14095

nox commented Nov 6, 2016

highfive commented Nov 6, 2016

nox commented Nov 6, 2016

zimurgh commented Nov 6, 2016

nox commented Nov 6, 2016

linclark commented Nov 21, 2016

jdm commented Nov 21, 2016

zimurgh commented Nov 26, 2016

linclark commented Nov 29, 2016

jdm commented Jan 27, 2017

enizor commented Feb 6, 2017

jdm commented Feb 6, 2017

enizor commented Feb 7, 2017 •

edited by jdm

Loading

jdm commented Feb 7, 2017

jdm commented Feb 7, 2017

jdm commented Feb 23, 2017

enizor commented Feb 23, 2017

jdm commented Mar 4, 2017 •

edited

Loading

enizor commented Mar 5, 2017

jdm commented Mar 21, 2017

enizor commented Mar 27, 2017

jdm commented Mar 27, 2017

cu1t commented Apr 11, 2017

jdm commented Apr 11, 2017

cu1t commented Apr 12, 2017

Fix namespaces of elements created in XML documents #14095

Fix namespaces of elements created in XML documents #14095

Comments

nox commented Nov 6, 2016

highfive commented Nov 6, 2016

nox commented Nov 6, 2016

zimurgh commented Nov 6, 2016

nox commented Nov 6, 2016

linclark commented Nov 21, 2016

jdm commented Nov 21, 2016

zimurgh commented Nov 26, 2016

linclark commented Nov 29, 2016

jdm commented Jan 27, 2017

enizor commented Feb 6, 2017

jdm commented Feb 6, 2017

enizor commented Feb 7, 2017 • edited by jdm Loading

jdm commented Feb 7, 2017

jdm commented Feb 7, 2017

jdm commented Feb 23, 2017

enizor commented Feb 23, 2017

jdm commented Mar 4, 2017 • edited Loading

enizor commented Mar 5, 2017

jdm commented Mar 21, 2017

enizor commented Mar 27, 2017

jdm commented Mar 27, 2017

cu1t commented Apr 11, 2017

jdm commented Apr 11, 2017

cu1t commented Apr 12, 2017

enizor commented Feb 7, 2017 •

edited by jdm

Loading

jdm commented Mar 4, 2017 •

edited

Loading