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

Implement CDATASection interface and createCDATASection method #22891

Merged
merged 1 commit into from Mar 18, 2019

Conversation

Projects
None yet
7 participants
@georgeroman
Copy link
Contributor

commented Feb 14, 2019


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22846

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 14, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/text.rs, components/script/lib.rs, components/script/dom/node.rs, components/script/dom/webidls/CDATASection.webidl and 6 more
  • @KiChjang: components/script/dom/document.rs, components/script/dom/text.rs, components/script/lib.rs, components/script/dom/node.rs, components/script/dom/webidls/CDATASection.webidl and 6 more
@highfive

This comment has been minimized.

Copy link

commented Feb 14, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth
Copy link
Member

left a comment

Thanks for these changes! It's a good start.

The spec distinguishes between "text nodes" and "exclusive text nodes" (i.e. text nodes that are not cdata nodes), we should allow for cdata nodes wherever the spec doesn't say "exclusive"

I'm not quite sure which of the layout calls need to be fixed

@@ -2244,9 +2255,11 @@ impl NodeMethods for Node {

// Step 4-5.
match node.type_id() {
NodeTypeId::CharacterData(CharacterDataTypeId::Text) if self.is::<Document>() => {
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text))

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

Probably should be CharacterDataTypeId::Text(_), since spec doesn't say exclusive Text node here https://dom.spec.whatwg.org/#concept-node-replace

@@ -2467,7 +2480,7 @@ impl NodeMethods for Node {
{
return false;
}
NodeTypeId::CharacterData(CharacterDataTypeId::Text) |
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) |

This comment has been minimized.

Copy link
@Manishearth
@@ -2922,7 +2935,9 @@ impl Into<LayoutNodeType> for NodeTypeId {
fn into(self) -> LayoutNodeType {
match self {
NodeTypeId::Element(e) => LayoutNodeType::Element(e.into()),
NodeTypeId::CharacterData(CharacterDataTypeId::Text) => LayoutNodeType::Text,
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => {

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

I feel like this should also be an underscore? We should be rendering CData nodes, yes?

@@ -707,27 +707,28 @@ impl RangeMethods for Range {
}
match start_node.type_id() {
// Handled under step 2.
NodeTypeId::CharacterData(CharacterDataTypeId::Text) => (),
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => (),

This comment has been minimized.

Copy link
@Manishearth
let child = start_node.ChildNodes().Item(start_offset);
(child, DomRoot::from_ref(&*start_node))
let (reference_node, parent) = if start_node.type_id() ==
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text))

This comment has been minimized.

Copy link
@Manishearth
@@ -953,7 +954,10 @@ impl RangeMethods for Range {
NodeTypeId::Document(_) | NodeTypeId::DocumentFragment => None,
NodeTypeId::Element(_) => Some(DomRoot::downcast::<Element>(node).unwrap()),
NodeTypeId::CharacterData(CharacterDataTypeId::Comment) |
NodeTypeId::CharacterData(CharacterDataTypeId::Text) => node.GetParentElement(),
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::CDATASection)) |
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => {

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

you can just use an _ here

@@ -232,7 +232,10 @@ impl<'a> Serialize for &'a Node {
serializer.write_doctype(&doctype.name())?;
},

NodeTypeId::CharacterData(CharacterDataTypeId::Text) => {
NodeTypeId::CharacterData(CharacterDataTypeId::Text(

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

use an _

@@ -153,7 +155,8 @@ impl<'ln> NodeInfo for ServoLayoutNode<'ln> {
}

fn is_text_node(&self) -> bool {
self.script_type_id() == NodeTypeId::CharacterData(CharacterDataTypeId::Text)
self.script_type_id() ==
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text))

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

Should this also allow for Cdata nodes? cc @nox

@@ -765,7 +768,7 @@ impl<'le> ::selectors::Element for ServoLayoutElement<'le> {
.dom_children()
.all(|node| match node.script_type_id() {
NodeTypeId::Element(..) => false,
NodeTypeId::CharacterData(CharacterDataTypeId::Text) => unsafe {
NodeTypeId::CharacterData(CharacterDataTypeId::Text(TextTypeId::Text)) => unsafe {

This comment has been minimized.

Copy link
@Manishearth

Manishearth Feb 16, 2019

Member

should this also allow for cdata nodes?

@georgeroman georgeroman force-pushed the georgeroman:implement_cdatasection branch 2 times, most recently from 2c83b9c to 70e2b9f Feb 17, 2019

@Manishearth

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@bors-servo r+

sorry about the delay!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

📌 Commit 70e2b9f has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

⌛️ Testing commit 70e2b9f with merge f245cdf...

bors-servo added a commit that referenced this pull request Mar 11, 2019

Auto merge of #22891 - georgeroman:implement_cdatasection, r=Manishearth
Implement CDATASection interface and createCDATASection method

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22846

<!-- 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/22891)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] Document interface: operation createCDATASection(DOMString)

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] Document interface: new Document() must inherit property "createCDATASection(DOMString)" with the proper type

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] Document interface: calling createCDATASection(DOMString) on new Document() with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] Document interface: xmlDoc must inherit property "createCDATASection(DOMString)" with the proper type

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] Document interface: calling createCDATASection(DOMString) on xmlDoc with too few arguments must throw TypeError

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface: existence and properties of interface object

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface object length

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface object name

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface: existence and properties of interface prototype object

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface: existence and properties of interface prototype object's "constructor" property

  ▶ Unexpected subtest result in /dom/interfaces.html?exclude=Node:
  └ PASS [expected FAIL] CDATASection interface: existence and properties of interface prototype object's @@unscopables property

tests/wpt/mozilla/tests/mozilla/interfaces.html will need to be updated as well.

@georgeroman georgeroman force-pushed the georgeroman:implement_cdatasection branch from 70e2b9f to 3fb6d4d Mar 12, 2019

@georgeroman georgeroman force-pushed the georgeroman:implement_cdatasection branch from 3fb6d4d to 2983d9d Mar 13, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

📌 Commit 2983d9d has been approved by Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

⌛️ Testing commit 2983d9d with merge 71d86b9...

bors-servo added a commit that referenced this pull request Mar 14, 2019

Auto merge of #22891 - georgeroman:implement_cdatasection, r=Manishearth
Implement CDATASection interface and createCDATASection method

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22846

<!-- 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/22891)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

💔 Test failed - linux-rel-wpt

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15844.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Should check the 'parent' type before checking whether 'child' is a child of 'parent'", 
    "test": "/dom/nodes/Node-replaceChild.html", 
    "line": 74566, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "If the context node is not a node that can contain children, a HierarchyRequestError exception should be thrown", 
    "test": "/dom/nodes/Node-replaceChild.html", 
    "line": 74581, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "defaultValue and value include CDATASection Text nodes", 
    "test": "/html/semantics/forms/the-textarea-element/value-defaultValue-textContent-xhtml.xhtml", 
    "line": 104653, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Should check the 'parent' type before checking whether 'child' is a child of 'parent'", 
    "test": "/dom/nodes/Node-insertBefore.html", 
    "line": 158814, 
    "action": "test_result", 
    "expected": "FAIL"
}
@domenic

This comment has been minimized.

Copy link

commented Mar 14, 2019

Why does this delete the value-defaultValue-textContent-xhtml.xhtml test?

@Manishearth

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Yeah, this should not be deleting any tests

@georgeroman

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Oops, I wanted to delete the metadata file for the test, I'll fix it in a moment!

@georgeroman georgeroman force-pushed the georgeroman:implement_cdatasection branch from b8a1b7e to 4b8282b Mar 14, 2019

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

No upstreamable changes; closed existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#15844.

@jdm

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@bors-servo r=Manishearth

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

📌 Commit 4b8282b has been approved by Manishearth

bors-servo added a commit that referenced this pull request Mar 18, 2019

Auto merge of #22891 - georgeroman:implement_cdatasection, r=Manishearth
Implement CDATASection interface and createCDATASection method

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #22846

<!-- 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/22891)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

⌛️ Testing commit 4b8282b with merge 34fda66...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@bors-servo bors-servo referenced this pull request Mar 18, 2019

Merged

Implement XMLSerializer interface #23044

3 of 3 tasks complete

@bors-servo bors-servo merged commit 4b8282b into servo:master Mar 18, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@bors-servo bors-servo referenced this pull request Mar 18, 2019

Merged

Partial ShadowDOM support #22743

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.