2010-08-11 Julie Parent <jparent@chromium.org>
authorjparent@chromium.org <jparent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Aug 2010 10:30:47 +0000 (10:30 +0000)
committerjparent@chromium.org <jparent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Aug 2010 10:30:47 +0000 (10:30 +0000)
        Reviewed by Justin Garcia.

        Crash in replaceSelectionCommand with RTL text.
        https://bugs.webkit.org/show_bug.cgi?id=41485

        Adds tests for 4 cases: latin text plus space in RTL region and LTR region,
        and hebrew text plus space in RTL region and LTR region.  Currently, the
        latin text in RTL region case crashes.  Added all combinations of test since
        earlier patch crashed with hebrew text in LTR region.

        * editing/execCommand/insert-image-on-top-of-directional-text-expected.txt: Added.
        * editing/execCommand/insert-image-on-top-of-directional-text.html: Added.
2010-08-11  Julie Parent  <jparent@chromium.org>

        Reviewed by Justin Garcia.

        Crash in replaceSelectionCommand with RTL text.
        https://bugs.webkit.org/show_bug.cgi?id=41485

        For text with mixed directionality, sort the text boxes before
        computing gaps, since that code assumes the boxes are in order.

        Test: editing/execCommand/insert-image-on-top-of-directional-text.html

        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::deleteInsignificantText): Sort boxes
        like we do in TextIterator before computing gaps.
        * editing/TextIterator.cpp:
        (WebCore::TextIterator::handleTextNode): Use new compareByStart
        rather than compareBoxStart.  No functional change.
        * rendering/InlineTextBox.h:
        (WebCore::InlineTextBox::compareByStart): Moved compareBoxStart
        from TextIterator here so it can be used in multiple places.

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

LayoutTests/ChangeLog
LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text-expected.txt [new file with mode: 0644]
LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/TextIterator.cpp
WebCore/rendering/InlineTextBox.h

index 4d464dd531357d8f7fa1c979cc6f5277c5147310..8f345604ff04e0c86be9c662e35d656b74d6b390 100644 (file)
@@ -1,3 +1,18 @@
+2010-08-11  Julie Parent  <jparent@chromium.org>
+
+        Reviewed by Justin Garcia.
+
+        Crash in replaceSelectionCommand with RTL text.
+        https://bugs.webkit.org/show_bug.cgi?id=41485
+
+        Adds tests for 4 cases: latin text plus space in RTL region and LTR region,
+        and hebrew text plus space in RTL region and LTR region.  Currently, the
+        latin text in RTL region case crashes.  Added all combinations of test since
+        earlier patch crashed with hebrew text in LTR region.
+        
+        * editing/execCommand/insert-image-on-top-of-directional-text-expected.txt: Added.
+        * editing/execCommand/insert-image-on-top-of-directional-text.html: Added.
+
 2010-08-11  Alejandro G. Castro  <alex@igalia.com>
 
         Reviewed by Dirk Schulze.
diff --git a/LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text-expected.txt b/LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text-expected.txt
new file mode 100644 (file)
index 0000000..4fedc09
--- /dev/null
@@ -0,0 +1,5 @@
+This test uses execCommand('insertImage') to replace the selection in a variety of mixed directionality text. If this doesn't crash, then the test passes.
diff --git a/LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text.html b/LayoutTests/editing/execCommand/insert-image-on-top-of-directional-text.html
new file mode 100644 (file)
index 0000000..887f234
--- /dev/null
@@ -0,0 +1,22 @@
+<div>This test uses execCommand('insertImage') to replace the selection in a variety of mixed directionality text.  If this doesn't crash, then the test passes.</div>\r
+<div dir='rtl' id='rtl1' style='white-space:pre' contentEditable>the </div>\r
+<div dir='rtl' id='rtl2' style='white-space:pre' contentEditable>אחת </div>\r
+<div dir='ltr' id='ltr1' style='white-space:pre' contentEditable>the </div>\r
+<div dir='ltr' id='ltr2' style='white-space:pre' contentEditable>אחת </div>\r
+<script>\r
+if (window.layoutTestController)\r
+     layoutTestController.dumpAsText();\r
+\r
+// Select only "the", not the space, LTR text in RTL region.\r
+document.getSelection().setBaseAndExtent(rtl1.firstChild, 0, rtl1.firstChild, 3);\r
+document.execCommand('InsertImage', false, "../resources/abe.png");\r
+// Select only "אחת", not the space, RTL text in RTL region.\r
+document.getSelection().setBaseAndExtent(rtl2.firstChild, 0, rtl2.firstChild, 3);\r
+document.execCommand('InsertImage', false, "../resources/abe.png");\r
+// Select only "the", not the space, LTR text in LTR region.\r
+document.getSelection().setBaseAndExtent(ltr1.firstChild, 0, ltr1.firstChild, 3);\r
+document.execCommand('InsertImage', false, "../resources/abe.png");\r
+// Select only "אחת", not the space, RTL text in LTR region.\r
+document.getSelection().setBaseAndExtent(ltr2.firstChild, 0, ltr2.firstChild, 3);\r
+document.execCommand('InsertImage', false, "../resources/abe.png");\r
+</script>\r
index 1cf2780b31b5e54e30d3a1b2e52589da49a37a22..8ebfc352e104057f2f6a28390b8201e1b890d54b 100644 (file)
@@ -1,3 +1,25 @@
+2010-08-11  Julie Parent  <jparent@chromium.org>
+
+        Reviewed by Justin Garcia.
+
+        Crash in replaceSelectionCommand with RTL text.
+        https://bugs.webkit.org/show_bug.cgi?id=41485
+        
+        For text with mixed directionality, sort the text boxes before
+        computing gaps, since that code assumes the boxes are in order.
+
+        Test: editing/execCommand/insert-image-on-top-of-directional-text.html
+
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::deleteInsignificantText): Sort boxes
+        like we do in TextIterator before computing gaps.
+        * editing/TextIterator.cpp:
+        (WebCore::TextIterator::handleTextNode): Use new compareByStart
+        rather than compareBoxStart.  No functional change.
+        * rendering/InlineTextBox.h:
+        (WebCore::InlineTextBox::compareByStart): Moved compareBoxStart
+        from TextIterator here so it can be used in multiple places.
+
 2010-08-11  Fumitoshi Ukai  <ukai@chromium.org>
 
         Unreviewed build fix of Leopard Intel Debug (Build)
index 5ec87d648ea8d4c250f4449fed48515c80839d0d..4216452c21abd3fb69091d324a7767ae55c35242 100644 (file)
@@ -489,7 +489,18 @@ void CompositeEditCommand::deleteInsignificantText(PassRefPtr<Text> textNode, un
     if (!textRenderer)
         return;
 
-    InlineTextBox* box = textRenderer->firstTextBox();
+    Vector<InlineTextBox*> sortedTextBoxes;
+    size_t sortedTextBoxesPosition = 0;
+   
+    for (InlineTextBox* textBox = textRenderer->firstTextBox(); textBox; textBox = textBox->nextTextBox())
+        sortedTextBoxes.append(textBox);
+    
+    // If there is mixed directionality text, the boxes can be out of order,
+    // (like Arabic with embedded LTR), so sort them first. 
+    if (textRenderer->containsReversedText())    
+        std::sort(sortedTextBoxes.begin(), sortedTextBoxes.end(), InlineTextBox::compareByStart);
+    InlineTextBox* box = sortedTextBoxes.isEmpty() ? 0 : sortedTextBoxes[sortedTextBoxesPosition];
+
     if (!box) {
         // whole text node is empty
         removeNode(textNode);
@@ -526,8 +537,12 @@ void CompositeEditCommand::deleteInsignificantText(PassRefPtr<Text> textNode, un
         }
         
         prevBox = box;
-        if (box)
-            box = box->nextTextBox();
+        if (box) {
+            if (++sortedTextBoxesPosition < sortedTextBoxes.size())
+                box = sortedTextBoxes[sortedTextBoxesPosition];
+            else
+                box = 0;
+        }
     }
 
     if (!str.isNull()) {
index 9589bffde91fcf45d00664698c6b33024e81b9a5..7226eba4b1679f396927623053290edf17ca6324 100644 (file)
@@ -442,11 +442,6 @@ void TextIterator::advance()
     }
 }
 
-static inline bool compareBoxStart(const InlineTextBox* first, const InlineTextBox* second)
-{
-    return first->start() < second->start();
-}
-
 bool TextIterator::handleTextNode()
 {
     if (m_fullyClippedStack.top())
@@ -507,7 +502,7 @@ bool TextIterator::handleTextNode()
         for (InlineTextBox* textBox = renderer->firstTextBox(); textBox; textBox = textBox->nextTextBox()) {
             m_sortedTextBoxes.append(textBox);
         }
-        std::sort(m_sortedTextBoxes.begin(), m_sortedTextBoxes.end(), compareBoxStart); 
+        std::sort(m_sortedTextBoxes.begin(), m_sortedTextBoxes.end(), InlineTextBox::compareByStart); 
         m_sortedTextBoxesPosition = 0;
     }
     
index fcbf23a0699667fe36b4ec1090f34cafabcc6e90..7d828f33ffd16a2d108fac391176d05177a46e5a 100644 (file)
@@ -67,6 +67,7 @@ public:
 
     bool hasHyphen() const { return m_hasEllipsisBoxOrHyphen; }
     void setHasHyphen(bool hasHyphen) { m_hasEllipsisBoxOrHyphen = hasHyphen; }
+    static inline bool compareByStart(const InlineTextBox* first, const InlineTextBox* second) { return first->start() < second->start(); }
 
 private:
     virtual int selectionTop();