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 <template> #7531

Merged
merged 6 commits into from Sep 8, 2015
Merged

Implement <template> #7531

merged 6 commits into from Sep 8, 2015

Conversation

@nox
Copy link
Member

nox commented Sep 3, 2015

All tests using iframes can't currently pass, same for innerHTML-related tests with elements. The latter contradicts the spec, see the links below.

Apart from this, they work, AFAICT.

servo/html5ever#164
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27314

Review on Reviewable

@nox
Copy link
Member Author

nox commented Sep 3, 2015

@jdm jdm added the S-fails-tidy label Sep 3, 2015
@nox nox force-pushed the nox:template branch 2 times, most recently from 9547c05 to 4ea53c7 Sep 4, 2015
@nox
Copy link
Member Author

nox commented Sep 4, 2015

I fixed the innerHTML bugs following w3c/DOM-Parsing#1.

@nox
Copy link
Member Author

nox commented Sep 4, 2015

r? @jdm

@nox nox assigned Ms2ger and unassigned jdm Sep 4, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 4, 2015

Reviewed 2 of 2 files at r1, 19 of 19 files at r2, 15 of 15 files at r3, 9 of 12 files at r4.
Review status: 23 of 26 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


components/script/dom/htmltemplateelement.rs, line 57 [r3] (raw file):
I think this is wrong. Consider

var doc1, doc2;
var templateA = doc1.createElement("template");
var templateB = doc1.createElement("template");
doc2.adoptNode(templateA)
assert_equals(templateA.content.ownerDocument, templateB.content.ownerDocument);

and I suspect there's a bug with the temporary document used for fragment parsing too.

Oh, hm, the spec handles this.


components/script/dom/htmltemplateelement.rs, line 72 [r4] (raw file):
Link to ttps://html.spec.whatwg.org/multipage/#template-adopting-steps (which will be added momentarily).


components/script/dom/node.rs, line 1426 [r1] (raw file):
It's really scary to have unsuspecting code running while nodes in the same tree have different node documents.


Comments from the review on Reviewable.io

@nox nox force-pushed the nox:template branch from 4ea53c7 to c4106f0 Sep 4, 2015
@nox nox force-pushed the nox:template branch from c4106f0 to acf733a Sep 6, 2015
@nox
Copy link
Member Author

nox commented Sep 6, 2015

@Ms2ger I put back the merged loops in Node::adopt(), because that's how I specified how adopting steps should be run in whatwg/dom#66. If you really want me to split them, and I don't think it's necessary given there is only one use of them in HTML for template, I would like the spec to be changed to two loops first.

@nox nox force-pushed the nox:template branch from acf733a to d081da9 Sep 6, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 7, 2015

I do really want you to split them.

@nox
Copy link
Member Author

nox commented Sep 7, 2015

@Ms2ger Done and reported upstream. whatwg/dom#71

@nox
Copy link
Member Author

nox commented Sep 7, 2015

@Ms2ger I find that quite pointless though, bind_to_tree and unbind_to_tree do the exact same thing.

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 8, 2015

You didn't split them.

-S-awaiting-review +S-needs-code-changes


Reviewed 3 of 12 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


Comments from the review on Reviewable.io

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

📌 Commit 43975bd has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

Testing commit 43975bd with merge 1537c45...

bors-servo pushed a commit that referenced this pull request Sep 8, 2015
Implement <template>

All tests using iframes can't currently pass, same for innerHTML-related tests with <template> elements. The latter contradicts the spec, see the links below.

Apart from this, they work, AFAICT.

servo/html5ever#164
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27314

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7531)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

💔 Test failed - linux-rel

nox added 4 commits Sep 2, 2015
The failing <img> test comes from the now-correct parsing of <font face> elements
in SVG.
HTMLDocument isn't a thing anymore.
@nox nox force-pushed the nox:template branch from 43975bd to b382004 Sep 8, 2015
@nox
Copy link
Member Author

nox commented Sep 8, 2015

@bors-servo r=Ms2ger

Fixed test expectations.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

📌 Commit b382004 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

Testing commit b382004 with merge 5a0be12...

bors-servo pushed a commit that referenced this pull request Sep 8, 2015
bors-servo
Implement <template>

All tests using iframes can't currently pass, same for innerHTML-related tests with <template> elements. The latter contradicts the spec, see the links below.

Apart from this, they work, AFAICT.

servo/html5ever#164
https://www.w3.org/Bugs/Public/show_bug.cgi?id=27314

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7531)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

💔 Test failed - mac-rel-wpt

@nox
Copy link
Member Author

nox commented Sep 8, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

Previous build results for android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css are reusable. Rebuilding only mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Sep 8, 2015

@bors-servo bors-servo merged commit b382004 into servo:master Sep 8, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nox nox removed the S-tests-failed label Sep 8, 2015
@nox nox deleted the nox:template branch Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.