[css3-text] Implement text-decoration property shorthand

Source/core/editing/EditingStyle.cpp

Index: Source/core/editing/EditingStyle.cpp
diff --git a/Source/core/editing/EditingStyle.cpp b/Source/core/editing/EditingStyle.cpp
index 82e5cd2352f5b2ca3b55eb150369922558b0388d..b1e51ee2eee304a1b7f43fc2d29724dca7f06d32 100644
--- a/Source/core/editing/EditingStyle.cpp
+++ b/Source/core/editing/EditingStyle.cpp
@@ -58,13 +58,14 @@ namespace WebCore {
 // Editing style properties must be preserved during editing operation.
 // e.g. when a user inserts a new paragraph, all properties listed here must be copied to the new paragraph.
 // NOTE: Use editingProperties() to respect runtime enabling of properties.
-static const unsigned nonInheritedStaticPropertiesCount = 2;
+static const unsigned nonInheritedStaticPropertiesCount = 3;
 
 static const CSSPropertyID staticEditingProperties[] = {
-    // NOTE: inheritableEditingProperties depends on these two properties being first.
+    // NOTE: inheritableEditingProperties depends on these properties being first.
     // If you change this list, make sure to update nonInheritedPropertyCount.
     CSSPropertyBackgroundColor,
     CSSPropertyTextDecoration,
+    CSSPropertyTextDecorationLine,
 
     // CSS inheritable properties
     CSSPropertyColor,
@@ -93,8 +94,11 @@ enum EditingPropertiesType { OnlyInheritableEditingProperties, AllEditingPropert
 static const Vector<CSSPropertyID>& allEditingProperties()
 {
     DEFINE_STATIC_LOCAL(Vector<CSSPropertyID>, properties, ());
-    if (properties.isEmpty())
+    if (properties.isEmpty()) {
         RuntimeCSSEnabled::filterEnabledCSSPropertiesIntoVector(staticEditingProperties, WTF_ARRAY_LENGTH(staticEditingProperties), properties);
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            properties.remove(properties.find(CSSPropertyTextDecoration));
+    }
     return properties;
 }
 
@@ -203,21 +207,27 @@ private:
 };
 
 HTMLTextDecorationEquivalent::HTMLTextDecorationEquivalent(CSSValueID primitiveValue, const QualifiedName& tagName)
-    : HTMLElementEquivalent(CSSPropertyTextDecoration, primitiveValue, tagName)
+    : HTMLElementEquivalent(RuntimeEnabledFeatures::css3TextDecorationsEnabled() ? CSSPropertyTextDecorationLine : CSSPropertyTextDecoration, primitiveValue, tagName)
     // m_propertyID is used in HTMLElementEquivalent::addToStyle
 {
 }
 
 bool HTMLTextDecorationEquivalent::propertyExistsInStyle(const StylePropertySet* style) const
 {
+    if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+        return style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecorationLine);
     return style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect) || style->getPropertyCSSValue(CSSPropertyTextDecoration);
 }
 
 bool HTMLTextDecorationEquivalent::valueIsPresentInStyle(Element* element, StylePropertySet* style) const
 {
     RefPtr<CSSValue> styleValue = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
-    if (!styleValue)
-        styleValue = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    if (!styleValue) {
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            styleValue = style->getPropertyCSSValue(CSSPropertyTextDecorationLine);
+        else
+            styleValue = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    }
     return matches(element) && styleValue && styleValue->isValueList() && static_cast<CSSValueList*>(styleValue.get())->hasValue(m_primitiveValue.get());
 }
 
@@ -441,8 +451,11 @@ void EditingStyle::init(Node* node, PropertiesToInclude propertiesToInclude)
     if (propertiesToInclude == EditingPropertiesInEffect) {
         if (RefPtr<CSSValue> value = backgroundColorInEffect(node))
             m_mutableStyle->setProperty(CSSPropertyBackgroundColor, value->cssText());
-        if (RefPtr<CSSValue> value = computedStyleAtPosition->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect))
+        if (RefPtr<CSSValue> value = computedStyleAtPosition->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect)) {
             m_mutableStyle->setProperty(CSSPropertyTextDecoration, value->cssText());
+            if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+                m_mutableStyle->setProperty(CSSPropertyTextDecorationLine, value->cssText());
+        }
     }
 
     if (node && node->computedStyle()) {
@@ -650,16 +663,23 @@ void EditingStyle::collapseTextDecorationProperties()
     if (!textDecorationsInEffect)
         return;
 
-    if (textDecorationsInEffect->isValueList())
-        m_mutableStyle->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(CSSPropertyTextDecoration));
-    else
+    if (textDecorationsInEffect->isValueList()) {
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            m_mutableStyle->setProperty(CSSPropertyTextDecorationLine, textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(CSSPropertyTextDecorationLine));
+        else
+            m_mutableStyle->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(CSSPropertyTextDecoration));
+    } else {
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            m_mutableStyle->removeProperty(CSSPropertyTextDecorationLine);
         m_mutableStyle->removeProperty(CSSPropertyTextDecoration);

[Julien - ping for review, 2013/07/29 18:53:12]

This should be in an 'else': shorthands are expanded at parsing time so we
shouldn't see them here.

[abinader, 2013/07/29 21:15:32]

You are right, I probably overlooked at it.

+    }
     m_mutableStyle->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
 }
 
 // CSS properties that create a visual difference only when applied to text.
 static const CSSPropertyID textOnlyProperties[] = {
     CSSPropertyTextDecoration,

[Julien - ping for review, 2013/07/29 18:53:12]

Note that once we remove RuntimeEnabledFeatures::css3TextDecorationsEnabled()
this should be removed from the list as we should never see a shorthand.

[abinader, 2013/07/29 21:15:32]

Ack, I'll add a FIXME: Needs to be removed when CSS3 Text Decoration feature is
no longer experimental.

+    CSSPropertyTextDecorationLine,
     CSSPropertyWebkitTextDecorationsInEffect,
     CSSPropertyFontStyle,
     CSSPropertyFontWeight,
@@ -732,13 +752,29 @@ bool EditingStyle::conflictsWithInlineStyleOfElement(Element* element, EditingSt
         if (propertyID == CSSPropertyWhiteSpace && isTabSpanNode(element))
             continue;
 
-        if (propertyID == CSSPropertyWebkitTextDecorationsInEffect && inlineStyle->getPropertyCSSValue(CSSPropertyTextDecoration)) {
-            if (!conflictingProperties)
-                return true;
-            conflictingProperties->append(CSSPropertyTextDecoration);
-            if (extractedStyle)
-                extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->propertyIsImportant(CSSPropertyTextDecoration));
-            continue;
+        if (propertyID == CSSPropertyWebkitTextDecorationsInEffect) {

[Julien - ping for review, 2013/07/29 18:53:12]

Probably overstepping my knowledge of editing here but
WebKitTextDecorationInEffect seems very much unused in our metrics. The extra
complexity from it seems nasty so maybe we could remove it and simplify this CL?
(it shouldn't block this CL though and could be removed as a follow-up CL but I
would prefer to do this change before)

[abinader, 2013/07/29 21:15:32]

Yes, totally agreed upon removing this somewhat hackish property - however, I'm
afraid due to its complexity it would be better to postpone this task to be done
after this patch gets landed (as we dicussed on IRC). 

+            if (RuntimeEnabledFeatures::css3TextDecorationsEnabled()) {
+                RefPtr<CSSValue> textDecorationLine = inlineStyle->getPropertyCSSValue(CSSPropertyTextDecorationLine);

[Julien - ping for review, 2013/07/29 18:53:12]

This is usually inlined into the if () statement as you only need
|textDecorationLine| inside the branch.

[abinader, 2013/07/29 21:15:32]

Indeed, makes code looks sharper as well :)

+                if (textDecorationLine) {
+                    if (!conflictingProperties)
+                        return true;
+                    conflictingProperties->append(CSSPropertyTextDecoration);
+                    conflictingProperties->append(CSSPropertyTextDecorationLine);
+                    if (extractedStyle)
+                        extractedStyle->setProperty(CSSPropertyTextDecorationLine, inlineStyle->getPropertyValue(CSSPropertyTextDecorationLine), inlineStyle->propertyIsImportant(CSSPropertyTextDecorationLine));
+                    continue;
+                }
+            } else {
+                RefPtr<CSSValue> textDecoration = inlineStyle->getPropertyCSSValue(CSSPropertyTextDecoration);
+                if (textDecoration) {
+                    if (!conflictingProperties)
+                        return true;
+                    conflictingProperties->append(CSSPropertyTextDecoration);
+                    if (extractedStyle)
+                        extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->propertyIsImportant(CSSPropertyTextDecoration));
+                    continue;
+                }
+            }
         }
 
         if (!inlineStyle->getPropertyCSSValue(propertyID))
@@ -1078,7 +1114,12 @@ void EditingStyle::mergeStyle(const StylePropertySet* style, CSSPropertyOverride
         RefPtr<CSSValue> value = m_mutableStyle->getPropertyCSSValue(property.id());
 
         // text decorations never override values
-        if ((property.id() == CSSPropertyTextDecoration || property.id() == CSSPropertyWebkitTextDecorationsInEffect) && property.value()->isValueList() && value) {
+        bool isPropertyTextDecorationEquivalent = property.id() == CSSPropertyWebkitTextDecorationsInEffect;
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+            isPropertyTextDecorationEquivalent |= property.id() == CSSPropertyTextDecorationLine;
+        else
+            isPropertyTextDecorationEquivalent |= property.id() == CSSPropertyTextDecoration;
+        if (isPropertyTextDecorationEquivalent && property.value()->isValueList() && value) {
             if (value->isValueList()) {
                 mergeTextDecorationValues(static_cast<CSSValueList*>(value.get()), static_cast<CSSValueList*>(property.value()));
                 continue;
@@ -1334,17 +1375,28 @@ static void reconcileTextDecorationProperties(MutableStylePropertySet* style)
 {
     RefPtr<CSSValue> textDecorationsInEffect = style->getPropertyCSSValue(CSSPropertyWebkitTextDecorationsInEffect);
     RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    RefPtr<CSSValue> textDecorationLine = style->getPropertyCSSValue(CSSPropertyTextDecorationLine);
     // We shouldn't have both text-decoration and -webkit-text-decorations-in-effect because that wouldn't make sense.
-    ASSERT(!textDecorationsInEffect || !textDecoration);
+    ASSERT(!textDecorationsInEffect || (!textDecoration || !textDecorationLine));
     if (textDecorationsInEffect) {
-        style->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText());
+        if (RuntimeEnabledFeatures::css3TextDecorationsEnabled()) {
+            style->setProperty(CSSPropertyTextDecorationLine, textDecorationsInEffect->cssText());
+            textDecorationLine = textDecorationsInEffect;
+        } else {
+            style->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText());
+            textDecoration = textDecorationsInEffect;
+        }
         style->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
-        textDecoration = textDecorationsInEffect;
     }
 
     // If text-decoration is set to "none", remove the property because we don't want to add redundant "text-decoration: none".
-    if (textDecoration && !textDecoration->isValueList())
-        style->removeProperty(CSSPropertyTextDecoration);
+    if (RuntimeEnabledFeatures::css3TextDecorationsEnabled()) {
+        if (textDecorationLine && !textDecorationLine->isValueList())
+            style->removeProperty(CSSPropertyTextDecorationLine);
+    } else {
+        if (textDecoration && !textDecoration->isValueList())
+            style->removeProperty(CSSPropertyTextDecoration);
+    }
 }
 
 StyleChange::StyleChange(EditingStyle* style, const Position& position)
@@ -1408,7 +1460,8 @@ void StyleChange::extractTextStyles(Document* document, MutableStylePropertySet*
 
     // Assuming reconcileTextDecorationProperties has been called, there should not be -webkit-text-decorations-in-effect
     // Furthermore, text-decoration: none has been trimmed so that text-decoration property is always a CSSValueList.
-    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(CSSPropertyTextDecoration);
+    CSSPropertyID textDecorationProperty = RuntimeEnabledFeatures::css3TextDecorationsEnabled() ? CSSPropertyTextDecorationLine : CSSPropertyTextDecoration;
+    RefPtr<CSSValue> textDecoration = style->getPropertyCSSValue(textDecorationProperty);
     if (textDecoration && textDecoration->isValueList()) {
         DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, underline, (CSSPrimitiveValue::createIdentifier(CSSValueUnderline)));
         DEFINE_STATIC_LOCAL(RefPtr<CSSPrimitiveValue>, lineThrough, (CSSPrimitiveValue::createIdentifier(CSSValueLineThrough)));
@@ -1420,7 +1473,7 @@ void StyleChange::extractTextStyles(Document* document, MutableStylePropertySet*
             m_applyLineThrough = true;
 
         // If trimTextDecorations, delete underline and line-through
-        setTextDecorationProperty(style, newTextDecoration.get(), CSSPropertyTextDecoration);
+        setTextDecorationProperty(style, newTextDecoration.get(), textDecorationProperty);
     }
 
     int verticalAlign = getIdentifierValue(style, CSSPropertyVerticalAlign);
@@ -1525,7 +1578,10 @@ PassRefPtr<MutableStylePropertySet> getPropertiesNotIn(StylePropertySet* styleWi
     result->removeEquivalentProperties(baseStyle);
 
     RefPtr<CSSValue> baseTextDecorationsInEffect = baseStyle->getPropertyCSSValueInternal(CSSPropertyWebkitTextDecorationsInEffect);
-    diffTextDecorations(result.get(), CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
+    if (RuntimeEnabledFeatures::css3TextDecorationsEnabled())
+        diffTextDecorations(result.get(), CSSPropertyTextDecorationLine, baseTextDecorationsInEffect.get());
+    else
+        diffTextDecorations(result.get(), CSSPropertyTextDecoration, baseTextDecorationsInEffect.get());
     diffTextDecorations(result.get(), CSSPropertyWebkitTextDecorationsInEffect, baseTextDecorationsInEffect.get());
 
     if (baseStyle->getPropertyCSSValueInternal(CSSPropertyFontWeight) && fontWeightIsBold(result.get()) == fontWeightIsBold(baseStyle))