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 upfix #18776: use XML fragment serialization for innerHTML in XML documents #18888
Conversation
highfive
commented
Oct 15, 2017
highfive
commented
Oct 15, 2017
|
|
|
That looks like the right idea to me. |
(WIP) fix #18776: use XML fragment serialization for innerHTML in XML documents <!-- Please describe your changes on the following line: --> I am not sure whether my solution is in the right way. @jdm Can you give some advises? --- <!-- 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 - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #18776 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- 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/18888) <!-- Reviewable:end -->
|
|
|
It will fail at https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/domparsing/innerhtml-03.xhtml. Keep working on it. |
This is exactly the result that we are hoping for :) Please remove the corresponding ini files in tests/wpt/metadata/domparsing/, since the tests are no longer expected to fail. |
|
Actually, I think I was overeager. So we pass some of the tests in that file now, which is an improvement. We should remove those specific lines from https://dxr.mozilla.org/servo/source/tests/wpt/metadata/domparsing/innerhtml-03.xhtml.ini?q=path%3Ainnerhtml-03.xhtml.ini&redirect_type=single#3 rather than the entire file. |
|
I guess https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/domparsing/innerhtml-03.xhtml#L32-L36 is the only test that we still fail in that file. |
|
Actually still remain two tests: |
|
I have no idea how to fix these two. |
|
We can investigate them separately from this PR. |
tigercosmos
changed the title
|
r? @jdm |
|
Let's fix outerHTML in the same way, too. |
|
|
||
| [Node for data] | ||
| expected: FAIL | ||
|
|
||
| [Node for area] |
This comment has been minimized.
This comment has been minimized.
|
@bors-servo: r+ |
|
|
fix #18776: use XML fragment serialization for innerHTML in XML documents <!-- Please describe your changes on the following line: --> I am not sure whether my solution is in the right way. @jdm Can you give some advises? --- <!-- 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 - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #18776 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- 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/18888) <!-- Reviewable:end -->
|
|
|
how to see the error log? |
|
Clicking |
|
Where is |
|
That one is a known intermittent failure, so you should ignore it (which is why it wasn't included in the list of |
|
Squash! |
|
@KiChjang Shall we separate |
|
Oh, I misread. I thought they're the same thing... I would still squash, but I don't mind either way. Thanks for your work! @bors-servo r=jdm |
|
|
fix #18776: use XML fragment serialization for innerHTML in XML documents <!-- Please describe your changes on the following line: --> I am not sure whether my solution is in the right way. @jdm Can you give some advises? --- <!-- 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 - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #18776 (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- 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/18888) <!-- Reviewable:end -->
|
|
tigercosmos commentedOct 15, 2017
•
edited by SimonSapin
I am not sure whether my solution is in the right way.
@jdm Can you give some advises?
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is