Implement Range#insertNode

[dzbarsky]

Gecko doesn't really follow the spec but it seems to throw a HierarchyRequest error when parent is null.
Any ideas who I should talk to about fixing the spec to account for the null checks?

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @kmcallister (or someone else) soon.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/5482

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[Ms2ger]

Reviewed 1 of 4 files at r1, 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

---

^{components/script/dom/range.rs, line 307 [r3] (raw file):}
Checking the wrong node here!

---

^{components/script/dom/range.rs, line 319 [r3] (raw file):}
If you merge step 1 into this as well, you can make this

let parent = match start_node.GetParentNode() {
    Some(parent) => parent,
    None => return Err(HierarchyRequest),
};

---

^{components/script/dom/range.rs, line 326 [r3] (raw file):}
What's the parent of one of start_node's children?

---

^{components/script/dom/range.rs, line 334 [r3] (raw file):}
This can't happen if you get the first step right :)

---

^{components/script/dom/range.rs, line 350 [r3] (raw file):}
Note that you're supposed to use the new reference_node here. However, I don't think this ever changesparent. Poking annevk.

---

^{components/script/dom/range.rs, line 357 [r3] (raw file):}
node.GetNextSibling()

---

^{components/script/dom/range.rs, line 363 [r3] (raw file):}
node.remove_self();

---

^{components/script/dom/range.rs, line 369 [r3] (raw file):}
Unreachable

---

^{components/script/dom/range.rs, line 391 [r3] (raw file):}
"set range’s end to (parent, …"

---

Comments from the review on Reviewable.io

[dzbarsky]

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

---

^{components/script/dom/range.rs, line 307 [r3] (raw file):}
Good catch!

---

^{components/script/dom/range.rs, line 319 [r3] (raw file):}
It's cleaner this way but the steps are out of order. Maybe we can change the spec to move " or a Text node whose parent is null" from step 1 to step 3?

---

^{components/script/dom/range.rs, line 334 [r3] (raw file):}
Yep :)

---

^{components/script/dom/range.rs, line 350 [r3] (raw file):}
I haven't looked at https://dom.spec.whatwg.org/#concept-Text-split too closely, but it seems like step 7.2 could maybe change start_node?

---

^{components/script/dom/range.rs, line 357 [r3] (raw file):}
True

---

^{components/script/dom/range.rs, line 391 [r3] (raw file):}
Man, I read this so many times!

---

Comments from the review on Reviewable.io

[Ms2ger]

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

---

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

---

^{components/script/dom/range.rs, line 319 [r3] (raw file):}
I think this way is fine.

---

^{components/script/dom/range.rs, line 350 [r3] (raw file):}
Perhaps, but I don't see how that's related. You're still getting the parent from the old reference_node rather than the new one (which is split_text), and which will always be the same as parent.

---

^{tests/wpt/metadata/dom/ranges/Range-insertNode.html.ini, line 3 [r5] (raw file):}
Have you investigated why those still fail?

---

Comments from the review on Reviewable.io

[dzbarsky]

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{tests/wpt/metadata/dom/ranges/Range-insertNode.html.ini, line 3 [r5] (raw file):}
Possibly because we haven't implemented the range updating steps in splitText and Node.insert

---

Comments from the review on Reviewable.io

[Ms2ger]

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

^{components/script/dom/range.rs, line 341 [r6] (raw file):}
new_parent is still parent, so you don't need to redefine it; feel free to add an assert!() too.

---

Comments from the review on Reviewable.io

[Ms2ger]

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

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from the review on Reviewable.io

[Ms2ger]

@bors-servo r+

[bors-servo]

📌 Commit 982855c has been approved by Ms2ger

[bors-servo]

⌛ Testing commit 982855c with merge 0842960...

[bors-servo]

💔 Test failed - mac1

[dzbarsky]

@bors-servo retry

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit c2664e5 has been approved by jdm

[bors-servo]

⌛ Testing commit c2664e5 with merge acf47a0...

[bors-servo]

💔 Test failed - mac1

[jdm]

@bors-servo: retry

• #6644

[bors-servo]

⚡ Previous build results for android, gonk, linux1, linux2, linux3, mac2, mac3 are reusable. Rebuilding only mac1...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3