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 upAdd a testcase for escaping script element contents #15148
Conversation
…innerHTML on a script tag escapes tag characters". I'm honestly unsure if it does this correctly, but learning by doing and all that....
jdm
changed the title
|
This looks like the test I was expecting to see! |
| @@ -58,6 +59,7 @@ | |||
| ["\"", "<span>\"</span>"], | |||
| ["<style><&></style>", "<span><style><&></style></span>"], | |||
| ["<script type=\"test\"><&><\/script>", "<span><script type=\"test\"><&><\/script></span>"], | |||
| ["<script type=\"test\"><&><\/script>", "<script type=\"test\"><&><\/script>"], | |||
This comment has been minimized.
This comment has been minimized.
jdm
Jan 22, 2017
Member
This test performs element.innerHTML on each test element, so the <script> and </script> output should not be in the expected output.
| @@ -25,6 +25,7 @@ | |||
| <span>"</span> | |||
| <span><style><&></style></span> | |||
| <span><script type="test"><&></script></span> | |||
| <script type="test"><&></script> | |||
This comment has been minimized.
This comment has been minimized.
jdm
Jan 22, 2017
Member
No need for the type="test" here, since we only care about the child contents of the element.
This comment has been minimized.
This comment has been minimized.
Ms2ger
Jan 23, 2017
Contributor
I imagine this would cause an unhandled error event when we try to execute <&> as JS,
|
Presumably |
Yeah, I have less than 10 hours of experience with javascript; sorry about the inevitable mistakes. I think these are the changes you were looking for, though.
|
I think these are the changes you are looking for. I have less than 10 hours of experience with javascript; sorry about the inevitable mistakes. |
|
Are you planning to include the fix for html5ever in this PR before we merge the new testcase? |
| @@ -59,7 +59,7 @@ | |||
| ["\"", "<span>\"</span>"], | |||
| ["<style><&></style>", "<span><style><&></style></span>"], | |||
| ["<script type=\"test\"><&><\/script>", "<span><script type=\"test\"><&><\/script></span>"], | |||
| ["<script type=\"test\"><&><\/script>", "<script type=\"test\"><&><\/script>"], | |||
| ["<script><&><\/script>", "<&>"], | |||
This comment has been minimized.
This comment has been minimized.
jdm
Jan 26, 2017
Member
Aren't these two values reversed? The first value is the expected output of innerHTML, the second is the expected output of outerHTML.
This comment has been minimized.
This comment has been minimized.
tormeh
Jan 26, 2017
Author
Can I put them both in a single PR? I thought the test being in the servo repository and the serializer being in the html5ever repository made that hard?
reversed inner/outerhtml
|
A bit unsure how this page looks to you, but I'll reply here as well: I didn't think the two changes could be in the same PR, since they're in different repositories: servo vs html5ever. |
|
Once the change is part of html5ever, we will need to update Servo to use the latest version of that library. We can either merge this new testcase immediately and mark this new test as an expected failure, or we can wait to merge it until we can include the change to update html5ever. Your choice! |
|
Hm... I think it's better to add it as an expected failure. Then I can test my patch against this test. |
|
You can run |
|
@tormeh Are you still planning to address my last comment? |
|
Yeah, I'm having a problem with doing that because the version of html5ever that servo uses is not the newest version, so the instructions for "working on a crate" under https://github.com/servo/servo/blob/master/docs/HACKING_QUICKSTART.md is not really working. I get compilation errors because the newest versions of servo and html5ever are incompatible. I guess I could checkout an earlier version with git but I haven't tried it yet, because I don't know how and honestly it sounds unappealing. I suspect Servo might be a more pleasant experience for people who are already comfortable with semi-advanced git usage and who has worked with big codebases before. I mean, I haven't yet touched any of the actual Rust code (or, well, I've written a print-statement in the html5ever source stored in the servo cargo cache but cargo doesn't really work like that, I found out). I think I'm bouncing off of this one. Sorry. |
|
Oh, you're running into the fact that servo/html5ever#249 just merged while #15283 is still outstanding. Shoot; we're not supposed to do that :< |
|
I'm going to go ahead and revert that html5ever commit. Please let me know if that causes you to change your mind, or go ahead and close this PR if you don't intend to keep working on this. Sorry it wasn't a positive experience so far! |
|
I'm going to go ahead and free this up for someone else to work on. |
tormeh commentedJan 22, 2017
•
edited by jdm
…innerHTML on a script tag escapes tag characters". I'm honestly unsure if it does this correctly, but learning by doing and all that....
Added new item to the test elements: <script type="test"><&></script>
and a corresponding item to the "expected" list: ["<script type="test"><&></script>", "<script type="test"><&></script>"]
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is