Changeset 46286 in webkit

Timestamp:

Jul 23, 2009 2:17:00 PM (15 years ago)

Author:

rniwa@webkit.org

Message:

WebCore:

2009-07-22 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Eric Seidel.

execCommand('underline') can't remove <U> underlines
​https://bugs.webkit.org/show_bug.cgi?id=20215

This patch adds support for u, s, and strike to implicitlyStyledElementShouldBeRemovedWhenApplyingStyle so that
WebKit can remove those presentational tags when necessary.
It also modifies StyleChange::init not to add "text-decoration: none". Not only is this style meaningless
(does not override inherited styles) but it was also causing WebKit to generate extra spans with this style.

• css/CSSValueList.cpp: (WebCore::CSSValueList::hasValue): True if the property contains the specified value
• css/CSSValueList.h: Updated prototype
• editing/ApplyStyleCommand.cpp: (WebCore::StyleChange::init): No longer adds "text-decoration: none" (WebCore::ApplyStyleCommand::implicitlyStyledElementShouldBeRemovedWhenApplyingStyle): Supports text-decoration-related elements

LayoutTests:

2009-07-22 Ryosuke Niwa <​rniwa@webkit.org>

Reviewed by Eric Seidel.

execCommand('underline') can't remove <U> underlines
​https://bugs.webkit.org/show_bug.cgi?id=20215

This patch rebaselines toggle-styles.html because WebKit now passes three tests it used to fail.
toggle-style-2.html is added to test cases in which multiple styles are specified with tags.
We still fail some tests because WebKit doesn't properly support non-CSS mode but they are visually correct.

• editing/execCommand/resources/toggle-style-2.js: Added. (testSingleToggle): (testDoubleToggle):
• editing/execCommand/toggle-style-2-expected.txt: Added.
• editing/execCommand/toggle-style-2.html: Added.
• editing/execCommand/toggle-styles-expected.txt: Passes all the tests

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/editing/execCommand/resources/toggle-style-2.js (added)
• LayoutTests/editing/execCommand/toggle-style-2-expected.txt (added)
• LayoutTests/editing/execCommand/toggle-style-2.html (added)
• LayoutTests/editing/execCommand/toggle-styles-expected.txt (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/css/CSSValueList.cpp (modified) (1 diff)
• WebCore/css/CSSValueList.h (modified) (1 diff)
• WebCore/editing/ApplyStyleCommand.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2009-07-22  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        execCommand('underline') can't remove <U> underlines
+        https://bugs.webkit.org/show_bug.cgi?id=20215
+
+        This patch rebaselines toggle-styles.html because WebKit now passes three tests it used to fail.
+        toggle-style-2.html is added to test cases in which multiple styles are specified with tags.
+        We still fail some tests because WebKit doesn't properly support non-CSS mode but they are visually correct.
+
+        * editing/execCommand/resources/toggle-style-2.js: Added.
+        (testSingleToggle):
+        (testDoubleToggle):
+        * editing/execCommand/toggle-style-2-expected.txt: Added.
+        * editing/execCommand/toggle-style-2.html: Added.
+        * editing/execCommand/toggle-styles-expected.txt: Passes all the tests
+
 2009-07-23  Jessie Berlin  <jberlin@apple.com>
 

trunk/LayoutTests/editing/execCommand/toggle-styles-expected.txt

@@ -20 +20 @@
 PASS superscript removing vertical-align: super
 PASS strikethrough toggle
-FAIL strikethrough removing strike -- <strike><span class="Apple-style-span" style="text-decoration: none;">test</span></strike>
+PASS strikethrough removing strike
 PASS strikethrough removing text-decoration: line-through
-FAIL strikethrough removing s -- <s><span class="Apple-style-span" style="text-decoration: none;">test</span></s>
+PASS strikethrough removing s
 PASS underline toggle
-FAIL underline removing u -- <u><span class="Apple-style-span" style="text-decoration: none;">test</span></u>
+PASS underline removing u
 PASS underline removing text-decoration: underline
 PASS successfullyParsed is true

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-07-22  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        execCommand('underline') can't remove <U> underlines
+        https://bugs.webkit.org/show_bug.cgi?id=20215
+
+        This patch adds support for u, s, and strike to implicitlyStyledElementShouldBeRemovedWhenApplyingStyle so that
+        WebKit can remove those presentational tags when necessary.
+        It also modifies StyleChange::init not to add "text-decoration: none".  Not only is this style meaningless
+        (does not override inherited styles) but it was also causing WebKit to generate extra spans with this style.
+
+        * css/CSSValueList.cpp:
+        (WebCore::CSSValueList::hasValue): True if the property contains the specified value
+        * css/CSSValueList.h: Updated prototype
+        * editing/ApplyStyleCommand.cpp:
+        (WebCore::StyleChange::init): No longer adds "text-decoration: none"
+        (WebCore::ApplyStyleCommand::implicitlyStyledElementShouldBeRemovedWhenApplyingStyle): Supports text-decoration-related elements
+
 2009-07-23  Jessie Berlin  <jberlin@apple.com>
         

trunk/WebCore/css/CSSValueList.cpp

@@ -85 +85 @@
     return found;
 }
+    
+bool CSSValueList::hasValue(CSSValue* val)
+{
+    // FIXME: we should be implementing operator== to CSSValue and its derived classes
+    // to make comparison more flexible and fast.
+    for (size_t index = 0; index < m_values.size(); index++) {
+        if (m_values.at(index)->cssText() == val->cssText())
+            return true;
+    }
+    return false;
+}
 
 String CSSValueList::cssText() const

trunk/WebCore/css/CSSValueList.h

@@ -54 +54 @@
     void prepend(PassRefPtr<CSSValue>);
     bool removeAll(CSSValue*);
+    bool hasValue(CSSValue*);
 
     virtual String cssText() const;

trunk/WebCore/editing/ApplyStyleCommand.cpp

@@ -99 +99 @@
     if (!document || !document->frame())
         return;
-        
+
     bool useHTMLFormattingTags = !document->frame()->editor()->shouldStyleWithCSS();
-            
     RefPtr<CSSMutableStyleDeclaration> mutableStyle = style->makeMutable();
-    
+    // We shouldn't have both text-decoration and -webkit-text-decorations-in-effect because that wouldn't make sense.
+    ASSERT(!mutableStyle->getPropertyCSSValue(CSSPropertyTextDecoration) || !mutableStyle->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect));
     String styleText("");
-
     bool addedDirection = false;
     CSSMutableStyleDeclaration::const_iterator end = mutableStyle->end();
@@ -131 +130 @@
 
         // Add this property
-
-        if (property->id() == CSSPropertyWebkitTextDecorationsInEffect) {
-            // we have to special-case text decorations
-            // FIXME: Why?
+        if (property->id() == CSSPropertyTextDecoration || property->id() == CSSPropertyWebkitTextDecorationsInEffect) {
+            // Always use text-decoration because -webkit-text-decoration-in-effect is internal.
             CSSProperty alteredProperty(CSSPropertyTextDecoration, property->value(), property->isImportant());
-            styleText += alteredProperty.cssText();
+            // We don't add "text-decoration: none" because it doesn't override the existing text decorations; i.e. redundant
+            if (alteredProperty.cssText() != "none")
+                styleText += alteredProperty.cssText();
         } else
             styleText += property->cssText();
@@ -963 +962 @@
                 return true;
             break;
+        case CSSPropertyTextDecoration:
+        case CSSPropertyWebkitTextDecorationsInEffect:
+                ASSERT(property.value());
+                if (property.value()->isValueList()) {
+                    CSSValueList* valueList = static_cast<CSSValueList*>(property.value());
+                    DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, underline, (CSSPrimitiveValue::createIdentifier(CSSValueUnderline)));
+                    DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, lineThrough, (CSSPrimitiveValue::createIdentifier(CSSValueLineThrough)));
+                    // Because style is new style to be applied, we delete element only if the element is not used in style.
+                    if (!valueList->hasValue(underline.get()) && elem->hasLocalName(uTag))
+                        return true;
+                    if (!valueList->hasValue(lineThrough.get()) && (elem->hasLocalName(strikeTag) || elem->hasLocalName(sTag)))
+                        return true;
+                } else {
+                    // If the value is NOT a list, then it must be "none", in which case we should remove all text decorations.
+                    ASSERT(property.value()->cssText() == "none");
+                    if (elem->hasLocalName(uTag) || elem->hasLocalName(strikeTag) || elem->hasLocalName(sTag))
+                        return true;
+                }
+                break;
         }
     }