LayoutTests:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Apr 2006 00:57:02 +0000 (00:57 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Apr 2006 00:57:02 +0000 (00:57 +0000)
        Reviewed by harrison

        * editing/deleting/merge-unrendered-space-expected.checksum: Added.
        * editing/deleting/merge-unrendered-space-expected.png: Added.
        * editing/deleting/merge-unrendered-space-expected.txt: Added.
        * editing/deleting/merge-unrendered-space.html: Added.

WebCore:

        Reviewed by harrison

        Some setup for work on paste performance.

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::moveParagraph):
        Moved code from mergeParagraphs so that it can be used in ReplaceSelectionCommand.
        * editing/CompositeEditCommand.h:
        * editing/DeleteSelectionCommand.cpp:
        (WebCore::DeleteSelectionCommand::mergeParagraphs):
        (WebCore::DeleteSelectionCommand::doApply):
        * editing/VisiblePosition.cpp:
        (WebCore::VisiblePosition::init): Put the code that chooses m_deepPosition into initDeepPosition.
        (WebCore::VisiblePosition::initDeepPosition):
        Fixed a bug: don't fall through to the code that's only for positions inside unrendered space between blocks when
        there's a candidate downstream() is a candidate.  Added a comment about why the fall through code is necessary.
        * editing/VisiblePosition.h:

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

LayoutTests/ChangeLog
LayoutTests/editing/deleting/merge-unrendered-space-expected.checksum [new file with mode: 0644]
LayoutTests/editing/deleting/merge-unrendered-space-expected.png [new file with mode: 0644]
LayoutTests/editing/deleting/merge-unrendered-space-expected.txt [new file with mode: 0644]
LayoutTests/editing/deleting/merge-unrendered-space.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/CompositeEditCommand.h
WebCore/editing/DeleteSelectionCommand.cpp
WebCore/editing/VisiblePosition.cpp
WebCore/editing/VisiblePosition.h

index a5f6c39977bda1af2e34c463403a4213283502ef..3c868a436071bd72cf987fe1d15ade29f05fdbe3 100644 (file)
@@ -1,3 +1,12 @@
+2006-04-11  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by harrison
+
+        * editing/deleting/merge-unrendered-space-expected.checksum: Added.
+        * editing/deleting/merge-unrendered-space-expected.png: Added.
+        * editing/deleting/merge-unrendered-space-expected.txt: Added.
+        * editing/deleting/merge-unrendered-space.html: Added.
+
 2006-04-10  Darin Adler  <darin@apple.com>
 
         * fast/dom/gc-4-expected.txt: Add one blank line (test result was failing).
diff --git a/LayoutTests/editing/deleting/merge-unrendered-space-expected.checksum b/LayoutTests/editing/deleting/merge-unrendered-space-expected.checksum
new file mode 100644 (file)
index 0000000..327e03f
--- /dev/null
@@ -0,0 +1 @@
+330385e63988b427faaae197a4a81494
\ No newline at end of file
diff --git a/LayoutTests/editing/deleting/merge-unrendered-space-expected.png b/LayoutTests/editing/deleting/merge-unrendered-space-expected.png
new file mode 100644 (file)
index 0000000..4725f01
Binary files /dev/null and b/LayoutTests/editing/deleting/merge-unrendered-space-expected.png differ
diff --git a/LayoutTests/editing/deleting/merge-unrendered-space-expected.txt b/LayoutTests/editing/deleting/merge-unrendered-space-expected.txt
new file mode 100644 (file)
index 0000000..c212652
--- /dev/null
@@ -0,0 +1,25 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 4 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 3 of #text > DIV > BODY > HTML > #document to 3 of #text > DIV > BODY > HTML > #document toDOMRange:range from 3 of #text > DIV > BODY > HTML > #document to 3 of #text > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+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 {P} at (0,0) size 784x18
+        RenderText {TEXT} at (0,0) size 534x18
+          text run at (0,0) width 534: "This tests deleting the line break before a paragraph that ends with unrendered space."
+      RenderBlock {DIV} at (0,34) size 784x36
+        RenderBlock (anonymous) at (0,0) size 784x18
+          RenderText {TEXT} at (0,0) size 21x18
+            text run at (0,0) width 21: "foo"
+          RenderText {TEXT} at (21,0) size 20x18
+            text run at (21,0) width 20: "bar"
+        RenderBlock {DIV} at (0,18) size 784x18
+          RenderText {TEXT} at (0,0) size 22x18
+            text run at (0,0) width 22: "baz"
+caret: position 3 of child 0 {TEXT} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/deleting/merge-unrendered-space.html b/LayoutTests/editing/deleting/merge-unrendered-space.html
new file mode 100644 (file)
index 0000000..40c7305
--- /dev/null
@@ -0,0 +1,12 @@
+<p>This tests deleting the line break before a paragraph that ends with unrendered space.</p>
+<div id="test" contenteditable="true">foo<br>bar
+<div>baz</div></div>
+
+<script>
+var e = document.getElementById("test");
+var s = window.getSelection();
+
+s.setPosition(e, 0);
+s.modify("move", "forward", "paragraph");
+document.execCommand("Delete");
+</script>
\ No newline at end of file
index c6232205a3f3dbbec35a7c4f7dd9798ac2521179..d206e1a3084ca47d95182e9aecbe7f7b74022b06 100644 (file)
@@ -1,3 +1,23 @@
+2006-04-11  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by harrison
+        
+        Some setup for work on paste performance.
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::moveParagraph): 
+        Moved code from mergeParagraphs so that it can be used in ReplaceSelectionCommand.
+        * editing/CompositeEditCommand.h:
+        * editing/DeleteSelectionCommand.cpp:
+        (WebCore::DeleteSelectionCommand::mergeParagraphs):
+        (WebCore::DeleteSelectionCommand::doApply):
+        * editing/VisiblePosition.cpp:
+        (WebCore::VisiblePosition::init): Put the code that chooses m_deepPosition into initDeepPosition.
+        (WebCore::VisiblePosition::initDeepPosition): 
+        Fixed a bug: don't fall through to the code that's only for positions inside unrendered space between blocks when
+        there's a candidate downstream() is a candidate.  Added a comment about why the fall through code is necessary.
+        * editing/VisiblePosition.h:
+
 2006-04-11  John Sullivan  <sullivan@apple.com>
 
         Reviewed by Darin Adler.
index d1e44631ba281b150b947ba385781d12adfd3fad..72571577b11d4293b66933fcedf1ee61bf579b9f 100644 (file)
@@ -31,6 +31,7 @@
 #include "DeleteFromTextNodeCommand.h"
 #include "DeleteSelectionCommand.h"
 #include "Document.h"
+#include "DocumentFragment.h"
 #include "Element.h"
 #include "HTMLNames.h"
 #include "InlineTextBox.h"
@@ -39,6 +40,7 @@
 #include "InsertParagraphSeparatorCommand.h"
 #include "InsertTextCommand.h"
 #include "JoinTextNodesCommand.h"
+#include "markup.h"
 #include "MergeIdenticalElementsCommand.h"
 #include "Range.h"
 #include "RebalanceWhitespaceCommand.h"
@@ -46,6 +48,7 @@
 #include "RemoveNodeAttributeCommand.h"
 #include "RemoveNodeCommand.h"
 #include "RemoveNodePreservingChildrenCommand.h"
+#include "ReplaceSelectionCommand.h"
 #include "SetNodeAttributeCommand.h"
 #include "SplitElementCommand.h"
 #include "SplitTextNodeCommand.h"
@@ -659,6 +662,53 @@ void CompositeEditCommand::pushPartiallySelectedAnchorElementsDown()
     setEndingSelection(originalSelection);
 }
 
+// This moves a paragraph preserving its style.
+void CompositeEditCommand::moveParagraph(const VisiblePosition& startOfParagraphToMove, const VisiblePosition& endOfParagraphToMove, const VisiblePosition& destination)
+{
+    if (startOfParagraphToMove == destination)
+        return;
+    
+    ASSERT(isStartOfParagraph(startOfParagraphToMove));
+    ASSERT(isEndOfParagraph(endOfParagraphToMove));
+    
+    Position start = startOfParagraphToMove.deepEquivalent().upstream();
+    // We upstream() the end so that we don't include collapsed whitespace in the move.
+    // If we must later add a br after the moved paragraph, doing so would cause the moved unrendered space to become rendered.
+    Position end = endOfParagraphToMove.deepEquivalent().upstream();
+    RefPtr<Range> range = new Range(document(), start.node(), start.offset(), end.node(), end.offset());
+
+    // FIXME: This is an inefficient way to preserve style on nodes in the paragraph to move.  It 
+    // shouldn't matter though, since moved paragraphs will usually be quite small.
+    RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(document(), range->toHTML(), "");
+    
+    setEndingSelection(Selection(start, end, DOWNSTREAM));
+    deleteSelection(false, false);
+    
+    ASSERT(destination.deepEquivalent().node()->inDocument());
+    
+    // Deleting a paragraph leaves a placeholder (it always does when a whole paragraph is deleted).
+    // We remove it and prune it's parents since we want to remove all traces of the paragraph we're moving.
+    Node* placeholder = endingSelection().end().node();
+    if (placeholder->hasTagName(brTag))
+        removeNodeAndPruneAncestors(placeholder);
+    // FIXME: Deletion has bugs and it doesn't always add a placeholder.  If it fails, still do pruning.
+    else
+        prune(placeholder);
+
+    // Add a br if pruning an empty block level element caused a collapse.  For example:
+    // foo^
+    // <div>bar</div>
+    // baz
+    // Imagine moving 'bar' to ^.  'bar' will be deleted and its div pruned.  That would
+    // cause 'baz' to collapse onto the line with 'foobar' unless we insert a br.
+    if (!isEndOfParagraph(destination))
+        insertNodeAt(createBreakElement(document()).get(), destination.deepEquivalent().node(), destination.deepEquivalent().offset());
+    
+    setEndingSelection(destination);
+    EditCommandPtr cmd(new ReplaceSelectionCommand(document(), fragment.get(), false));
+    applyCommandToComposite(cmd);
+}
+
 PassRefPtr<Element> createBlockPlaceholderElement(Document* document)
 {
     ExceptionCode ec = 0;
index 1abcf8125ea7c7ac0f0737c0326acbedc2a0200d..51e32963c54d43ec73d327a661086c411dcb811f 100644 (file)
@@ -101,6 +101,8 @@ protected:
     
     void pushAnchorElementDown(WebCore::Node*);
     void pushPartiallySelectedAnchorElementsDown();
+    
+    void moveParagraph(const VisiblePosition&, const VisiblePosition&, const VisiblePosition&);
 
     DeprecatedValueList<EditCommandPtr> m_cmds;
 };
index a06c28d05ebb6d387c4895fe1b9b2290102830dc..9111960edc3fc10dbcb280aaf34a90bc4e9c941c 100644 (file)
@@ -514,39 +514,7 @@ void DeleteSelectionCommand::mergeParagraphs()
         
     VisiblePosition endOfParagraphToMove = endOfParagraph(startOfParagraphToMove);
     
-    Position start = startOfParagraphToMove.deepEquivalent().upstream();
-    // We upstream() the end so that we don't include collapsed whitespace in the move.
-    // If we must later add a br after the merged paragraph, doing so would cause the moved unrendered space to become rendered.
-    Position end = endOfParagraphToMove.deepEquivalent().upstream();
-    RefPtr<Range> range = new Range(document(), start.node(), start.offset(), end.node(), end.offset());
-
-    // FIXME: This is an inefficient way to preserve style on nodes in the paragraph to move.  It 
-    // shouldn't matter though, since moved paragraphs will usually be quite small.
-    RefPtr<DocumentFragment> fragment = createFragmentFromMarkup(document(), range->toHTML(), "");
-    
-    setEndingSelection(Selection(startOfParagraphToMove.deepEquivalent(), endOfParagraphToMove.deepEquivalent(), DOWNSTREAM));
-    deleteSelection(false, false);
-    
-    // The above deletion leaves a placeholder (it always does when a whole paragraph is deleted).
-    // We remove it and prune it's parents since we want to remove all traces of the paragraph to move.
-    Node* placeholder = endingSelection().end().node();
-    // FIXME: Deletion has bugs and it doesn't always add a placeholder.  If it fails, still do pruning.
-    if (placeholder->hasTagName(brTag))
-        removeNodeAndPruneAncestors(placeholder);
-    else
-        prune(placeholder);
-
-    // Add a br if pruning an empty block level element caused a collapse.  For example:
-    // foo
-    // <div>bar</div>
-    // baz
-    // Placing the cursor before 'bar' and hitting delete will merge 'foo' and 'bar' and prune the empty div.    
-    if (!isEndOfParagraph(mergeDestination))
-        insertNodeAt(createBreakElement(document()).get(), m_upstreamStart.node(), m_upstreamStart.offset());
-    
-    setEndingSelection(mergeDestination);
-    EditCommandPtr cmd(new ReplaceSelectionCommand(document(), fragment.get(), false));
-    applyCommandToComposite(cmd);
+    moveParagraph(startOfParagraphToMove, endOfParagraphToMove, mergeDestination);
 }
 
 void DeleteSelectionCommand::calculateEndingPosition()
@@ -648,8 +616,8 @@ void DeleteSelectionCommand::doApply()
 
     // if all we are deleting is complete paragraph(s), we need to make
     // sure a blank paragraph remains when we are done
-    bool forceBlankParagraph = isStartOfParagraph(VisiblePosition(m_upstreamStart, VP_DEFAULT_AFFINITY)) &&
-                               isEndOfParagraph(VisiblePosition(m_downstreamEnd, VP_DEFAULT_AFFINITY));
+    bool forceBlankParagraph = isStartOfParagraph(VisiblePosition(m_upstreamStart)) &&
+                               isEndOfParagraph(VisiblePosition(m_downstreamEnd));
 
     // Delete any text that may hinder our ability to fixup whitespace after the detele
     deleteInsignificantTextDownstream(m_trailingWhitespace);    
index 7b828c301b0466b54643c605ea01885ba99ed7d0..a286d34cf948f76c23afc8301c59f5526a5bae7a 100644 (file)
@@ -49,48 +49,14 @@ VisiblePosition::VisiblePosition(Node *node, int offset, EAffinity affinity)
     init(Position(node, offset), affinity);
 }
 
-void VisiblePosition::init(const Position &pos, EAffinity affinity)
+void VisiblePosition::init(const Position& position, EAffinity affinity)
 {
     m_affinity = affinity;
     
-    if (pos.isNull()) {
-        m_deepPosition = Position();
-        return;
-    }
+    initDeepPosition(position, affinity);
     
-    pos.node()->document()->updateLayoutIgnorePendingStylesheets();
-    Position deepPos = deepEquivalent(pos);
-    if (deepPos.inRenderedContent()) {
-        // If two visually equivalent positions are both candidates for being made the m_deepPosition,
-        // (this can happen when two rendered positions have only collapsed whitespace between them for example),
-        // we always choose the one that occurs first in the DOM to canonicalize VisiblePositions.
-        Position upstreamPosition = deepPos.upstream();
-        m_deepPosition = upstreamPosition.inRenderedContent() ? upstreamPosition : deepPos;
-    } else {
-        Position next = nextVisiblePosition(deepPos);
-        Position prev = previousVisiblePosition(deepPos);
-        
-        if (next.isNull() && prev.isNull())
-            m_deepPosition = Position();
-        else if (next.isNull())
-            m_deepPosition = prev;
-        else if (prev.isNull())
-            m_deepPosition = next;
-        else {
-            Node *originalBlock = pos.node() ? pos.node()->enclosingBlockFlowElement() : 0;
-            bool nextIsOutsideOriginalBlock = !next.node()->isAncestor(originalBlock) && next.node() != originalBlock;
-            bool prevIsOutsideOriginalBlock = !prev.node()->isAncestor(originalBlock) && prev.node() != originalBlock;
-            
-            if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock || 
-                (deepPos.node()->hasTagName(brTag) && deepPos.offset() == 0))
-                m_deepPosition = prev;
-            else
-                m_deepPosition = next;
-        }
-    }
-
     // When not at a line wrap, make sure to end up with DOWNSTREAM affinity.
-    if (m_affinity == UPSTREAM && (isNull() || inSameLine(VisiblePosition(pos, DOWNSTREAM), *this)))
+    if (m_affinity == UPSTREAM && (isNull() || inSameLine(VisiblePosition(position, DOWNSTREAM), *this)))
         m_affinity = DOWNSTREAM;
 }
 
@@ -163,6 +129,53 @@ Position VisiblePosition::nextVisiblePosition(const Position &pos)
     return Position();
 }
 
+void VisiblePosition::initDeepPosition(const Position& position, EAffinity affinity)
+{
+    if (position.isNull()) {
+        m_deepPosition = Position();
+        return;
+    }
+
+    position.node()->document()->updateLayoutIgnorePendingStylesheets();
+    Position deepPos = deepEquivalent(position);
+    // If two visually equivalent positions are both candidates for being made the m_deepPosition,
+    // (this can happen when two rendered positions have only collapsed whitespace between them),
+    // we always choose the one that occurs first in the DOM to canonicalize VisiblePositions.
+    m_deepPosition = deepPos.upstream();
+    if (m_deepPosition.inRenderedContent())
+        return;
+    
+    m_deepPosition = deepPos;
+    if (m_deepPosition.inRenderedContent())
+        return;
+
+    m_deepPosition = deepPos.downstream();
+    if (m_deepPosition.inRenderedContent())
+        return;
+        
+    // Do something rational for an input position inside unrendered whitespace that isn't really 
+    // equivalent to any rendered position, such as: <div>foo</div>{unrendered whitespace}<div>bar</div>
+    Position next = nextVisiblePosition(deepPos);
+    Position prev = previousVisiblePosition(deepPos);
+    
+    if (next.isNull() && prev.isNull())
+        m_deepPosition = Position();
+    else if (next.isNull())
+        m_deepPosition = prev;
+    else if (prev.isNull())
+        m_deepPosition = next;
+    else {
+        Node *originalBlock = position.node()->enclosingBlockFlowElement();
+        bool nextIsOutsideOriginalBlock = !next.node()->isAncestor(originalBlock) && next.node() != originalBlock;
+        bool prevIsOutsideOriginalBlock = !prev.node()->isAncestor(originalBlock) && prev.node() != originalBlock;
+        
+        if (nextIsOutsideOriginalBlock && !prevIsOutsideOriginalBlock)
+            m_deepPosition = prev;
+        else
+            m_deepPosition = next;
+    }
+}
+
 Position VisiblePosition::deepEquivalent(const Position &pos)
 {
     Node *node = pos.node();
index ca9bf0c95643247ab9292bfe15e61fab273d9d84..3183243dd2bb4165b2b345938b7ddb0e3c65d3b3 100644 (file)
@@ -79,6 +79,7 @@ public:
     
 private:
     void init(const Position&, EAffinity);
+    void initDeepPosition(const Position&, EAffinity affinity);
 
     static int maxOffset(const Node*);