Fix rare case where string was inserted when list was removed

Closure Team · copybara-github · commit e69cdb6acdf1 · 2022-08-03T11:42:24.000-07:00
RELNOTES: Fix rare case where string was inserted when list was removed

PiperOrigin-RevId: 465107556
Change-Id: I7eb6f3748b2a01766a36f9b41f93ca6ed785942b
diff --git a/closure/goog/editor/plugins/basictextformatter.js b/closure/goog/editor/plugins/basictextformatter.js
@@ -365,15 +365,31 @@ goog.editor.plugins.BasicTextFormatter.prototype.execCommandInternal = function(
 
       if (hasPlaceholderContent && placeholderValue) {
         // Unfortunately, the browser sometimes removes the element we added and
-        // creates a new element with the same content, so we can't add an
+        // creates a new element with the same content or appends the text to
+        // an existing text node, so we can't add an
         // id/class to the placeholder node and rely on that to find/delete the
         // content, and instead have to manually search for the content.
-        const placeholderEl = goog.dom.findNode(
+        // Example:
+        //<ol>
+        //   <li><span>goog_12345</span>abc</li>
+        //   <li>def</li>
+        // </ol>
+        // can become
+        // goog_12345abc<br>
+        // def<br>
+        // after execCommand is called
+        const POTENTIAL_PLACEHOLDER_TAGS = [goog.dom.TagName.SPAN, '#text'];
+        const placeholderNode = goog.dom.findNode(
             this.getFieldObject().getElement(),
-            el => el.nodeName == goog.dom.TagName.SPAN &&
-                el.textContent === placeholderValue);
-        if (placeholderEl) {
-          goog.dom.removeNode(placeholderEl);
+            node => POTENTIAL_PLACEHOLDER_TAGS.includes(node.nodeName) &&
+                node.textContent.includes(placeholderValue));
+        if (placeholderNode) {
+          if (placeholderNode.textContent === placeholderValue) {
+            goog.dom.removeNode(placeholderNode);
+          } else {
+            placeholderNode.textContent =
+                placeholderNode.textContent.replaceAll(placeholderValue, '');
+          }
         }
       }
 
diff --git a/closure/goog/editor/plugins/basictextformatter_test.js b/closure/goog/editor/plugins/basictextformatter_test.js
@@ -602,7 +602,31 @@ testSuite({
     assertEquals(String(TagName.OL), list.tagName);
     assertEquals(
         3, dom.getElementsByTagNameAndClass(TagName.LI, null, list).length);
+    tearDownListAndBlockquoteTests();
+  },
+
+  /** @suppress {visibility} suppression added to enable type checking */
+  testAddAndRemoveList_placeholderRemoved() {
+    if (!userAgent.WEBKIT) {
+      return;
+    }
+    // Test that we're not seeing https://bugs.webkit.org/show_bug.cgi?id=19539,
+    // the type of multi-item lists.
+    setUpListAndBlockquoteTests();
+
+    FIELDMOCK.$replay();
+    let parent = dom.getElement('addAndRemoveList');
 
+    Range.createFromNodeContents(parent).select();
+    // Add the ordered list
+    FORMATTER.execCommandInternal(BasicTextFormatter.COMMAND.ORDERED_LIST);
+    assertEquals(
+        3, dom.getElementsByTagNameAndClass(TagName.LI, null, parent).length);
+    FORMATTER.execCommandInternal(BasicTextFormatter.COMMAND.ORDERED_LIST);
+    assertEquals(
+        0, dom.getElementsByTagNameAndClass(TagName.LI, null, parent).length);
+    // Assert that no placeholder is left behind
+    assertFalse(parent.textContent.includes('goog'));
     tearDownListAndBlockquoteTests();
   },
 
diff --git a/closure/goog/editor/plugins/basictextformatter_test_dom.html b/closure/goog/editor/plugins/basictextformatter_test_dom.html
@@ -58,6 +58,12 @@
     </ul>
   </div>
 
+  <div id="addAndRemoveList">
+    aaa
+    <div>bbb</div>
+    <div>ccc</div>
+  </div
+
   <p>before <span id="toQuote">Foo. Bar, baz.</span> after</p>
   <p>before <div id="toQuote2">Foo.<p/>Bar,<p/>baz.</div> after</p>
 
