2010-08-09 Ryosuke Niwa <rniwa@webkit.org>
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Aug 2010 23:25:15 +0000 (23:25 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Aug 2010 23:25:15 +0000 (23:25 +0000)
        Reviewed by Justin Garcia.

        Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed
        https://bugs.webkit.org/show_bug.cgi?id=43639

        The bug was caused when RemoveCSSPropertyCommand is called with CSSMutableStyleDeclaration of some styled element,
        and the style attribute of the element is removed subsequently. When the attribute removal is undone, new instance of
        CSSMutableStyleDeclaration is created and RemoveCSSPropertyCommand's m_style became detached from the element.

        Modified RemoveCSSPropertyCommand to store the styled element directly instead of its CSSMutableStyleDeclaration.

        Test: editing/undo/remove-css-property-and-remove-style.html

        * WebCore.order:
        * editing/ApplyStyleCommand.cpp:
        (WebCore::ApplyStyleCommand::removeCSSStyle): Calls removeCSSProperty.
        (WebCore::ApplyStyleCommand::extractTextDecorationStyle): Calls removeCSSProperty.
        * editing/CompositeEditCommand.cpp:
        (WebCore::CompositeEditCommand::removeCSSProperty): Takes StyledElement instead of CSSMutableStyleDeclaration.
        * editing/CompositeEditCommand.h:
        * editing/RemoveCSSPropertyCommand.cpp:
        (WebCore::RemoveCSSPropertyCommand::RemoveCSSPropertyCommand): Takes StyledElement instead of CSSMutableStyleDeclaration.
        (WebCore::RemoveCSSPropertyCommand::doApply): See above.
        (WebCore::RemoveCSSPropertyCommand::doUnapply): See above.
        * editing/RemoveCSSPropertyCommand.h:
        (WebCore::RemoveCSSPropertyCommand::create): See above.
2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>

        Reviewed by Justin Garcia.

        Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed
        https://bugs.webkit.org/show_bug.cgi?id=43639

        Added a test to remove style attribute after removing a CSS property from the style.
        Undo should restore both style attribute and the removed CSS property.

        * editing/undo/remove-css-property-and-remove-style-expected.txt: Added.
        * editing/undo/remove-css-property-and-remove-style.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/editing/undo/remove-css-property-and-remove-style-expected.txt [new file with mode: 0644]
LayoutTests/editing/undo/remove-css-property-and-remove-style.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/WebCore.order
WebCore/editing/ApplyStyleCommand.cpp
WebCore/editing/CompositeEditCommand.cpp
WebCore/editing/CompositeEditCommand.h
WebCore/editing/RemoveCSSPropertyCommand.cpp
WebCore/editing/RemoveCSSPropertyCommand.h

index dd633e6..74dc7cb 100644 (file)
@@ -1,3 +1,16 @@
+2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Justin Garcia.
+
+        Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed
+        https://bugs.webkit.org/show_bug.cgi?id=43639
+
+        Added a test to remove style attribute after removing a CSS property from the style.
+        Undo should restore both style attribute and the removed CSS property.
+
+        * editing/undo/remove-css-property-and-remove-style-expected.txt: Added.
+        * editing/undo/remove-css-property-and-remove-style.html: Added.
+
 2010-08-09  Eric Seidel  <eric@webkit.org>
 
         Reviewed by Adam Barth.
diff --git a/LayoutTests/editing/undo/remove-css-property-and-remove-style-expected.txt b/LayoutTests/editing/undo/remove-css-property-and-remove-style-expected.txt
new file mode 100644 (file)
index 0000000..c2bebf5
--- /dev/null
@@ -0,0 +1,76 @@
+This tests removing style attribute after removing CSS property. Undo should bring back the CSS property we removed.
+
+Initially "test" should be bold wrapped with a span:
+| <span>
+|   style="font-weight: 900;"
+|   "test"
+
+Unbolding should remove the CSS style and also remove the span:
+| "<#selection-anchor>test<#selection-focus>"
+
+Undo should bring back both the span and style attribute so that "test" is once again bold:
+| <span>
+|   style="font-weight: 900; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Redo should unbold "test" and there should be no span:
+| "<#selection-anchor>test<#selection-focus>"
+
+Undo once more:
+| <span>
+|   style="font-weight: 900; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Reset, and added id and title:
+| <span>
+|   id="test_span"
+|   style="font-weight: 900;"
+|   title="hello, world"
+|   "<#selection-anchor>test<#selection-focus>"
+
+Unbolding should remove the CSS style but shouldn't remove the span:
+| <span>
+|   id="test_span"
+|   title="hello, world"
+|   "<#selection-anchor>test<#selection-focus>"
+
+Undo should restore the style attribute and "test" should be bold:
+| <span>
+|   id="test_span"
+|   style="font-weight: 900; "
+|   title="hello, world"
+|   "<#selection-anchor>test<#selection-focus>"
+
+Redo should remove the style attribute again:
+| <span>
+|   id="test_span"
+|   title="hello, world"
+|   "<#selection-anchor>test<#selection-focus>"
+
+Reset, and added color:blue:
+| <span>
+|   style="font-weight: 900; color: blue; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Unbolding should remove the font-weight but shouldn't remove the style attribute:
+| <span>
+|   style="color: blue; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Undo should reset the style attribute so that "test" is both bold and blue:
+| <span>
+|   style="color: blue; font-weight: 900; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Redo should only remove font-weight and leave "test" blue:
+| <span>
+|   style="color: blue; "
+|   "<#selection-anchor>test<#selection-focus>"
+
+Setting the forecolor to black should remove both the style attribute and the span:
+| "<#selection-anchor>test<#selection-focus>"
+
+Undo should make "test" blue again:
+| <span>
+|   style="color: blue; "
+|   "<#selection-anchor>test<#selection-focus>"
diff --git a/LayoutTests/editing/undo/remove-css-property-and-remove-style.html b/LayoutTests/editing/undo/remove-css-property-and-remove-style.html
new file mode 100644 (file)
index 0000000..ee68821
--- /dev/null
@@ -0,0 +1,59 @@
+<script src="../../resources/dump-as-markup.js"></script>
+<p>This tests removing style attribute after removing CSS property. Undo should bring back the CSS property we removed.</p>
+<div id="test" contenteditable><span style="font-weight: 900;">test</span></div>
+<script>
+
+Markup.description('This tests removing style attribute after removing CSS property. Undo should bring back the CSS property we removed.');
+
+Markup.dump('test', 'Initially "test" should be bold wrapped with a span');
+window.getSelection().selectAllChildren(test);
+document.execCommand('bold', false, null);
+Markup.dump('test', 'Unbolding should remove the CSS style and also remove the span');
+
+document.execCommand('undo', false, null);
+Markup.dump('test', 'Undo should bring back both the span and style attribute so that "test" is once again bold');
+
+document.execCommand('redo', false, null);
+Markup.dump('test', 'Redo should unbold "test" and there should be no span');
+
+document.execCommand('undo', false, null);
+Markup.dump('test', 'Undo once more');
+
+document.getElementById('test').innerHTML = '<span style="font-weight: 900;">test</span>';
+var span = document.getElementById('test').firstChild;
+span.id = 'test_span';
+span.title = 'hello, world';
+window.getSelection().selectAllChildren(test);
+Markup.dump('test', 'Reset, and added id and title');
+
+document.execCommand('bold', false, null);
+Markup.dump('test', 'Unbolding should remove the CSS style but shouldn\'t remove the span');
+
+document.execCommand('undo', false, null);
+Markup.dump('test', 'Undo should restore the style attribute and "test" should be bold');
+
+document.execCommand('redo', false, null);
+Markup.dump('test', 'Redo should remove the style attribute again');
+
+document.getElementById('test').innerHTML = '<span style="font-weight: 900;">test</span>';
+var span = document.getElementById('test').firstChild;
+span.style.color = 'blue';
+window.getSelection().selectAllChildren(test);
+Markup.dump('test', 'Reset, and added color:blue');
+
+document.execCommand('bold', false, null);
+Markup.dump('test', 'Unbolding should remove the font-weight but shouldn\'t remove the style attribute');
+
+document.execCommand('undo', false, null);
+Markup.dump('test', 'Undo should reset the style attribute so that "test" is both bold and blue');
+
+document.execCommand('redo', false, null);
+Markup.dump('test', 'Redo should only remove font-weight and leave "test" blue');
+
+document.execCommand('foreColor', false, "#000000");
+Markup.dump('test', 'Setting the forecolor to black should remove both the style attribute and the span');
+
+document.execCommand('undo', false, null);
+Markup.dump('test', 'Undo should make "test" blue again');
+
+</script>
index 362ba21..c5ec3b8 100644 (file)
@@ -1,5 +1,34 @@
 2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Reviewed by Justin Garcia.
+
+        Undo fails in RemoveCSSPropertyCommand when the corresponding style attribute is removed
+        https://bugs.webkit.org/show_bug.cgi?id=43639
+
+        The bug was caused when RemoveCSSPropertyCommand is called with CSSMutableStyleDeclaration of some styled element,
+        and the style attribute of the element is removed subsequently. When the attribute removal is undone, new instance of
+        CSSMutableStyleDeclaration is created and RemoveCSSPropertyCommand's m_style became detached from the element.
+
+        Modified RemoveCSSPropertyCommand to store the styled element directly instead of its CSSMutableStyleDeclaration.
+
+        Test: editing/undo/remove-css-property-and-remove-style.html
+
+        * WebCore.order:
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::ApplyStyleCommand::removeCSSStyle): Calls removeCSSProperty.
+        (WebCore::ApplyStyleCommand::extractTextDecorationStyle): Calls removeCSSProperty.
+        * editing/CompositeEditCommand.cpp:
+        (WebCore::CompositeEditCommand::removeCSSProperty): Takes StyledElement instead of CSSMutableStyleDeclaration.
+        * editing/CompositeEditCommand.h:
+        * editing/RemoveCSSPropertyCommand.cpp:
+        (WebCore::RemoveCSSPropertyCommand::RemoveCSSPropertyCommand): Takes StyledElement instead of CSSMutableStyleDeclaration.
+        (WebCore::RemoveCSSPropertyCommand::doApply): See above.
+        (WebCore::RemoveCSSPropertyCommand::doUnapply): See above.
+        * editing/RemoveCSSPropertyCommand.h:
+        (WebCore::RemoveCSSPropertyCommand::create): See above.
+
+2010-08-09  Ryosuke Niwa  <rniwa@webkit.org>
+
         Reviewed by Tony Chang.
 
         Use getIdentifierValue to obtain direction and unicode-bidi properties in ApplyStyleCommand
index 0ac5610..c815bbe 100644 (file)
@@ -17635,9 +17635,9 @@ __ZN7WebCore16comparePositionsERKNS_15VisiblePositionES2_
 __ZN7WebCore20CompositeEditCommand10applyStyleEPNS_19CSSStyleDeclarationERKNS_8PositionES5_NS_10EditActionE
 __ZN7WebCore17ApplyStyleCommandC1EPNS_8DocumentEPNS_19CSSStyleDeclarationERKNS_8PositionES7_NS_10EditActionENS0_14EPropertyLeve
 __ZN7WebCore17ApplyStyleCommandC2EPNS_8DocumentEPNS_19CSSStyleDeclarationERKNS_8PositionES7_NS_10EditActionENS0_14EPropertyLeve
-__ZN7WebCore20CompositeEditCommand17removeCSSPropertyEN3WTF10PassRefPtrINS_26CSSMutableStyleDeclarationEEE13CSSPropertyID
-__ZN7WebCore24RemoveCSSPropertyCommandC1EPNS_8DocumentEN3WTF10PassRefPtrINS_26CSSMutableStyleDeclarationEEE13CSSPropertyID
-__ZN7WebCore24RemoveCSSPropertyCommandC2EPNS_8DocumentEN3WTF10PassRefPtrINS_26CSSMutableStyleDeclarationEEE13CSSPropertyID
+__ZN7WebCore20CompositeEditCommand17removeCSSPropertyEN3WTF10PassRefPtrINS_13StyledElementEEE13CSSPropertyID
+__ZN7WebCore24RemoveCSSPropertyCommandC1EPNS_8DocumentEN3WTF10PassRefPtrINS_13StyledElementEEE13CSSPropertyID
+__ZN7WebCore24RemoveCSSPropertyCommandC2EPNS_8DocumentEN3WTF10PassRefPtrINS_13StyledElementEEE13CSSPropertyID
 __ZN7WebCore24RemoveCSSPropertyCommand7doApplyEv
 __ZN7WebCore20CompositeEditCommand19removeNodeAttributeEN3WTF10PassRefPtrINS_7ElementEEERKNS_13QualifiedNameE
 __ZNK7WebCore18FormatBlockCommand13editingActionEv
index f2780f7..50654c6 100644 (file)
@@ -1291,9 +1291,9 @@ bool ApplyStyleCommand::removeCSSStyle(CSSMutableStyleDeclaration* style, HTMLEl
             removed = true;
             if (mode == RemoveNone)
                 return true;
-            removeCSSProperty(decl, propertyID);
+            removeCSSProperty(elem, propertyID);
             if (propertyID == CSSPropertyUnicodeBidi && !decl->getPropertyValue(CSSPropertyDirection).isEmpty())
-                removeCSSProperty(decl, CSSPropertyDirection);
+                removeCSSProperty(elem, CSSPropertyDirection);
         }
     }
 
@@ -1353,7 +1353,7 @@ PassRefPtr<CSSMutableStyleDeclaration> ApplyStyleCommand::extractTextDecorationS
 
     RefPtr<CSSValue> property = style->getPropertyCSSValue(CSSPropertyTextDecoration);
     if (property && !equalIgnoringCase(property->cssText(), "none"))
-        removeCSSProperty(style.get(), CSSPropertyTextDecoration);
+        removeCSSProperty(element, CSSPropertyTextDecoration);
 
     return textDecorationStyle.release();
 }
index c5a8959..5ec87d6 100644 (file)
@@ -370,9 +370,9 @@ void CompositeEditCommand::deleteSelection(const VisibleSelection &selection, bo
         applyCommandToComposite(DeleteSelectionCommand::create(selection, smartDelete, mergeBlocksAfterDelete, replace, expandForSpecialElements));
 }
 
-void CompositeEditCommand::removeCSSProperty(PassRefPtr<CSSMutableStyleDeclaration> style, CSSPropertyID property)
+void CompositeEditCommand::removeCSSProperty(PassRefPtr<StyledElement> element, CSSPropertyID property)
 {
-    applyCommandToComposite(RemoveCSSPropertyCommand::create(document(), style, property));
+    applyCommandToComposite(RemoveCSSPropertyCommand::create(document(), element, property));
 }
 
 void CompositeEditCommand::removeNodeAttribute(PassRefPtr<Element> element, const QualifiedName& attribute)
index 0cceaaa..9d69925 100644 (file)
@@ -34,6 +34,7 @@ namespace WebCore {
 
 class CSSStyleDeclaration;
 class HTMLElement;
+class StyledElement;
 class Text;
 
 class CompositeEditCommand : public EditCommand {
@@ -68,7 +69,7 @@ protected:
     void rebalanceWhitespace();
     void rebalanceWhitespaceAt(const Position&);
     void prepareWhitespaceAtPositionForSplit(Position&);
-    void removeCSSProperty(PassRefPtr<CSSMutableStyleDeclaration>, CSSPropertyID);
+    void removeCSSProperty(PassRefPtr<StyledElement>, CSSPropertyID);
     void removeNodeAttribute(PassRefPtr<Element>, const QualifiedName& attribute);
     void removeChildrenInRange(PassRefPtr<Node>, unsigned from, unsigned to);
     virtual void removeNode(PassRefPtr<Node>);
index 17870a7..8b37db8 100644 (file)
 
 namespace WebCore {
 
-RemoveCSSPropertyCommand::RemoveCSSPropertyCommand(Document* document, PassRefPtr<CSSMutableStyleDeclaration> style, CSSPropertyID property)
+RemoveCSSPropertyCommand::RemoveCSSPropertyCommand(Document* document, PassRefPtr<StyledElement> element, CSSPropertyID property)
     : SimpleEditCommand(document)
-    , m_style(style)
+    , m_element(element)
     , m_property(property)
     , m_important(false)
 {
-    ASSERT(m_style);
+    ASSERT(m_element);
 }
 
 void RemoveCSSPropertyCommand::doApply()
 {
-    m_oldValue = m_style->getPropertyValue(m_property);
-    m_important = m_style->getPropertyPriority(m_property);
-    m_style->removeProperty(m_property);
+    CSSMutableStyleDeclaration* style = m_element->inlineStyleDecl();
+    m_oldValue = style->getPropertyValue(m_property);
+    m_important = style->getPropertyPriority(m_property);
+    style->removeProperty(m_property);
 }
 
 void RemoveCSSPropertyCommand::doUnapply()
 {
-    m_style->setProperty(m_property, m_oldValue, m_important);
+    CSSMutableStyleDeclaration* style = m_element->inlineStyleDecl();
+    style->setProperty(m_property, m_oldValue, m_important);
 }
 
 } // namespace WebCore
index 836f9d7..46e0498 100644 (file)
 #include "EditCommand.h"
 #include "CSSMutableStyleDeclaration.h"
 #include "CSSPropertyNames.h"
+#include "StyledElement.h"
 
 namespace WebCore {
 
 class RemoveCSSPropertyCommand : public SimpleEditCommand {
 public:
-    static PassRefPtr<RemoveCSSPropertyCommand> create(Document* document, PassRefPtr<CSSMutableStyleDeclaration> style, CSSPropertyID property)
+    static PassRefPtr<RemoveCSSPropertyCommand> create(Document* document, PassRefPtr<StyledElement> element, CSSPropertyID property)
     {
-        return adoptRef(new RemoveCSSPropertyCommand(document, style, property));
+        return adoptRef(new RemoveCSSPropertyCommand(document, element, property));
     }
 
 private:
-    RemoveCSSPropertyCommand(Document*, PassRefPtr<CSSMutableStyleDeclaration>, CSSPropertyID property);
+    RemoveCSSPropertyCommand(Document*, PassRefPtr<StyledElement>, CSSPropertyID property);
 
     virtual void doApply();
     virtual void doUnapply();
 
-    RefPtr<CSSMutableStyleDeclaration> m_style;
+    RefPtr<StyledElement> m_element;
     CSSPropertyID m_property;
     String m_oldValue;
     bool m_important;