Changeset 96553 in webkit

Timestamp:

Oct 3, 2011 4:05:39 PM (13 years ago)

Author:

rniwa@webkit.org

Message:

Replace m_firstNodeInserted and m_lastLeafInserted in ReplaceSelectionCommand by positions
​https://bugs.webkit.org/show_bug.cgi?id=68874

Reviewed by Enrica Casucci.

Replaced m_firstNodeInserted and m_lastLeafInserted by m_startOfInsertedContent and m_endOfInsertedContent
respectively. Also removed removeNodePreservingChildren and removeNodeAndPruneAncestors in ReplaceSelectionCommand
because they were not virtual in CompositeEditCommand and implicitly overriding the functions was confusing.
Since each of these two functions is used at exactly one place, just update positions and insertedNodes explicitly.

• editing/CompositeEditCommand.cpp:
• editing/ReplaceSelectionCommand.cpp:

(WebCore::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds):
(WebCore::ReplaceSelectionCommand::positionAtEndOfInsertedContent):
(WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent):
(WebCore::ReplaceSelectionCommand::handleStyleSpans):
(WebCore::ReplaceSelectionCommand::mergeEndIfNeeded): Update m_endOfInsertedContent by endingSelection().visibleEnd()
instead of m_lastLeafInserted with destination.previous() because moveParagraph could have removed leading whitespace in
the text node referenced by destination. This is tested by an existing layout test.
(WebCore::ReplaceSelectionCommand::doApply):
(WebCore::ReplaceSelectionCommand::addSpacesForSmartReplace): Update positions as needed. All changes are tested
by the existing layout tests in editing/pasteboard.
(WebCore::ReplaceSelectionCommand::updateNodesInserted):

• editing/ReplaceSelectionCommand.h:
• editing/htmlediting.cpp:

(WebCore::hasARenderedDescendant): Moved from CompositeEditCommand.cpp.
(WebCore::highestNodeToRemoveInPruning): Ditto.

• editing/htmlediting.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• editing/CompositeEditCommand.cpp (modified) (1 diff)
• editing/ReplaceSelectionCommand.cpp (modified) (15 diffs)
• editing/ReplaceSelectionCommand.h (modified) (2 diffs)
• editing/htmlediting.cpp (modified) (1 diff)
• editing/htmlediting.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-10-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Replace m_firstNodeInserted and m_lastLeafInserted in ReplaceSelectionCommand by positions
+        https://bugs.webkit.org/show_bug.cgi?id=68874
+
+        Reviewed by Enrica Casucci.
+
+        Replaced m_firstNodeInserted and m_lastLeafInserted by m_startOfInsertedContent and m_endOfInsertedContent
+        respectively. Also removed removeNodePreservingChildren and removeNodeAndPruneAncestors in ReplaceSelectionCommand
+        because they were not virtual in CompositeEditCommand and implicitly overriding the functions was confusing.
+        Since each of these two functions is used at exactly one place, just update positions and insertedNodes explicitly.
+
+        * editing/CompositeEditCommand.cpp:
+        * editing/ReplaceSelectionCommand.cpp:
+        (WebCore::ReplaceSelectionCommand::removeUnrenderedTextNodesAtEnds):
+        (WebCore::ReplaceSelectionCommand::positionAtEndOfInsertedContent):
+        (WebCore::ReplaceSelectionCommand::positionAtStartOfInsertedContent):
+        (WebCore::ReplaceSelectionCommand::handleStyleSpans):
+        (WebCore::ReplaceSelectionCommand::mergeEndIfNeeded): Update m_endOfInsertedContent by endingSelection().visibleEnd()
+        instead of m_lastLeafInserted with destination.previous() because moveParagraph could have removed leading whitespace in
+        the text node referenced by destination. This is tested by an existing layout test.
+        (WebCore::ReplaceSelectionCommand::doApply):
+        (WebCore::ReplaceSelectionCommand::addSpacesForSmartReplace): Update positions as needed. All changes are tested
+        by the existing layout tests in editing/pasteboard.
+        (WebCore::ReplaceSelectionCommand::updateNodesInserted):
+        * editing/ReplaceSelectionCommand.h:
+        * editing/htmlediting.cpp:
+        (WebCore::hasARenderedDescendant): Moved from CompositeEditCommand.cpp.
+        (WebCore::highestNodeToRemoveInPruning): Ditto.
+        * editing/htmlediting.h:
+
 2011-10-03  Ryosuke Niwa  <rniwa@webkit.org>
 

trunk/Source/WebCore/editing/CompositeEditCommand.cpp

@@ -248 +248 @@
 }
 
-static bool hasARenderedDescendant(Node* node, Node* excludedNode)
-{
-    for (Node* n = node->firstChild(); n;) {
-        if (n == excludedNode) {
-            n = n->traverseNextSibling(node);
-            continue;
-        }
-        if (n->renderer())
-            return true;
-        n = n->traverseNextNode(node);
-    }
-    return false;
-}
-
-static Node* highestNodeToRemoveInPruning(Node* node)
-{
-    Node* previousNode = 0;
-    Node* rootEditableElement = node ? node->rootEditableElement() : 0;
-    for (; node; node = node->parentNode()) {
-        if (RenderObject* renderer = node->renderer()) {
-            if (!renderer->canHaveChildren() || hasARenderedDescendant(node, previousNode) || rootEditableElement == node)
-                return previousNode;
-        }
-        previousNode = node;
-    }
-    return 0;
-}
-
 void CompositeEditCommand::prune(PassRefPtr<Node> node)
 {

trunk/Source/WebCore/editing/ReplaceSelectionCommand.cpp

@@ -443 +443 @@
 }
 
-// Wrap CompositeEditCommand::removeNodePreservingChildren() so we can update the nodes we track
-void ReplaceSelectionCommand::removeNodePreservingChildren(Node* node)
-{
-    if (m_firstNodeInserted == node)
-        m_firstNodeInserted = node->traverseNextNode();
-    if (m_lastLeafInserted == node)
-        m_lastLeafInserted = node->lastChild() ? node->lastChild() : node->traverseNextSibling();
-    CompositeEditCommand::removeNodePreservingChildren(node);
-}
-
-// Wrap CompositeEditCommand::removeNodeAndPruneAncestors() so we can update the nodes we track
-void ReplaceSelectionCommand::removeNodeAndPruneAncestors(Node* node)
-{
-    // prepare in case m_firstNodeInserted and/or m_lastLeafInserted get removed
-    // FIXME: shouldn't m_lastLeafInserted be adjusted using traversePreviousNode()?
-    Node* afterFirst = m_firstNodeInserted ? m_firstNodeInserted->traverseNextSibling() : 0;
-    Node* afterLast = m_lastLeafInserted ? m_lastLeafInserted->traverseNextSibling() : 0;
-    
-    CompositeEditCommand::removeNodeAndPruneAncestors(node);
-    
-    // adjust m_firstNodeInserted and m_lastLeafInserted since either or both may have been removed
-    if (m_lastLeafInserted && !m_lastLeafInserted->inDocument())
-        m_lastLeafInserted = afterLast;
-    if (m_firstNodeInserted && !m_firstNodeInserted->inDocument())
-        m_firstNodeInserted = m_lastLeafInserted && m_lastLeafInserted->inDocument() ? afterFirst : 0;
-}
-
 static bool isHeaderElement(Node* a)
 {
@@ -583 +556 @@
     }
 
-    // We don't have to make sure that m_firstNodeInserted isn't inside a select or script element, because
+    // We don't have to make sure that firstNodeInserted isn't inside a select or script element, because
     // it is a top level node in the fragment and the user can't insert into those elements.
     Node* firstNodeInserted = insertedNodes.firstNodeInserted();
@@ -595 +568 @@
 VisiblePosition ReplaceSelectionCommand::positionAtEndOfInsertedContent() const
 {
-    Node* lastNode = m_lastLeafInserted.get();
     // FIXME: Why is this hack here?  What's special about <select> tags?
-    Node* enclosingSelect = enclosingNodeWithTag(firstPositionInOrBeforeNode(lastNode), selectTag);
-    if (enclosingSelect)
-        lastNode = enclosingSelect;
-    return lastPositionInOrAfterNode(lastNode);
+    Node* enclosingSelect = enclosingNodeWithTag(m_endOfInsertedContent, selectTag);
+    return enclosingSelect ? lastPositionInOrAfterNode(enclosingSelect) : m_endOfInsertedContent;
 }
 
 VisiblePosition ReplaceSelectionCommand::positionAtStartOfInsertedContent() const
 {
-    return firstPositionInOrBeforeNode(m_firstNodeInserted.get());
+    return m_startOfInsertedContent;
 }
 
@@ -661 +631 @@
 // We should remove styles from spans that are overridden by all of their children, either here
 // or at copy time.
-void ReplaceSelectionCommand::handleStyleSpans(Node* firstNodeInserted)
+void ReplaceSelectionCommand::handleStyleSpans(InsertedNodes& insertedNodes)
 {
     HTMLElement* wrappingStyleSpan = 0;
@@ -667 +637 @@
     // the top of the fragment, but Mail sometimes adds a wrapper (for Paste As Quotation),
     // so search for the top level style span instead of assuming it's at the top.
-    for (Node* node = firstNodeInserted; node; node = node->traverseNextNode()) {
+    for (Node* node = insertedNodes.firstNodeInserted(); node; node = node->traverseNextNode()) {
         if (isLegacyAppleStyleSpan(node)) {
             wrappingStyleSpan = toHTMLElement(node);
@@ -698 +668 @@
     style->removeBlockProperties();
 
-    if (style->isEmpty() || !wrappingStyleSpan->firstChild())
+    if (style->isEmpty() || !wrappingStyleSpan->firstChild()) {
+        insertedNodes.willRemoveNodePreservingChildren(wrappingStyleSpan);
         removeNodePreservingChildren(wrappingStyleSpan);
-    else
+    } else
         setNodeAttribute(wrappingStyleSpan, styleAttr, style->style()->cssText());
 }
@@ -737 +708 @@
     moveParagraph(startOfParagraphToMove, endOfParagraph(startOfParagraphToMove), destination);
     
-    // Merging forward will remove m_lastLeafInserted from the document.
-    // FIXME: Maintain positions for the start and end of inserted content instead of keeping nodes.  The nodes are
-    // only ever used to create positions where inserted content starts/ends.  Also, we sometimes insert content
-    // directly into text nodes already in the document, in which case tracking inserted nodes is inadequate.
+    // Merging forward will remove m_endOfInsertedContent from the document.
     if (mergeForward) {
-        m_lastLeafInserted = destination.previous().deepEquivalent().deprecatedNode();
-        if (!m_firstNodeInserted->inDocument())
-            m_firstNodeInserted = endingSelection().visibleStart().deepEquivalent().deprecatedNode();
-        // If we merged text nodes, m_lastLeafInserted could be null. If this is the case,
-        // we use m_firstNodeInserted.
-        if (!m_lastLeafInserted)
-            m_lastLeafInserted = m_firstNodeInserted;
+        if (m_startOfInsertedContent.isOrphan())
+            m_startOfInsertedContent = endingSelection().visibleStart().deepEquivalent();
+         m_endOfInsertedContent = endingSelection().visibleEnd().deepEquivalent();
+        // If we merged text nodes, m_endOfInsertedContent could be null. If this is the case, we use m_startOfInsertedContent.
+        if (m_endOfInsertedContent.isNull())
+            m_endOfInsertedContent = m_startOfInsertedContent;
     }
 }
@@ -966 +933 @@
     // 5) Add spaces for smart replace.
     // 6) Select the replacement if requested, and match style if requested.
-    
-    VisiblePosition startOfInsertedContent, endOfInsertedContent;
 
     InsertedNodes insertedNodes;
@@ -1009 +974 @@
 
     removeUnrenderedTextNodesAtEnds(insertedNodes);
-    
-    m_firstNodeInserted = insertedNodes.firstNodeInserted();
-    m_lastLeafInserted = insertedNodes.lastLeafInserted();
 
     if (!handledStyleSpans)
-        handleStyleSpans(m_firstNodeInserted.get());
+        handleStyleSpans(insertedNodes);
 
     // Mutation events (bug 20161) may have already removed the inserted content
-    if (!m_firstNodeInserted || !m_firstNodeInserted->inDocument())
-        return;
-    
-    endOfInsertedContent = positionAtEndOfInsertedContent();
-    startOfInsertedContent = positionAtStartOfInsertedContent();
-    
+    if (!insertedNodes.firstNodeInserted() || !insertedNodes.firstNodeInserted()->inDocument())
+        return;
+
+    VisiblePosition startOfInsertedContent = firstPositionInOrBeforeNode(insertedNodes.firstNodeInserted());
+
     // We inserted before the startBlock to prevent nesting, and the content before the startBlock wasn't in its own block and
     // didn't have a br after it, so the inserted content ended up in the same paragraph.
@@ -1028 +989 @@
         insertNodeAt(createBreakElement(document()).get(), startOfInsertedContent.deepEquivalent());
 
-    bool interchangeNewlineAtEnd = fragment.hasInterchangeNewlineAtEnd();
-
-    if (endBR && (plainTextFragment || shouldRemoveEndBR(endBR, originalVisPosBeforeEndBR)))
-        removeNodeAndPruneAncestors(endBR);
-    
+    if (endBR && (plainTextFragment || shouldRemoveEndBR(endBR, originalVisPosBeforeEndBR))) {
+        RefPtr<Node> parent = endBR->parentNode();
+        insertedNodes.willRemoveNode(endBR);
+        removeNode(endBR);
+        if (Node* nodeToRemove = highestNodeToRemoveInPruning(parent.get())) {
+            insertedNodes.willRemoveNode(nodeToRemove);
+            removeNode(nodeToRemove);
+        }
+    }
+
+    // Setup m_startOfInsertedContent and m_endOfInsertedContent. This should be the last two lines of code that access insertedNodes.
+    m_startOfInsertedContent = firstPositionInOrBeforeNode(insertedNodes.firstNodeInserted());
+    m_endOfInsertedContent = lastPositionInOrAfterNode(insertedNodes.lastLeafInserted());
+
     // Determine whether or not we should merge the end of inserted content with what's after it before we do
     // the start merge so that the start merge doesn't effect our decision.
@@ -1038 +1008 @@
     
     if (shouldMergeStart(selectionStartWasStartOfParagraph, fragment.hasInterchangeNewlineAtStart(), startIsInsideMailBlockquote)) {
-        VisiblePosition destination = startOfInsertedContent.previous();
-        VisiblePosition startOfParagraphToMove = startOfInsertedContent;
+        VisiblePosition startOfParagraphToMove = positionAtStartOfInsertedContent();
+        VisiblePosition destination = startOfParagraphToMove.previous();
         // We need to handle the case where we need to merge the end
         // but our destination node is inside an inline that is the last in the block.
@@ -1065 +1035 @@
         // only ever used to create positions where inserted content starts/ends.
         moveParagraph(startOfParagraphToMove, endOfParagraph(startOfParagraphToMove), destination);
-        m_firstNodeInserted = endingSelection().visibleStart().deepEquivalent().downstream().deprecatedNode();
-        if (!m_lastLeafInserted->inDocument())
-            m_lastLeafInserted = endingSelection().visibleEnd().deepEquivalent().upstream().deprecatedNode();
-    }
-            
-    endOfInsertedContent = positionAtEndOfInsertedContent();
-    startOfInsertedContent = positionAtStartOfInsertedContent();
+        m_startOfInsertedContent = endingSelection().visibleStart().deepEquivalent().downstream();
+        if (m_endOfInsertedContent.isOrphan())
+            m_endOfInsertedContent = endingSelection().visibleEnd().deepEquivalent().upstream();
+    }
 
     Position lastPositionToSelect;
-    if (interchangeNewlineAtEnd) {
+    if (fragment.hasInterchangeNewlineAtEnd()) {
+        VisiblePosition endOfInsertedContent = positionAtEndOfInsertedContent();
         VisiblePosition next = endOfInsertedContent.next(CannotCrossEditingBoundary);
 
@@ -1163 +1131 @@
         if (endNode->isTextNode()) {
             Text* text = static_cast<Text*>(endNode);
+            // FIXME: we shouldn't always be inserting the space at the end
             insertTextIntoNode(text, text->length(), collapseWhiteSpace ? nonBreakingSpaceString() : " ");
+            if (m_endOfInsertedContent.containerNode() == text)
+                m_endOfInsertedContent.moveToOffset(m_endOfInsertedContent.offsetInContainerNode() + 1);
         } else {
             RefPtr<Node> node = document()->createEditingTextNode(collapseWhiteSpace ? nonBreakingSpaceString() : " ");
@@ -1181 +1152 @@
         if (startNode->isTextNode()) {
             Text* text = static_cast<Text*>(startNode);
+            // FIXME: we shouldn't always be inserting the space at the beginning
             insertTextIntoNode(text, 0, collapseWhiteSpace ? nonBreakingSpaceString() : " ");
+            if (m_endOfInsertedContent.containerNode() == text && m_endOfInsertedContent.offsetInContainerNode())
+                m_endOfInsertedContent.moveToOffset(m_endOfInsertedContent.offsetInContainerNode() + 1);
         } else {
             RefPtr<Node> node = document()->createEditingTextNode(collapseWhiteSpace ? nonBreakingSpaceString() : " ");
-            // Don't updateNodesInserted. Doing so would set m_lastLeafInserted to be the node containing the leading space,
-            // but m_lastLeafInserted is supposed to mark the end of pasted content.
+            // Don't updateNodesInserted. Doing so would set m_endOfInsertedContent to be the node containing the leading space,
+            // but m_endOfInsertedContent is supposed to mark the end of pasted content.
             insertNodeBefore(node, startNode);
-            // FIXME: Use positions to track the start/end of inserted content.
-            m_firstNodeInserted = node;
+            m_startOfInsertedContent = firstPositionInNode(node.get());
         }
     }
@@ -1276 +1249 @@
         return;
 
-    if (!m_firstNodeInserted)
-        m_firstNodeInserted = node;
-    
-    if (node == m_lastLeafInserted)
-        return;
-    
-    m_lastLeafInserted = node->lastDescendant();
+    if (m_startOfInsertedContent.isNull())
+        m_startOfInsertedContent = firstPositionInOrBeforeNode(node);
+
+    m_endOfInsertedContent = lastPositionInOrAfterNode(node->lastDescendant());
 }
 

trunk/Source/WebCore/editing/ReplaceSelectionCommand.h

@@ -89 +89 @@
     
     void removeRedundantStylesAndKeepStyleSpanInline(InsertedNodes&);
-    void handleStyleSpans(Node* firstNodeInserted);
+    void handleStyleSpans(InsertedNodes&);
     void handlePasteAsQuotationNode();
-    
-    virtual void removeNodePreservingChildren(Node*);
-    virtual void removeNodeAndPruneAncestors(Node*);
     
     VisiblePosition positionAtStartOfInsertedContent() const;
@@ -104 +101 @@
     bool performTrivialReplace(const ReplacementFragment&);
 
-    RefPtr<Node> m_firstNodeInserted;
-    RefPtr<Node> m_lastLeafInserted;
+    Position m_startOfInsertedContent;
+    Position m_endOfInsertedContent;
     RefPtr<EditingStyle> m_insertionStyle;
     bool m_selectReplacement;

trunk/Source/WebCore/editing/htmlediting.cpp

@@ -622 +622 @@
     
     return highest;
+}
+
+static bool hasARenderedDescendant(Node* node, Node* excludedNode)
+{
+    for (Node* n = node->firstChild(); n;) {
+        if (n == excludedNode) {
+            n = n->traverseNextSibling(node);
+            continue;
+        }
+        if (n->renderer())
+            return true;
+        n = n->traverseNextNode(node);
+    }
+    return false;
+}
+
+Node* highestNodeToRemoveInPruning(Node* node)
+{
+    Node* previousNode = 0;
+    Node* rootEditableElement = node ? node->rootEditableElement() : 0;
+    for (; node; node = node->parentNode()) {
+        if (RenderObject* renderer = node->renderer()) {
+            if (!renderer->canHaveChildren() || hasARenderedDescendant(node, previousNode) || rootEditableElement == node)
+                return previousNode;
+        }
+        previousNode = node;
+    }
+    return 0;
 }
 

trunk/Source/WebCore/editing/htmlediting.h

@@ -58 +58 @@
 Node* highestEnclosingNodeOfType(const Position&, bool (*nodeIsOfType)(const Node*),
     EditingBoundaryCrossingRule = CannotCrossEditingBoundary, Node* stayWithin = 0);
-Node* lowestEditableAncestor(Node*);   
+Node* highestNodeToRemoveInPruning(Node*);
+Node* lowestEditableAncestor(Node*);
 
 Node* enclosingBlock(Node*, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);