Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix namespaces of elements created in XML documents #16472

Merged
merged 1 commit into from Apr 20, 2017

Conversation

@cu1t
Copy link
Contributor

cu1t commented Apr 15, 2017

Correctly implement following step of Dom Document Spec:

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.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14095 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because because there are already tests which were being ignored

This change is Reviewable

@@ -2772,7 +2772,14 @@ impl DocumentMethods for Document {
if self.is_html_document {
local_name.make_ascii_lowercase();
}
let name = QualName::new(ns!(html), LocalName::from(local_name));

let ns = if self.is_html_document || self.content_type == DOMString::from("application/xhtml+xml") {

This comment has been minimized.

Copy link
@nox

nox Apr 16, 2017

Member

Don't create a DOMString just for the equality check, the type DOMString implements PartialEq<&str>.

self.content_type == "application/xhtml+xml"
@nox
Copy link
Member

nox commented Apr 16, 2017

Thanks for your contribution! Just a small change to do and this will be ready to be merged.

@cu1t
Copy link
Contributor Author

cu1t commented Apr 16, 2017

Thanks for review! Fixed.

@nox
Copy link
Member

nox commented Apr 16, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

📌 Commit 4664c18 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

Testing commit 4664c18 with merge 4be3dc1...

bors-servo added a commit that referenced this pull request Apr 16, 2017
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because because there are already tests which were being ignored

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16472)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

💔 Test failed - windows-msvc-dev

@jdm
Copy link
Member

jdm commented Apr 16, 2017

@bors-servo: retry

@jdm
Copy link
Member

jdm commented Apr 16, 2017

Also, this happened:


  ▶ Unexpected subtest result in /_mozilla/mozilla/document_head.html:
  │ FAIL [expected PASS] append head to a new document
  │   → assert_true: test2-1, append head to a new document: should be HTMLHeadElement expected true got false
  │ 
  │ @http://web-platform.test:8000/_mozilla/mozilla/document_head.html:20:17
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  └ @http://web-platform.test:8000/_mozilla/mozilla/document_head.html:14:13
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

Testing commit 4664c18 with merge 9667090...

bors-servo added a commit that referenced this pull request Apr 16, 2017
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because because there are already tests which were being ignored

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16472)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

💔 Test failed - windows-msvc-dev

@aneeshusa
Copy link
Member

aneeshusa commented Apr 16, 2017

@bors-servo retry

  • Buildbot didn't fetch the right commit
@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1 are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt2, windows-msvc-dev...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2017

💔 Test failed - linux-rel-wpt

@cu1t
Copy link
Contributor Author

cu1t commented Apr 17, 2017

Failing test seems to be similar case to one with Document-constructor.html (marked failing lines with <--- here):

test(function() {
    var new_document = new Document();
    new_document.appendChild(new_document.createElement("html"));
    var new_head = new_document.createElement("head");

    assert_not_equals(new_head, null, "test2-0, append head to a new document");
    assert_true(new_head instanceof HTMLHeadElement, "test2-1, append head to a new document: should be HTMLHeadElement"); <--- here
    assert_equals(new_head && new_head.tagName, "head", "test2-2, append head to a new document");

    // Document::head is read-only.
    new_document.head = new_head;
    assert_equals(new_document.head, null, "test2-3, append head to a new document");

    new_document.documentElement.appendChild(new_head); <--- this WILL create element
    assert_equals(new_document.head, new_head, "test2-4, append head to a new document"); <--- here
}, "append head to a new document");

From what I see, test expects new_document to be treated as html, but it gets treated as xml instead. Should I mark this as failing too then?

@jdm
Copy link
Member

jdm commented Apr 17, 2017

We can fix that test by doing var new_head = new_document.createElementNS('http://www.w3.org/1999/xhtml', 'head');.

@cu1t
Copy link
Contributor Author

cu1t commented Apr 17, 2017

Still fails at 2-4. Change last line to assert_equals(new_document.head, null, "test2-4, append head to a new document"); perhaps? Or remove it altogether?

@jdm
Copy link
Member

jdm commented Apr 17, 2017

That can be fixed by changing the other use of createElement to use createElementNS as well.

@jdm
Copy link
Member

jdm commented Apr 17, 2017

In fact, please modify all uses in that file, since the subsequent test also uses the Document constructor.

@jdm
Copy link
Member

jdm commented Apr 18, 2017

Looks good! You will need to run ./mach update-manifest and commit the results, then squash all of the commits into one, please.

@cu1t cu1t force-pushed the cu1t:#14095-fix-xml-doc-namespaces branch from a2a1bc4 to bef86cb Apr 19, 2017
@cu1t
Copy link
Contributor Author

cu1t commented Apr 20, 2017

Done.

@jdm
Copy link
Member

jdm commented Apr 20, 2017

@bors-servo: r=nox
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2017

📌 Commit bef86cb has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2017

Testing commit bef86cb with merge 2d732d8...

bors-servo added a commit that referenced this pull request Apr 20, 2017
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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because because there are already tests which were being ignored

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16472)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 20, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: nox
Pushing 2d732d8 to master...

@bors-servo bors-servo merged commit bef86cb into servo:master Apr 20, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@cu1t cu1t deleted the cu1t:#14095-fix-xml-doc-namespaces branch Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.