WebCore:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2007 00:04:01 +0000 (00:04 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Sep 2007 00:04:01 +0000 (00:04 +0000)
        Reviewed by Tristan.

        <rdar://problem/5469868>
        GoogleDocs: A hang occurs when applying list styling to a selection in a <table>

        When list insertion moves selected paragraphs into list items, it relies on
        the selection preservation code inside moveParagraphs to iterate over the
        selected paragraphs.  If a selection is ever restored incorrectly (before
        the original, or inside the original) list insertion will go into an infinite loop.

        In this hang, a table was selected and the selection preservation code incorrectly
        restored a selection, placing it inside the table.

        The bug was that a TextIterator, when being used for selection preservation, must
        emit a character between every VisiblePosition in the Range used to create the
        iterator.

        * editing/TextIterator.cpp:
        (WebCore::TextIterator::TextIterator): Renamed the boolean that we use for
        selection preservation.  It used to be m_emitForReplacedElements because
        we believed that replaced elements were the only case where TextIterators
        should have emitted differently when used for selection preservation.
        (WebCore::TextIterator::handleReplacedElement): Ditto.
        (WebCore::TextIterator::shouldRepresentNodeOffsetZero): Represent the
        position before block tables, but only if we are emitting for selection
        preservation.
        (WebCore::TextIterator::shouldEmitSpaceBeforeAndAfterNode): We should emit
        a space before and after block tables if we are emitting for selection
        preservation (because we have VisiblePositions before and after them).
        (WebCore::TextIterator::handleNonTextNode): Use a renamed variable.
        * editing/TextIterator.h: Made shouldEmitSpaceBeforeAndAfterNode a member
        function, because whether or not we emit spaces before and after a block
        table depends we're emitting for selection preservation.

LayoutTests:

        Reviewed by Tristan.

        Demonstrates bug:
        * editing/execCommand/5469868-expected.txt: Added.
        * editing/execCommand/5469868.html: Added.

        Fixed (and moved expected results to platform/mac):
        * editing/style/table-selection-expected.checksum: Removed.
        * editing/style/table-selection-expected.png: Removed.
        * editing/style/table-selection-expected.txt: Removed.
        * platform/mac/editing/style: Added.
        * platform/mac/editing/style/table-selection-expected.checksum: Added.
        * platform/mac/editing/style/table-selection-expected.png: Added.
        * platform/mac/editing/style/table-selection-expected.txt: Added.

        Fixed:
        * platform/mac/editing/execCommand/5432254-2-expected.checksum:
        * platform/mac/editing/execCommand/5432254-2-expected.png:
        * platform/mac/editing/execCommand/5432254-2-expected.txt:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/5469868-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5469868.html [new file with mode: 0644]
LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.checksum
LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.png
LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.txt
LayoutTests/platform/mac/editing/style/table-selection-expected.checksum [moved from LayoutTests/editing/style/table-selection-expected.checksum with 100% similarity]
LayoutTests/platform/mac/editing/style/table-selection-expected.png [moved from LayoutTests/editing/style/table-selection-expected.png with 100% similarity]
LayoutTests/platform/mac/editing/style/table-selection-expected.txt [moved from LayoutTests/editing/style/table-selection-expected.txt with 71% similarity]
WebCore/ChangeLog
WebCore/editing/TextIterator.cpp
WebCore/editing/TextIterator.h

index f2fd214c4887af7036099a140efd90c2dce3afb0..841bf73dbbbb3cdea6f81f35bdc188bf4f15ccbb 100644 (file)
@@ -1,3 +1,25 @@
+2007-09-12  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Tristan.
+
+        Demonstrates bug:
+        * editing/execCommand/5469868-expected.txt: Added.
+        * editing/execCommand/5469868.html: Added.
+        
+        Fixed (and moved expected results to platform/mac):
+        * editing/style/table-selection-expected.checksum: Removed.
+        * editing/style/table-selection-expected.png: Removed.
+        * editing/style/table-selection-expected.txt: Removed.
+        * platform/mac/editing/style: Added.
+        * platform/mac/editing/style/table-selection-expected.checksum: Added.
+        * platform/mac/editing/style/table-selection-expected.png: Added.
+        * platform/mac/editing/style/table-selection-expected.txt: Added.
+        
+        Fixed:
+        * platform/mac/editing/execCommand/5432254-2-expected.checksum:
+        * platform/mac/editing/execCommand/5432254-2-expected.png:
+        * platform/mac/editing/execCommand/5432254-2-expected.txt:
+
 2007-09-12  Beth Dakin  <bdakin@apple.com>
 
         Reviewed by Hyatt.
diff --git a/LayoutTests/editing/execCommand/5469868-expected.txt b/LayoutTests/editing/execCommand/5469868-expected.txt
new file mode 100644 (file)
index 0000000..ecf3d96
--- /dev/null
@@ -0,0 +1,8 @@
+This tests for a hang when turning a selection that starts before a table and ends inside that table into a list. You should see one list item with the table. There is a bug that causes an extra empty list item to be created after the table.
+
+foo
+
+
+bar
+
+
diff --git a/LayoutTests/editing/execCommand/5469868.html b/LayoutTests/editing/execCommand/5469868.html
new file mode 100644 (file)
index 0000000..a0ecd4d
--- /dev/null
@@ -0,0 +1,13 @@
+<p>This tests for a hang when turning a selection that starts before a table and ends inside that table into a list. You should see one list item with the table.  <b>There is a bug that causes an extra empty list item to be created after the table.</b></p>
+<div id="div" contenteditable="true"><table border="1" width="100px"><tr><td>foo<br><br><br><span id="span">bar</span></td></tr></table><br></div>
+
+<script>
+if (window.layoutTestController)
+    window.layoutTestController.dumpAsText();
+div = document.getElementById("div");
+span = document.getElementById("span");
+
+sel = window.getSelection();
+sel.setBaseAndExtent(div, 0, span, span.childNodes.length);
+document.execCommand("InsertOrderedList");
+</script>
index 20527445fdbf962d69262b426487e9539a3e908a..cf641efc8f7e09619f2b9a6f6b281b1d40fd2e14 100644 (file)
@@ -1 +1 @@
-ab5114af2304436556dc70e0f32ad275
\ No newline at end of file
+bba523523fdf4b3b91867d25f3bb026f
\ No newline at end of file
index 65c6888835e4aa29b8872291c49d6b90fb78a4b2..d2f3ec48dc8438093985d13af80ad38e21f1fe0d 100644 (file)
Binary files a/LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.png and b/LayoutTests/platform/mac/editing/execCommand/5432254-2-expected.png differ
index 0641428c92e13b62fbd4165c96a242f27802496b..accc5bafab63970cd448593632d84a5b1fc0a49b 100644 (file)
@@ -21,5 +21,5 @@ layer at (0,0) size 800x600
                   RenderBlock (anonymous) at (2,36) size 61x0
         RenderBlock (anonymous) at (0,44) size 784x18
           RenderBR {BR} at (0,0) size 0x18
-selection start: position 0 of child 0 {TABLE} of child 0 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
+selection start: position 0 of child 0 {#text} of child 0 {LI} of child 0 {UL} of child 0 {TD} of child 0 {TR} of child 0 {TBODY} of child 0 {TABLE} of child 0 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
 selection end:   position 1 of child 0 {TABLE} of child 0 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
similarity index 71%
rename from LayoutTests/editing/style/table-selection-expected.txt
rename to LayoutTests/platform/mac/editing/style/table-selection-expected.txt
index 0f42e490587dd6113e8761c618ebf9f3fb7e7b9e..0cf4bced74c5f861d9ea51e1d0a406a2653da92f 100644 (file)
@@ -4,8 +4,6 @@ EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotificatio
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 1 of #text > TD > TR > TBODY > TABLE > DIV > BODY > HTML > #document to 2 of DIV > BODY > HTML > #document toDOMRange:range from 1 of #text > TD > TR > TBODY > TABLE > DIV > BODY > HTML > #document to 4 of #text > TD > TR > TBODY > TABLE > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 layer at (0,0) size 800x600
@@ -25,4 +23,4 @@ layer at (0,0) size 800x600
                 RenderText {#text} at (1,1) size 23x20
                   text run at (1,1) width 23: "bar"
 selection start: position 1 of child 0 {#text} of child 1 {TD} of child 0 {TR} of child 1 {TBODY} of child 1 {TABLE} of child 1 {DIV} of child 1 {BODY} of child 0 {HTML} of document
-selection end:   position 4 of child 0 {#text} of child 1 {TD} of child 2 {TR} of child 1 {TBODY} of child 1 {TABLE} of child 1 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+selection end:   position 2 of child 1 {TABLE} of child 1 {DIV} of child 1 {BODY} of child 0 {HTML} of document
index a532e93aefa75664e52f75bf80523778a7d7f8a1..5a87c51092d3c0fdf30ee107d8c5816ae2de0b80 100644 (file)
@@ -1,3 +1,39 @@
+2007-09-12  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Tristan.
+
+        <rdar://problem/5469868> 
+        GoogleDocs: A hang occurs when applying list styling to a selection in a <table>
+        
+        When list insertion moves selected paragraphs into list items, it relies on 
+        the selection preservation code inside moveParagraphs to iterate over the 
+        selected paragraphs.  If a selection is ever restored incorrectly (before
+        the original, or inside the original) list insertion will go into an infinite loop.
+        
+        In this hang, a table was selected and the selection preservation code incorrectly
+        restored a selection, placing it inside the table.
+        
+        The bug was that a TextIterator, when being used for selection preservation, must
+        emit a character between every VisiblePosition in the Range used to create the
+        iterator.
+        
+        * editing/TextIterator.cpp:
+        (WebCore::TextIterator::TextIterator): Renamed the boolean that we use for 
+        selection preservation.  It used to be m_emitForReplacedElements because
+        we believed that replaced elements were the only case where TextIterators
+        should have emitted differently when used for selection preservation.
+        (WebCore::TextIterator::handleReplacedElement): Ditto.
+        (WebCore::TextIterator::shouldRepresentNodeOffsetZero): Represent the 
+        position before block tables, but only if we are emitting for selection 
+        preservation.
+        (WebCore::TextIterator::shouldEmitSpaceBeforeAndAfterNode): We should emit 
+        a space before and after block tables if we are emitting for selection 
+        preservation (because we have VisiblePositions before and after them).
+        (WebCore::TextIterator::handleNonTextNode): Use a renamed variable.
+        * editing/TextIterator.h: Made shouldEmitSpaceBeforeAndAfterNode a member
+        function, because whether or not we emit spaces before and after a block
+        table depends we're emitting for selection preservation.
+
 2007-09-12  Beth Dakin  <bdakin@apple.com>
 
         Reviewed by Hyatt.
index b08709803edbbb3edcc3a1395a1a6f91c508d630..2eaf40b1ddeba9d57a556397070cb3e3c5270006 100644 (file)
@@ -76,13 +76,13 @@ TextIterator::TextIterator() : m_startContainer(0), m_startOffset(0), m_endConta
 {
 }
 
-TextIterator::TextIterator(const Range* r, bool emitForReplacedElements
+TextIterator::TextIterator(const Range* r, bool emitForSelectionPreservation
     : m_startContainer(0) 
     , m_startOffset(0)
     , m_endContainer(0)
     , m_endOffset(0)
     , m_positionNode(0)
-    , m_emitForReplacedElements(emitForReplacedElements)
+    , m_emitForSelectionPreservation(emitForSelectionPreservation)
 {
     if (!r)
         return;
@@ -353,7 +353,7 @@ bool TextIterator::handleReplacedElement()
 
     m_haveEmitted = true;
     
-    if (m_emitForReplacedElements) {
+    if (m_emitForSelectionPreservation) {
         // We want replaced elements to behave like punctuation for boundary 
         // finding, and to simply take up space for the selection preservation 
         // code in moveParagraphs, so we use a comma.
@@ -397,13 +397,6 @@ static bool shouldEmitTabBeforeNode(Node* node)
     return t && (t->cellBefore(rc) || t->cellAbove(rc));
 }
 
-static bool shouldEmitSpaceBeforeAndAfterNode(Node* node)
-{
-    RenderObject* r = node->renderer();
-    
-    return r && r->isTable() && r->isInline();
-}
-
 static bool shouldEmitNewlineForNode(Node* node)
 {
     // br elements are represented by a single newline.
@@ -508,6 +501,9 @@ static bool shouldEmitExtraNewlineForNode(Node* node)
 
 bool TextIterator::shouldRepresentNodeOffsetZero()
 {
+    if (m_emitForSelectionPreservation && m_node->renderer() && m_node->renderer()->isTable())
+        return true;
+        
     // Leave element positioned flush with start of a paragraph
     // (e.g. do not insert tab before a table cell at the start of a paragraph)
     if (m_lastCharacter == '\n')
@@ -537,6 +533,11 @@ bool TextIterator::shouldRepresentNodeOffsetZero()
     return currPos.isNotNull() && !inSameLine(startPos, currPos);
 }
 
+bool TextIterator::shouldEmitSpaceBeforeAndAfterNode(Node* node)
+{
+    return node->renderer() && node->renderer()->isTable() && (node->renderer()->isInline() || m_emitForSelectionPreservation);
+}
+
 void TextIterator::representNodeOffsetZero()
 {
     // emit a character to show the positioning of m_node
@@ -555,7 +556,7 @@ bool TextIterator::handleNonTextNode()
 {
     if (shouldEmitNewlineForNode(m_node))
         emitCharacter('\n', m_node->parentNode(), m_node, 0, 1);
-    else if (m_emitForReplacedElements && m_node->renderer() && m_node->renderer()->isHR())
+    else if (m_emitForSelectionPreservation && m_node->renderer() && m_node->renderer()->isHR())
         emitCharacter(' ', m_node->parentNode(), m_node, 0, 1);
     else
         representNodeOffsetZero();
index 9b22c19f76dd607a71e2c4cf5a56c3dd98d35bf7..d5ad74f7843f07a0f1cbb125e7869bed063dcddb 100644 (file)
@@ -76,6 +76,7 @@ public:
 private:
     void exitNode();
     bool shouldRepresentNodeOffsetZero();
+    bool shouldEmitSpaceBeforeAndAfterNode(Node*);
     void representNodeOffsetZero();
     bool handleTextNode();
     bool handleReplacedElement();
@@ -126,8 +127,9 @@ private:
     // Used when deciding whether to emit a "positioning" (e.g. newline) before any other content
     bool m_haveEmitted;
     
-    // Used by selection preservation code.
-    bool m_emitForReplacedElements;
+    // Used by selection preservation code.  There should be one character emitted between every VisiblePosition
+    // in the Range used to create the TextIterator.
+    bool m_emitForSelectionPreservation;
 };
 
 // Iterates through the DOM range, returning all the text, and 0-length boundaries