Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[WIP] Fix namespaces of elements created in XML documents #14401
Conversation
|
r? @nox |
|
This should probably wait for #14423 to land. |
|
#14423 landed. |
|
@nox Review ping. |
|
@linclark Why not just follow current step 4 correctly in
I'm wary of changing how |
| // Before, the call to XMLDocument::new was hardcoded to pass in NonHTMLDocument as the doc type. That means | ||
| // that document.is_html_document is false for documents that are actually HTML. That makes the tests fail | ||
| // because I use that property in document.createElement to decide whether to use the HTML namespace or not. | ||
| let doc_type = match maybe_doctype { |
This comment has been minimized.
This comment has been minimized.
Ms2ger
Dec 16, 2016
Contributor
This does look problematic; for example, consider the case
var doctype = document.implementation.createDocumentType("html", "", "");
var doc = document.implementation.createDocument("http://www.w3.org/1999/xhtml", "html", doctype);
assert_equals(doc.documentElement.tagName, "html")I believe this should pass per spec, and it does in Chrome, though not in Gecko. We should add this test somewhere.
|
Let's see if this fails anything else |
bors-servo
added a commit
that referenced
this pull request
Dec 16, 2016
[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"
```
---
<!-- 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 there are already tests which were being ignored.
<!-- 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/14401)
<!-- Reviewable:end -->
|
|
|
@bors-servo retry
|
bors-servo
added a commit
that referenced
this pull request
Dec 16, 2016
[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"
```
---
<!-- 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 there are already tests which were being ignored.
<!-- 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/14401)
<!-- Reviewable:end -->
|
I strongly suspect those are actual failures caused by these changes. |
|
|
|
|
|
@linclark Are you planning to address the review feedback, or should we close this PR? |
|
Unfortunately I'm heads down on an assignment over the next few weeks, so won't get a chance to tackle this. Feel free to close it out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
linclark commentedNov 29, 2016
•
edited
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_documentand assign the namespace based on that.However, there were two problems with this approach:
Document-constructor.htmlfails. I'm not sure whether the test is correct, though. I tried running something similar in Firefox DevEdition (replacingassert_equalswithconsole.log) and it looks to me like that test would fail in Firefox as well.Result:
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is