2011-03-10 Levi Weintraub <leviw@chromium.org>
authorleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Mar 2011 23:50:56 +0000 (23:50 +0000)
committerleviw@chromium.org <leviw@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Mar 2011 23:50:56 +0000 (23:50 +0000)
        Reviewed by Ryosuke Niwa.

        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
        https://bugs.webkit.org/show_bug.cgi?id=53409

        Avoiding crashes and infinite loops when listifying content with mixed-editability

        * editing/execCommand/insert-list-with-noneditable-content-expected.txt: Added.
        * editing/execCommand/insert-list-with-noneditable-content.html: Added.
2011-03-10  Levi Weintraub  <leviw@chromium.org>

        Reviewed by Ryosuke Niwa.

        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
        https://bugs.webkit.org/show_bug.cgi?id=53409

        Fixing broken handling of mixed-editability content for InsertListCommand. Previously, if the selection
        spanned non-contenteditable regions, it would get stuck endlessly iterating the same region as the algorithm
        didn't skip the editable boundary.

        Test: editing/execCommand/insert-list-with-noneditable-content.html

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::cleanupAfterDeletion): Changed signature to take the destination
        position for the active editing command. Without this, there are cases when the destination happens
        to be a placeholder, and we remove it.
        (WebCore::CompositeEditCommand::moveParagraphs):
        * editing/CompositeEditCommand.h:
        * editing/InsertListCommand.cpp:
        (WebCore::InsertListCommand::doApply): Added logic to the paragraph iteration loop to handle pockets of
        non-editable content in an editable context. Previously, this could cause an infinite loop.
        * editing/visible_units.cpp:
        (WebCore::startOfParagraph): Added a mode of operation where we'll jump across non-editable
        content in the same paragraph to reach the actual editable paragraph start.
        (WebCore::endOfParagraph): Ditto.
        (WebCore::startOfNextParagraph): Now uses the aforementioned non-editable content skipping mode of
        endOfParagraph.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@80780 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/WebCore.xcodeproj/project.pbxproj
Source/WebCore/editing/CompositeEditCommand.cpp
Source/WebCore/editing/CompositeEditCommand.h
Source/WebCore/editing/InsertListCommand.cpp
Source/WebCore/editing/visible_units.cpp
Source/WebCore/editing/visible_units.h

index 939bc09..60cd494 100644 (file)
@@ -1,3 +1,15 @@
+2011-03-10  Levi Weintraub  <leviw@chromium.org>
+
+        Reviewed by Ryosuke Niwa.
+
+        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
+        https://bugs.webkit.org/show_bug.cgi?id=53409
+
+        Avoiding crashes and infinite loops when listifying content with mixed-editability
+
+        * editing/execCommand/insert-list-with-noneditable-content-expected.txt: Added.
+        * editing/execCommand/insert-list-with-noneditable-content.html: Added.
+
 2011-03-10  Berend-Jan Wever  <skylined@chromium.org>
 
         Reviewed by Darin Adler.
diff --git a/LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt b/LayoutTests/editing/execCommand/insert-list-with-noneditable-content-expected.txt
new file mode 100644 (file)
index 0000000..418930c
--- /dev/null
@@ -0,0 +1,13 @@
+This tests list creation in an editable context but across non-editable content. Editable content should be pulled into the list and not crash.
+| <ul>
+|   <li>
+|     "Editabl<#selection-anchor>e paragraph containing a "
+|     <span>
+|       contenteditable="false"
+|       style="background-color: LightGray;"
+|       "non-editable"
+|     " in the middle"
+|     <br>
+|   <li>
+|     "Anothe<#selection-focus>r editable paragraph."
+|     <br>
diff --git a/LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html b/LayoutTests/editing/execCommand/insert-list-with-noneditable-content.html
new file mode 100644 (file)
index 0000000..9586578
--- /dev/null
@@ -0,0 +1,24 @@
+<!DOCTYPE html>
+<div id="description">This tests list creation in an editable context but across non-editable content. Editable content should be pulled into the list and not crash.</div>
+<div contenteditable="true" id="test" style="padding: 1em;">
+  Editable paragraph containing a <span contenteditable="false" style="background-color: LightGray;">non-editable</span> in the middle<br>
+  Another editable paragraph.
+</div>
+<button onclick="insertList();">Insert List</button>
+<script src="../../resources/dump-as-markup.js"></script>
+<script>
+function insertList() {
+  document.execCommand('insertunorderedlist', false, '');
+}
+
+var s = window.getSelection();
+var div = document.getElementById("test");
+s.setPosition(div.childNodes[0], 10);
+s.modify("extend", "forward", "line");
+
+if (window.layoutTestController) {
+  insertList();
+  Markup.description(document.getElementById('description').innerText);
+  Markup.dump(div);
+}
+</script>
index d6cff9b..014ae6f 100644 (file)
@@ -1,3 +1,32 @@
+2011-03-10  Levi Weintraub  <leviw@chromium.org>
+
+        Reviewed by Ryosuke Niwa.
+
+        InsertUnorderedList over a non-editable region and multiple lines enters an infinite loop
+        https://bugs.webkit.org/show_bug.cgi?id=53409
+
+        Fixing broken handling of mixed-editability content for InsertListCommand. Previously, if the selection
+        spanned non-contenteditable regions, it would get stuck endlessly iterating the same region as the algorithm
+        didn't skip the editable boundary.
+
+        Test: editing/execCommand/insert-list-with-noneditable-content.html
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::cleanupAfterDeletion): Changed signature to take the destination
+        position for the active editing command. Without this, there are cases when the destination happens
+        to be a placeholder, and we remove it.
+        (WebCore::CompositeEditCommand::moveParagraphs):
+        * editing/CompositeEditCommand.h:
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::doApply): Added logic to the paragraph iteration loop to handle pockets of
+        non-editable content in an editable context. Previously, this could cause an infinite loop.
+        * editing/visible_units.cpp:
+        (WebCore::startOfParagraph): Added a mode of operation where we'll jump across non-editable
+        content in the same paragraph to reach the actual editable paragraph start.
+        (WebCore::endOfParagraph): Ditto.
+        (WebCore::startOfNextParagraph): Now uses the aforementioned non-editable content skipping mode of
+        endOfParagraph.
+
 2011-03-10  Berend-Jan Wever  <skylined@chromium.org>
 
         Reviewed by Darin Adler.
index 18d8646..885dd70 100644 (file)
                        isa = PBXProject;
                        buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
                        compatibilityVersion = "Xcode 2.4";
+                       developmentRegion = English;
                        hasScannedForEncodings = 1;
                        knownRegions = (
                                English,
index 242f709..b2f58c6 100644 (file)
@@ -830,10 +830,10 @@ void CompositeEditCommand::cloneParagraphUnderNewElement(Position& start, Positi
 // Deleting a paragraph will leave a placeholder. Remove it (and prune
 // empty or unrendered parents).
 
-void CompositeEditCommand::cleanupAfterDeletion()
+void CompositeEditCommand::cleanupAfterDeletion(VisiblePosition destination)
 {
     VisiblePosition caretAfterDelete = endingSelection().visibleStart();
-    if (isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
+    if (caretAfterDelete != destination && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
         // Note: We want the rightmost candidate.
         Position position = caretAfterDelete.deepEquivalent().downstream();
         Node* node = position.deprecatedNode();
@@ -947,8 +947,8 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
         }
     }
     
-    VisiblePosition beforeParagraph = startOfParagraphToMove.previous();
-    VisiblePosition afterParagraph(endOfParagraphToMove.next());
+    VisiblePosition beforeParagraph = startOfParagraphToMove.previous(CannotCrossEditingBoundary);
+    VisiblePosition afterParagraph(endOfParagraphToMove.next(CannotCrossEditingBoundary));
 
     // We upstream() the end and downstream() the start so that we don't include collapsed whitespace in the move.
     // When we paste a fragment, spaces after the end and before the start are treated as though they were rendered.
@@ -985,8 +985,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
     deleteSelection(false, false, false, false);
 
     ASSERT(destination.deepEquivalent().anchorNode()->inDocument());
-
-    cleanupAfterDeletion();
+    cleanupAfterDeletion(destination);
     ASSERT(destination.deepEquivalent().anchorNode()->inDocument());
 
     // Add a br if pruning an empty block level element caused a collapse. For example:
index b2fd58e..a955b3a 100644 (file)
@@ -109,7 +109,7 @@ protected:
     void moveParagraphs(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&, bool preserveSelection = false, bool preserveStyle = true);
     void moveParagraphWithClones(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, Element* blockElement, Node* outerNode);
     void cloneParagraphUnderNewElement(Position& start, Position& end, Node* outerNode, Element* blockElement);
-    void cleanupAfterDeletion();
+    void cleanupAfterDeletion(VisiblePosition destination = VisiblePosition());
     
     bool breakOutOfEmptyListItem();
     bool breakOutOfEmptyMailBlockquotedParagraph();
index 97305cb..8634e28 100644 (file)
@@ -121,7 +121,7 @@ void InsertListCommand::doApply()
     // FIXME: We paint the gap before some paragraphs that are indented with left 
     // margin/padding, but not others.  We should make the gap painting more consistent and 
     // then use a left margin/padding rule here.
-    if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd))
+    if (visibleEnd != visibleStart && isStartOfParagraph(visibleEnd, CanSkipOverEditingBoundary))
         setEndingSelection(VisibleSelection(visibleStart, visibleEnd.previous(CannotCrossEditingBoundary)));
 
     const QualifiedName& listTag = (m_type == OrderedList) ? olTag : ulTag;
@@ -130,14 +130,14 @@ void InsertListCommand::doApply()
         ASSERT(selection.isRange());
         VisiblePosition startOfSelection = selection.visibleStart();
         VisiblePosition endOfSelection = selection.visibleEnd();
-        VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection);
+        VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
 
-        if (startOfParagraph(startOfSelection) != startOfLastParagraph) {
+        if (startOfParagraph(startOfSelection, CanSkipOverEditingBoundary) != startOfLastParagraph) {
             bool forceCreateList = !selectionHasListOfType(selection, listTag);
 
             RefPtr<Range> currentSelection = endingSelection().firstRange();
             VisiblePosition startOfCurrentParagraph = startOfSelection;
-            while (startOfCurrentParagraph != startOfLastParagraph) {
+            while (!inSameParagraph(startOfCurrentParagraph, startOfLastParagraph, CanCrossEditingBoundary)) {
                 // doApply() may operate on and remove the last paragraph of the selection from the document 
                 // if it's in the same list item as startOfCurrentParagraph.  Return early to avoid an 
                 // infinite loop and because there is no more work to be done.
@@ -162,7 +162,7 @@ void InsertListCommand::doApply()
                     if (!lastSelectionRange)
                         return;
                     endOfSelection = lastSelectionRange->startPosition();
-                    startOfLastParagraph = startOfParagraph(endOfSelection);
+                    startOfLastParagraph = startOfParagraph(endOfSelection, CanSkipOverEditingBoundary);
                 }
 
                 // Fetch the start of the selection after moving the first paragraph,
@@ -263,8 +263,8 @@ void InsertListCommand::unlistifyParagraph(const VisiblePosition& originalStart,
         previousListChild = listChildNode->previousSibling();
     } else {
         // A paragraph is visually a list item minus a list marker.  The paragraph will be moved.
-        start = startOfParagraph(originalStart);
-        end = endOfParagraph(start);
+        start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
+        end = endOfParagraph(start, CanSkipOverEditingBoundary);
         nextListChild = enclosingListChild(end.next().deepEquivalent().deprecatedNode(), listNode);
         ASSERT(nextListChild != listChildNode);
         previousListChild = enclosingListChild(start.previous().deepEquivalent().deprecatedNode(), listNode);
@@ -327,8 +327,8 @@ static Element* adjacentEnclosingList(const VisiblePosition& pos, const VisibleP
 
 PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePosition& originalStart, const QualifiedName& listTag)
 {
-    VisiblePosition start = startOfParagraph(originalStart);
-    VisiblePosition end = endOfParagraph(start);
+    VisiblePosition start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
+    VisiblePosition end = endOfParagraph(start, CanSkipOverEditingBoundary);
     
     if (start.isNull() || end.isNull())
         return 0;
@@ -375,8 +375,11 @@ PassRefPtr<HTMLElement> InsertListCommand::listifyParagraph(const VisiblePositio
 
         // We inserted the list at the start of the content we're about to move
         // Update the start of content, so we don't try to move the list into itself.  bug 19066
-        if (insertionPos == start.deepEquivalent())
-            start = startOfParagraph(originalStart);
+        // Layout is necessary since start's node's inline renderers may have been destroyed by the insertion
+        if (insertionPos == start.deepEquivalent()) {
+            listElement->document()->updateLayoutIgnorePendingStylesheets();
+            start = startOfParagraph(originalStart, CanSkipOverEditingBoundary);
+        }
     }
 
     moveParagraph(start, end, positionBeforeNode(placeholder.get()), true);
index 5079b56..43ce264 100644 (file)
@@ -751,14 +751,21 @@ VisiblePosition startOfParagraph(const VisiblePosition& c, EditingBoundaryCrossi
 
     Node* startBlock = enclosingBlock(startNode);
 
-    Node *node = startNode;
+    Node* node = startNode;
+    Node* highestRoot = highestEditableRoot(p);
     int offset = p.deprecatedEditingOffset();
     Position::AnchorType type = p.anchorType();
 
-    Node *n = startNode;
+    Noden = startNode;
     while (n) {
         if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable())
             break;
+        if (boundaryCrossingRule == CanSkipOverEditingBoundary) {
+            while (n && n->isContentEditable() != startNode->isContentEditable())
+                n = n->traversePreviousNodePostOrder(startBlock);
+            if (!n || !n->isDescendantOf(highestRoot))
+                break;
+        }
         RenderObject *r = n->renderer();
         if (!r) {
             n = n->traversePreviousNodePostOrder(startBlock);
@@ -814,16 +821,24 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing
         return lastDeepEditingPositionForNode(startNode);
     
     Node* startBlock = enclosingBlock(startNode);
-    Node *stayInsideBlock = startBlock;
+    NodestayInsideBlock = startBlock;
     
-    Node *node = startNode;
+    Node* node = startNode;
+    Node* highestRoot = highestEditableRoot(p);
     int offset = p.deprecatedEditingOffset();
     Position::AnchorType type = p.anchorType();
 
-    Node *n = startNode;
+    Noden = startNode;
     while (n) {
         if (boundaryCrossingRule == CannotCrossEditingBoundary && n->isContentEditable() != startNode->isContentEditable())
             break;
+        if (boundaryCrossingRule == CanSkipOverEditingBoundary) {
+            while (n && n->isContentEditable() != startNode->isContentEditable())
+                n = n->traverseNextNode(stayInsideBlock);
+            if (!n || !n->isDescendantOf(highestRoot))
+                break;
+        }
+
         RenderObject *r = n->renderer();
         if (!r) {
             n = n->traverseNextNode(stayInsideBlock);
@@ -866,9 +881,10 @@ VisiblePosition endOfParagraph(const VisiblePosition &c, EditingBoundaryCrossing
     return VisiblePosition(Position(node, type), DOWNSTREAM);
 }
 
+// FIXME: isStartOfParagraph(startOfNextParagraph(pos)) is not always true
 VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
 {
-    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition));
+    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition, CanSkipOverEditingBoundary));
     VisiblePosition afterParagraphEnd(paragraphEnd.next(CannotCrossEditingBoundary));
     // The position after the last position in the last cell of a table
     // is not the start of the next paragraph.
@@ -877,9 +893,9 @@ VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
     return afterParagraphEnd;
 }
 
-bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b)
+bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b, EditingBoundaryCrossingRule boundaryCrossingRule)
 {
-    return a.isNotNull() && startOfParagraph(a) == startOfParagraph(b);
+    return a.isNotNull() && startOfParagraph(a, boundaryCrossingRule) == startOfParagraph(b, boundaryCrossingRule);
 }
 
 bool isStartOfParagraph(const VisiblePosition &pos, EditingBoundaryCrossingRule boundaryCrossingRule)
index 167bd2c..5717e29 100644 (file)
@@ -70,7 +70,7 @@ VisiblePosition previousParagraphPosition(const VisiblePosition &, int x);
 VisiblePosition nextParagraphPosition(const VisiblePosition &, int x);
 bool isStartOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
 bool isEndOfParagraph(const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
-bool inSameParagraph(const VisiblePosition &, const VisiblePosition &);
+bool inSameParagraph(const VisiblePosition &, const VisiblePosition &, EditingBoundaryCrossingRule = CannotCrossEditingBoundary);
 
 // blocks (true paragraphs; line break elements don't break blocks)
 VisiblePosition startOfBlock(const VisiblePosition &);