LayoutTests:
authorjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Mar 2006 11:30:44 +0000 (11:30 +0000)
committerjusting <justing@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 28 Mar 2006 11:30:44 +0000 (11:30 +0000)
        Reviewed by mjs

        <http://bugzilla.opendarwin.org/show_bug.cgi?id=7152>
        REGRESSION: Select All does not highlight table if it's last in the document

        * editing/selection/7152-1-expected.checksum: Added.
        * editing/selection/7152-1-expected.png: Added.
        * editing/selection/7152-1-expected.txt: Added.
        * editing/selection/7152-1.html: Added.
        * editing/selection/7152-2-expected.checksum: Added.
        * editing/selection/7152-2-expected.png: Added.
        * editing/selection/7152-2-expected.txt: Added.
        * editing/selection/7152-2.html: Added.

WebCore:

        Reviewed by mjs

        <http://bugzilla.opendarwin.org/attachment.cgi?id=7322>
        REGRESSION: Select All does not highlight table if it's last in the document

        * rendering/RenderCanvas.cpp:
        (WebCore::rendererAfterPosition):
        Added, returns the render object that a pre-order traversal over a range
        of render objects ending at the input position should stop at.
        (WebCore::RenderCanvas::selectionRect):
        Stop at rendererAfterPosition(m_selectionEnd, m_selectionEndPos), moved code
        for traversal to nextInPreOrder. Also, the travesal doesn't need to fetch the
        next object before doing work, since the work it does will never change what
        the next object in the traversal will be.
        (WebCore::RenderCanvas::setSelection): Ditto.
        * rendering/RenderObject.cpp:
        (WebCore::RenderObject::nextInPreOrder): Renamed from nextRenderer, cleaned up the logic a little.
        (WebCore::RenderObject::nextInPreOrderAfterChildren): Added.
        (WebCore::RenderObject::previousInPreOrder): Renamed from previousRenderer.
        (WebCore::RenderObject::childAt): Added.
        * rendering/RenderObject.h:
        * rendering/RenderText.cpp:
        (WebCore::RenderText::setText):

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/editing/selection/7152-1-expected.checksum [new file with mode: 0644]
LayoutTests/editing/selection/7152-1-expected.png [new file with mode: 0644]
LayoutTests/editing/selection/7152-1-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/7152-1.html [new file with mode: 0644]
LayoutTests/editing/selection/7152-2-expected.checksum [new file with mode: 0644]
LayoutTests/editing/selection/7152-2-expected.png [new file with mode: 0644]
LayoutTests/editing/selection/7152-2-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/7152-2.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/rendering/RenderCanvas.cpp
WebCore/rendering/RenderObject.cpp
WebCore/rendering/RenderObject.h
WebCore/rendering/RenderText.cpp

index 6a6a500b5e5787ce8e5d496a0c7043fa1cf17475..9dd13db735f817577da6223ff22151554520d297 100644 (file)
@@ -1,3 +1,19 @@
+2006-03-28  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by mjs
+        
+        <http://bugzilla.opendarwin.org/show_bug.cgi?id=7152>
+        REGRESSION: Select All does not highlight table if it's last in the document
+
+        * editing/selection/7152-1-expected.checksum: Added.
+        * editing/selection/7152-1-expected.png: Added.
+        * editing/selection/7152-1-expected.txt: Added.
+        * editing/selection/7152-1.html: Added.
+        * editing/selection/7152-2-expected.checksum: Added.
+        * editing/selection/7152-2-expected.png: Added.
+        * editing/selection/7152-2-expected.txt: Added.
+        * editing/selection/7152-2.html: Added.
+        
 2006-03-27  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Beth.
diff --git a/LayoutTests/editing/selection/7152-1-expected.checksum b/LayoutTests/editing/selection/7152-1-expected.checksum
new file mode 100644 (file)
index 0000000..fe80a70
--- /dev/null
@@ -0,0 +1 @@
+aabfc2c0993d499c50967eb71d2999fb
\ No newline at end of file
diff --git a/LayoutTests/editing/selection/7152-1-expected.png b/LayoutTests/editing/selection/7152-1-expected.png
new file mode 100644 (file)
index 0000000..cb49206
Binary files /dev/null and b/LayoutTests/editing/selection/7152-1-expected.png differ
diff --git a/LayoutTests/editing/selection/7152-1-expected.txt b/LayoutTests/editing/selection/7152-1-expected.txt
new file mode 100644 (file)
index 0000000..296d0d5
--- /dev/null
@@ -0,0 +1,34 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of BODY > HTML > #document to 7 of BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of P > BODY > HTML > #document to 0 of P > BODY > HTML > #document toDOMRange:range from 0 of #text > P > BODY > HTML > #document to 1 of TABLE > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+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 784x54
+        RenderText {TEXT} at (0,0) size 131x18
+          text run at (0,0) width 131: "This is a testcase for "
+        RenderInline {A} at (0,0) size 343x18 [color=#0000EE]
+          RenderText {TEXT} at (131,0) size 343x18
+            text run at (131,0) width 343: "http://bugzilla.opendarwin.org/show_bug.cgi?id=7152"
+        RenderText {TEXT} at (474,0) size 783x54
+          text run at (474,0) width 8: ". "
+          text run at (482,0) width 301: "Adding visible candidates after tables, at [table, "
+          text run at (0,18) width 783: "numberOfChildren], threw RenderCanvas::setSelection for a loop because it assumed the end of a selection would be inside "
+          text run at (0,36) width 100: "an atomic node."
+      RenderBlock {HR} at (0,70) size 784x2 [border: (1px inset #000000)]
+      RenderTable {TABLE} at (0,80) size 161x52 [border: (1px outset #808080)]
+        RenderTableSection {TBODY} at (1,1) size 0x50
+          RenderTableRow {TR} at (0,0) size 0x0
+            RenderTableCell {TD} at (2,2) size 155x22 [border: (1px inset #808080)] [r=0 c=0 rs=1 cs=1]
+              RenderText {TEXT} at (2,2) size 151x18
+                text run at (2,2) width 151: "This should be selected."
+          RenderTableRow {TR} at (0,0) size 0x0
+            RenderTableCell {TD} at (2,26) size 155x22 [border: (1px inset #808080)] [r=1 c=0 rs=1 cs=1]
+              RenderText {TEXT} at (2,2) size 151x18
+                text run at (2,2) width 151: "This should be selected."
+selection start: position 0 of child 0 {TEXT} of child 1 {P} of child 1 {BODY} of child 0 {HTML} of document
+selection end:   position 1 of child 5 {TABLE} of child 1 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/selection/7152-1.html b/LayoutTests/editing/selection/7152-1.html
new file mode 100644 (file)
index 0000000..7a91ba3
--- /dev/null
@@ -0,0 +1,15 @@
+<html>
+<head>
+<title>REGRESSION: Select All does not highlight table if it's last in the document</title>
+<script src=../editing.js type="text/javascript"></script>
+<script>
+function editingTest() {
+    selectAllCommand();
+}
+</script>
+</head>
+<body id="test" contenteditable="true" onLoad="runEditingTest();">
+<p>This is a testcase for <a href="http://bugzilla.opendarwin.org/show_bug.cgi?id=7152">http://bugzilla.opendarwin.org/show_bug.cgi?id=7152</a>.  Adding visible candidates after tables, at [table, numberOfChildren], threw RenderCanvas::setSelection for a loop because it assumed the end of a selection would be inside an atomic node.</p>
+<hr>
+<table border="1" ><tr><td>This should be selected.</td></tr><tr><td>This should be selected.</td></tr></table>
+</body></html>
\ No newline at end of file
diff --git a/LayoutTests/editing/selection/7152-2-expected.checksum b/LayoutTests/editing/selection/7152-2-expected.checksum
new file mode 100644 (file)
index 0000000..729ca54
--- /dev/null
@@ -0,0 +1 @@
+21082b043c41a5fff849a17d53165cc5
\ No newline at end of file
diff --git a/LayoutTests/editing/selection/7152-2-expected.png b/LayoutTests/editing/selection/7152-2-expected.png
new file mode 100644 (file)
index 0000000..277e854
Binary files /dev/null and b/LayoutTests/editing/selection/7152-2-expected.png differ
diff --git a/LayoutTests/editing/selection/7152-2-expected.txt b/LayoutTests/editing/selection/7152-2-expected.txt
new file mode 100644 (file)
index 0000000..826c2bd
--- /dev/null
@@ -0,0 +1,36 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of BODY > HTML > #document to 7 of BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:range from 0 of TABLE > BODY > HTML > #document to 0 of TABLE > BODY > HTML > #document toDOMRange:range from 0 of TABLE > BODY > HTML > #document to 343 of #text > P > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+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 784x576
+      RenderTable {TABLE} at (0,0) size 161x52 [border: (1px outset #808080)]
+        RenderTableSection {TBODY} at (1,1) size 0x50
+          RenderTableRow {TR} at (0,0) size 0x0
+            RenderTableCell {TD} at (2,2) size 155x22 [border: (1px inset #808080)] [r=0 c=0 rs=1 cs=1]
+              RenderText {TEXT} at (2,2) size 151x18
+                text run at (2,2) width 151: "This should be selected."
+          RenderTableRow {TR} at (0,0) size 0x0
+            RenderTableCell {TD} at (2,26) size 155x22 [border: (1px inset #808080)] [r=1 c=0 rs=1 cs=1]
+              RenderText {TEXT} at (2,2) size 151x18
+                text run at (2,2) width 151: "This should be selected."
+      RenderBlock {HR} at (0,60) size 784x2 [border: (1px inset #000000)]
+      RenderBlock {P} at (0,78) size 784x72
+        RenderText {TEXT} at (0,0) size 131x18
+          text run at (0,0) width 131: "This is a testcase for "
+        RenderInline {A} at (0,0) size 343x18 [color=#0000EE]
+          RenderText {TEXT} at (131,0) size 343x18
+            text run at (131,0) width 343: "http://bugzilla.opendarwin.org/show_bug.cgi?id=7152"
+        RenderText {TEXT} at (474,0) size 783x72
+          text run at (474,0) width 8: ". "
+          text run at (482,0) width 301: "Adding visible candidates after tables, at [table, "
+          text run at (0,18) width 783: "numberOfChildren], threw RenderCanvas::setSelection for a loop because it assumed the end of a selection would be inside "
+          text run at (0,36) width 104: "an atomic node. "
+          text run at (104,36) width 673: "It didn't make the same assumption about start of a selection, but for good measure, we also test a selection "
+          text run at (0,54) width 232: "that starts at a position before a table."
+selection start: position 0 of child 1 {TABLE} of child 1 {BODY} of child 0 {HTML} of document
+selection end:   position 343 of child 2 {TEXT} of child 5 {P} of child 1 {BODY} of child 0 {HTML} of document
diff --git a/LayoutTests/editing/selection/7152-2.html b/LayoutTests/editing/selection/7152-2.html
new file mode 100644 (file)
index 0000000..5ddae18
--- /dev/null
@@ -0,0 +1,15 @@
+<html>
+<head>
+<title>REGRESSION: Select All does not highlight table if it's last in the document</title>
+<script src=../editing.js type="text/javascript"></script>
+<script>
+function editingTest() {
+    selectAllCommand();
+}
+</script>
+</head>
+<body id="test" contenteditable="true" onLoad="runEditingTest();">
+<table border="1" ><tr><td>This should be selected.</td></tr><tr><td>This should be selected.</td></tr></table>
+<hr>
+<p>This is a testcase for <a href="http://bugzilla.opendarwin.org/show_bug.cgi?id=7152">http://bugzilla.opendarwin.org/show_bug.cgi?id=7152</a>.  Adding visible candidates after tables, at [table, numberOfChildren], threw RenderCanvas::setSelection for a loop because it assumed the end of a selection would be inside an atomic node.  It didn't make the same assumption about start of a selection, but for good measure, we also test a selection that starts at a position before a table.</p>
+</body></html>
\ No newline at end of file
index 3a68744095faf02b135d9824e86d8601b879e106..546379b412eaa69cb7013efbe5cab6ad859eb7c2 100644 (file)
@@ -1,3 +1,29 @@
+2006-03-28  Justin Garcia  <justin.garcia@apple.com>
+
+        Reviewed by mjs
+        
+        <http://bugzilla.opendarwin.org/attachment.cgi?id=7322>
+        REGRESSION: Select All does not highlight table if it's last in the document
+        
+        * rendering/RenderCanvas.cpp:
+        (WebCore::rendererAfterPosition): 
+        Added, returns the render object that a pre-order traversal over a range 
+        of render objects ending at the input position should stop at.
+        (WebCore::RenderCanvas::selectionRect): 
+        Stop at rendererAfterPosition(m_selectionEnd, m_selectionEndPos), moved code 
+        for traversal to nextInPreOrder. Also, the travesal doesn't need to fetch the
+        next object before doing work, since the work it does will never change what 
+        the next object in the traversal will be.
+        (WebCore::RenderCanvas::setSelection): Ditto.
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::nextInPreOrder): Renamed from nextRenderer, cleaned up the logic a little.
+        (WebCore::RenderObject::nextInPreOrderAfterChildren): Added.
+        (WebCore::RenderObject::previousInPreOrder): Renamed from previousRenderer.
+        (WebCore::RenderObject::childAt): Added.
+        * rendering/RenderObject.h:
+        * rendering/RenderText.cpp:
+        (WebCore::RenderText::setText):
+
 2006-03-28  Maciej Stachowiak  <mjs@apple.com>
 
         Reviewed by Justin.
index c8193a639a87a79e1076aa29ff33a684b1072f02..98e360087534761a431d07800d9f01f593252637 100644 (file)
@@ -251,25 +251,22 @@ void RenderCanvas::absoluteRects(DeprecatedValueList<IntRect>& rects, int _tx, i
     rects.append(IntRect(_tx, _ty, m_layer->width(), m_layer->height()));
 }
 
+RenderObject* rendererAfterPosition(RenderObject* object, unsigned offset)
+{
+    if (!object)
+        return 0;
+    RenderObject* child = object->childAt(offset);
+    return child ? child : object->nextInPreOrderAfterChildren();
+}
+
 IntRect RenderCanvas::selectionRect() const
 {
     typedef HashMap<RenderObject*, SelectionInfo*> SelectionMap;
     SelectionMap selectedObjects;
 
     RenderObject* os = m_selectionStart;
-    while (os) {
-        RenderObject* no = 0;
-        if (os != m_selectionEnd) {
-            if (!(no = os->firstChild())) {
-                if (!(no = os->nextSibling())) {
-                    no = os->parent();
-                    while (no && no != m_selectionEnd && !no->nextSibling())
-                        no = no->parent();
-                    if (no && no != m_selectionEnd)
-                        no = no->nextSibling();
-                }
-            }
-        }
+    RenderObject* stop = rendererAfterPosition(m_selectionEnd, m_selectionEndPos);
+    while (os && os != stop) {
         
         if ((os->canBeSelectionLeaf() || os == m_selectionStart || os == m_selectionEnd) && os->selectionState() != SelectionNone) {
             // Blocks are responsible for painting line gaps and margin gaps. They must be examined as well.
@@ -285,7 +282,7 @@ IntRect RenderCanvas::selectionRect() const
             }
         }
         
-        os = no;
+        os = os->nextInPreOrder();
     }
 
     // Now create a single bounding box rect that encloses the whole selection.
@@ -329,20 +326,8 @@ void RenderCanvas::setSelection(RenderObject *s, int sp, RenderObject *e, int ep
     SelectedBlockMap newSelectedBlocks;
 
     RenderObject* os = m_selectionStart;
-    while (os) {
-        RenderObject* no = 0;
-        if (os != m_selectionEnd) {
-            if (!(no = os->firstChild())) {
-                if (!(no = os->nextSibling())) {
-                    no = os->parent();
-                    while (no && no != m_selectionEnd && !no->nextSibling())
-                        no = no->parent();
-                    if (no && no != m_selectionEnd)
-                        no = no->nextSibling();
-                }
-            }
-        }
-
+    RenderObject* stop = rendererAfterPosition(m_selectionEnd, m_selectionEndPos);
+    while (os && os != stop) {
         if ((os->canBeSelectionLeaf() || os == m_selectionStart || os == m_selectionEnd) && os->selectionState() != SelectionNone) {
             // Blocks are responsible for painting line gaps and margin gaps.  They must be examined as well.
             oldSelectedObjects.set(os, new SelectionInfo(os));
@@ -355,8 +340,8 @@ void RenderCanvas::setSelection(RenderObject *s, int sp, RenderObject *e, int ep
                 cb = cb->containingBlock();
             }
         }
-
-        os = no;
+        
+        os = os->nextInPreOrder();
     }
 
     // Now clear the selection.
@@ -381,42 +366,18 @@ void RenderCanvas::setSelection(RenderObject *s, int sp, RenderObject *e, int ep
     }
 
     RenderObject* o = s;
-    while (o) {
-        RenderObject* no = 0;
+    stop = rendererAfterPosition(e, ep);
+    
+    while (o && o != stop) {
         if (o != s && o != e && o->canBeSelectionLeaf())
             o->setSelectionState(SelectionInside);
-        
-        if (o != e) {
-            if (!(no = o->firstChild())) {
-                if ( !(no = o->nextSibling())) {
-                    no = o->parent();
-                    while (no && no != e && !no->nextSibling())
-                        no = no->parent();
-                    if (no && no != e)
-                        no = no->nextSibling();
-                }
-            }
-        }
-        
-        o=no;
+        o = o->nextInPreOrder();
     }
 
     // Now that the selection state has been updated for the new objects, walk them again and
     // put them in the new objects list.
     o = s;
-    while (o) {
-        RenderObject* no = 0;
-        if (o != e) {
-            if (!(no = o->firstChild())) {
-                if ( !(no = o->nextSibling())) {
-                    no = o->parent();
-                    while (no && no != e && !no->nextSibling())
-                        no = no->parent();
-                    if (no && no != e)
-                        no = no->nextSibling();
-                }
-            }
-        }
+    while (o && o != stop) {
         
         if ((o->canBeSelectionLeaf() || o == s || o == e) && o->selectionState() != SelectionNone) {
             newSelectedObjects.set(o, new SelectionInfo(o));
@@ -430,7 +391,7 @@ void RenderCanvas::setSelection(RenderObject *s, int sp, RenderObject *e, int ep
             }
         }
 
-        o=no;
+        o = o->nextInPreOrder();
     }
 
     if (!m_view)
index ab351627355df18d4e9ff2a7c98963d25a36b3a9..e6a4e2c85334c69c832b1458ae59d9cc097da3eb 100644 (file)
@@ -252,35 +252,46 @@ void RenderObject::insertChildNode(RenderObject*, RenderObject*)
     KHTMLAssert(0);
 }
 
-RenderObject *RenderObject::nextRenderer() const
+RenderObject *RenderObject::nextInPreOrder() const
 {
-    if (firstChild())
-        return firstChild();
-
-    if (nextSibling())
-        return nextSibling();
-
-    const RenderObject *r = this;
-    while (r && !r->nextSibling())
-        r = r->parent();
-    if (r)
-        return r->nextSibling();
+    if (RenderObject* o = firstChild())
+        return o;
+        
+    return nextInPreOrderAfterChildren();
+}
 
-    return 0;
+RenderObject* RenderObject::nextInPreOrderAfterChildren() const
+{
+    RenderObject* o;
+    if (!(o = nextSibling())) {
+        o = parent();
+        while (o && !o->nextSibling())
+            o = o->parent();
+        if (o)
+            o = o->nextSibling();
+    }
+    return o;
 }
 
-RenderObject *RenderObject::previousRenderer() const
+RenderObject *RenderObject::previousInPreOrder() const
 {
-    if (previousSibling()) {
-        RenderObject *r = previousSibling();
-        while (r->lastChild())
-            r = r->lastChild();
-        return r;
+    if (RenderObject* o = previousSibling()) {
+        while (o->lastChild())
+            o = o->lastChild();
+        return o;
     }
 
     return parent();
 }
 
+RenderObject* RenderObject::childAt(unsigned index) const
+{
+    RenderObject* child = firstChild();
+    for (unsigned i = 0; child && i < index; i++)
+        child = child->nextSibling();
+    return child;
+}
+
 bool RenderObject::isEditable() const
 {
     RenderText *textRenderer = 0;
index dba4bf5f3212823af6ab48ad22d980e93f74100b..29d1db4e219051071f6abc3ca9abac6255f2d274 100644 (file)
@@ -136,8 +136,10 @@ public:
     virtual RenderObject *firstChild() const { return 0; }
     virtual RenderObject *lastChild() const { return 0; }
 
-    RenderObject *nextRenderer() const; 
-    RenderObject *previousRenderer() const; 
+    RenderObject* nextInPreOrder() const;
+    RenderObject* nextInPreOrderAfterChildren() const;
+    RenderObject* previousInPreOrder() const;
+    RenderObject* childAt(unsigned) const;
 
     RenderObject *nextEditable() const; 
     RenderObject *previousEditable() const; 
index de8ff9e842c87811a523186bbbc02cdeafb1edf4..5cb34e0abefd4f389ef3d30cc5ee64769e4877f0 100644 (file)
@@ -882,7 +882,7 @@ void RenderText::setText(StringImpl *text, bool force)
                     // find previous text renderer if one exists
                     RenderObject* o;
                     QChar previous = ' ';
-                    for (o = previousRenderer(); o && o->isInlineFlow(); o = o->previousRenderer())
+                    for (o = previousInPreOrder(); o && o->isInlineFlow(); o = o->previousInPreOrder())
                         ;
                     if (o && o->isText()) {
                         StringImpl* prevStr = static_cast<RenderText*>(o)->string();