Reviewed by John
authorkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Feb 2005 23:43:26 +0000 (23:43 +0000)
committerkocienda <kocienda@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Feb 2005 23:43:26 +0000 (23:43 +0000)
        Fix for this bug:

        <rdar://problem/3972665> 8A360: HTML message partially truncated on left hand side, text-indent from Script Editor

        * khtml/css/css_computedstyle.cpp: inheritableProperties array now defined in css_valueimpl.cpp.
        * khtml/css/css_valueimpl.cpp: Define inheritableProperties array here.
        (DOM::CSSMutableStyleDeclarationImpl::copyBlockProperties): Use new name for blockProperties, and use the new
        constant for the number of items in the array.
        (DOM::CSSMutableStyleDeclarationImpl::removeBlockProperties): Ditto.
        (DOM::CSSMutableStyleDeclarationImpl::removeInheritableProperties): New function.
        * khtml/css/css_valueimpl.h: Declare inheritableProperties array and numInheritableProperties extern so they
        can be defined in css_valueimpl.cpp and used in css_computedstyle.cpp.
        * khtml/editing/htmlediting.cpp:
        (khtml::ReplacementFragment::removeStyleNodes): This code was misguided, and removed too much style from HTML
        elements. Now, it removes from HTML elements only the styles that we replace later with a call to applyStyle().
        Also, add ID_B to list of inline "style" nodes we are willing to remove. Leaving it off was an oversight.

        * layout-tests/editing/pasteboard/paste-text-011-expected.txt: ID_B fix made this result change, without any
        visible change in the test.

        New test:

        * layout-tests/editing/style/smoosh-styles-003.html
        * layout-tests/editing/style/smoosh-styles-003-expected.txt

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

LayoutTests/editing/pasteboard/paste-text-011-expected.txt
LayoutTests/editing/style/smoosh-styles-003-expected.txt [new file with mode: 0644]
LayoutTests/editing/style/smoosh-styles-003.html [new file with mode: 0644]
WebCore/ChangeLog-2005-08-23
WebCore/khtml/css/css_computedstyle.cpp
WebCore/khtml/css/css_valueimpl.cpp
WebCore/khtml/css/css_valueimpl.h
WebCore/khtml/editing/htmlediting.cpp

index 3b86531..5ddbcc4 100644 (file)
@@ -14,16 +14,16 @@ layer at (0,0) size 800x600
             RenderText {TEXT} at (0,0) size 39x18
               text run at (0,0) width 39: "there"
       RenderBlock {P} at (0,68) size 784x18
-        RenderInline {B} at (0,0) size 37x18
-          RenderInline {FONT} at (0,0) size 37x18
+        RenderInline {FONT} at (0,0) size 37x18
+          RenderInline {B} at (0,0) size 37x18
             RenderText {TEXT} at (0,0) size 37x18
               text run at (0,0) width 37: "hello"
       RenderBlock {P} at (0,102) size 784x18
-        RenderInline {B} at (0,0) size 39x18
-          RenderInline {FONT} at (0,0) size 39x18
+        RenderInline {FONT} at (0,0) size 39x18
+          RenderInline {B} at (0,0) size 39x18
             RenderText {TEXT} at (0,0) size 39x18
               text run at (0,0) width 39: "there"
 selection is CARET:
-start:      position 5 of child 1 {TEXT} of child 1 {FONT} of child 1 {B} of child 5 {P} of root {BODY}
-upstream:   position 5 of child 1 {TEXT} of child 1 {FONT} of child 1 {B} of child 5 {P} of root {BODY}
-downstream: position 5 of child 1 {TEXT} of child 1 {FONT} of child 1 {B} of child 5 {P} of root {BODY}
+start:      position 5 of child 1 {TEXT} of child 1 {B} of child 1 {FONT} of child 5 {P} of root {BODY}
+upstream:   position 5 of child 1 {TEXT} of child 1 {B} of child 1 {FONT} of child 5 {P} of root {BODY}
+downstream: position 5 of child 1 {TEXT} of child 1 {B} of child 1 {FONT} of child 5 {P} of root {BODY}
diff --git a/LayoutTests/editing/style/smoosh-styles-003-expected.txt b/LayoutTests/editing/style/smoosh-styles-003-expected.txt
new file mode 100644 (file)
index 0000000..53e9e7b
--- /dev/null
@@ -0,0 +1,36 @@
+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 {DIV} at (0,0) size 784x240 [border: (2px solid #0000FF)]
+        RenderBlock {DIV} at (14,14) size 756x112
+          RenderText {TEXT} at (0,0) size 67x28
+            text run at (0,0) width 67: "Tests: "
+          RenderBR {BR} at (0,0) size 0x0
+          RenderText {TEXT} at (0,28) size 750x56
+            text run at (0,28) width 750: "Pasting styles we should not remove during the \"smoosh styles\" process. This"
+            text run at (0,56) width 152: "tests the fix for "
+          RenderInline {A} at (0,0) size 260x28 [color=#0000EE]
+            RenderText {TEXT} at (152,56) size 260x28
+              text run at (152,56) width 260: "<rdar://problem/3972665>"
+          RenderText {TEXT} at (412,56) size 740x56
+            text run at (412,56) width 18: " 8"
+            text run at (430,56) width 310: "A360: HTML message partially"
+            text run at (0,84) width 560: "truncated on left hand side, text-indent from Script Editor"
+        RenderBlock {DIV} at (14,142) size 756x84
+          RenderText {TEXT} at (0,0) size 189x28
+            text run at (0,0) width 189: "Expected Results: "
+          RenderBR {BR} at (0,0) size 0x0
+          RenderText {TEXT} at (0,28) size 730x56
+            text run at (0,28) width 730: "Should see this content in the red box below: foo. Note that there should be"
+            text run at (0,56) width 168: "no visible indent."
+      RenderBlock {DIV} at (0,264) size 784x32
+        RenderBlock {DIV} at (0,0) size 784x32 [border: (2px solid #FF0000)]
+          RenderBlock {DIV} at (42,2) size 740x28
+            RenderText {TEXT} at (-40,0) size 40x28
+              text run at (-40,0) width 32: "foo"
+selection is CARET:
+start:      position 3 of child 1 {TEXT} of child 2 {DIV} of child 1 {DIV} of root {DIV}
+upstream:   position 3 of child 1 {TEXT} of child 2 {DIV} of child 1 {DIV} of root {DIV}
+downstream: position 3 of child 1 {TEXT} of child 2 {DIV} of child 1 {DIV} of root {DIV}
diff --git a/LayoutTests/editing/style/smoosh-styles-003.html b/LayoutTests/editing/style/smoosh-styles-003.html
new file mode 100644 (file)
index 0000000..367fc8f
--- /dev/null
@@ -0,0 +1,61 @@
+<html> 
+<head>
+
+<style>
+.editing { 
+    border: 2px solid red; 
+    font-size: 24px; 
+}
+.explanation { 
+    border: 2px solid blue; 
+    padding: 12px; 
+    font-size: 24px; 
+    margin-bottom: 24px;
+}
+.scenario { margin-bottom: 16px;}
+.scenario:first-line { font-weight: bold; margin-bottom: 16px;}
+.expected-results:first-line { font-weight: bold }
+</style>
+<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
+
+<script>
+
+function editingTest() {
+    extendSelectionForwardByLineCommand();
+    cutCommand();
+    pasteCommand();
+}
+
+</script>
+
+<title>Editing Test</title> 
+</head> 
+<body>
+
+<div class="explanation">
+<div class="scenario">
+Tests: 
+<br>
+Pasting styles we should not remove during the "smoosh styles" process. This tests the fix for
+<a href="rdar://problem/3972665">&lt;rdar://problem/3972665&gt;</a> 8A360: HTML message partially truncated on left hand side, text-indent from Script Editor
+</div>
+<div class="expected-results">
+Expected Results:
+<br>
+Should see this content in the red box below: foo. Note that there should be no visible indent.
+</div>
+</div>
+
+<div contenteditable id="root" style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space;">
+<div id="test" class="editing">
+<div style="margin-left: 40px; text-indent: -40px">
+foo
+</div>
+</div>
+
+<script>
+runEditingTest();
+</script>
+
+</body>
+</html>
index f5ed3a2..c4cb04a 100644 (file)
@@ -1,3 +1,32 @@
+2005-02-28  Ken Kocienda  <kocienda@apple.com>
+
+        Reviewed by John
+
+        Fix for this bug:
+        
+        <rdar://problem/3972665> 8A360: HTML message partially truncated on left hand side, text-indent from Script Editor
+
+        * khtml/css/css_computedstyle.cpp: inheritableProperties array now defined in css_valueimpl.cpp.
+        * khtml/css/css_valueimpl.cpp: Define inheritableProperties array here.
+        (DOM::CSSMutableStyleDeclarationImpl::copyBlockProperties): Use new name for blockProperties, and use the new
+        constant for the number of items in the array.
+        (DOM::CSSMutableStyleDeclarationImpl::removeBlockProperties): Ditto.
+        (DOM::CSSMutableStyleDeclarationImpl::removeInheritableProperties): New function.
+        * khtml/css/css_valueimpl.h: Declare inheritableProperties array and numInheritableProperties extern so they
+        can be defined in css_valueimpl.cpp and used in css_computedstyle.cpp.
+        * khtml/editing/htmlediting.cpp:
+        (khtml::ReplacementFragment::removeStyleNodes): This code was misguided, and removed too much style from HTML
+        elements. Now, it removes from HTML elements only the styles that we replace later with a call to applyStyle().
+        Also, add ID_B to list of inline "style" nodes we are willing to remove. Leaving it off was an oversight.
+
+        * layout-tests/editing/pasteboard/paste-text-011-expected.txt: ID_B fix made this result change, without any
+        visible change in the test.
+        
+        New test:
+        
+        * layout-tests/editing/style/smoosh-styles-003.html
+        * layout-tests/editing/style/smoosh-styles-003-expected.txt
+
 2005-02-28  Richard Williamson   <rjw@apple.com>
 
        Fixed <rdar://problem/4026985> CrashTracer: ...14 crashes at com.apple.WebCore: -[KWQPageState invalidate] + 32
index 789925e..f397a2d 100644 (file)
@@ -157,33 +157,6 @@ static const int computedProperties[] = {
 
 const unsigned numComputedProperties = sizeof(computedProperties) / sizeof(computedProperties[0]);
 
-// This is the list of properties we want to copy in the copyInheritableProperties() function.
-// It is the intersection of the list of inherited CSS properties and the
-// properties for which we have a computed implementation in this file.
-static const int inheritableProperties[] = {
-    CSS_PROP_BORDER_COLLAPSE,
-    CSS_PROP_BORDER_SPACING,
-    CSS_PROP_COLOR,
-    CSS_PROP_FONT_FAMILY,
-    CSS_PROP_FONT_SIZE,
-    CSS_PROP_FONT_STYLE,
-    CSS_PROP_FONT_VARIANT,
-    CSS_PROP_FONT_WEIGHT,
-    CSS_PROP_LETTER_SPACING,
-    CSS_PROP_LINE_HEIGHT,
-    CSS_PROP_TEXT_ALIGN,
-    CSS_PROP__KHTML_TEXT_DECORATIONS_IN_EFFECT,
-    CSS_PROP_TEXT_INDENT,
-    CSS_PROP__APPLE_TEXT_SIZE_ADJUST,
-    CSS_PROP_TEXT_TRANSFORM,
-    CSS_PROP_ORPHANS,
-    CSS_PROP_WHITE_SPACE,
-    CSS_PROP_WIDOWS,
-    CSS_PROP_WORD_SPACING,
-};
-
-const unsigned numInheritableProperties = sizeof(inheritableProperties) / sizeof(inheritableProperties[0]);
-
 static CSSValueImpl* valueForLength(const Length &length)
 {
     switch (length.type) {
index a40ebc5..bdc17e4 100644 (file)
@@ -459,9 +459,36 @@ void CSSStyleDeclarationImpl::diff(CSSMutableStyleDeclarationImpl *style) const
         style->removeProperty(*it);
 }
 
+// This is the list of properties we want to copy in the copyInheritableProperties() function.
+// It is the intersection of the list of inherited CSS properties and the
+// properties for which we have a computed implementation in this file.
+const int inheritableProperties[] = {
+    CSS_PROP_BORDER_COLLAPSE,
+    CSS_PROP_BORDER_SPACING,
+    CSS_PROP_COLOR,
+    CSS_PROP_FONT_FAMILY,
+    CSS_PROP_FONT_SIZE,
+    CSS_PROP_FONT_STYLE,
+    CSS_PROP_FONT_VARIANT,
+    CSS_PROP_FONT_WEIGHT,
+    CSS_PROP_LETTER_SPACING,
+    CSS_PROP_LINE_HEIGHT,
+    CSS_PROP_TEXT_ALIGN,
+    CSS_PROP__KHTML_TEXT_DECORATIONS_IN_EFFECT,
+    CSS_PROP_TEXT_INDENT,
+    CSS_PROP__APPLE_TEXT_SIZE_ADJUST,
+    CSS_PROP_TEXT_TRANSFORM,
+    CSS_PROP_ORPHANS,
+    CSS_PROP_WHITE_SPACE,
+    CSS_PROP_WIDOWS,
+    CSS_PROP_WORD_SPACING,
+};
+
+const unsigned numInheritableProperties = sizeof(inheritableProperties) / sizeof(inheritableProperties[0]);
+
 // This is the list of properties we want to copy in the copyBlockProperties() function.
 // It is the list of CSS properties that apply to specially to block-level elements.
-static const int BlockProperties[] = {
+static const int blockProperties[] = {
     CSS_PROP_ORPHANS,
     CSS_PROP_OVERFLOW, // This can be also be applied to replaced elements
     CSS_PROP_PAGE_BREAK_AFTER,
@@ -472,14 +499,21 @@ static const int BlockProperties[] = {
     CSS_PROP_WIDOWS
 };
 
+const unsigned numBlockProperties = sizeof(blockProperties) / sizeof(blockProperties[0]);
+
 CSSMutableStyleDeclarationImpl *CSSMutableStyleDeclarationImpl::copyBlockProperties() const
 {
-    return copyPropertiesInSet(BlockProperties, sizeof(BlockProperties) / sizeof(BlockProperties[0]));
+    return copyPropertiesInSet(blockProperties, numBlockProperties);
 }
 
 void CSSMutableStyleDeclarationImpl::removeBlockProperties()
 {
-    removePropertiesInSet(BlockProperties, sizeof(BlockProperties) / sizeof(BlockProperties[0]));
+    removePropertiesInSet(blockProperties, numBlockProperties);
+}
+
+void CSSMutableStyleDeclarationImpl::removeInheritableProperties()
+{
+    removePropertiesInSet(inheritableProperties, numInheritableProperties);
 }
 
 CSSMutableStyleDeclarationImpl *CSSStyleDeclarationImpl::copyPropertiesInSet(const int *set, unsigned length) const
index 389a2d8..976b719 100644 (file)
@@ -39,6 +39,9 @@ namespace DOM {
 class CSSMutableStyleDeclarationImpl;
 class CounterImpl;
 
+extern const int inheritableProperties[];
+extern const unsigned numInheritableProperties;
+
 class CSSStyleDeclarationImpl : public StyleBaseImpl
 {
 public:
@@ -486,6 +489,7 @@ public:
  
     CSSMutableStyleDeclarationImpl *copyBlockProperties() const;
     void removeBlockProperties();
+    void removeInheritableProperties();
     void removePropertiesInSet(const int *set, unsigned length);
 
     void merge(CSSMutableStyleDeclarationImpl *, bool argOverridesOnConflict = true);
index 87e27f3..3de26b8 100644 (file)
@@ -4354,7 +4354,8 @@ void ReplacementFragment::removeStyleNodes()
         NodeImpl *next = node->traverseNextNode();
         // This list of tags change the appearance of content
         // in ways we can add back on later with CSS, if necessary.
-        if (node->id() == ID_BIG || 
+        if (node->id() == ID_B || 
+            node->id() == ID_BIG || 
             node->id() == ID_CENTER || 
             node->id() == ID_FONT || 
             node->id() == ID_I || 
@@ -4372,20 +4373,8 @@ void ReplacementFragment::removeStyleNodes()
             HTMLElementImpl *elem = static_cast<HTMLElementImpl *>(node);
             CSSMutableStyleDeclarationImpl *inlineStyleDecl = elem->inlineStyleDecl();
             if (inlineStyleDecl) {
-                if (elem->id() == ID_P) {
-                    // For <p> elements, we want to retain any margin-cancelling properties
-                    // that have been added by our old editing code or by current Mail.app code.
-                    DOMString marginTop = inlineStyleDecl->getPropertyValue(CSS_PROP_MARGIN_TOP);                        
-                    DOMString marginBottom = inlineStyleDecl->getPropertyValue(CSS_PROP_MARGIN_BOTTOM);
-                    inlineStyleDecl->clear();
-                    if (!marginTop.isEmpty())
-                        inlineStyleDecl->setProperty(CSS_PROP_MARGIN_TOP, marginTop);
-                    if (!marginBottom.isEmpty())
-                        inlineStyleDecl->setProperty(CSS_PROP_MARGIN_BOTTOM, marginBottom);
-                }
-                else {
-                    inlineStyleDecl->clear();
-                }
+                inlineStyleDecl->removeBlockProperties();
+                inlineStyleDecl->removeInheritableProperties();
             }
         }
         node = next;