WebCore:
authoraliceli1 <aliceli1@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Aug 2007 23:02:33 +0000 (23:02 +0000)
committeraliceli1 <aliceli1@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Aug 2007 23:02:33 +0000 (23:02 +0000)
        Reviewed by Tim Hatcher.

        Fixed <rdar://problem/5420682> Mail crashes at WebCore::InsertLineBreakCommand::doApply() after dropping a selected image over container's close box

        * editing/DeleteButtonController.cpp:
        (WebCore::DeleteButtonController::show):
        Factored out the code in ::show() that created and styled the elements of the Deletion UI

        (WebCore::DeleteButtonController::createDeletionUI):
        Neglecting to move the append of the deletionUI elements into the same clause that handles the creation
        of them ended up creating multiple elements at were repeatedly appended to the target, resulting in a
        bloated table deletion UI which was slow to show and hide.

        * editing/DeleteButtonController.h:
        (WebCore::DeleteButtonController::enabled):
        Restore this function to how it used to be pre-r25305, sans asserts

        * editing/EditCommand.cpp:
        Add disable/enable sandwich when undoing/redoing commands too
        (WebCore::EditCommand::unapply):
        (WebCore::EditCommand::reapply):

LayoutTests:

        Reviewed by Tim Hatcher.

        Test for <rdar://problem/5420682> Mail crashes at WebCore::InsertLineBreakCommand::doApply() after dropping a selected image over container's close box

        * editing/deleting/deletionUI-single-instance.html: Added.
        * platform/mac/editing/deleting/deletionUI-single-instance-expected.checksum: Added.
        * platform/mac/editing/deleting/deletionUI-single-instance-expected.png: Added.
        * platform/mac/editing/deleting/deletionUI-single-instance-expected.txt: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/deleting/deletionUI-single-instance.html [new file with mode: 0644]
LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.checksum [new file with mode: 0644]
LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.txt [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/DeleteButtonController.cpp
WebCore/editing/DeleteButtonController.h
WebCore/editing/EditCommand.cpp

index 79ddbbcc35119d4a14aa011db2196f2965b1bb3a..b67c5361417660b2324ee67968d55c78750bd5dd 100644 (file)
@@ -1,3 +1,14 @@
+2007-08-31  Alice Liu  <alice.liu@apple.com>
+
+        Reviewed by Tim Hatcher.
+
+        Test for <rdar://problem/5420682> Mail crashes at WebCore::InsertLineBreakCommand::doApply() after dropping a selected image over container's close box
+
+        * editing/deleting/deletionUI-single-instance.html: Added.
+        * platform/mac/editing/deleting/deletionUI-single-instance-expected.checksum: Added.
+        * platform/mac/editing/deleting/deletionUI-single-instance-expected.png: Added.
+        * platform/mac/editing/deleting/deletionUI-single-instance-expected.txt: Added.
+
 2007-08-30  Adele Peterson  <adele@apple.com>
 
         Reviewed by Justin.
diff --git a/LayoutTests/editing/deleting/deletionUI-single-instance.html b/LayoutTests/editing/deleting/deletionUI-single-instance.html
new file mode 100644 (file)
index 0000000..824a8de
--- /dev/null
@@ -0,0 +1,32 @@
+<body>
+
+<p>
+This test can only be run with DRT and not manually from the browser.  This test makes sure that we don't make the mistake of adding new m_containerElements to the DeletionUI over and over again.
+A successful run of the test will not have multiple instances of deletion UI render objects at the same coordinates:
+</p>
+
+<div contenteditable="true" style="padding: 1em;">
+<table class="needsDeletionUI">
+
+<tr>
+<td id="td">
+Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Maecenas odio. Morbi sed tortor id nisl bibendum commodo. Donec pede. Praesent accumsan dui vitae mauris. Mauris non dui at neque lacinia pulvinar. Quisque nibh. Nulla vitae lectus. Pellentesque enim. Mauris hendrerit molestie dui. Etiam pretium ligula a pede. Fusce consectetuer purus sit amet sem. Morbi tincidunt mollis libero. Maecenas molestie.
+</td>
+</tr>
+</table>
+
+</div>
+
+<script>
+
+sel = window.getSelection();
+td = document.getElementById("td");
+sel.setPosition(td, td.textContent.length);
+
+if (window.layoutTestController) {
+    for (i = 0; i < 5; i++)
+        document.execCommand("Delete");
+}
+
+</script>
+</body>
diff --git a/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.checksum b/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.checksum
new file mode 100644 (file)
index 0000000..8b37c13
--- /dev/null
@@ -0,0 +1 @@
+cc44dad406d4b46bbd4e1cfb20c2aba0
\ No newline at end of file
diff --git a/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.png b/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.png
new file mode 100644 (file)
index 0000000..157ab25
Binary files /dev/null and b/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.png differ
diff --git a/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.txt b/LayoutTests/platform/mac/editing/deleting/deletionUI-single-instance-expected.txt
new file mode 100644 (file)
index 0000000..a541cf1
--- /dev/null
@@ -0,0 +1,30 @@
+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 782x54
+          text run at (0,0) width 457: "This test can only be run with DRT and not manually from the browser. "
+          text run at (457,0) width 325: "This test makes sure that we don't make the mistake"
+          text run at (0,18) width 738: "of adding new m_containerElements to the DeletionUI over and over again. A successful run of the test will not have"
+          text run at (0,36) width 450: "multiple instances of deletion UI render objects at the same coordinates:"
+      RenderBlock {DIV} at (0,70) size 784x110
+layer at (24,94) size 752x78 layerType: background only
+layer at (20,90) size 760x86
+  RenderBlock (positioned) zI: -1000000 {DIV} at (-4,-4) size 760x86 [border: (4px solid #00000099)]
+layer at (24,94) size 752x78 layerType: foreground only
+  RenderTable {TABLE} at (16,16) size 752x78
+    RenderTableSection {TBODY} at (0,0) size 752x78
+      RenderTableRow {TR} at (0,2) size 752x74
+        RenderTableCell {TD} at (2,2) size 748x74 [r=0 c=0 rs=1 cs=1]
+          RenderText {#text} at (1,1) size 746x72
+            text run at (1,1) width 743: "Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Maecenas odio. Morbi sed tortor id nisl bibendum commodo."
+            text run at (744,1) width 3: " "
+            text run at (1,19) width 743: "Donec pede. Praesent accumsan dui vitae mauris. Mauris non dui at neque lacinia pulvinar. Quisque nibh. Nulla vitae "
+            text run at (1,37) width 719: "lectus. Pellentesque enim. Mauris hendrerit molestie dui. Etiam pretium ligula a pede. Fusce consectetuer purus sit "
+            text run at (1,55) width 353: "amet sem. Morbi tincidunt mollis libero. Maecenas mole"
+    RenderBlock {DIV} at (0,0) size 752x0
+layer at (7,79) size 30x30
+  RenderImage zI: 1000000 {IMG} at (-17,-15) size 30x30
+caret: position 403 of child 0 {#text} of child 1 {TD} of child 0 {TR} of child 1 {TBODY} of child 1 {TABLE} of child 3 {DIV} of child 0 {BODY} of child 0 {HTML} of document
index 5fadd64f485f638368c5b26306308086c7808a5b..7a8aaa3d6e86696acd3280d2d5088ee5aee6526f 100644 (file)
@@ -1,3 +1,27 @@
+2007-08-31  Alice Liu  <alice.liu@apple.com>
+
+        Reviewed by Tim Hatcher.
+
+        Fixed <rdar://problem/5420682> Mail crashes at WebCore::InsertLineBreakCommand::doApply() after dropping a selected image over container's close box
+
+        * editing/DeleteButtonController.cpp:
+        (WebCore::DeleteButtonController::show):
+        Factored out the code in ::show() that created and styled the elements of the Deletion UI
+
+        (WebCore::DeleteButtonController::createDeletionUI):
+        Neglecting to move the append of the deletionUI elements into the same clause that handles the creation
+        of them ended up creating multiple elements at were repeatedly appended to the target, resulting in a 
+        bloated table deletion UI which was slow to show and hide. 
+
+        * editing/DeleteButtonController.h:
+        (WebCore::DeleteButtonController::enabled):
+        Restore this function to how it used to be pre-r25305, sans asserts
+
+        * editing/EditCommand.cpp:
+        Add disable/enable sandwich when undoing/redoing commands too
+        (WebCore::EditCommand::unapply):
+        (WebCore::EditCommand::reapply):
+
 2007-08-31  Antti Koivisto  <antti@apple.com>
 
         Reviewed by Anders.
index 9b68d6ea7d604cbef9dc61c26aebbf99f79250ee..bf66ff594a9b070cbd42b42211787a8d0cff241b 100644 (file)
@@ -146,46 +146,23 @@ void DeleteButtonController::respondToChangedSelection(const Selection& oldSelec
         hide();
 }
 
-void DeleteButtonController::show(HTMLElement* element)
+void DeleteButtonController::createDeletionUI()
 {
-    hide();
-
-    if (!enabled() || !element || !element->inDocument() || !isDeletableElement(element))
-        return;
-
-    if (!m_frame->editor()->shouldShowDeleteInterface(static_cast<HTMLElement*>(element)))
-        return;
-
-    // we rely on the renderer having current information, so we should update the layout if needed
-    m_frame->document()->updateLayoutIgnorePendingStylesheets();
-
-    m_target = element;
-
-    if (!m_containerElement) {
-        m_containerElement = new HTMLDivElement(m_target->document());
-        m_containerElement->setId(containerElementIdentifier);
-    }
+    RefPtr<HTMLDivElement> container = new HTMLDivElement(m_target->document());
+    container->setId(containerElementIdentifier);
 
-    CSSMutableStyleDeclaration* style = m_containerElement->getInlineStyleDecl();
+    CSSMutableStyleDeclaration* style = container->getInlineStyleDecl();
     style->setProperty(CSS_PROP__WEBKIT_USER_DRAG, CSS_VAL_NONE);
     style->setProperty(CSS_PROP__WEBKIT_USER_SELECT, CSS_VAL_IGNORE);
     style->setProperty(CSS_PROP__WEBKIT_USER_MODIFY, CSS_VAL_NONE);
 
-    ExceptionCode ec = 0;
-    m_target->appendChild(m_containerElement.get(), ec);
-    ASSERT(ec == 0);
-    if (ec) {
-        hide();
-        return;
-    }
-
-    m_outlineElement = new HTMLDivElement(m_target->document());
-    m_outlineElement->setId(outlineElementIdentifier);
+    RefPtr<HTMLDivElement> outline = new HTMLDivElement(m_target->document());
+    outline->setId(outlineElementIdentifier);
 
     const int borderWidth = 4;
     const int borderRadius = 6;
 
-    style = m_outlineElement->getInlineStyleDecl();
+    style = outline->getInlineStyleDecl();
     style->setProperty(CSS_PROP_POSITION, CSS_VAL_ABSOLUTE);
     style->setProperty(CSS_PROP_CURSOR, CSS_VAL_DEFAULT);
     style->setProperty(CSS_PROP__WEBKIT_USER_DRAG, CSS_VAL_NONE);
@@ -199,21 +176,20 @@ void DeleteButtonController::show(HTMLElement* element)
     style->setProperty(CSS_PROP_BORDER, String::number(borderWidth) + "px solid rgba(0, 0, 0, 0.6)");
     style->setProperty(CSS_PROP__WEBKIT_BORDER_RADIUS, String::number(borderRadius) + "px");
 
-    m_containerElement->appendChild(m_outlineElement.get(), ec);
+    ExceptionCode ec = 0;
+    container->appendChild(outline.get(), ec);
     ASSERT(ec == 0);
-    if (ec) {
-        hide();
+    if (ec)
         return;
-    }
 
-    m_buttonElement = new DeleteButton(m_target->document());
-    m_buttonElement->setId(buttonElementIdentifier);
+    RefPtr<DeleteButton> button = new DeleteButton(m_target->document());
+    button->setId(buttonElementIdentifier);
 
     const int buttonWidth = 30;
     const int buttonHeight = 30;
     const int buttonBottomShadowOffset = 2;
 
-    style = m_buttonElement->getInlineStyleDecl();
+    style = button->getInlineStyleDecl();
     style->setProperty(CSS_PROP_POSITION, CSS_VAL_ABSOLUTE);
     style->setProperty(CSS_PROP_CURSOR, CSS_VAL_DEFAULT);
     style->setProperty(CSS_PROP__WEBKIT_USER_DRAG, CSS_VAL_NONE);
@@ -225,9 +201,43 @@ void DeleteButtonController::show(HTMLElement* element)
     style->setProperty(CSS_PROP_WIDTH, String::number(buttonWidth) + "px");
     style->setProperty(CSS_PROP_HEIGHT, String::number(buttonHeight) + "px");
 
-    m_buttonElement->setCachedImage(new CachedImage(Image::loadPlatformResource("deleteButton")));
+    button->setCachedImage(new CachedImage(Image::loadPlatformResource("deleteButton")));
 
-    m_containerElement->appendChild(m_buttonElement.get(), ec);
+    container->appendChild(button.get(), ec);
+    ASSERT(ec == 0);
+    if (ec)
+        return;
+
+    m_containerElement = container.release();
+    m_outlineElement = outline.release();
+    m_buttonElement = button.release();
+}
+
+void DeleteButtonController::show(HTMLElement* element)
+{
+    hide();
+
+    if (!enabled() || !element || !element->inDocument() || !isDeletableElement(element))
+        return;
+
+    if (!m_frame->editor()->shouldShowDeleteInterface(static_cast<HTMLElement*>(element)))
+        return;
+
+    // we rely on the renderer having current information, so we should update the layout if needed
+    m_frame->document()->updateLayoutIgnorePendingStylesheets();
+
+    m_target = element;
+
+    if (!m_containerElement) {
+        createDeletionUI();
+        if (!m_containerElement) {
+            hide();
+            return;
+        }
+    }
+
+    ExceptionCode ec = 0;
+    m_target->appendChild(m_containerElement.get(), ec);
     ASSERT(ec == 0);
     if (ec) {
         hide();
@@ -243,8 +253,6 @@ void DeleteButtonController::show(HTMLElement* element)
         m_target->getInlineStyleDecl()->setProperty(CSS_PROP_Z_INDEX, "0");
         m_wasAutoZIndex = true;
     }
-
-    m_target->renderer()->repaint(true);
 }
 
 void DeleteButtonController::hide()
index 7a9ff202af88ca7a5c35dc23cd8ef474b835e248..ab2d0b0704723a7915f27d44f38278863683e065 100644 (file)
@@ -50,7 +50,7 @@ public:
     void show(HTMLElement*);
     void hide();
 
-    bool enabled() const { return (m_disableStack == 0 || m_target); }
+    bool enabled() const { return (m_disableStack == 0); }
     void enable();
     void disable();
 
@@ -60,6 +60,8 @@ private:
     static const char* const buttonElementIdentifier;
     static const char* const outlineElementIdentifier;
 
+    void createDeletionUI();
+
     Frame* m_frame;
     RefPtr<HTMLElement> m_target;
     RefPtr<HTMLElement> m_containerElement;
index 7c1968fca10cef911e4b98bdd81a7676a7ef518b..270de2f74cfa021f9ac3d2910aed3a88042fb168 100644 (file)
@@ -112,7 +112,10 @@ void EditCommand::unapply()
  
     Frame* frame = m_document->frame();
     
+    DeleteButtonController *deleteButtonController = frame->editor()->deleteButtonController();
+    deleteButtonController->disable();
     doUnapply();
+    deleteButtonController->enable();
 
     if (!m_parent) {
         updateLayout();
@@ -127,7 +130,10 @@ void EditCommand::reapply()
  
     Frame* frame = m_document->frame();
 
+    DeleteButtonController *deleteButtonController = frame->editor()->deleteButtonController();
+    deleteButtonController->disable();
     doReapply();
+    deleteButtonController->enable();
 
     if (!m_parent) {
         updateLayout();