Implement Range::createContextualFragment

[GuillaumeGomez]

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/element.rs, components/script/dom/webidls/Range.webidl, components/script/dom/range.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[GuillaumeGomez]

r? @nox

[GuillaumeGomez]

This seems incorrect but cannot find another way to get the expected type.

[GuillaumeGomez]

It seems strange that I have to upcast to get back a node in order to use this method.

[GuillaumeGomez]

Fixes #11111.

[highfive]

New code was committed to pull request.

[nox]

Step 3 needs to be rewritten, and there should be tests for this.

-S-awaiting-review +S-needs-code-changes +S-needs-tests +C-needs-test

Previously, highfive wrote…

New code was committed to pull request.

---

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

components/script/dom/range.rs, line 913 [r1] (raw file):

        // Step 2.
        let elem_copy = element.clone().unwrap();

This is wrong to unwrap, given it could be None if node was a document or a document fragment.

---

components/script/dom/range.rs, line 916 [r1] (raw file):

        let document = elem_copy.downcast::<Document>().unwrap();
        let element = if element.is_none() {
            Element::new(Atom::from("body"),

Element::new would create an Element instead of the correct HTMLBodyElement. You want Element::create.

---

components/script/dom/range.rs, line 929 [r1] (raw file):

                             &document)
            } else {
                elem.root_element()

There is no returning the root element in the spec AFAICT.

---

components/script/dom/range.rs, line 932 [r1] (raw file):

Previously, GuillaumeGomez (Guillaume Gomez) wrote…

This seems incorrect but cannot find another way to get the expected type.

So you just need to create a body element in a specific case when a set of conditions are met. You can do:

let should_create_body = element.map_or(true, |elem| {
    elem.local_name == &atom!("html") && elem.html_element_in_html_document()
});
let element = if should_create_body {
    Root::upcast(HTMLBodyElement::new(atom!("body"), None, &node.owner_doc()))
} else {
    element
};

---

components/script/dom/range.rs, line 936 [r1] (raw file):

Previously, GuillaumeGomez (Guillaume Gomez) wrote…

It seems strange that I have to upcast to get back a node in order to use this method.

Nope, that's because parsing a fragment is an operation on `Node`, we do that all the time.

---

components/script/dom/range.rs, line 936 [r1] (raw file):

        // Step 3.
        let fragment_node = element.upcast::<Node>().parse_fragment(fragment);

Please do try!() around the whole call so that fragment_node is actually a Root<Node> and not a Fallible<Root<Node>>, since all the steps haven't been executed yet at this point.

---

components/script/dom/range.rs, line 939 [r1] (raw file):

        // Step 4.
        // can be ignored

Please write a proper sentence as to why it can be ignored.

---

components/script/dom/range.rs, line 942 [r1] (raw file):

        // Step 5.
        fragment_node

Ok(fragment_node)

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[nox]

Just a few nits, but it still needs tests.

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

Previously, highfive wrote…

New code was committed to pull request.

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

components/script/dom/range.rs, line 913 [r3] (raw file):

        // Step 2.
        let should_create_body = element.clone().map_or(true, |elem| {

Do element.as_ref() instead of element.clone().

---

components/script/dom/range.rs, line 917 [r3] (raw file):

            elem.local_name() == &atom!("html") && elem.html_element_in_html_document()
        });
        let element: Root<Node> = if should_create_body || element.is_none() {

You can do it this way:

let element = Root::upcast::<Node>(if should_create_body { ... });

This removes the double call to Root::upcast and the Root<Node type annotation, and also the element.is_none() test that is already covered by element.map_or(true, ...).

---

components/script/dom/range.rs, line 927 [r3] (raw file):

        // Step 4.
        // Can be ignored because at the initialization are supposed to be unmarked as

What are supposed to be unmarked as "already started"? There lack a subject in that sentence.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[nox]

-S-awaiting-review -S-needs-tests

Seems like there are tests already in WPT. Let's try.

@bors-servo try

Previously, highfive wrote…

New code was committed to pull request.

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[bors-servo]

⌛ Trying commit bf8b3d0 with merge 3e724eb...

[bors-servo]

💔 Test failed - mac-rel-wpt

[highfive]

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] Must not throw INVALID_STATE_ERR for a detached node.

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] Simple test with paragraphs

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] Don't auto-create <body> when applied to <html>

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] <html> and <body> must work the same, 1

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] <html> and <body> must work the same, 2

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] Implicit <body> creation

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] null should be stringified

  ▶ Unexpected subtest result in /domparsing/createContextualFragment.html:
  └ PASS [expected FAIL] undefined should be stringified

  ▶ Unexpected subtest result in /old-tests/submission/Opera/script_scheduling/119.html:
  └ PASS [expected FAIL] scheduler: external defer script created with createContextualFragment

[highfive]

New code was committed to pull request.

[bors-servo]

⌛ Trying commit 5ab7f54 with merge fd10ab3...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[nox]

@bors-servo r+

I'll file a followup for the <script> issue.

[bors-servo]

📌 Commit 5ab7f54 has been approved by nox

[bors-servo]

⌛ Testing commit 5ab7f54 with merge 51d41c5...

[bors-servo]

💔 Test failed - linux-rel

[highfive]

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8

[nox]

@bors-servo retry #11561

[bors-servo]

⚡ Previous build results for android, linux-dev, mac-dev-unit, windows are reusable. Rebuilding only arm32, arm64, linux-rel, mac-rel-css, mac-rel-wpt...

[bors-servo]

💔 Test failed - mac-rel-css

[highfive]

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm b8e11894fc87a1b1a2130bc2f522ce5dd20a0b4d
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 9531333297f9339df08ebd5395fdab407f71a7ef
Testing b8e11894fc87a1b1a2130bc2f522ce5dd20a0b4d == 9531333297f9339df08ebd5395fdab407f71a7ef

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm e93c9642827c4e2d15f86f1319d5d92ef443366b
/css-transforms-1_dev/html/reference/transform-blank-ref.htm a4b5a51b35256f1981240981ac8e70bb3f00cde6
Testing e93c9642827c4e2d15f86f1319d5d92ef443366b == a4b5a51b35256f1981240981ac8e70bb3f00cde6

[nox]

@bors-servo retry #11561 #11574

[bors-servo]

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-wpt, windows are reusable. Rebuilding only mac-rel-css...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows