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 51ba805068ed411c497def357d77e76ccebfe4d4..512db2df8910bc1c705bd5e862f037e3749030d1 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 e90c150c8b73418db9b9433591d8239f3a1f1766..83b48570f540c17db27f8e23d7ee2cd02027d656 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 a7ab6449ae07ad090b6a70fb493baf8c1b1bdada..0d6646733d55dff8d759f8859a1f9666d9ddf848 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 403297ae2479175ed0657d4e7af4adbadcac70a3..584eb7839be335aa6e20f1843fd17ef689294899 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 d46274be4ce8ce13bf5f9606974a21a2ca86671e..8408a20257afb27f06cddbe86242c826b318d3b2 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 9a10f857b35ae76bc463887ee1264eaaddd2e90d..7165d7fe3edc1202a60ae23f025c0d1a2db343b4 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 8d3d1346738868507f96f225ebc57e3e1a0f4915..76a8e2d8d6094ca69240476bbf1337a01011dd2a 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 4ee66954eac20ffd4fecbe5bddeea943e543b4b9..62138e5e23f73022a6e4067760e9100ae9652b08 100644 (file)
@@ -125,6 +125,10 @@ bool isTableCell(const Node*);
 
 bool lineBreakExistsAtPosition(const VisiblePosition&);
 
+Selection selectionForParagraphIteration(const Selection&);
+
+int indexForVisiblePosition(VisiblePosition&);
+
 }
 
 #endif
index 7110f3eb937a41164a27c9cec852711bfd9a6d8f..3bcdc0920e36f4251e085da2902ce28a514f942f 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 39a938bcf0a11b1765286150e292b0cd7475ed35..26638882ccbb59d3f2c3a6a3949153610edc663a 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 &);