LayoutTests:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2007 20:42:15 +0000 (20:42 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2007 20:42:15 +0000 (20:42 +0000)
        Reviewed by darin

        Fixed (was missing a newline):
        * editing/execCommand/4917055-expected.txt:
        Tests setting a caet before the image and changing its alignment:
        * editing/execCommand/5080333-1-expected.checksum: Added.
        * editing/execCommand/5080333-1-expected.png: Added.
        * editing/execCommand/5080333-1-expected.txt: Added.
        * editing/execCommand/5080333-1.html: Added.
        Tests selecting the image and changing its alignment:
        * editing/execCommand/5080333-2-expected.checksum: Added.
        * editing/execCommand/5080333-2-expected.png: Added.
        * editing/execCommand/5080333-2-expected.txt: Added.
        * editing/execCommand/5080333-2.html: Added.

WebCore:

        Reviewed by darin

        <rdar://problem/5080333>
        REGRESSION: Selection changes when changing the alignment of an image

        Regression occurred when we started using moveParagraphs
        to move content in applyBlockStyle.  moveParagraphs
        moves by copying, deleting and reinserting content, and
        so must be accompanied by selection preservation code.
        That code uses rangeFromLocationAndLength and rangeLength,
        which use TextIterators, which don't emit anything for images
        and other replaced elements, causing this bug.

        * editing/ApplyStyleCommand.cpp:
        (WebCore::ApplyStyleCommand::applyBlockStyle): Ask rangeLength
        and rangeFromLocationAndLength to request that their
        TextIterators emit spaces for replaced elements.
        Use rangeCompliantEquivalent()s when creating a Range from
        VisiblePositions, since some VisiblePositions have illegal
        deepEquivalent()s.
        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::moveParagraphs): Ditto.
        * editing/TextIterator.cpp:
        (WebCore::TextIterator::TextIterator):
        (WebCore::TextIterator::handleReplacedElement): Emit
        a space if requested.
        (WebCore::TextIterator::representNodeOffsetZero): Emit
        ranges before m_node, not around m_lastTextNode.  These
        ranges should represent the part of the document associated
        with the emitted character.
        (WebCore::TextIterator::rangeLength): Take in the new bool.
        (WebCore::TextIterator::rangeFromLocationAndLength): Ditto.
        Also, don't loop an extra time after finding the end of the
        range when we're looking for zero length ranges.  This appeared
        to be a workaround for the bugs fixed in representNodeOffsetZero
        in this patch.
        * editing/TextIterator.h:

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/4917055-expected.txt
LayoutTests/editing/execCommand/5080333-1-expected.checksum [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-1-expected.png [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-1-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-1.html [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-2-expected.checksum [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-2-expected.png [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-2-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/5080333-2.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/ApplyStyleCommand.cpp
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/TextIterator.cpp
WebCore/editing/TextIterator.h

index da06cee6b845f30181c6d254251e2268f4c8f63d..49370014c21925e01e53cd31f10c0310320381e3 100644 (file)
@@ -1,3 +1,20 @@
+2007-03-23  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by darin
+
+        Fixed (was missing a newline):
+        * editing/execCommand/4917055-expected.txt:
+        Tests setting a caet before the image and changing its alignment:
+        * editing/execCommand/5080333-1-expected.checksum: Added.
+        * editing/execCommand/5080333-1-expected.png: Added.
+        * editing/execCommand/5080333-1-expected.txt: Added.
+        * editing/execCommand/5080333-1.html: Added.
+        Tests selecting the image and changing its alignment:
+        * editing/execCommand/5080333-2-expected.checksum: Added.
+        * editing/execCommand/5080333-2-expected.png: Added.
+        * editing/execCommand/5080333-2-expected.txt: Added.
+        * editing/execCommand/5080333-2.html: Added.
+
 2007-03-23  Adele Peterson  <adele@apple.com>
 
         Reviewed by Darin.
index 1d2f8177a477eab62b7ac24a1590789ac4d92585..1bbe31d9eccab2e25e366da294c2e7bd03f74a56 100644 (file)
@@ -3,3 +3,4 @@ This tests for a hang when performing Insert{Un}OrderedList.
 foo
 bar
 
+
diff --git a/LayoutTests/editing/execCommand/5080333-1-expected.checksum b/LayoutTests/editing/execCommand/5080333-1-expected.checksum
new file mode 100644 (file)
index 0000000..6404449
--- /dev/null
@@ -0,0 +1 @@
+76386b683b85d7193166d8a0022f696a
\ No newline at end of file
diff --git a/LayoutTests/editing/execCommand/5080333-1-expected.png b/LayoutTests/editing/execCommand/5080333-1-expected.png
new file mode 100644 (file)
index 0000000..4d90a5c
Binary files /dev/null and b/LayoutTests/editing/execCommand/5080333-1-expected.png differ
diff --git a/LayoutTests/editing/execCommand/5080333-1-expected.txt b/LayoutTests/editing/execCommand/5080333-1-expected.txt
new file mode 100644 (file)
index 0000000..9afe623
--- /dev/null
@@ -0,0 +1,22 @@
+layer at (0,0) size 800x600
+  RenderView 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 784x54
+        RenderText {#text} at (0,0) size 758x54
+          text run at (0,0) width 737: "This tests for a bug where changing the alignment of an image would result in a selection that wasn't the one that was"
+          text run at (0,18) width 235: "present before the alignment change. "
+          text run at (235,18) width 523: "The image should be centered and the caret should be the same before and after the"
+          text run at (0,36) width 63: "operation."
+      RenderBlock {DIV} at (0,70) size 784x139
+        RenderBlock (anonymous) at (0,0) size 784x18
+          RenderText {#text} at (0,0) size 21x18
+            text run at (0,0) width 21: "foo"
+          RenderBR {BR} at (21,14) size 0x0
+        RenderBlock {DIV} at (0,18) size 784x103
+          RenderImage {IMG} at (354,0) size 76x103
+        RenderBlock (anonymous) at (0,121) size 784x18
+          RenderText {#text} at (0,0) size 22x18
+            text run at (0,0) width 22: "baz"
+caret: position 0 of child 0 {IMG} of child 2 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/execCommand/5080333-1.html b/LayoutTests/editing/execCommand/5080333-1.html
new file mode 100644 (file)
index 0000000..36e51c9
--- /dev/null
@@ -0,0 +1,13 @@
+<p>This tests for a bug where changing the alignment of an image would result in a selection that wasn't the one that was present before the alignment change.  The image should be centered and the caret should be the same before and after the operation.</p>
+<div id="div" contenteditable="true">foo<br><img src="../resources/abe.jpg"><br>baz</div>
+
+<script>
+var div = document.getElementById("div");
+var sel = window.getSelection();
+
+sel.setPosition(div, 0);
+sel.modify("move", "forward", "paragraphBoundary");
+sel.modify("move", "forward", "character");
+
+document.execCommand("JustifyCenter");
+</script>
diff --git a/LayoutTests/editing/execCommand/5080333-2-expected.checksum b/LayoutTests/editing/execCommand/5080333-2-expected.checksum
new file mode 100644 (file)
index 0000000..ce28b35
--- /dev/null
@@ -0,0 +1 @@
+26e7fb895fc46b6efcc6eb14a9da7034
\ No newline at end of file
diff --git a/LayoutTests/editing/execCommand/5080333-2-expected.png b/LayoutTests/editing/execCommand/5080333-2-expected.png
new file mode 100644 (file)
index 0000000..8523b36
Binary files /dev/null and b/LayoutTests/editing/execCommand/5080333-2-expected.png differ
diff --git a/LayoutTests/editing/execCommand/5080333-2-expected.txt b/LayoutTests/editing/execCommand/5080333-2-expected.txt
new file mode 100644 (file)
index 0000000..8916671
--- /dev/null
@@ -0,0 +1,23 @@
+layer at (0,0) size 800x600
+  RenderView 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 784x54
+        RenderText {#text} at (0,0) size 783x54
+          text run at (0,0) width 737: "This tests for a bug where changing the alignment of an image would result in a selection that wasn't the one that was"
+          text run at (0,18) width 235: "present before the alignment change. "
+          text run at (235,18) width 548: "The image should be centered and the selection should be the same before and after the"
+          text run at (0,36) width 63: "operation."
+      RenderBlock {DIV} at (0,70) size 784x139
+        RenderBlock (anonymous) at (0,0) size 784x18
+          RenderText {#text} at (0,0) size 21x18
+            text run at (0,0) width 21: "foo"
+          RenderBR {BR} at (21,14) size 0x0
+        RenderBlock {DIV} at (0,18) size 784x103
+          RenderImage {IMG} at (354,0) size 76x103
+        RenderBlock (anonymous) at (0,121) size 784x18
+          RenderText {#text} at (0,0) size 22x18
+            text run at (0,0) width 22: "baz"
+selection start: position 0 of child 0 {IMG} of child 2 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
+selection end:   position 1 of child 0 {IMG} of child 2 {DIV} of child 2 {DIV} of child 0 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/execCommand/5080333-2.html b/LayoutTests/editing/execCommand/5080333-2.html
new file mode 100644 (file)
index 0000000..81a6a86
--- /dev/null
@@ -0,0 +1,14 @@
+<p>This tests for a bug where changing the alignment of an image would result in a selection that wasn't the one that was present before the alignment change.  The image should be centered and the selection should be the same before and after the operation.</p>
+<div id="div" contenteditable="true">foo<br><img src="../resources/abe.jpg"><br>baz</div>
+
+<script>
+var div = document.getElementById("div");
+var sel = window.getSelection();
+
+sel.setPosition(div, 0);
+sel.modify("move", "forward", "paragraphBoundary");
+sel.modify("move", "forward", "character");
+sel.modify("extend", "forward", "character");
+
+document.execCommand("JustifyCenter");
+</script>
index 11766acf01cb14837dcf35baa1d4b75bc05ebdea..6e1aac1f62fcc8d386e60834c2b764f8803552c3 100644 (file)
@@ -1,3 +1,43 @@
+2007-03-23  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by darin
+
+        <rdar://problem/5080333> 
+        REGRESSION: Selection changes when changing the alignment of an image
+        
+        Regression occurred when we started using moveParagraphs
+        to move content in applyBlockStyle.  moveParagraphs 
+        moves by copying, deleting and reinserting content, and
+        so must be accompanied by selection preservation code.
+        That code uses rangeFromLocationAndLength and rangeLength,
+        which use TextIterators, which don't emit anything for images 
+        and other replaced elements, causing this bug.
+
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::applyBlockStyle): Ask rangeLength 
+        and rangeFromLocationAndLength to request that their
+        TextIterators emit spaces for replaced elements.
+        Use rangeCompliantEquivalent()s when creating a Range from
+        VisiblePositions, since some VisiblePositions have illegal
+        deepEquivalent()s.
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::moveParagraphs): Ditto.
+        * editing/TextIterator.cpp:
+        (WebCore::TextIterator::TextIterator):
+        (WebCore::TextIterator::handleReplacedElement): Emit
+        a space if requested.
+        (WebCore::TextIterator::representNodeOffsetZero): Emit
+        ranges before m_node, not around m_lastTextNode.  These
+        ranges should represent the part of the document associated
+        with the emitted character. 
+        (WebCore::TextIterator::rangeLength): Take in the new bool.
+        (WebCore::TextIterator::rangeFromLocationAndLength): Ditto.
+        Also, don't loop an extra time after finding the end of the
+        range when we're looking for zero length ranges.  This appeared
+        to be a workaround for the bugs fixed in representNodeOffsetZero
+        in this patch.
+        * editing/TextIterator.h:
+
 2007-03-24  Mark Rowe  <mrowe@apple.com>
 
         Rubber-stamped by Darin.
index 9f55c1d37d9f02684d7d07c278a2ce48ce3a4c4e..c8d4b58ec4621a9c79cf7b32453dc1f77b55c5d0 100644 (file)
@@ -380,10 +380,10 @@ void ApplyStyleCommand::applyBlockStyle(CSSMutableStyleDeclaration *style)
     // Calculate start and end indices from the start of the tree that they're in.
     Node* scope = highestAncestor(visibleStart.deepEquivalent().node());
     Position rangeStart(scope, 0);
-    RefPtr<Range> startRange = new Range(document(), rangeStart, visibleStart.deepEquivalent());
-    RefPtr<Range> endRange = new Range(document(), rangeStart, visibleEnd.deepEquivalent());
-    int startIndex = TextIterator::rangeLength(startRange.get());
-    int endIndex = TextIterator::rangeLength(endRange.get());
+    RefPtr<Range> startRange = new Range(document(), rangeStart, rangeCompliantEquivalent(visibleStart.deepEquivalent()));
+    RefPtr<Range> endRange = new Range(document(), rangeStart, rangeCompliantEquivalent(visibleEnd.deepEquivalent()));
+    int startIndex = TextIterator::rangeLength(startRange.get(), true);
+    int endIndex = TextIterator::rangeLength(endRange.get(), true);
     
     VisiblePosition paragraphStart(startOfParagraph(visibleStart));
     VisiblePosition nextParagraphStart(endOfParagraph(paragraphStart).next());
@@ -406,8 +406,8 @@ void ApplyStyleCommand::applyBlockStyle(CSSMutableStyleDeclaration *style)
         nextParagraphStart = endOfParagraph(paragraphStart).next();
     }
     
-    updateStartEnd(TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), startIndex, 0)->startPosition(),
-                   TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), endIndex, 0)->startPosition());
+    updateStartEnd(TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), startIndex, 0, true)->startPosition(),
+                   TextIterator::rangeFromLocationAndLength(static_cast<Element*>(scope), endIndex, 0, true)->startPosition());
 }
 
 #define NoFontDelta (0.0f)
index a9897c81e380ca9fdf6e8930d07fd0c5cde091dd..e2d92e44abd6e88970e2b2dbf590df83c9a8b228 100644 (file)
@@ -707,13 +707,13 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
             startIndex = 0;
             if (startInParagraph) {
                 RefPtr<Range> startRange = new Range(document(), startOfParagraphToMove.deepEquivalent(), visibleStart.deepEquivalent());
-                startIndex = TextIterator::rangeLength(startRange.get());
+                startIndex = TextIterator::rangeLength(startRange.get(), true);
             }
 
             endIndex = 0;
             if (endInParagraph) {
                 RefPtr<Range> endRange = new Range(document(), startOfParagraphToMove.deepEquivalent(), visibleEnd.deepEquivalent());
-                endIndex = TextIterator::rangeLength(endRange.get());
+                endIndex = TextIterator::rangeLength(endRange.get(), true);
             }
         }
     }
@@ -781,14 +781,14 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
     }
         
     RefPtr<Range> startToDestinationRange(new Range(document(), Position(document(), 0), rangeCompliantEquivalent(destination.deepEquivalent())));
-    destinationIndex = TextIterator::rangeLength(startToDestinationRange.get());
+    destinationIndex = TextIterator::rangeLength(startToDestinationRange.get(), true);
     
     setEndingSelection(destination);
     applyCommandToComposite(new ReplaceSelectionCommand(document(), fragment.get(), true, false, !preserveStyle, false));
     
     if (preserveSelection && startIndex != -1) {
-        setEndingSelection(Selection(TextIterator::rangeFromLocationAndLength(document()->documentElement(), destinationIndex + startIndex, 0)->startPosition(), 
-                                     TextIterator::rangeFromLocationAndLength(document()->documentElement(), destinationIndex + endIndex, 0)->startPosition(), 
+        setEndingSelection(Selection(TextIterator::rangeFromLocationAndLength(document()->documentElement(), destinationIndex + startIndex, 0, true)->startPosition(), 
+                                     TextIterator::rangeFromLocationAndLength(document()->documentElement(), destinationIndex + endIndex, 0, true)->startPosition(), 
                                      DOWNSTREAM));
     }
 }
index ba0a62cfaf79ba708e14c68f6cf5f3c416fe11d0..afefb8238cc6d019aacdb5373e4d98d52b9a226c 100644 (file)
@@ -78,7 +78,13 @@ TextIterator::TextIterator() : m_startContainer(0), m_startOffset(0), m_endConta
 {
 }
 
-TextIterator::TextIterator(const Range *r) : m_startContainer(0), m_startOffset(0), m_endContainer(0), m_endOffset(0), m_positionNode(0)
+TextIterator::TextIterator(const Range* r, bool emitSpaceForReplacedElements) 
+    : m_startContainer(0) 
+    , m_startOffset(0)
+    , m_endContainer(0)
+    , m_endOffset(0)
+    , m_positionNode(0)
+    , m_emitSpaceForReplacedElements(emitSpaceForReplacedElements)
 {
     if (!r)
         return;
@@ -343,6 +349,11 @@ bool TextIterator::handleReplacedElement()
 
     m_haveEmitted = true;
     
+    if (m_emitSpaceForReplacedElements) {
+        emitCharacter(' ', m_node->parentNode(), m_node, 0, 1);
+        return true;
+    }
+    
     m_positionNode = m_node->parentNode();
     m_positionOffsetBaseNode = m_node;
     m_positionStartOffset = 0;
@@ -522,11 +533,11 @@ void TextIterator::representNodeOffsetZero()
         return;
     
     if (shouldEmitTabBeforeNode(m_node))
-        emitCharacter('\t', m_lastTextNode->parentNode(), m_lastTextNode, 0, 1);
+        emitCharacter('\t', m_node->parentNode(), m_node, 0, 0);
     else if (shouldEmitNewlineBeforeNode(m_node))
-        emitCharacter('\n', m_lastTextNode->parentNode(), m_lastTextNode, 0, 1);
+        emitCharacter('\n', m_node->parentNode(), m_node, 0, 0);
     else if (shouldEmitSpaceBeforeAndAfterNode(m_node))
-        emitCharacter(' ', m_lastTextNode->parentNode(), m_lastTextNode, 0, 1);
+        emitCharacter(' ', m_node->parentNode(), m_node, 0, 0);
 }
 
 bool TextIterator::handleNonTextNode()
@@ -1095,10 +1106,10 @@ bool CircularSearchBuffer::isMatch() const
         && memcmp(m_buffer, m_target.characters() + tailSpace, headSpace * sizeof(UChar)) == 0;
 }
 
-int TextIterator::rangeLength(const Range *r)
+int TextIterator::rangeLength(const Range *r, bool spacesForReplacedElements)
 {
     int length = 0;
-    for (TextIterator it(r); !it.atEnd(); it.advance())
+    for (TextIterator it(r, spacesForReplacedElements); !it.atEnd(); it.advance())
         length += it.length();
     
     return length;
@@ -1125,7 +1136,7 @@ PassRefPtr<Range> TextIterator::subrange(Range* entireRange, int characterOffset
     return result.release();
 }
 
-PassRefPtr<Range> TextIterator::rangeFromLocationAndLength(Element *scope, int rangeLocation, int rangeLength)
+PassRefPtr<Range> TextIterator::rangeFromLocationAndLength(Element *scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements)
 {
     RefPtr<Range> resultRange = scope->document()->createRange();
 
@@ -1135,7 +1146,7 @@ PassRefPtr<Range> TextIterator::rangeFromLocationAndLength(Element *scope, int r
 
     RefPtr<Range> textRunRange;
 
-    TextIterator it(rangeOfContents(scope).get());
+    TextIterator it(rangeOfContents(scope).get(), spacesForReplacedElements);
     
     // FIXME: the atEnd() check shouldn't be necessary, workaround for <http://bugs.webkit.org/show_bug.cgi?id=6289>.
     if (rangeLocation == 0 && rangeLength == 0 && it.atEnd()) {
@@ -1197,10 +1208,8 @@ PassRefPtr<Range> TextIterator::rangeFromLocationAndLength(Element *scope, int r
                 else
                     resultRange->setEnd(textRunRange->endContainer(exception), textRunRange->endOffset(exception), exception);
             }
-            if (!(rangeLength == 0 && rangeEnd == docTextPosition + len)) {
-                docTextPosition += len;
-                break;
-            }
+            docTextPosition += len;
+            break;
         }
         docTextPosition += len;
     }
index e94ffe588b152938143b03cd1c4398631ddc646a..ffed79e4977d96d27b4491be9070f5768ab1b541 100644 (file)
@@ -58,7 +58,7 @@ class TextIterator
 {
 public:
     TextIterator();
-    explicit TextIterator(const Range *);
+    explicit TextIterator(const Range*, bool emitSpaceForReplacedElements = false);
     
     bool atEnd() const { return !m_positionNode; }
     void advance();
@@ -68,8 +68,8 @@ public:
     
     PassRefPtr<Range> range() const;
      
-    static int rangeLength(const Range *r);
-    static PassRefPtr<Range> rangeFromLocationAndLength(Element *scope, int rangeLocation, int rangeLength);
+    static int rangeLength(const Range*, bool spacesForReplacedElements = false);
+    static PassRefPtr<Range> rangeFromLocationAndLength(Element* scope, int rangeLocation, int rangeLength, bool spacesForReplacedElements = false);
     static PassRefPtr<Range> subrange(Range* entireRange, int characterOffset, int characterCount);
     
 private:
@@ -124,6 +124,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_emitSpaceForReplacedElements;
 };
 
 // Iterates through the DOM range, returning all the text, and 0-length boundaries