Skip to content

/ servo

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

Serialize the children of void html elements as empty strings #22542

Merged
merged 1 commit into from Jan 3, 2019
Merged

Serialize the children of void html elements as empty strings #22542

merged 1 commit into from Jan 3, 2019

Conversation

@georgeroman
Copy link
Contributor

georgeroman commented Dec 23, 2018 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22502 (GitHub issue number if applicable)

This change is Reviewable
@highfive
Copy link

highfive commented Dec 23, 2018

Heads up! This PR modifies the following files:
@CYBAI
Copy link
Collaborator

CYBAI commented Dec 23, 2018

I found the menuitem is not in the list of Void Elements; maybe it's worth removing it in this PR?

servo/components/script/dom/element.rs

Lines 1099 to 1101 in 29eb8bd

local_name!("link") |
local_name!("menuitem") |
local_name!("meta") |

Ref:
@CYBAI
CYBAI reviewed Dec 23, 2018
View changes
components/script/dom/element.rs Outdated
@@ -1108,6 +1108,15 @@ impl Element {
}

pub fn serialize(&self, traversal_scope: TraversalScope) -> Fallible<DOMString> {
match traversal_scope {
TraversalScope::ChildrenOnly(_) => {

This comment has been minimized.

Sign in to view
Copy link
@CYBAI

CYBAI Dec 23, 2018

Collaborator

Maybe we can use if let here?

if let TraversalScope::ChildrenOnly(_) = traversal_scope {
  if self.is_void() {
    return Ok(DOMString::from(""));
  }
}
@paulrouget
Copy link
Contributor

paulrouget commented Dec 26, 2018

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 26, 2018

📌 Commit 715ee7b has been approved by paulrouget
@bors-servo
Copy link
Contributor

bors-servo commented Dec 26, 2018

⌛ Testing commit 715ee7b with merge 1862c82...
bors-servo added a commit that referenced this pull request Dec 26, 2018
@bors-servo
Auto merge of #22542 - georgeroman:serialize_children_of_void_as_empt…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
1862c82 
…y_strings, r=paulrouget

Serialize the children of void html elements as empty strings

<!-- 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 #22502  (GitHub issue number if applicable)

<!-- 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/22542)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 26, 2018

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Dec 26, 2018

It's not totally clear to me whether this code belongs in html5ever or Servo. I would like to review it further.
@jdm
Copy link
Member

jdm commented Jan 2, 2019

I have reviewed the specification and this code change looks too specific to me. I'll add some more information to the github issue, since the code change likely belongs in the html5ever repository.
@jdm jdm closed this Jan 2, 2019
@jdm jdm reopened this Jan 2, 2019
@jdm
Copy link
Member

jdm commented Jan 2, 2019

I've changed my mind - I think this change is necessary, but I think it belongs in components/script/dom/servoparser/html.rs in Node::serialize instead.
@jdm jdm mentioned this pull request Jan 2, 2019
Closed
@jdm jdm assigned jdm and unassigned paulrouget Jan 2, 2019
@jdm
jdm requested changes Jan 2, 2019
View changes
components/script/dom/servoparser/html.rs Outdated
@@ -210,6 +210,12 @@ impl<'a> Serialize for &'a Node {
) -> io::Result<()> {
let node = *self;

if let TraversalScope::ChildrenOnly(_) = traversal_scope {
if node.downcast::<Element>().unwrap().is_void() {

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jan 2, 2019

Member

We should use if node.downcast::<Element>().map_or(false, |e| e.is_void()) { instead, so we do not panic when serializing non-element nodes.
@georgeroman
Serialize the children of void html elements as empty strings
Loading status checks…
a2d5d4f
@jdm
Copy link
Member

jdm commented Jan 3, 2019

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

📌 Commit a2d5d4f has been approved by jdm
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

⌛ Testing commit a2d5d4f with merge c19cc66...
bors-servo added a commit that referenced this pull request Jan 3, 2019
@bors-servo
Auto merge of #22542 - georgeroman:serialize_children_of_void_as_empt…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c19cc66 
…y_strings, r=jdm

Serialize the children of void html elements as empty strings

<!-- 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 #22502  (GitHub issue number if applicable)

<!-- 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/22542)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

💔 Test failed - linux-rel-wpt
@jdm
Copy link
Member

jdm commented Jan 3, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

⌛ Testing commit a2d5d4f with merge 7c34a70...
bors-servo added a commit that referenced this pull request Jan 3, 2019
@bors-servo
Auto merge of #22542 - georgeroman:serialize_children_of_void_as_empt…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
7c34a70 
…y_strings, r=jdm

Serialize the children of void html elements as empty strings

<!-- 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 #22502  (GitHub issue number if applicable)

<!-- 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/22542)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: jdm
Pushing 7c34a70 to master...
@bors-servo bors-servo merged commit a2d5d4f into servo:master Jan 3, 2019
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
@jdm jdm mentioned this pull request Jan 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CYBAI CYBAI

@jdm jdm

Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Update serialization of children of void elements
6 participants
@georgeroman @highfive @CYBAI @paulrouget @bors-servo @jdm
You can’t perform that action at this time.