LayoutTests:
authorsullivan <sullivan@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Dec 2006 00:55:56 +0000 (00:55 +0000)
committersullivan <sullivan@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 1 Dec 2006 00:55:56 +0000 (00:55 +0000)
        Reviewed by Justin

        * editing/execCommand/create-list-from-range-selection-expected.checksum:
        * editing/execCommand/create-list-from-range-selection-expected.png:
        * editing/execCommand/create-list-from-range-selection-expected.txt:
        * editing/execCommand/create-list-from-range-selection.html:
        This test's results were broken by using rangeCompliantEquivalent at
        appropriate places in CompositeEditCommand.cpp::moveParagraphs(), presumably
        by revealing a different bug. Justin is going to look into this; for now I've
        updated the result so that it won't appear to fail for others.

        * editing/execCommand/create-list-with-hr-expected.checksum:
        * editing/execCommand/create-list-with-hr-expected.png:
        * editing/execCommand/create-list-with-hr-expected.txt:
        * editing/execCommand/create-list-with-hr.html:
        This test's results were improved by using rangeCompliantEquivalent at
        appropriate places in CompositeEditCommand.cpp::moveParagraphs() -- it used
        to create an extra <div>, and now it does not.

WebCore:

        Reviewed by Justin

        With Darin, fixed a problem in the Range constructors found while implementing grammar checking.
        That revealed another problem in the layout tests involving bad parameters passed to the Range
        constructors.

        With these fixes in place, one layout test (editing/execCommand/create-list-from-range-selection.html)
        no longer works as intended. This is apparently due to yet another bug being flushed out somewhere.
        I'm going to update the results for that test and file a separate radar about it, which Justin will
        investigate.

        * dom/Position.h:
        removed equivalentRangeCompliantPosition(), which was declared but not implemented or called.

        * dom/Range.cpp:
        (WebCore::Range::Range):
        Call setStart and setEnd in the two Range constructors that take parameters, rather than just
        directly setting the instance variables. This makes Range perform the boundary checks and
        compensations that the DOM spec requires.

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::moveParagraphs):
        Use rangeCompliantEquivalent() on "editing-style" Positions before creating Ranges from them.

        * editing/TextIterator.cpp:
        (WebCore::TextIterator::TextIterator):
        Assert that the boundary points of the range are valid.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/execCommand/create-list-from-range-selection-expected.checksum
LayoutTests/editing/execCommand/create-list-from-range-selection-expected.png
LayoutTests/editing/execCommand/create-list-from-range-selection-expected.txt
LayoutTests/editing/execCommand/create-list-from-range-selection.html
LayoutTests/editing/execCommand/create-list-with-hr-expected.checksum
LayoutTests/editing/execCommand/create-list-with-hr-expected.png
LayoutTests/editing/execCommand/create-list-with-hr-expected.txt
LayoutTests/editing/execCommand/create-list-with-hr.html
WebCore/ChangeLog
WebCore/WebCore.xcodeproj/project.pbxproj
WebCore/dom/Position.h
WebCore/dom/Range.cpp
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/TextIterator.cpp

index 81f6482c637857907e49b3df640941e649ad5499..7389866454844647c3604298e6c7f5a8883da0f1 100644 (file)
@@ -1,3 +1,24 @@
+2006-11-30  John Sullivan  <sullivan@apple.com>
+
+        Reviewed by Justin
+
+        * editing/execCommand/create-list-from-range-selection-expected.checksum:
+        * editing/execCommand/create-list-from-range-selection-expected.png:
+        * editing/execCommand/create-list-from-range-selection-expected.txt:
+        * editing/execCommand/create-list-from-range-selection.html:
+        This test's results were broken by using rangeCompliantEquivalent at
+        appropriate places in CompositeEditCommand.cpp::moveParagraphs(), presumably
+        by revealing a different bug. Justin is going to look into this; for now I've
+        updated the result so that it won't appear to fail for others.
+        
+        * editing/execCommand/create-list-with-hr-expected.checksum:
+        * editing/execCommand/create-list-with-hr-expected.png:
+        * editing/execCommand/create-list-with-hr-expected.txt:
+        * editing/execCommand/create-list-with-hr.html:
+        This test's results were improved by using rangeCompliantEquivalent at
+        appropriate places in CompositeEditCommand.cpp::moveParagraphs() -- it used
+        to create an extra <div>, and now it does not.
+
 2006-11-29  Justin Garcia  <justin.garcia@apple.com>
 
         Reviewed by sullivan
index f743cb3e66bd7aba5eb8e1b171a35cde6c7c9076..33a52b79a0d27deed2b766bedbfa42f9c2adc053 100644 (file)
@@ -1 +1 @@
-df73fe3bb7ebfa11d32d699f1d302fba
\ No newline at end of file
+faa44b9a0e599d64d8ad837d19d613b9
\ No newline at end of file
index da1e7a40eaedfc7a2446980bf8a2b80197759ce8..06f5a4cb4fc7323ddbf4a581060474b992fb5df1 100644 (file)
Binary files a/LayoutTests/editing/execCommand/create-list-from-range-selection-expected.png and b/LayoutTests/editing/execCommand/create-list-from-range-selection-expected.png differ
index 6d228e17f7cd1689ab8fff4940d75a845cd00836..4ea08bcde2fd6d1161dfaf4a99595ab633dd6340 100644 (file)
@@ -10,40 +10,49 @@ layer at (0,0) size 800x600
 layer at (0,0) size 800x600
   RenderBlock {HTML} at (0,0) size 800x600
     RenderBody {BODY} at (8,8) size 784x576
-      RenderBlock {P} at (0,0) size 784x18
-        RenderText {#text} at (0,0) size 632x18
-          text run at (0,0) width 632: "This tests Insert{Un}OrderedList on a range selection that contains a mix of list and non-list content."
-      RenderBlock {DIV} at (0,34) size 784x108
+      RenderBlock {P} at (0,0) size 784x72
+        RenderText {#text} at (0,0) size 636x18
+          text run at (0,0) width 636: "This tests Insert{Un}OrderedList on a range selection that contains a mix of list and non-list content. "
+        RenderInline {B} at (0,0) size 781x72
+          RenderText {#text} at (636,0) size 781x72
+            text run at (636,0) width 138: "This test used to end"
+            text run at (0,18) width 781: "with a flat list from 1 to 6, with a selection extending from just before the last \"o\" in \"1. asdfoo\" to just after the \"a\""
+            text run at (0,36) width 538: "in \"6. baz\". This result changed with John's rangeCompliantEquivalent fixes in"
+            text run at (0,54) width 400: "CompositeEditCommand::moveParagraphs on 11/30/2006."
+      RenderBlock {DIV} at (0,88) size 784x126
         RenderBlock (anonymous) at (0,0) size 784x0
-        RenderBlock {OL} at (0,0) size 784x108
-          RenderListItem {LI} at (40,0) size 744x18
-            RenderListMarker at (-20,0) size 16x18
-            RenderText {#text} at (0,0) size 21x18
-              text run at (0,0) width 21: "asd"
-            RenderInline {SPAN} at (0,0) size 21x18
-              RenderText {#text} at (21,0) size 21x18
-                text run at (21,0) width 21: "foo"
-          RenderListItem {LI} at (40,18) size 744x18
+        RenderBlock {OL} at (0,0) size 784x126
+          RenderListItem {LI} at (40,0) size 744x36
+            RenderBlock (anonymous) at (0,0) size 744x18
+              RenderListMarker at (-20,0) size 16x18
+            RenderBlock {OL} at (0,18) size 744x18
+              RenderListItem {LI} at (40,0) size 704x18
+                RenderListMarker at (-20,0) size 16x18
+                RenderText {#text} at (0,0) size 21x18
+                  text run at (0,0) width 21: "asd"
+                RenderInline {SPAN} at (0,0) size 21x18
+                  RenderText {#text} at (21,0) size 21x18
+                    text run at (21,0) width 21: "foo"
+          RenderListItem {LI} at (40,36) size 744x18
             RenderListMarker at (-20,0) size 16x18
             RenderText {#text} at (0,0) size 20x18
               text run at (0,0) width 20: "bar"
-          RenderListItem {LI} at (40,36) size 744x18
+          RenderListItem {LI} at (40,54) size 744x18
             RenderListMarker at (-20,0) size 16x18
             RenderText {#text} at (0,0) size 22x18
               text run at (0,0) width 22: "baz"
-          RenderListItem {LI} at (40,54) size 744x18
+          RenderListItem {LI} at (40,72) size 744x18
             RenderListMarker at (-20,0) size 16x18
             RenderText {#text} at (0,0) size 21x18
               text run at (0,0) width 21: "foo"
-          RenderListItem {LI} at (40,72) size 744x18
+          RenderListItem {LI} at (40,90) size 744x18
             RenderListMarker at (-20,0) size 16x18
             RenderText {#text} at (0,0) size 20x18
               text run at (0,0) width 20: "bar"
-          RenderListItem {LI} at (40,90) size 744x18
+          RenderListItem {LI} at (40,108) size 744x18
             RenderListMarker at (-20,0) size 16x18
             RenderInline {SPAN} at (0,0) size 22x18
               RenderText {#text} at (0,0) size 22x18
                 text run at (0,0) width 22: "baz"
-        RenderBlock (anonymous) at (0,124) size 784x0
-selection start: position 2 of child 0 {#text} of child 1 {SPAN} of child 0 {LI} of child 0 {OL} of child 2 {DIV} of child 1 {BODY} of child 0 {HTML} of document
-selection end:   position 2 of child 0 {#text} of child 0 {SPAN} of child 5 {LI} of child 0 {OL} of child 2 {DIV} of child 1 {BODY} of child 0 {HTML} of document
+        RenderBlock (anonymous) at (0,142) size 784x0
+caret: position 2 of child 0 {#text} of child 0 {SPAN} of child 5 {LI} of child 0 {OL} of child 2 {DIV} of child 1 {BODY} of child 0 {HTML} of document
index c8c005d347aa073a41b5be34485abc6084481d41..241a8ba6622ed96f44c68e319d5e6d83621c5f4c 100644 (file)
@@ -2,7 +2,10 @@
 if (window.layoutTestController)
      layoutTestController.dumpEditingCallbacks();
 </script>
-<p>This tests Insert{Un}OrderedList on a range selection that contains a mix of list and non-list content.</p>
+<p>This tests Insert{Un}OrderedList on a range selection that contains a mix of list and non-list content.
+<b>This test used to end with a flat list from 1 to 6, with a selection extending from just before the last "o"
+in "1. asdfoo" to just after the "a" in "6. baz". This result changed with John's rangeCompliantEquivalent
+fixes in CompositeEditCommand::moveParagraphs on 11/30/2006.</b></p>
 <div id="test" contenteditable="true">asd<span id="start">foo</span><br><ul><li>bar</li></ul>baz<br>foo<ol><li>bar</li></ol><span id="end">baz</span></div>
 <script>
 var s = window.getSelection();
index 1277b2d61d48d5bfcea74cf3ed5e25096d934844..7a04cabb8c92ba6aabcaf4f31c6f1cf5146ff101 100644 (file)
@@ -1 +1 @@
-3aa8d95ca4314bc71d4ea99e0f639506
\ No newline at end of file
+b2bf164df2a52ffb7be2056a5ec11b36
\ No newline at end of file
index 932c0a3ce4e3b26ced7731e49cbb4d7280fbcaeb..7e4041b88dd17f9d8a169756e4fbfdafb81a2fd8 100644 (file)
Binary files a/LayoutTests/editing/execCommand/create-list-with-hr-expected.png and b/LayoutTests/editing/execCommand/create-list-with-hr-expected.png differ
index 02378e25126ab270a72e8496bc962c1bfa329689..0c4ea423d27a35f700217b65130d67479e9137d5 100644 (file)
@@ -2,7 +2,7 @@ EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML
 EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
-EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 122 of #text > B > P > BODY > HTML > #document to 122 of #text > B > P > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 83 of #text > P > BODY > HTML > #document to 83 of #text > P > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
 layer at (0,0) size 800x600
@@ -10,20 +10,15 @@ layer at (0,0) size 800x600
 layer at (0,0) size 800x600
   RenderBlock {HTML} at (0,0) size 800x600
     RenderBody {BODY} at (8,8) size 784x576
-      RenderBlock {P} at (0,0) size 784x36
-        RenderText {#text} at (0,0) size 514x18
-          text run at (0,0) width 514: "This test pushes a horizontal rule into an unordered list with InsertUnorderedList. "
-        RenderInline {B} at (0,0) size 771x36
-          RenderText {#text} at (514,0) size 771x36
-            text run at (514,0) width 257: "The fact that the horizontal rule is put"
-            text run at (0,18) width 540: "into an unnecessary div when it's pushed into the list might be considered a bug."
-      RenderBlock {DIV} at (0,52) size 784x28
+      RenderBlock {P} at (0,0) size 784x18
+        RenderText {#text} at (0,0) size 510x18
+          text run at (0,0) width 510: "This test pushes a horizontal rule into an unordered list with InsertUnorderedList."
+      RenderBlock {DIV} at (0,34) size 784x28
         RenderBlock {UL} at (0,0) size 784x28
           RenderListItem {LI} at (40,0) size 744x28
             RenderBlock (anonymous) at (0,0) size 744x18
               RenderListMarker at (-17,0) size 7x18
-            RenderBlock {DIV} at (0,26) size 744x2
-              RenderBlock {HR} at (0,0) size 744x2 [border: (1px inset #000000)]
+            RenderBlock {HR} at (0,26) size 744x2 [border: (1px inset #000000)]
             RenderBlock (anonymous) at (0,36) size 744x0
         RenderBlock (anonymous) at (0,44) size 784x0
-caret: position 122 of child 0 {#text} of child 1 {B} of child 0 {P} of child 1 {BODY} of child 0 {HTML} of document
+caret: position 83 of child 0 {#text} of child 0 {P} of child 1 {BODY} of child 0 {HTML} of document
index c1e99d633105073548bae22aeea5a6ed1976995c..c42f27a8dcd1b3cb284ce8cfaef7c8f3b37aa94a 100644 (file)
@@ -2,7 +2,7 @@
 if (window.layoutTestController)
      layoutTestController.dumpEditingCallbacks();
 </script>
-<p>This test pushes a horizontal rule into an unordered list with InsertUnorderedList. <b>The fact that the horizontal rule is put into an unnecessary div when it's pushed into the list might be considered a bug.</b></p>
+<p>This test pushes a horizontal rule into an unordered list with InsertUnorderedList.</p>
 <div contenteditable="true" id="div"><hr></div>
 
 <script>
index 03049bae3fca937a79212656544c12a07207e117..76d04d38cb7e3f5f5e12ae800887bae8f3623edc 100644 (file)
@@ -1,3 +1,33 @@
+2006-11-30  John Sullivan  <sullivan@apple.com>
+
+        Reviewed by Justin
+
+        With Darin, fixed a problem in the Range constructors found while implementing grammar checking.
+        That revealed another problem in the layout tests involving bad parameters passed to the Range
+        constructors.
+        
+        With these fixes in place, one layout test (editing/execCommand/create-list-from-range-selection.html)
+        no longer works as intended. This is apparently due to yet another bug being flushed out somewhere.
+        I'm going to update the results for that test and file a separate radar about it, which Justin will
+        investigate.
+
+        * dom/Position.h:
+        removed equivalentRangeCompliantPosition(), which was declared but not implemented or called.
+        
+        * dom/Range.cpp:
+        (WebCore::Range::Range):
+        Call setStart and setEnd in the two Range constructors that take parameters, rather than just 
+        directly setting the instance variables. This makes Range perform the boundary checks and
+        compensations that the DOM spec requires.
+        
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::moveParagraphs):
+        Use rangeCompliantEquivalent() on "editing-style" Positions before creating Ranges from them.
+        
+        * editing/TextIterator.cpp:
+        (WebCore::TextIterator::TextIterator):
+        Assert that the boundary points of the range are valid.
+
 2006-11-30  Lou Amadio  <lamadio@apple.com>
 
         Reviewed by Dave Hyatt
index 5730ea7141a192c452eb763799746bd88d8aa46b..6491cc919b3729f3928c4cabd3abf8135fb4c531 100644 (file)
                        mainGroup = 0867D691FE84028FC02AAC07 /* WebKit */;
                        productRefGroup = 034768DFFF38A50411DB9C8B /* Products */;
                        projectDirPath = "";
-                       projectRoot = "";
                        targets = (
                                93F198A508245E59001E9ABC /* WebCore */,
                                DD041FBE09D9DDBE0010AF2A /* Derived Sources */,
index 51caab31ad7abade19f5efae912b851dde628958..976a83c73a6faa3a835ab74200622ad8c0d32bde 100644 (file)
@@ -70,7 +70,6 @@ public:
     Position upstream() const;
     Position downstream() const;
     
-    Position equivalentRangeCompliantPosition() const;
     bool inRenderedContent() const;
     bool isRenderedCharacter() const;
     bool rendersInDifferentPosition(const Position &pos) const;
index 6647ee85a743a03f6a389c7f46dcc4ff467199e5..8b7661d8f218c244abdd857dfc302adfbda66566 100644 (file)
@@ -56,22 +56,36 @@ Range::Range(Document* ownerDocument,
               Node* startContainer, int startOffset,
               Node* endContainer, int endOffset)
     : m_ownerDocument(ownerDocument)
-    , m_startContainer(startContainer)
-    , m_startOffset(startOffset)
-    , m_endContainer(endContainer)
-    , m_endOffset(endOffset)
+    , m_startContainer(ownerDocument)
+    , m_startOffset(0)
+    , m_endContainer(ownerDocument)
+    , m_endOffset(0)
     , m_detached(false)
 {
+    // Simply setting the containers and offsets directly would not do any of the checking
+    // that setStart and setEnd do, so we must call those functions.
+    ExceptionCode ec = 0;
+    setStart(startContainer, startOffset, ec);
+    ASSERT(ec == 0);
+    setEnd(endContainer, endOffset, ec);
+    ASSERT(ec == 0);
 }
 
 Range::Range(Document* ownerDocument, const Position& start, const Position& end)
     : m_ownerDocument(ownerDocument)
-    , m_startContainer(start.node())
-    , m_startOffset(start.offset())
-    , m_endContainer(end.node())
-    , m_endOffset(end.offset())
+    , m_startContainer(ownerDocument)
+    , m_startOffset(0)
+    , m_endContainer(ownerDocument)
+    , m_endOffset(0)
     , m_detached(false)
 {
+    // Simply setting the containers and offsets directly would not do any of the checking
+    // that setStart and setEnd do, so we must call those functions.
+    ExceptionCode ec = 0;
+    setStart(start.node(), start.offset(), ec);
+    ASSERT(ec == 0);
+    setEnd(end.node(), end.offset(), ec);
+    ASSERT(ec == 0);
 }
 
 Range::~Range()
index e4dfc4b8ab2145729b273397d2d4b225f9ee5cd9..6931eb31332d8aaa94068849d63931625e863bc9 100644 (file)
@@ -660,7 +660,11 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
     // When we paste a fragment, spaces after the end and before the start are treated as though they were rendered.    
     Position start = startOfParagraphToMove.deepEquivalent().downstream();
     Position end = endOfParagraphToMove.deepEquivalent().upstream();
-    RefPtr<Range> range = new Range(document(), start.node(), start.offset(), end.node(), end.offset());
+    
+    // start and end can't be used directly to create a Range; they are "editing positions"
+    Position startRangeCompliant = rangeCompliantEquivalent(start);
+    Position endRangeCompliant = rangeCompliantEquivalent(end);
+    RefPtr<Range> range = new Range(document(), startRangeCompliant.node(), startRangeCompliant.offset(), endRangeCompliant.node(), endRangeCompliant.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.
@@ -704,7 +708,7 @@ void CompositeEditCommand::moveParagraphs(const VisiblePosition& startOfParagrap
     if (beforeParagraph.isNotNull() && !isEndOfParagraph(beforeParagraph))
         insertNodeAt(createBreakElement(document()).get(), beforeParagraph.deepEquivalent().node(), beforeParagraph.deepEquivalent().offset());
         
-    RefPtr<Range> startToDestinationRange(new Range(document(), Position(document(), 0), destination.deepEquivalent()));
+    RefPtr<Range> startToDestinationRange(new Range(document(), Position(document(), 0), rangeCompliantEquivalent(destination.deepEquivalent())));
     destinationIndex = TextIterator::rangeLength(startToDestinationRange.get());
     
     setEndingSelection(destination);
index 11cfbecd419b1968bc2fbac56b0a6ac008b0f550..756b5c3023020431db14a76d3334931716630d05 100644 (file)
@@ -92,6 +92,10 @@ TextIterator::TextIterator(const Range *r, IteratorKind kind) : m_endContainer(0
     if (ec != 0)
         return;
 
+    // Callers should be handing us well-formed ranges. If we discover that this isn't
+    // the case, we could consider changing this assertion to an early return.
+    ASSERT(r->boundaryPointsValid());
+
     // remember ending place - this does not change
     m_endContainer = endContainer;
     m_endOffset = endOffset;