Reviewed by Chris
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Oct 2004 21:52:20 +0000 (21:52 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Oct 2004 21:52:20 +0000 (21:52 +0000)
        Fix for this bug:

        <rdar://problem/3820349> REGRESSION (Mail): select all, delete does not always delete everything

        * khtml/editing/htmlediting.cpp:
        (khtml::DeleteSelectionCommand::startPositionForDelete): New helper that determines when to
        expand the selection outwards when the selection is on the visible boundary of a root
        editable element. This fixes the bug. Note that this function also contains a little code
        I factored out of doApply: it also takes care of adjusting the selection in the smart delete case.
        (khtml::DeleteSelectionCommand::endPositionForDelete): Ditto.
        (khtml::DeleteSelectionCommand::doApply): Call new helpers. Refactored out the code as described.
        * khtml/editing/htmlediting.h: Declare new helpers.
        * layout-tests/editing/deleting/delete-select-all-001-expected.txt: Added.
        * layout-tests/editing/deleting/delete-select-all-001.html: Added.
        * layout-tests/editing/deleting/delete-select-all-002-expected.txt: Added.
        * layout-tests/editing/deleting/delete-select-all-002.html: Added.
        * layout-tests/editing/deleting/delete-select-all-003-expected.txt: Added.
        * layout-tests/editing/deleting/delete-select-all-003.html: Added.

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

LayoutTests/editing/deleting/delete-select-all-001-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/delete-select-all-001.html [new file with mode: 0644]
LayoutTests/editing/deleting/delete-select-all-002-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/delete-select-all-002.html [new file with mode: 0644]
LayoutTests/editing/deleting/delete-select-all-003-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/delete-select-all-003.html [new file with mode: 0644]
WebCore/ChangeLog-2005-08-23
WebCore/khtml/editing/htmlediting.cpp
WebCore/khtml/editing/htmlediting.h

diff --git a/LayoutTests/editing/deleting/delete-select-all-001-expected.txt b/LayoutTests/editing/deleting/delete-select-all-001-expected.txt
new file mode 100644 (file)
index 0000000..ae0614d
--- /dev/null
@@ -0,0 +1,10 @@
+layer at (0,0) size 800x600
+  RenderCanvas at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {DIV} at (0,0) size 784x28 [border: (2px solid #FF0000)]
+selection is CARET:
+start:      position 0 of  of root {DIV}
+upstream:   position 0 of  of root {DIV}
+downstream: position 1 of  of root {DIV}
diff --git a/LayoutTests/editing/deleting/delete-select-all-001.html b/LayoutTests/editing/deleting/delete-select-all-001.html
new file mode 100644 (file)
index 0000000..2264839
--- /dev/null
@@ -0,0 +1,51 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    padding: 12px; 
+    font-size: 24px;
+    word-wrap: break-word;
+    -khtml-nbsp-mode: space;
+    -khtml-line-break: after-white-space;
+}
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    selectAllCommand();
+    deleteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+
+<!-- 
+
+Expected results:
+
+All the content in the contenteditable div should be deleted, 
+including the table structure.
+
+-->
+
+</head> 
+<body>
+<div contenteditable id="root" class="editing">
+<table border="1">
+<tr><td id="test">One</td><td>Two</td><td>Three</td></tr>
+<tr><td>One</td><td>Two</td><td>Three</td></tr>
+<tr><td>One</td><td>Two</td><td>Three</td></tr>
+</table>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/editing/deleting/delete-select-all-002-expected.txt b/LayoutTests/editing/deleting/delete-select-all-002-expected.txt
new file mode 100644 (file)
index 0000000..214cc89
--- /dev/null
@@ -0,0 +1,10 @@
+layer at (0,0) size 800x600
+  RenderCanvas at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {DIV} at (0,0) size 784x56 [border: (2px solid #FF0000)]
+selection is CARET:
+start:      position 0 of  of root {DIV}
+upstream:   position 0 of  of root {DIV}
+downstream: position 1 of  of root {DIV}
diff --git a/LayoutTests/editing/deleting/delete-select-all-002.html b/LayoutTests/editing/deleting/delete-select-all-002.html
new file mode 100644 (file)
index 0000000..3e6f711
--- /dev/null
@@ -0,0 +1,48 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    padding: 12px; 
+    font-size: 24px;
+    word-wrap: break-word;
+    -khtml-nbsp-mode: space;
+    -khtml-line-break: after-white-space;
+}
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    selectAllCommand();
+    deleteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+
+<!-- 
+
+Expected results:
+
+All the content in the contenteditable div should be deleted, 
+including the "you can't see this." span.
+
+-->
+
+</head> 
+<body>
+<div contenteditable id="root" class="editing">
+<span style="display: hidden">you can't see this.</span> 
+<span id="test">you can see this.</span>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
diff --git a/LayoutTests/editing/deleting/delete-select-all-003-expected.txt b/LayoutTests/editing/deleting/delete-select-all-003-expected.txt
new file mode 100644 (file)
index 0000000..ae0614d
--- /dev/null
@@ -0,0 +1,10 @@
+layer at (0,0) size 800x600
+  RenderCanvas at (0,0) size 800x600
+layer at (0,0) size 800x600
+  RenderBlock {HTML} at (0,0) size 800x600
+    RenderBody {BODY} at (8,8) size 784x584
+      RenderBlock {DIV} at (0,0) size 784x28 [border: (2px solid #FF0000)]
+selection is CARET:
+start:      position 0 of  of root {DIV}
+upstream:   position 0 of  of root {DIV}
+downstream: position 1 of  of root {DIV}
diff --git a/LayoutTests/editing/deleting/delete-select-all-003.html b/LayoutTests/editing/deleting/delete-select-all-003.html
new file mode 100644 (file)
index 0000000..b1b2ab9
--- /dev/null
@@ -0,0 +1,47 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    padding: 12px; 
+    font-size: 24px;
+    word-wrap: break-word;
+    -khtml-nbsp-mode: space;
+    -khtml-line-break: after-white-space;
+}
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    selectAllCommand();
+    deleteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+
+<!-- 
+
+Expected results:
+
+All the content in the contenteditable div should be deleted, 
+including the unordered list element.
+
+-->
+
+</head> 
+<body>
+<div contenteditable id="root" class="editing">
+<ul id="test"><li>one</li><li>two</li><li>three</li></ul>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
index 7079498..b00055e 100644 (file)
@@ -1,5 +1,28 @@
 2004-10-25  Ken Kocienda  <kocienda@apple.com>
 
 2004-10-25  Ken Kocienda  <kocienda@apple.com>
 
+        Reviewed by Chris
+
+        Fix for this bug:
+        
+        <rdar://problem/3820349> REGRESSION (Mail): select all, delete does not always delete everything
+
+        * khtml/editing/htmlediting.cpp:
+        (khtml::DeleteSelectionCommand::startPositionForDelete): New helper that determines when to
+        expand the selection outwards when the selection is on the visible boundary of a root
+        editable element. This fixes the bug. Note that this function also contains a little code
+        I factored out of doApply: it also takes care of adjusting the selection in the smart delete case.
+        (khtml::DeleteSelectionCommand::endPositionForDelete): Ditto.
+        (khtml::DeleteSelectionCommand::doApply): Call new helpers. Refactored out the code as described.
+        * khtml/editing/htmlediting.h: Declare new helpers.
+        * layout-tests/editing/deleting/delete-select-all-001-expected.txt: Added.
+        * layout-tests/editing/deleting/delete-select-all-001.html: Added.
+        * layout-tests/editing/deleting/delete-select-all-002-expected.txt: Added.
+        * layout-tests/editing/deleting/delete-select-all-002.html: Added.
+        * layout-tests/editing/deleting/delete-select-all-003-expected.txt: Added.
+        * layout-tests/editing/deleting/delete-select-all-003.html: Added.
+
+2004-10-25  Ken Kocienda  <kocienda@apple.com>
+
         Reviewed by me
         
         Added some more editing layout tests.
         Reviewed by me
         
         Added some more editing layout tests.
index bf2b51f..37b8852 100644 (file)
@@ -1296,6 +1296,34 @@ void DeleteSelectionCommand::moveNodesAfterNode(NodeImpl *startNode, NodeImpl *d
     }
 }
 
     }
 }
 
+Position DeleteSelectionCommand::startPositionForDelete() const
+{
+    Position pos = m_selectionToDelete.start();
+    ASSERT(pos.node()->inDocument());
+
+    ElementImpl *rootElement = pos.node()->rootEditableElement();
+    Position rootStart = Position(rootElement, 0);
+    if (pos == VisiblePosition(rootStart).deepEquivalent())
+        pos = rootStart;
+    else if (m_smartDelete && pos.leadingWhitespacePosition().isNotNull())
+        pos = VisiblePosition(pos).previous().deepEquivalent();
+    return pos;
+}
+
+Position DeleteSelectionCommand::endPositionForDelete() const
+{
+    Position pos = m_selectionToDelete.end();
+    ASSERT(pos.node()->inDocument());
+
+    ElementImpl *rootElement = pos.node()->rootEditableElement();
+    Position rootEnd = Position(rootElement, rootElement ? rootElement->childNodeCount() : 0).equivalentDeepPosition();
+    if (pos == VisiblePosition(rootEnd).deepEquivalent())
+        pos = rootEnd;
+    else if (m_smartDelete && pos.leadingWhitespacePosition().isNotNull())
+        pos = VisiblePosition(pos).next().deepEquivalent();
+    return pos;
+}
+
 void DeleteSelectionCommand::doApply()
 {
     // If selection has not been set to a custom selection when the command was created,
 void DeleteSelectionCommand::doApply()
 {
     // If selection has not been set to a custom selection when the command was created,
@@ -1306,21 +1334,12 @@ void DeleteSelectionCommand::doApply()
     if (!m_selectionToDelete.isRange())
         return;
 
     if (!m_selectionToDelete.isRange())
         return;
 
-    ASSERT(m_selectionToDelete.start().node()->inDocument());
-    ASSERT(m_selectionToDelete.end().node()->inDocument());
-
-    if (m_smartDelete) {
-        if (!m_selectionToDelete.start().leadingWhitespacePosition().isNull()) {
-            m_selectionToDelete.modify(Selection::EXTEND, Selection::LEFT, CHARACTER);
-        } else if (!m_selectionToDelete.end().trailingWhitespacePosition().isNull()) {
-            m_selectionToDelete.modify(Selection::EXTEND, Selection::RIGHT, CHARACTER);
-        }
-    }
-    
-    Position upstreamStart(m_selectionToDelete.start().upstream(StayInBlock));
-    Position downstreamStart(m_selectionToDelete.start().downstream(StayInBlock));
-    Position upstreamEnd(m_selectionToDelete.end().upstream(StayInBlock));
-    Position downstreamEnd(m_selectionToDelete.end().downstream(StayInBlock));
+    Position start = startPositionForDelete();
+    Position end = endPositionForDelete();
+    Position upstreamStart(start.upstream(StayInBlock));
+    Position downstreamStart(start.downstream(StayInBlock));
+    Position upstreamEnd(end.upstream(StayInBlock));
+    Position downstreamEnd(end.downstream(StayInBlock));
     Position endingPosition;
 
     // Save away whitespace situation before doing any deletions
     Position endingPosition;
 
     // Save away whitespace situation before doing any deletions
index 6fadb20..5239e79 100644 (file)
@@ -273,9 +273,9 @@ public:
 private:
     virtual bool preservesTypingStyle() const;
 
 private:
     virtual bool preservesTypingStyle() const;
 
-    void deleteDownstreamWS(const DOM::Position &start);
-    bool containsOnlyWhitespace(const DOM::Position &start, const DOM::Position &end);
     void moveNodesAfterNode(DOM::NodeImpl *startNode, DOM::NodeImpl *dstNode);
     void moveNodesAfterNode(DOM::NodeImpl *startNode, DOM::NodeImpl *dstNode);
+    DOM::Position startPositionForDelete() const;
+    DOM::Position endPositionForDelete() const;
 
     khtml::Selection m_selectionToDelete;
     bool m_hasSelectionToDelete;
 
     khtml::Selection m_selectionToDelete;
     bool m_hasSelectionToDelete;