WebCore:
authorjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Dec 2007 02:55:44 +0000 (02:55 +0000)
committerjustin.garcia@apple.com <justin.garcia@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 21 Dec 2007 02:55:44 +0000 (02:55 +0000)
        Reviewed by Oliver Hunt.

        <rdar://problem/5543472> GoogleDocs: Safari hangs when creating a list from a particular selection

        Still need to fix similar issues with the other operations that iterate
        over selected paragraphs, like FormatBlock, Indent and Outdent (<rdar://problem/5658933>).

        * editing/IndentOutdentCommand.cpp:
        (WebCore::IndentOutdentCommand::indentRegion): Added a FIXME.
        * editing/IndentOutdentCommand.h: Removed an unused function.
        * editing/InsertListCommand.cpp:
        (WebCore::InsertListCommand::modifyRange):
        Renamed visibleStart to startOfSelection and visibleEnd to endOfSelection.
        Call the new selectionForParagraphIteration, which a) prevents operations like this
        one from being performed on a table that isn't fully selected (where the selection
        starts just before the table and ends inside it), and b) helps prevent paragraph
        iteration from going past the end of the selection.
        Call the new startOfNextParagraph, instead of using endOfParagraph(v).next(),
        since when v is in the last paragraph of the last cell of a table, that expression
        will return the position after the table, not the start of the next paragraph.
        * editing/htmlediting.cpp:
        (WebCore::enclosingListChild): Don't go above a table cell, so that list operations
        take effect inside the table cell where they are performed.
        (WebCore::selectionForParagraphIteration): Added, see above.
        (WebCore::indexForVisiblePosition): Moved from IndentOutdentCommand.cpp.
        * editing/htmlediting.h:
        * editing/visible_units.cpp:
        (WebCore::startOfNextParagraph): Added, see above.
        * editing/visible_units.h:
        * editing/TextIterator.h:
        (WebCore::TextIterator::exitNode): Added a FIXME.

LayoutTests:

        Reviewed by Oliver Hunt.

        <rdar://problem/5543472> GoogleDocs: Safari hangs when creating a list from a particular selection

        * editing/execCommand/5543472-1-expected.txt: Added.
        * editing/execCommand/5543472-1.html: Added.
        * editing/execCommand/5543472-2-expected.txt: Added.
        * editing/execCommand/5543472-2.html: Added.
        * editing/execCommand/5543472-3-expected.txt: Added.
        * editing/execCommand/5543472-3.html: Added.

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

16 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/5543472-1-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5543472-1.html [new file with mode: 0644]
LayoutTests/editing/execCommand/5543472-2-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5543472-2.html [new file with mode: 0644]
LayoutTests/editing/execCommand/5543472-3-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5543472-3.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/IndentOutdentCommand.cpp
WebCore/editing/IndentOutdentCommand.h
WebCore/editing/InsertListCommand.cpp
WebCore/editing/TextIterator.cpp
WebCore/editing/htmlediting.cpp
WebCore/editing/htmlediting.h
WebCore/editing/visible_units.cpp
WebCore/editing/visible_units.h

index 51ba805..512db2d 100644 (file)
@@ -1,3 +1,16 @@
+2007-12-20  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Oliver Hunt.
+        
+        <rdar://problem/5543472> GoogleDocs: Safari hangs when creating a list from a particular selection
+
+        * editing/execCommand/5543472-1-expected.txt: Added.
+        * editing/execCommand/5543472-1.html: Added.
+        * editing/execCommand/5543472-2-expected.txt: Added.
+        * editing/execCommand/5543472-2.html: Added.
+        * editing/execCommand/5543472-3-expected.txt: Added.
+        * editing/execCommand/5543472-3.html: Added.
+
 2007-12-20  Dan Bernstein  <mitz@apple.com>
 
         Reviewed by John Sullivan.
diff --git a/LayoutTests/editing/execCommand/5543472-1-expected.txt b/LayoutTests/editing/execCommand/5543472-1-expected.txt
new file mode 100644 (file)
index 0000000..7c36f36
--- /dev/null
@@ -0,0 +1,6 @@
+This tests for a hang when creating a list from a particular selection, of table content that is already in a list item. The table should be in a list, and each table cell should contain a list item.
+
+DOM:
+<ul><li id="li"><table border="1"><tbody><tr><td id="td"><ul><li>foo</li></ul></td><td><ul><li>bar</li></ul></td></tr></tbody></table></li></ul>
+
+<br>
diff --git a/LayoutTests/editing/execCommand/5543472-1.html b/LayoutTests/editing/execCommand/5543472-1.html
new file mode 100644 (file)
index 0000000..13f474b
--- /dev/null
@@ -0,0 +1,18 @@
+<p id="p">This tests for a hang when creating a list from a particular selection, of table content that is already in a list item.  The table should be in a list, and each table cell should contain a list item.</p>
+<div id="div" contenteditable="true"><ul><li id="li"><table border="1"><tr><td id="td">foo</td><td>bar</td></tr></table></li></ul>
+
+<br></div>
+
+<script>
+li = document.getElementById("li");
+td = document.getElementById("td");
+window.getSelection().setBaseAndExtent(td, 0, li, li.childNodes.length);
+document.execCommand("InsertUnorderedList");
+
+if (window.layoutTestController) {
+    window.layoutTestController.dumpAsText();
+    div = document.getElementById("div");
+    p = document.getElementById("p");
+    document.body.innerText = p.innerText + "\n\nDOM:\n" + div.innerHTML;
+}
+</script>
diff --git a/LayoutTests/editing/execCommand/5543472-2-expected.txt b/LayoutTests/editing/execCommand/5543472-2-expected.txt
new file mode 100644 (file)
index 0000000..2d32b96
--- /dev/null
@@ -0,0 +1,4 @@
+This tests for a bug when performing the list operation on a selection that starts just before a list and ends inside the list. This should only create list items in selected cells.
+
+DOM:
+<table border="1"><tbody><tr><td><ul><li>foo</li></ul></td><td id="td"><ul><li>bar</li></ul></td><td>baz</td></tr></tbody></table>
diff --git a/LayoutTests/editing/execCommand/5543472-2.html b/LayoutTests/editing/execCommand/5543472-2.html
new file mode 100644 (file)
index 0000000..b8cb12d
--- /dev/null
@@ -0,0 +1,16 @@
+<p id="p">This tests for a bug when performing the list operation on a selection that starts just before a list and ends inside the list.  This should only create list items in selected cells.</p>
+<div id="div" contenteditable="true"><table border="1"><tr><td>foo</td><td id="td">bar</td><td>baz</td></tr></table></div>
+
+<script>
+div = document.getElementById("div");
+td = document.getElementById("td");
+window.getSelection().setBaseAndExtent(div, 0, td, td.childNodes.length);
+document.execCommand("InsertUnorderedList");
+
+if (window.layoutTestController) {
+    window.layoutTestController.dumpAsText();
+    div = document.getElementById("div");
+    p = document.getElementById("p");
+    document.body.innerText = p.innerText + "\n\nDOM:\n" + div.innerHTML;
+}
+</script>
diff --git a/LayoutTests/editing/execCommand/5543472-3-expected.txt b/LayoutTests/editing/execCommand/5543472-3-expected.txt
new file mode 100644 (file)
index 0000000..a73ccd3
--- /dev/null
@@ -0,0 +1,4 @@
+This tests for a bug where a creating a list from a particular selection inside a table would create list items beyond the end of the selection. Only the second two table cells should contain list items.
+
+DOM:
+<table border="1"><tbody><tr><td>foo</td><td id="td"><ul><li>bar</li></ul></td><td><ul><li>baz</li></ul></td></tr></tbody></table><div>foo</div><div>bar</div>
diff --git a/LayoutTests/editing/execCommand/5543472-3.html b/LayoutTests/editing/execCommand/5543472-3.html
new file mode 100644 (file)
index 0000000..4ba012c
--- /dev/null
@@ -0,0 +1,16 @@
+<p id="p">This tests for a bug where a creating a list from a particular selection inside a table would create list items beyond the end of the selection.  Only the second two table cells should contain list items.</p>
+<div id="div" contenteditable="true"><table border="1"><tr><td>foo</td><td id="td">bar</td><td>baz</td></tr></table><div>foo</div><div>bar</div></div>
+
+<script>
+div = document.getElementById("div");
+td = document.getElementById("td");
+window.getSelection().setBaseAndExtent(td, 0, div, 1);
+document.execCommand("InsertUnorderedList");
+
+if (window.layoutTestController) {
+    window.layoutTestController.dumpAsText();
+    div = document.getElementById("div");
+    p = document.getElementById("p");
+    document.body.innerText = p.innerText + "\n\nDOM:\n" + div.innerHTML;
+}
+</script>
index e90c150..83b4857 100644 (file)
@@ -1,3 +1,37 @@
+2007-12-20  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        <rdar://problem/5543472> GoogleDocs: Safari hangs when creating a list from a particular selection
+        
+        Still need to fix similar issues with the other operations that iterate 
+        over selected paragraphs, like FormatBlock, Indent and Outdent (<rdar://problem/5658933>).
+
+        * editing/IndentOutdentCommand.cpp:
+        (WebCore::IndentOutdentCommand::indentRegion): Added a FIXME.
+        * editing/IndentOutdentCommand.h: Removed an unused function.
+        * editing/InsertListCommand.cpp:
+        (WebCore::InsertListCommand::modifyRange): 
+        Renamed visibleStart to startOfSelection and visibleEnd to endOfSelection.
+        Call the new selectionForParagraphIteration, which a) prevents operations like this
+        one from being performed on a table that isn't fully selected (where the selection
+        starts just before the table and ends inside it), and b) helps prevent paragraph
+        iteration from going past the end of the selection.
+        Call the new startOfNextParagraph, instead of using endOfParagraph(v).next(),
+        since when v is in the last paragraph of the last cell of a table, that expression 
+        will return the position after the table, not the start of the next paragraph.
+        * editing/htmlediting.cpp:
+        (WebCore::enclosingListChild): Don't go above a table cell, so that list operations
+        take effect inside the table cell where they are performed.
+        (WebCore::selectionForParagraphIteration): Added, see above.
+        (WebCore::indexForVisiblePosition): Moved from IndentOutdentCommand.cpp.
+        * editing/htmlediting.h:
+        * editing/visible_units.cpp:
+        (WebCore::startOfNextParagraph): Added, see above.
+        * editing/visible_units.h:
+        * editing/TextIterator.h:
+        (WebCore::TextIterator::exitNode): Added a FIXME.
+
 2007-12-20  Alp Toker  <alp@atoker.com>
 
         Rubber-stamped by Maciej.
index a7ab644..0d66467 100644 (file)
@@ -104,15 +104,6 @@ Node* IndentOutdentCommand::prepareBlockquoteLevelForInsertion(VisiblePosition&
     return placeholder.get();
 }
 
-static int indexForVisiblePosition(VisiblePosition& visiblePosition)
-{
-    if (visiblePosition.isNull())
-        return 0;
-    Position p(visiblePosition.deepEquivalent());
-    RefPtr<Range> range = new Range(p.node()->document(), Position(p.node()->document(), 0), rangeCompliantEquivalent(p));
-    return TextIterator::rangeLength(range.get(), true);
-}
-
 void IndentOutdentCommand::indentRegion()
 {
     VisiblePosition startOfSelection = endingSelection().visibleStart();
@@ -171,6 +162,8 @@ void IndentOutdentCommand::indentRegion()
             // this by splitting all parents of the current paragraph up to that point.
             RefPtr<Node> blockquote = createIndentBlockquoteElement(document());
             Position start = startOfParagraph(endOfCurrentParagraph).deepEquivalent();
+
+            // FIXME: This will break table structure.
             Node* startOfNewBlock = splitTreeToNode(start.node(), editableRootForPosition(start));
             insertNodeBefore(blockquote.get(), startOfNewBlock);
             newBlockquote = blockquote.get();
index 403297a..584eb78 100644 (file)
@@ -38,7 +38,6 @@ public:
     virtual void doApply();
     virtual EditAction editingAction() const { return m_typeOfAction == Indent ? EditActionIndent : EditActionOutdent; }
 private:
-    bool modifyRange();
     EIndentType m_typeOfAction;
     int m_marginInPixels;
     void indentRegion();
index d46274b..8408a20 100644 (file)
@@ -30,6 +30,7 @@
 #include "htmlediting.h"
 #include "HTMLElement.h"
 #include "HTMLNames.h"
+#include "TextIterator.h"
 #include "visible_units.h"
 
 namespace WebCore {
@@ -60,44 +61,39 @@ InsertListCommand::InsertListCommand(Document* document, Type type, const String
 
 bool InsertListCommand::modifyRange()
 {
-    ASSERT(endingSelection().isRange());
-    VisiblePosition visibleStart = endingSelection().visibleStart();
-    VisiblePosition visibleEnd = endingSelection().visibleEnd();
-    VisiblePosition startOfLastParagraph = startOfParagraph(visibleEnd);
+    Selection selection = selectionForParagraphIteration(endingSelection());
+    ASSERT(selection.isRange());
+    VisiblePosition startOfSelection = selection.visibleStart();
+    VisiblePosition endOfSelection = selection.visibleEnd();
+    VisiblePosition startOfLastParagraph = startOfParagraph(endOfSelection);
     
-    // If the end of the selection to modify is just after a table, and
-    // if the start of the selection is inside that table, the last paragraph
-    // that we'll want modify is the last one inside the table, not the table itself.
-    // Adjust startOfLastParagraph and visibleEnd here to avoid infinite recursion.
-    if (Node* table = isFirstPositionAfterTable(visibleEnd))
-        if (visibleStart.deepEquivalent().node()->isDescendantOf(table)) {
-            visibleEnd = visibleEnd.previous(true);
-            startOfLastParagraph = startOfParagraph(visibleEnd);
-        }
-        
-    if (startOfParagraph(visibleStart) == startOfLastParagraph)
+    if (startOfParagraph(startOfSelection) == startOfLastParagraph)
         return false;
-    
-    Node* startList = enclosingList(visibleStart.deepEquivalent().node());
-    Node* endList = enclosingList(visibleEnd.deepEquivalent().node());
+
+    Node* startList = enclosingList(startOfSelection.deepEquivalent().node());
+    Node* endList = enclosingList(endOfSelection.deepEquivalent().node());
     if (!startList || startList != endList)
         m_forceCreateList = true;
 
-    setEndingSelection(visibleStart);
+    setEndingSelection(startOfSelection);
     doApply();
-    visibleStart = endingSelection().visibleStart();
-    VisiblePosition nextParagraph = endOfParagraph(visibleStart).next();
-    while (nextParagraph.isNotNull() && nextParagraph != startOfLastParagraph) {
-        setEndingSelection(nextParagraph);
+    // Fetch the start of the selection after moving the first paragraph,
+    // because moving the paragraph will invalidate the original start.  
+    // We'll use the new start to restore the original selection after 
+    // we modified all selected paragraphs.
+    startOfSelection = endingSelection().visibleStart();
+    VisiblePosition startOfCurrentParagraph = startOfNextParagraph(startOfSelection);
+    while (startOfCurrentParagraph != startOfLastParagraph) {
+        setEndingSelection(startOfCurrentParagraph);
         doApply();
-        nextParagraph = endOfParagraph(endingSelection().visibleStart()).next();
+        startOfCurrentParagraph = startOfNextParagraph(endingSelection().visibleStart());
     }
-    setEndingSelection(visibleEnd);
+    setEndingSelection(endOfSelection);
     doApply();
-    visibleEnd = endingSelection().visibleEnd();
-    setEndingSelection(Selection(visibleStart.deepEquivalent(), visibleEnd.deepEquivalent(), DOWNSTREAM));
+    // Fetch the end of the selection, for the reason mentioned above.
+    endOfSelection = endingSelection().visibleEnd();
+    setEndingSelection(Selection(startOfSelection, endOfSelection));
     m_forceCreateList = false;
-    
     return true;
 }
 
@@ -125,6 +121,9 @@ void InsertListCommand::doApply()
     if (endingSelection().isRange() && modifyRange())
         return;
     
+    // FIXME: This will produce unexpected results for a selection that starts just before a
+    // table and ends inside the first cell, selectionForParagraphIteration should probably
+    // be renamed and deployed inside setEndingSelection().
     Node* selectionNode = endingSelection().start().node();
     const QualifiedName listTag = (m_type == OrderedList) ? olTag : ulTag;
     Node* listChildNode = enclosingListChild(selectionNode);
index 9a10f85..7165d7f 100644 (file)
@@ -595,6 +595,8 @@ void TextIterator::exitNode()
         // use extra newline to represent margin bottom, as needed
         bool addNewline = shouldEmitExtraNewlineForNode(m_node);
         
+        // FIXME: We need to emit a '\n' as we leave an empty block(s) that
+        // contain a VisiblePosition when doing selection preservation.
         if (m_lastCharacter != '\n') {
             // insert a newline with a position following this block's contents.
             emitCharacter('\n', baseNode->parentNode(), baseNode, 1, 1);
index 8d3d134..76a8e2d 100644 (file)
@@ -38,6 +38,7 @@
 #include "Range.h"
 #include "Selection.h"
 #include "Text.h"
+#include "TextIterator.h"
 #include "VisiblePosition.h"
 #include "visible_units.h"
 
@@ -639,7 +640,7 @@ Node* enclosingListChild(Node *node)
     for (Node* n = node; n && n->parentNode(); n = n->parentNode()) {
         if (n->hasTagName(liTag) || isListElement(n->parentNode()))
             return n;
-        if (n == root)
+        if (n == root || isTableCell(n))
             return 0;
     }
     
@@ -897,6 +898,44 @@ bool lineBreakExistsAtPosition(const VisiblePosition& visiblePosition)
            downstream.node()->isTextNode() && downstream.node()->renderer()->style()->preserveNewline() && visiblePosition.characterAfter() == '\n';
 }
 
+// Modifies selections that have an end point at the edge of a table
+// that contains the other endpoint so that they don't confuse
+// code that iterates over selected paragraphs.
+Selection selectionForParagraphIteration(const Selection& original)
+{
+    Selection newSelection(original);
+    VisiblePosition startOfSelection(newSelection.visibleStart());
+    VisiblePosition endOfSelection(newSelection.visibleEnd());
+    
+    // If the end of the selection to modify is just after a table, and
+    // if the start of the selection is inside that table, then the last paragraph
+    // that we'll want modify is the last one inside the table, not the table itself
+    // (a table is itself a paragraph).
+    if (Node* table = isFirstPositionAfterTable(endOfSelection))
+        if (startOfSelection.deepEquivalent().node()->isDescendantOf(table))
+            newSelection = Selection(startOfSelection, endOfSelection.previous(true));
+    
+    // If the start of the selection to modify is just before a table,
+    // and if the end of the selection is inside that table, then the first paragraph
+    // we'll want to modify is the first one inside the table, not the paragraph
+    // containing the table itself.
+    if (Node* table = isLastPositionBeforeTable(startOfSelection))
+        if (endOfSelection.deepEquivalent().node()->isDescendantOf(table))
+            newSelection = Selection(startOfSelection.next(true), endOfSelection);
+    
+    return newSelection;
+}
+
+
+int indexForVisiblePosition(VisiblePosition& visiblePosition)
+{
+    if (visiblePosition.isNull())
+        return 0;
+    Position p(visiblePosition.deepEquivalent());
+    RefPtr<Range> range = new Range(p.node()->document(), Position(p.node()->document(), 0), rangeCompliantEquivalent(p));
+    return TextIterator::rangeLength(range.get(), true);
+}
+
 PassRefPtr<Range> avoidIntersectionWithNode(const Range* range, Node* node)
 {
     if (!range || range->isDetached())
index 4ee6695..62138e5 100644 (file)
@@ -125,6 +125,10 @@ bool isTableCell(const Node*);
 
 bool lineBreakExistsAtPosition(const VisiblePosition&);
 
+Selection selectionForParagraphIteration(const Selection&);
+
+int indexForVisiblePosition(VisiblePosition&);
+
 }
 
 #endif
index 7110f3e..3bcdc09 100644 (file)
@@ -747,6 +747,17 @@ VisiblePosition endOfParagraph(const VisiblePosition &c)
     return VisiblePosition(node, offset, DOWNSTREAM);
 }
 
+VisiblePosition startOfNextParagraph(const VisiblePosition& visiblePosition)
+{
+    VisiblePosition paragraphEnd(endOfParagraph(visiblePosition));
+    VisiblePosition afterParagraphEnd(paragraphEnd.next(true));
+    // The position after the last position in the last cell of a table
+    // is not the start of the next paragraph.
+    if (isFirstPositionAfterTable(afterParagraphEnd))
+        return afterParagraphEnd.next(true);
+    return afterParagraphEnd;
+}
+
 bool inSameParagraph(const VisiblePosition &a, const VisiblePosition &b)
 {
     return a.isNotNull() && startOfParagraph(a) == startOfParagraph(b);
index 39a938b..2663888 100644 (file)
@@ -57,8 +57,9 @@ bool isStartOfLine(const VisiblePosition &);
 bool isEndOfLine(const VisiblePosition &);
 
 // paragraphs (perhaps a misnomer, can be divided by line break elements)
-VisiblePosition startOfParagraph(const VisiblePosition &);
-VisiblePosition endOfParagraph(const VisiblePosition &);
+VisiblePosition startOfParagraph(const VisiblePosition&);
+VisiblePosition endOfParagraph(const VisiblePosition&);
+VisiblePosition startOfNextParagraph(const VisiblePosition&);
 VisiblePosition previousParagraphPosition(const VisiblePosition &, int x);
 VisiblePosition nextParagraphPosition(const VisiblePosition &, int x);
 bool inSameParagraph(const VisiblePosition &, const VisiblePosition &);