Bug 1297633 - Fixed editing of SVG elements in Inspector. r=jdescottes
authorSebastian Zartner <sebastianzartner@gmail.com>
Wed, 10 Feb 2021 20:45:44 +0000
changeset 566877 cfaa6bb3bb55e17d36abead05b42ca1c4b0c4a7c
parent 566876 423a59653735977cbcac4faac5cfe66d541b9113
child 566878 e9bf083da681d349998b7899236fd95c0ae7e1d0
push id38191
push userbtara@mozilla.com
push dateThu, 11 Feb 2021 05:02:45 +0000
treeherdermozilla-central@5cbcb80f72bd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes
bugs1297633
milestone87.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1297633 - Fixed editing of SVG elements in Inspector. r=jdescottes Differential Revision: https://phabricator.services.mozilla.com/D102844
devtools/client/inspector/markup/markup.js
devtools/client/inspector/markup/test/browser.ini
devtools/client/inspector/markup/test/browser_markup_html_edit_04.js
devtools/server/actors/inspector/walker.js
--- a/devtools/client/inspector/markup/markup.js
+++ b/devtools/client/inspector/markup/markup.js
@@ -1762,48 +1762,50 @@ MarkupView.prototype = {
    * newly inserted node gets selected instead of the one that just got removed.
    */
   reselectOnRemoved: function(removedNode, reason) {
     // Only allow one removed node reselection at a time, so that when there are
     // more than 1 request in parallel, the last one wins.
     this.cancelReselectOnRemoved();
 
     // Get the removedNode index in its parent node to reselect the right node.
-    const isHTMLTag = removedNode.tagName.toLowerCase() === "html";
+    const isRootElement = ["html", "svg"].includes(
+      removedNode.tagName.toLowerCase()
+    );
     const oldContainer = this.getContainer(removedNode);
     const parentContainer = this.getContainer(removedNode.parentNode());
     const childIndex = parentContainer
       .getChildContainers()
       .indexOf(oldContainer);
 
     const onMutations = (this._removedNodeObserver = mutations => {
       let isNodeRemovalMutation = false;
       for (const mutation of mutations) {
         const containsRemovedNode =
           mutation.removed && mutation.removed.some(n => n === removedNode);
         if (
           mutation.type === "childList" &&
-          (containsRemovedNode || isHTMLTag)
+          (containsRemovedNode || isRootElement)
         ) {
           isNodeRemovalMutation = true;
           break;
         }
       }
       if (!isNodeRemovalMutation) {
         return;
       }
 
       this.inspector.off("markupmutation", onMutations);
       this._removedNodeObserver = null;
 
       // Don't select the new node if the user has already changed the current
       // selection.
       if (
         this.inspector.selection.nodeFront === parentContainer.node ||
-        (this.inspector.selection.nodeFront === removedNode && isHTMLTag)
+        (this.inspector.selection.nodeFront === removedNode && isRootElement)
       ) {
         const childContainers = parentContainer.getChildContainers();
         if (childContainers?.[childIndex]) {
           const childContainer = childContainers[childIndex];
           this._markContainerAsSelected(childContainer, reason);
           if (childContainer.hasChildren) {
             this.expandNode(childContainer.node);
           }
--- a/devtools/client/inspector/markup/test/browser.ini
+++ b/devtools/client/inspector/markup/test/browser.ini
@@ -161,16 +161,17 @@ skip-if = (os == 'linux' && bits == 32 &
 [browser_markup_links_05.js]
 [browser_markup_links_06.js]
 [browser_markup_links_07.js]
 [browser_markup_load_01.js]
 skip-if = verify
 [browser_markup_html_edit_01.js]
 [browser_markup_html_edit_02.js]
 [browser_markup_html_edit_03.js]
+[browser_markup_html_edit_04.js]
 [browser_markup_html_edit_undo-redo.js]
 [browser_markup_image_tooltip.js]
 [browser_markup_image_tooltip_mutations.js]
 [browser_markup_keybindings_01.js]
 [browser_markup_keybindings_02.js]
 [browser_markup_keybindings_03.js]
 [browser_markup_keybindings_04.js]
 [browser_markup_keybindings_delete_attributes.js]
new file mode 100644
--- /dev/null
+++ b/devtools/client/inspector/markup/test/browser_markup_html_edit_04.js
@@ -0,0 +1,97 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+"use strict";
+
+// Test that outerHTML editing keybindings work as expected and that the <svg>
+// root element can be edited correctly.
+
+const TEST_URL =
+  "data:image/svg+xml," +
+  '<svg width="100" height="100" xmlns="http://www.w3.org/2000/svg">' +
+  '<circle cx="50" cy="50" r="50"/>' +
+  "</svg>";
+
+requestLongerTimeout(2);
+
+add_task(async function() {
+  const { inspector, testActor } = await openInspectorForURL(TEST_URL);
+
+  inspector.markup._frame.focus();
+
+  info("Check that editing the <svg> element works like other nodes");
+  await testDocumentElement(inspector, testActor);
+
+  info("Check (again) that editing the <svg> element works like other nodes");
+  await testDocumentElement2(inspector, testActor);
+});
+
+async function testDocumentElement(inspector, testActor) {
+  const currentDocElementOuterHTML = await testActor.eval(
+    "document.documentElement.outerHTML"
+  );
+  const docElementSVG =
+    '<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg">' +
+    '<rect x="10" y="10" width="180" height="180"/>' +
+    "</svg>";
+  const docElementFront = await inspector.markup.walker.documentElement();
+
+  const onReselected = inspector.markup.once("reselectedonremoved");
+  await inspector.markup.updateNodeOuterHTML(
+    docElementFront,
+    docElementSVG,
+    currentDocElementOuterHTML
+  );
+  await onReselected;
+
+  is(
+    await testActor.getAttribute("svg", "width"),
+    "200",
+    "<svg> width has been updated"
+  );
+  is(
+    await testActor.getAttribute("svg", "height"),
+    "200",
+    "<svg> height has been updated"
+  );
+  is(
+    await testActor.getProperty("svg", "outerHTML"),
+    docElementSVG,
+    "<svg> markup has been updated"
+  );
+}
+
+async function testDocumentElement2(inspector, testActor) {
+  const currentDocElementOuterHTML = await testActor.eval(
+    "document.documentElement.outerHTML"
+  );
+  const docElementSVG =
+    '<svg width="300" height="300" xmlns="http://www.w3.org/2000/svg">' +
+    '<ellipse cx="150" cy="150" rx="150" ry="100"/>' +
+    "</svg>";
+  const docElementFront = await inspector.markup.walker.documentElement();
+
+  const onReselected = inspector.markup.once("reselectedonremoved");
+  inspector.markup.updateNodeOuterHTML(
+    docElementFront,
+    docElementSVG,
+    currentDocElementOuterHTML
+  );
+  await onReselected;
+
+  is(
+    await testActor.getAttribute("svg", "width"),
+    "300",
+    "<svg> width has been updated"
+  );
+  is(
+    await testActor.getAttribute("svg", "height"),
+    "300",
+    "<svg> height has been updated"
+  );
+  is(
+    await testActor.getProperty("svg", "outerHTML"),
+    docElementSVG,
+    "<svg> markup has been updated"
+  );
+}
--- a/devtools/server/actors/inspector/walker.js
+++ b/devtools/server/actors/inspector/walker.js
@@ -1703,17 +1703,19 @@ var WalkerActor = protocol.ActorClassWit
       throw new Error("The window object shouldn't be null");
     } else {
       // We create DOMParser under window object because we want a content
       // DOMParser, which means all the DOM objects created by this DOMParser
       // will be in the same DocGroup as rawNode.parentNode. Then the newly
       // created nodes can be adopted into rawNode.parentNode.
       parser = new win.DOMParser();
     }
-    const parsedDOM = parser.parseFromString(value, "text/html");
+
+    const mimeType = rawNode.tagName === "svg" ? "image/svg+xml" : "text/html";
+    const parsedDOM = parser.parseFromString(value, mimeType);
     const parentNode = rawNode.parentNode;
 
     // Special case for head and body.  Setting document.body.outerHTML
     // creates an extra <head> tag, and document.head.outerHTML creates
     // an extra <body>.  So instead we will call replaceChild with the
     // parsed DOM, assuming that they aren't trying to set both tags at once.
     if (rawNode.tagName === "BODY") {
       if (parsedDOM.head.innerHTML === "") {
@@ -1725,35 +1727,35 @@ var WalkerActor = protocol.ActorClassWit
     } else if (rawNode.tagName === "HEAD") {
       if (parsedDOM.body.innerHTML === "") {
         parentNode.replaceChild(parsedDOM.head, rawNode);
       } else {
         // eslint-disable-next-line no-unsanitized/property
         rawNode.outerHTML = value;
       }
     } else if (node.isDocumentElement()) {
-      // Unable to set outerHTML on the document element.  Fall back by
-      // setting attributes manually, then replace the body and head elements.
+      // Unable to set outerHTML on the document element. Fall back by
+      // setting attributes manually. Then replace all the child nodes.
       const finalAttributeModifications = [];
       const attributeModifications = {};
       for (const attribute of rawNode.attributes) {
         attributeModifications[attribute.name] = null;
       }
       for (const attribute of parsedDOM.documentElement.attributes) {
         attributeModifications[attribute.name] = attribute.value;
       }
       for (const key in attributeModifications) {
         finalAttributeModifications.push({
           attributeName: key,
           newValue: attributeModifications[key],
         });
       }
       node.modifyAttributes(finalAttributeModifications);
-      rawNode.replaceChild(parsedDOM.head, rawNode.querySelector("head"));
-      rawNode.replaceChild(parsedDOM.body, rawNode.querySelector("body"));
+
+      rawNode.replaceChildren(...parsedDOM.firstElementChild.childNodes);
     } else {
       // eslint-disable-next-line no-unsanitized/property
       rawNode.outerHTML = value;
     }
   },
 
   /**
    * Insert adjacent HTML to a node.