Improve paragraph wrapping and DOM implementation

This change improves the logic for wrapping phrasing content in
paragraphs, fixing two bugs:

1. Trailing whitespace was not always trimmed correctly from phrasing
   content at the end of a container <div>.
2. Leading whitespace nodes could be left behind as direct children of
   the <div> instead of being discarded.

The logic in Readability.js has been refactored to use a more robust
"collect and transform" pattern. It now uses a DocumentFragment to
gather all consecutive phrasing content, correctly trims leading /
trailing whitespace, and then wraps the non-empty result in a <p> tag.
This produces cleaner HTML, as reflected in the updated test pages.

To support this, several enhancements were made to the JSDOMParser.js
DOM implementation:
* Added support for DocumentFragment, including
  doc.createDocumentFragment().
* Centralized DOM insertion logic (appendChild(), insertBefore(),
  and replaceChild()) into a single, efficient _insertNodesAtIndex()
  private helper, ensuring consistency.
* Made replaceChild() more robust by simplifying its implementation
  and fixing an edge case where replacing a node with itself failed.
* Fixed a bug in remove() where it incorrectly modified
  element-specific properties on non-element nodes.

The test suite in test-jsdomparser.js was expanded to validate
these improvements, with new tests for DocumentFragment handling,
node moving, and self-insertion/replacement edge cases.

Author: samuelhuang

JSDOMParser.js

@@ -347,26 +347,129 @@
       return this.children[this.children.length - 1] || null;
     },
 
-    appendChild(child) {
-      if (child.parentNode) {
-        child.remove();
+    /**
+     * The workhorse for all node insertion operations. The public methods
+     * (`appendChild()`, `insertBefore()`, `replaceChild()`) are thin wrappers
+     * around this.
+     *
+     * @private
+     * @param {Node[]} nodes - An array of nodes to insert. It is assumed that
+     *   these nodes are distinct, and are not children of this object.
+     * @param {Number} index - A valid index to insert `nodes` at, or -1 to
+     *   indicate insertion as the last children.
+     * @returns {void}
+     */
+    _insertNodesAtIndex(nodes, index) {
+      if (!nodes.length) {
+        return;
+      }
+
+      // Detach nodes from their previous parents.
+      for (var i = 0; i < nodes.length; i++) {
+        if (nodes[i].parentNode) {
+          nodes[i].remove();
+        }
       }
 
-      var last = this.lastChild;
-      if (last) {
-        last.nextSibling = child;
+      var afterSibling = index === -1 ? null : this.childNodes[index];
+
+      // Store the previous sibling before we modify the DOM.
+      var prevSibling = afterSibling
+        ? afterSibling.previousSibling
+        : this.lastChild;
+
+      // Insert nodes into childNodes.
+      var insertionPoint = index === -1 ? this.childNodes.length : index;
+      Array.prototype.splice.apply(
+        this.childNodes,
+        [insertionPoint, 0].concat(nodes)
+      );
+
+      // Update parentNode and sibling pointers for the new nodes.
+      for (var j = 0; j < nodes.length; j++) {
+        var node = nodes[j];
+        node.parentNode = this;
+        node.previousSibling = prevSibling;
+        if (prevSibling) {
+          prevSibling.nextSibling = node;
+        }
+        prevSibling = node;
+      }
+      var lastInsertedNode = nodes[nodes.length - 1];
+      lastInsertedNode.nextSibling = afterSibling;
+      if (afterSibling) {
+        afterSibling.previousSibling = lastInsertedNode;
       }
-      child.previousSibling = last;
-
-      if (child.nodeType === Node.ELEMENT_NODE) {
-        child.previousElementSibling =
-          this.children[this.children.length - 1] || null;
-        this.children.push(child);
-        child.previousElementSibling &&
-          (child.previousElementSibling.nextElementSibling = child);
+
+      // Filter for element nodes and update children array and pointers.
+      var elementsToInsert = [];
+      for (var k = 0; k < nodes.length; k++) {
+        if (nodes[k].nodeType === Node.ELEMENT_NODE) {
+          elementsToInsert.push(nodes[k]);
+        }
       }
-      this.childNodes.push(child);
-      child.parentNode = this;
+
+      if (elementsToInsert.length) {
+        // Find the next element sibling to use as an insertion reference.
+        // This is done after `childNodes` is modified, as the forward
+        // traversal from `afterSibling` remains valid.
+        var afterElem = afterSibling;
+        while (afterElem && afterElem.nodeType !== Node.ELEMENT_NODE) {
+          afterElem = afterElem.nextSibling;
+        }
+
+        // Store the previous element sibling before more DOM modifications.
+        var prevElem = afterElem
+          ? afterElem.previousElementSibling
+          : this.lastElementChild;
+
+        var afterElemIndex = afterElem ? this.children.indexOf(afterElem) : -1;
+        var elemInsertionPoint =
+          afterElemIndex === -1 ? this.children.length : afterElemIndex;
+        Array.prototype.splice.apply(
+          this.children,
+          [elemInsertionPoint, 0].concat(elementsToInsert)
+        );
+
+        for (var l = 0; l < elementsToInsert.length; l++) {
+          var elem = elementsToInsert[l];
+          elem.previousElementSibling = prevElem;
+          if (prevElem) {
+            prevElem.nextElementSibling = elem;
+          }
+          prevElem = elem;
+        }
+        var lastInsertedElem = elementsToInsert[elementsToInsert.length - 1];
+        lastInsertedElem.nextElementSibling = afterElem;
+        if (afterElem) {
+          afterElem.previousElementSibling = lastInsertedElem;
+        }
+      }
+    },
+
+    appendChild(child) {
+      var nodes =
+        child.nodeType === Node.DOCUMENT_FRAGMENT_NODE
+          ? Array.from(child.childNodes)
+          : [child];
+      this._insertNodesAtIndex(nodes, -1);
+      return child;
+    },
+
+    insertBefore(newNode, referenceNode) {
+      if (newNode === referenceNode) {
+        return newNode;
+      }
+      var nodes =
+        newNode.nodeType === Node.DOCUMENT_FRAGMENT_NODE
+          ? Array.from(newNode.childNodes)
+          : [newNode];
+      var index = referenceNode ? this.childNodes.indexOf(referenceNode) : -1;
+      if (referenceNode && index === -1) {
+        throw new Error("insertBefore: reference node not found");
+      }
+      this._insertNodesAtIndex(nodes, index);
+      return newNode;
     },
 
     remove() {
@@ -392,19 +495,19 @@
       childNodes.splice(childIndex, 1);
 
       if (this.nodeType === Node.ELEMENT_NODE) {
-        prev = this.previousElementSibling;
-        next = this.nextElementSibling;
-        if (prev) {
-          prev.nextElementSibling = next;
+        var prevElem = this.previousElementSibling;
+        var nextElem = this.nextElementSibling;
+        if (prevElem) {
+          prevElem.nextElementSibling = nextElem;
         }
-        if (next) {
-          next.previousElementSibling = prev;
+        if (nextElem) {
+          nextElem.previousElementSibling = prevElem;
         }
         parent.children.splice(parent.children.indexOf(this), 1);
+        this.previousElementSibling = this.nextElementSibling = null;
       }
 
       this.previousSibling = this.nextSibling = null;
-      this.previousElementSibling = this.nextElementSibling = null;
 
       return this;
     },
@@ -414,110 +517,19 @@
     },
 
     replaceChild(newNode, oldNode) {
-      var childNodes = this.childNodes;
-      var childIndex = childNodes.indexOf(oldNode);
-      if (childIndex === -1) {
-        throw new Error("replaceChild: node not found");
-      } else {
-        // This will take care of updating the new node if it was somewhere else before:
-        if (newNode.parentNode) {
-          newNode.remove();
-        }
-
-        childNodes[childIndex] = newNode;
-
-        // update the new node's sibling properties, and its new siblings' sibling properties
-        newNode.nextSibling = oldNode.nextSibling;
-        newNode.previousSibling = oldNode.previousSibling;
-        if (newNode.nextSibling) {
-          newNode.nextSibling.previousSibling = newNode;
-        }
-        if (newNode.previousSibling) {
-          newNode.previousSibling.nextSibling = newNode;
-        }
-
-        newNode.parentNode = this;
-
-        // Now deal with elements before we clear out those values for the old node,
-        // because it can help us take shortcuts here:
-        if (newNode.nodeType === Node.ELEMENT_NODE) {
-          if (oldNode.nodeType === Node.ELEMENT_NODE) {
-            // Both were elements, which makes this easier, we just swap things out:
-            newNode.previousElementSibling = oldNode.previousElementSibling;
-            newNode.nextElementSibling = oldNode.nextElementSibling;
-            if (newNode.previousElementSibling) {
-              newNode.previousElementSibling.nextElementSibling = newNode;
-            }
-            if (newNode.nextElementSibling) {
-              newNode.nextElementSibling.previousElementSibling = newNode;
-            }
-            this.children[this.children.indexOf(oldNode)] = newNode;
-          } else {
-            // Hard way:
-            newNode.previousElementSibling = (function () {
-              for (var i = childIndex - 1; i >= 0; i--) {
-                if (childNodes[i].nodeType === Node.ELEMENT_NODE) {
-                  return childNodes[i];
-                }
-              }
-              return null;
-            })();
-            if (newNode.previousElementSibling) {
-              newNode.nextElementSibling =
-                newNode.previousElementSibling.nextElementSibling;
-            } else {
-              newNode.nextElementSibling = (function () {
-                for (var i = childIndex + 1; i < childNodes.length; i++) {
-                  if (childNodes[i].nodeType === Node.ELEMENT_NODE) {
-                    return childNodes[i];
-                  }
-                }
-                return null;
-              })();
-            }
-            if (newNode.previousElementSibling) {
-              newNode.previousElementSibling.nextElementSibling = newNode;
-            }
-            if (newNode.nextElementSibling) {
-              newNode.nextElementSibling.previousElementSibling = newNode;
-            }
-
-            if (newNode.nextElementSibling) {
-              this.children.splice(
-                this.children.indexOf(newNode.nextElementSibling),
-                0,
-                newNode
-              );
-            } else {
-              this.children.push(newNode);
-            }
-          }
-        } else if (oldNode.nodeType === Node.ELEMENT_NODE) {
-          // new node is not an element node.
-          // if the old one was, update its element siblings:
-          if (oldNode.previousElementSibling) {
-            oldNode.previousElementSibling.nextElementSibling =
-              oldNode.nextElementSibling;
-          }
-          if (oldNode.nextElementSibling) {
-            oldNode.nextElementSibling.previousElementSibling =
-              oldNode.previousElementSibling;
-          }
-          this.children.splice(this.children.indexOf(oldNode), 1);
-
-          // If the old node wasn't an element, neither the new nor the old node was an element,
-          // and the children array and its members shouldn't need any updating.
-        }
-
-        oldNode.parentNode = null;
-        oldNode.previousSibling = null;
-        oldNode.nextSibling = null;
-        if (oldNode.nodeType === Node.ELEMENT_NODE) {
-          oldNode.previousElementSibling = null;
-          oldNode.nextElementSibling = null;
-        }
+      if (newNode === oldNode) {
         return oldNode;
       }
+      if (oldNode.parentNode !== this) {
+        throw new Error(
+          "replaceChild: node to be replaced is not a child of this node"
+        );
+      }
+      // Insert the new node(s) before the node to be replaced.
+      this.insertBefore(newNode, oldNode);
+      // Now, remove the old node.
+      oldNode.remove();
+      return oldNode;
     },
 
     __JSDOMParser__: true,
@@ -559,6 +571,17 @@
     nodeType: Node.COMMENT_NODE,
   };
 
+  var DocumentFragment = function () {
+    this.childNodes = [];
+    this.children = [];
+  };
+
+  DocumentFragment.prototype = {
+    __proto__: Node.prototype,
+    nodeName: "#document-fragment",
+    nodeType: Node.DOCUMENT_FRAGMENT_NODE,
+  };
+
   var Text = function () {
     this.childNodes = [];
   };
@@ -635,6 +658,10 @@
       return node;
     },
 
+    createDocumentFragment() {
+      return new DocumentFragment();
+    },
+
     get baseURI() {
       if (!this.hasOwnProperty("_baseURI")) {
         this._baseURI = this.documentURI;
@@ -1264,6 +1291,7 @@
   global.Node = Node;
   global.Comment = Comment;
   global.Document = Document;
+  global.DocumentFragment = DocumentFragment;
   global.Element = Element;
   global.Text = Text;
 

Readability.js

@@ -1174,23 +1174,39 @@ Readability.prototype = {
         // Turn all divs that don't have children block level elements into p's
         if (node.tagName === "DIV") {
           // Put phrasing content into paragraphs.
-          var p = null;
           var childNode = node.firstChild;
           while (childNode) {
             var nextSibling = childNode.nextSibling;
             if (this._isPhrasingContent(childNode)) {
-              if (p !== null) {
-                p.appendChild(childNode);
-              } else if (!this._isWhitespace(childNode)) {
-                p = doc.createElement("p");
-                node.replaceChild(p, childNode);
-                p.appendChild(childNode);
+              var fragment = doc.createDocumentFragment();
+              // Collect all consecutive phrasing content into a fragment.
+              do {
+                nextSibling = childNode.nextSibling;
+                fragment.appendChild(childNode);
+                childNode = nextSibling;
+              } while (childNode && this._isPhrasingContent(childNode));
+
+              // Trim leading and trailing whitespace from the fragment.
+              while (
+                fragment.firstChild &&
+                this._isWhitespace(fragment.firstChild)
+              ) {
+                fragment.firstChild.remove();
               }
-            } else if (p !== null) {
-              while (p.lastChild && this._isWhitespace(p.lastChild)) {
-                p.lastChild.remove();
+              while (
+                fragment.lastChild &&
+                this._isWhitespace(fragment.lastChild)
+              ) {
+                fragment.lastChild.remove();
+              }
+
+              // If the fragment contains anything, wrap it in a paragraph and
+              // insert it before the next non-phrasing node.
+              if (fragment.firstChild) {
+                var p = doc.createElement("p");
+                p.appendChild(fragment);
+                node.insertBefore(p, nextSibling);
               }
-              p = null;
             }
             childNode = nextSibling;
           }