Relax "parent must be an HTMLElement" restriction in outerHTML setter

https://bugs.webkit.org/show_bug.cgi?id=226808 Reviewed by Ryosuke Niwa. Source/WebCore: Made the following change to our outerHTML setter for better compatibility and to better match the specification [1]: - Stop throwing an exception when the parent is not an HTML element. This new behavior matches the specification, Blink and Gecko behavior. I did not fully align us with the specification because we are mostly aligned with Blink at the moment. In particular: - The specification says the outerHTML setter should be a no-op when the parent is null. Firefox matches the specification but WebKit & Blink throw a NoModificationAllowedError. - The specification says we should allow setting outerHTML if the parent is a DocumentFragment. Firefox allows this but WebKit & Blink throw a NoModificationAllowedError. - WebKit & Blink have some Text node merging logic that is not present in the specification and which Gecko doesn't implement. [1] https://w3c.github.io/DOM-Parsing/#dom-element-outerhtml Test: fast/dom/set-outer-html-special-cases.html * dom/Element.cpp: (WebCore::Element::setOuterHTML): LayoutTests: * fast/dom/set-outer-html-special-cases-expected.txt: Added. * fast/dom/set-outer-html-special-cases.html: Added. Add layout test coverage * fast/dynamic/outerHTML-no-element-expected.txt: Rebaseline test due to different exception message. * platform/mac-wk1/imported/w3c/web-platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt: * platform/mac-wk2/imported/w3c/web-platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt: Rebaseline WPT test. This is actually a progression because we're no longer throwing. However, the test is still failing later on. Canonical link: https://commits.webkit.org/238774@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@278821 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jun 13, 2021 · dc33e39 · dc33e39
1 parent 5b3025b
commit dc33e39
Show file tree

Hide file tree

Showing 10 changed files with 132 additions and 144 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,22 @@
+2021-06-13  Chris Dumez  <cdumez@apple.com>
+
+        Relax "parent must be an HTMLElement" restriction in outerHTML setter
+        https://bugs.webkit.org/show_bug.cgi?id=226808
+
+        Reviewed by Ryosuke Niwa.
+
+        * fast/dom/set-outer-html-special-cases-expected.txt: Added.
+        * fast/dom/set-outer-html-special-cases.html: Added.
+        Add layout test coverage
+
+        * fast/dynamic/outerHTML-no-element-expected.txt:
+        Rebaseline test due to different exception message.
+
+        * platform/mac-wk1/imported/w3c/web-platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt:
+        * platform/mac-wk2/imported/w3c/web-platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt:
+        Rebaseline WPT test. This is actually a progression because we're no longer throwing. However, the test is still failing
+        later on.
+
 2021-06-13  Alan Bujtas  <zalan@apple.com>
 
         [LFC][TFC] Add support for over-constrained cases for available space distribution

diff --git a/LayoutTests/fast/dom/set-outer-html-special-cases-expected.txt b/LayoutTests/fast/dom/set-outer-html-special-cases-expected.txt
@@ -0,0 +1,13 @@
+Tests some special cases of the outerHTML setter.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.getElementById('svgElement').innerHTML is "<g></g>"
+PASS document.documentElement.outerHTML = '' threw exception NoModificationAllowedError: Cannot set outerHTML on element because its parent is not an Element.
+PASS a.parentNode is null
+PASS a.outerHTML = '' threw exception NoModificationAllowedError: Cannot set outerHTML on element because it doesn't have a parent.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/dom/set-outer-html-special-cases.html b/LayoutTests/fast/dom/set-outer-html-special-cases.html
@@ -0,0 +1,27 @@
+<html>
+<head>
+    <script src="../../resources/js-test.js"></script>
+    <script>
+    description("Tests some special cases of the outerHTML setter.");
+    function runTest()
+    {
+        // Parent does not need to be a HTML element.
+        document.getElementById('testSVG').outerHTML = '<g></g>';
+        shouldBeEqualToString("document.getElementById('svgElement').innerHTML", "<g></g>");
+
+        // We should throw a NoModificationAllowedError if the parent is a Document.
+        shouldThrowErrorName("document.documentElement.outerHTML = ''", "NoModificationAllowedError");
+
+        // We currently throw an exception when the parent is null, as does Blink. Gecko and the specification
+        // say this should be a no-op though.
+        a = document.createElement("a");
+        shouldBe("a.parentNode", "null");
+        shouldThrowErrorName("a.outerHTML = ''", "NoModificationAllowedError");
+    }
+    </script>
+</head>
+<body onload="runTest()">
+    <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="svgElement"><symbol id="testSVG"></symbol></svg>
+</body>
+</html>
+
diff --git a/LayoutTests/fast/dynamic/outerHTML-no-element-expected.txt b/LayoutTests/fast/dynamic/outerHTML-no-element-expected.txt
@@ -2,4 +2,4 @@ test for 4110775 Crash will occur when double-clicking outerHTML link on W3 DOM
 
 If the test fails, Safari may crash, or you may see a failure message below. If the test passes, you should see a message with a description of the expected exception.
 
-This test passed - expected error - NoModificationAllowedError: The object can not be modified.
+This test passed - expected error - NoModificationAllowedError: Cannot set outerHTML on element because it doesn't have a parent
diff --git a/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt b/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt
@@ -0,0 +1,34 @@
+
+FAIL Margin properties on the children of menclose assert_approx_equals: inline size expected 56.15625 +/- 1 but got 31.15625
+FAIL Margin properties on the children of merror assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of mfrac assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of mmultiscripts assert_approx_equals: inline size expected 93.1875 +/- 1 but got 43.1875
+FAIL Margin properties on the children of mover assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of mpadded assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of mphantom assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of mroot assert_approx_equals: inline size expected 100.40625 +/- 1 but got 50.40625
+FAIL Margin properties on the children of mrow assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of msqrt assert_approx_equals: inline size expected 59.84375 +/- 1 but got 34.84375
+FAIL Margin properties on the children of mstyle assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of msub assert_approx_equals: inline size expected 93.1875 +/- 1 but got 43.1875
+FAIL Margin properties on the children of msubsup assert_approx_equals: inline size expected 93.1875 +/- 1 but got 43.1875
+FAIL Margin properties on the children of msup assert_approx_equals: inline size expected 93.1875 +/- 1 but got 43.1875
+FAIL Margin properties on the children of munder assert_approx_equals: inline size expected 45 +/- 1 but got 20
+FAIL Margin properties on the children of munderover assert_approx_equals: inline size expected 45 +/- 1 but got 20
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
diff --git a/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt b/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt
diff --git a/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt b/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt
diff --git a/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt b/...platform-tests/mathml/relations/css-styling/padding-border-margin/margin-003-expected.txt
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2021-06-13  Chris Dumez  <cdumez@apple.com>
+
+        Relax "parent must be an HTMLElement" restriction in outerHTML setter
+        https://bugs.webkit.org/show_bug.cgi?id=226808
+
+        Reviewed by Ryosuke Niwa.
+
+        Made the following change to our outerHTML setter for better compatibility and to better
+        match the specification [1]:
+        - Stop throwing an exception when the parent is not an HTML element. This new behavior matches
+          the specification, Blink and Gecko behavior.
+
+        I did not fully align us with the specification because we are mostly aligned with Blink at
+        the moment. In particular:
+        - The specification says the outerHTML setter should be a no-op when the parent is null.
+          Firefox matches the specification but WebKit & Blink throw a NoModificationAllowedError.
+        - The specification says we should allow setting outerHTML if the parent is a DocumentFragment.
+          Firefox allows this but WebKit & Blink throw a NoModificationAllowedError.
+        - WebKit & Blink have some Text node merging logic that is not present in the specification
+          and which Gecko doesn't implement.
+
+        [1] https://w3c.github.io/DOM-Parsing/#dom-element-outerhtml
+
+        Test: fast/dom/set-outer-html-special-cases.html
+
+        * dom/Element.cpp:
+        (WebCore::Element::setOuterHTML):
+
 2021-06-13  Sam Weinig  <weinig@apple.com>
 
         SimulatedXRDevice::shutDownTrackingAndRendering() should clear it's GraphicsContextGL to ensure the resource is cleaned up quickly

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -3248,22 +3248,27 @@ String Element::outerHTML() const
 
 ExceptionOr<void> Element::setOuterHTML(const String& html)
 {
-    auto* parentElement = this->parentElement();
-    if (!is<HTMLElement>(parentElement))
-        return Exception { NoModificationAllowedError };
+    // The specification allows setting outerHTML on an Element whose parent is a DocumentFragment and Gecko supports this.
+    // However, as of June 2021, Blink matches our behavior and throws a NoModificationAllowedError for non-Element parents.
+    RefPtr parent = parentElement();
+    if (UNLIKELY(!parent)) {
+        if (!parentNode())
+            return Exception { NoModificationAllowedError, "Cannot set outerHTML on element because it doesn't have a parent" };
+        return Exception { NoModificationAllowedError, "Cannot set outerHTML on element because its parent is not an Element" };
+    }
 
-    Ref<HTMLElement> parent = downcast<HTMLElement>(*parentElement);
     RefPtr<Node> prev = previousSibling();
     RefPtr<Node> next = nextSibling();
 
-    auto fragment = createFragmentForInnerOuterHTML(parent, html, AllowScriptingContent);
+    auto fragment = createFragmentForInnerOuterHTML(*parent, html, AllowScriptingContent);
     if (fragment.hasException())
         return fragment.releaseException();
 
     auto replaceResult = parent->replaceChild(fragment.releaseReturnValue().get(), *this);
     if (replaceResult.hasException())
         return replaceResult.releaseException();
 
+    // The following is not part of the specification but matches Blink's behavior as of June 2021.
     RefPtr<Node> node = next ? next->previousSibling() : nullptr;
     if (is<Text>(node)) {
         auto result = mergeWithNextTextNode(downcast<Text>(*node));