REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text...
authoradele@apple.com <adele@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Oct 2009 23:37:17 +0000 (23:37 +0000)
committeradele@apple.com <adele@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 29 Oct 2009 23:37:17 +0000 (23:37 +0000)
<rdar://problem/7067033>
https://bugs.webkit.org/show_bug.cgi?id=30892

WebCore:

Patch by Enrica Casucci <enrica@apple.com> on 2009-10-29
Reviewed by Darin Adler.

This problem shows in any scenario where it is necessary to split a text
node to apply a style. SplitElementCommand and WrapContentsInDummySpanCommand both
have member variables initialized in the constructor to keep reference to elements
they need to operate upon. These reference are not updated when reapplying the command.
For this reason it is necessary to guarantee that unapply doesn not delete the references
and that these commands implement doReapply to correctly reuse the existing
elements.

Test: editing/undo/redo-style.html

* editing/SplitElementCommand.cpp:
(WebCore::SplitElementCommand::executeApply): Added.
(WebCore::SplitElementCommand::doApply): Modified to call executeApply.
(WebCore::SplitElementCommand::doUnapply): Doesn't release m_element1.
(WebCore::SplitElementCommand::doReapply): Added.
* editing/SplitElementCommand.h: Added doReapply and executeApply.
* editing/WrapContentsInDummySpanCommand.cpp:
(WebCore::WrapContentsInDummySpanCommand::executeApply): Added.
(WebCore::WrapContentsInDummySpanCommand::doApply): Modified to call executeApply.
(WebCore::WrapContentsInDummySpanCommand::doUnapply): Doesn't release m_dummySpan.
(WebCore::WrapContentsInDummySpanCommand::doReapply): Added.
* editing/WrapContentsInDummySpanCommand.h: Added doReapply and executeApply.

LayoutTests:

Patch by Enrica Casucci <enrica@apple.com> on 2009-10-29
* editing/undo/redo-style-expected.txt: Added.
* editing/undo/redo-style.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/undo/redo-style-expected.txt [new file with mode: 0644]
LayoutTests/editing/undo/redo-style.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/editing/SplitElementCommand.cpp
WebCore/editing/SplitElementCommand.h
WebCore/editing/WrapContentsInDummySpanCommand.cpp
WebCore/editing/WrapContentsInDummySpanCommand.h

index d88b955..9acfa39 100644 (file)
@@ -1,3 +1,12 @@
+2009-10-29  Enrica Casucci  <enrica@apple.com>
+
+        REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text in Mail, undo/redo behaves strangely
+        <rdar://problem/7067033>
+        https://bugs.webkit.org/show_bug.cgi?id=30892
+
+        * editing/undo/redo-style-expected.txt: Added.
+        * editing/undo/redo-style.html: Added.
+
 2009-10-29  Shinichiro Hamaji  <hamaji@chromium.org>
 
         Reviewed by Darin Adler.
diff --git a/LayoutTests/editing/undo/redo-style-expected.txt b/LayoutTests/editing/undo/redo-style-expected.txt
new file mode 100644 (file)
index 0000000..b09c578
--- /dev/null
@@ -0,0 +1,51 @@
+EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > BODY > HTML > #document to 7 of DIV > BODY > HTML > #document
+EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > DIV > DIV > BODY > HTML > #document to 2 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 2 of #text > DIV > DIV > BODY > HTML > #document to 4 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 4 of #text > DIV > DIV > BODY > HTML > #document to 6 of #text > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: shouldChangeSelectedDOMRange:(null) toDOMRange:range from 0 of #text > SPAN > DIV > DIV > BODY > HTML > #document to 2 of #text > SPAN > DIV > DIV > BODY > HTML > #document affinity:NSSelectionAffinityDownstream stillSelecting:FALSE
+EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
+EDITING DELEGATE: webViewDidChange:WebViewDidChangeNotification
+This tests applying a background color at the beginning, the middle and the end of one sentence, then undo and redo.
+Bug 30892
+
+Hello!
+Hello!
+Hello!
+
+Test 1: PASSED
+Test 2: PASSED
+Test 3: PASSED
diff --git a/LayoutTests/editing/undo/redo-style.html b/LayoutTests/editing/undo/redo-style.html
new file mode 100644 (file)
index 0000000..b2095d3
--- /dev/null
@@ -0,0 +1,45 @@
+<html>
+<body>
+This tests applying a background color at the beginning, the middle and the end of one sentence, then undo and redo.
+<p>
+<a href="https://bugs.webkit.org/show_bug.cgi?id=30892">Bug 30892</a>
+</p>
+<div contenteditable="true">
+<div id="test1">Hello!</div>
+<div id="test2">Hello!</div>
+<div id="test3">Hello!</div>
+</div>
+<br>
+<ul>
+<li>Test 1: <span id="c1"></span></li>
+<li>Test 2: <span id="c2"></span></li>
+<li>Test 3: <span id="c3"></span></li>
+</ul>
+
+<script type="text/javascript">
+function runTest(elem, node)
+{
+    var e = document.getElementById(node);
+    var before = e.innerHTML;
+    document.execCommand("hilitecolor", false, "#FF0000");
+    var after = e.innerHTML;
+    document.execCommand("undo");
+    var afterundo = e.innerHTML;
+    document.execCommand("redo");
+
+    document.getElementById(elem).appendChild(document.createTextNode(((before == afterundo) && (after == e.innerHTML))? "PASSED": "FAILED"));
+}
+
+if (window.layoutTestController) {
+    layoutTestController.dumpEditingCallbacks();
+    layoutTestController.dumpAsText();
+}
+
+var s = window.getSelection();
+s.setBaseAndExtent(document.getElementById('test1').firstChild, 0, document.getElementById('test1').firstChild, 2);
+runTest('c1', 'test1');
+s.setBaseAndExtent(document.getElementById('test2').firstChild, 2, document.getElementById('test2').firstChild, 4);
+runTest('c2', 'test2');
+s.setBaseAndExtent(document.getElementById('test3').firstChild, 4, document.getElementById('test3').firstChild, 6);
+runTest('c3', 'test3');
+</script>
index 3843276..6b0e7b2 100644 (file)
@@ -1,3 +1,34 @@
+2009-10-29  Enrica Casucci  <enrica@apple.com>
+
+        Reviewed by Darin Adler.
+
+        REGRESSION(3.2.3 - 4.0.2): Message composing: when I undo a color change to text in Mail, undo/redo behaves strangely
+        <rdar://problem/7067033>
+        https://bugs.webkit.org/show_bug.cgi?id=30892
+
+        This problem shows in any scenario where it is necessary to split a text
+        node to apply a style. SplitElementCommand and WrapContentsInDummySpanCommand both
+        have member variables initialized in the constructor to keep reference to elements
+        they need to operate upon. These reference are not updated when reapplying the command.
+        For this reason it is necessary to guarantee that unapply doesn not delete the references
+        and that these commands implement doReapply to correctly reuse the existing
+        elements.
+
+        Test: editing/undo/redo-style.html
+
+        * editing/SplitElementCommand.cpp:
+        (WebCore::SplitElementCommand::executeApply): Added.
+        (WebCore::SplitElementCommand::doApply): Modified to call executeApply. 
+        (WebCore::SplitElementCommand::doUnapply): Doesn't release m_element1.
+        (WebCore::SplitElementCommand::doReapply): Added.
+        * editing/SplitElementCommand.h: Added doReapply and executeApply.
+        * editing/WrapContentsInDummySpanCommand.cpp:
+        (WebCore::WrapContentsInDummySpanCommand::executeApply): Added.
+        (WebCore::WrapContentsInDummySpanCommand::doApply): Modified to call executeApply.
+        (WebCore::WrapContentsInDummySpanCommand::doUnapply): Doesn't release m_dummySpan.
+        (WebCore::WrapContentsInDummySpanCommand::doReapply): Added.
+        * editing/WrapContentsInDummySpanCommand.h: Added doReapply and executeApply.
+
 2009-10-29  Jeremy Orlow  <jorlow@chromium.org>
 
         Reviewed by Darin Fisher.
index 35dfc6f..37b725c 100644 (file)
@@ -41,31 +41,35 @@ SplitElementCommand::SplitElementCommand(PassRefPtr<Element> element, PassRefPtr
     ASSERT(m_atChild->parentNode() == m_element2);
 }
 
-void SplitElementCommand::doApply()
+void SplitElementCommand::executeApply()
 {
-    RefPtr<Element> prefixElement = m_element2->cloneElementWithoutChildren();
-
     if (m_atChild->parentNode() != m_element2)
         return;
-
+    
     Vector<RefPtr<Node> > children;
     for (Node* node = m_element2->firstChild(); node != m_atChild; node = node->nextSibling())
         children.append(node);
-
+    
     ExceptionCode ec = 0;
-
+    
     Node* parent = m_element2->parentNode();
     if (!parent)
         return;
-    parent->insertBefore(prefixElement.get(), m_element2.get(), ec);
+    parent->insertBefore(m_element1.get(), m_element2.get(), ec);
     if (ec)
         return;
-    m_element1 = prefixElement.release();
-
+    
     size_t size = children.size();
     for (size_t i = 0; i < size; ++i)
         m_element1->appendChild(children[i], ec);
 }
+    
+void SplitElementCommand::doApply()
+{
+    m_element1 = m_element2->cloneElementWithoutChildren();
+    
+    executeApply();
+}
 
 void SplitElementCommand::doUnapply()
 {
@@ -85,7 +89,14 @@ void SplitElementCommand::doUnapply()
         m_element2->insertBefore(children[i].get(), refChild.get(), ec);
 
     m_element1->remove(ec);
-    m_element1 = 0;
 }
 
+void SplitElementCommand::doReapply()
+{
+    if (!m_element1)
+        return;
+    
+    executeApply();
+}
+    
 } // namespace WebCore
index 2732762..7ea8f5b 100644 (file)
@@ -42,6 +42,8 @@ private:
 
     virtual void doApply();
     virtual void doUnapply();
+    virtual void doReapply();
+    void executeApply();
 
     RefPtr<Element> m_element1;
     RefPtr<Element> m_element2;
index 7622c1e..5320b69 100644 (file)
@@ -38,35 +38,37 @@ WrapContentsInDummySpanCommand::WrapContentsInDummySpanCommand(PassRefPtr<Elemen
     ASSERT(m_element);
 }
 
-void WrapContentsInDummySpanCommand::doApply()
+void WrapContentsInDummySpanCommand::executeApply()
 {
     Vector<RefPtr<Node> > children;
     for (Node* child = m_element->firstChild(); child; child = child->nextSibling())
         children.append(child);
-
-    RefPtr<HTMLElement> span = createStyleSpanElement(document());
+    
     ExceptionCode ec;
-
+    
     size_t size = children.size();
     for (size_t i = 0; i < size; ++i)
-        span->appendChild(children[i].release(), ec);
-
-    m_element->appendChild(span.get(), ec);
-
-    m_dummySpan = span.release();
+        m_dummySpan->appendChild(children[i].release(), ec);
+    
+    m_element->appendChild(m_dummySpan.get(), ec);
 }
 
+void WrapContentsInDummySpanCommand::doApply()
+{
+    m_dummySpan = createStyleSpanElement(document());
+    
+    executeApply();
+}
+    
 void WrapContentsInDummySpanCommand::doUnapply()
 {
     ASSERT(m_element);
 
-    RefPtr<HTMLElement> span = m_dummySpan.release();
-    if (!span)
+    if (!m_dummySpan)
         return;
 
     Vector<RefPtr<Node> > children;
-    for (Node* child = span->firstChild(); child; child = child->nextSibling())
+    for (Node* child = m_dummySpan->firstChild(); child; child = child->nextSibling())
         children.append(child);
 
     ExceptionCode ec;
@@ -75,7 +77,17 @@ void WrapContentsInDummySpanCommand::doUnapply()
     for (size_t i = 0; i < size; ++i)
         m_element->appendChild(children[i].release(), ec);
 
-    span->remove(ec);
+    m_dummySpan->remove(ec);
 }
 
+void WrapContentsInDummySpanCommand::doReapply()
+{
+    ASSERT(m_element);
+    
+    if (!m_dummySpan)
+        return;
+
+    executeApply();
+}
+    
 } // namespace WebCore
index b12131a..be3af66 100644 (file)
@@ -44,6 +44,8 @@ private:
 
     virtual void doApply();
     virtual void doUnapply();
+    virtual void doReapply();
+    void executeApply();
 
     RefPtr<Element> m_element;
     RefPtr<HTMLElement> m_dummySpan;