CSSStyleDeclaration.getPropertyPriority() fails for CSS shorthand pro…

…perties with 'important' priority https://bugs.webkit.org/show_bug.cgi?id=49058 Reviewed by Andreas Kling. Source/WebCore: CSSMutableStyleDeclaration::getPropertyPriority was not handling shorthands properly. Shorthands are not part of the property list of the style so we need to query the longhands which are the one added in the list. Only if the longhands have equal priority the shorthand priority is known. I also renamed getPropertyPriority (not the CSSOM exposed method) to something more consistent with WebKit naming guidelines. Test: fast/css/shorthand-priority.html * css/CSSMutableStyleDeclaration.cpp: (WebCore::CSSMutableStyleDeclaration::propertyIsImportant): (WebCore::CSSMutableStyleDeclaration::addParsedProperty): (WebCore::CSSMutableStyleDeclaration::getPropertyPriority): * css/CSSMutableStyleDeclaration.h: (CSSMutableStyleDeclaration): * editing/EditingStyle.cpp: (WebCore::EditingStyle::extractAndRemoveTextDirection): (WebCore::EditingStyle::collapseTextDecorationProperties): (WebCore::EditingStyle::conflictsWithInlineStyleOfElement): (WebCore::setTextDecorationProperty): * editing/RemoveCSSPropertyCommand.cpp: (WebCore::RemoveCSSPropertyCommand::doApply): Source/WebKit/qt: Update the code as getPropertyPriority has been renamed to propertyIsImportant. * Api/qwebelement.cpp: (QWebElement::styleProperty): LayoutTests: * fast/css/shorthand-priority-expected.txt: Added. * fast/css/shorthand-priority.html: Added. Canonical link: https://commits.webkit.org/94440@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@106490 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Feb 1, 2012 · 7a3a15b0635d426625be394186c19c5ea0102e3b · 7a3a15b
1 parent 98f98ad
commit 7a3a15b0635d426625be394186c19c5ea0102e3b
Showing 10 changed files with 130 additions and 16 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2012-02-01  Alexis Menard  <alexis.menard@openbossa.org>
+
+        CSSStyleDeclaration.getPropertyPriority() fails for CSS shorthand properties with 'important' priority
+        https://bugs.webkit.org/show_bug.cgi?id=49058
+
+        Reviewed by Andreas Kling.
+
+        * fast/css/shorthand-priority-expected.txt: Added.
+        * fast/css/shorthand-priority.html: Added.
+
 2012-02-01  Ryosuke Niwa  <rniwa@webkit.org>
 
         Crash in EventHandler::updateDragAndDrop

diff --git a/LayoutTests/fast/css/shorthand-priority-expected.txt b/LayoutTests/fast/css/shorthand-priority-expected.txt
@@ -0,0 +1,15 @@
+Tests that querying the priority for a shorthand works.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS e.style.getPropertyValue('border-bottom-style') is 'solid'
+PASS e.style.getPropertyPriority('border-bottom-style') is 'important'
+PASS e.style.getPropertyValue('border') is '20px solid green'
+PASS e.style.getPropertyPriority('border') is ''
+PASS e.style.getPropertyValue('border') is '20px solid green'
+PASS e.style.getPropertyPriority('border') is 'important'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/shorthand-priority.html b/LayoutTests/fast/css/shorthand-priority.html
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<html>
+<head>
+<meta charset="utf-8">
+<script src="../js/resources/js-test-pre.js"></script>
+</head>
+<body>
+<script>
+
+description("Tests that querying the priority for a shorthand works.");
+
+var testContainer = document.createElement("div");
+document.body.appendChild(testContainer);
+
+testContainer.innerHTML = '<div id="test">hello</div>';
+
+e = document.getElementById('test');
+
+// Sanity check.
+e.style.setProperty("border-bottom-style", "solid", "!important");
+shouldBe("e.style.getPropertyValue('border-bottom-style')", "'solid'");
+shouldBe("e.style.getPropertyPriority('border-bottom-style')", "'important'");
+
+e.style.borderBottomStyle = "";
+e.style.border = "20px solid green";
+shouldBe("e.style.getPropertyValue('border')", "'20px solid green'");
+shouldBe("e.style.getPropertyPriority('border')", "''");
+
+e.style.border = "";
+e.style.setProperty("border", "20px solid green", "!important");
+shouldBe("e.style.getPropertyValue('border')", "'20px solid green'");
+shouldBe("e.style.getPropertyPriority('border')", "'important'");
+
+document.body.removeChild(testContainer);
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,31 @@
+2012-02-01  Alexis Menard  <alexis.menard@openbossa.org>
+
+        CSSStyleDeclaration.getPropertyPriority() fails for CSS shorthand properties with 'important' priority
+        https://bugs.webkit.org/show_bug.cgi?id=49058
+
+        Reviewed by Andreas Kling.
+
+        CSSMutableStyleDeclaration::getPropertyPriority was not handling shorthands properly. Shorthands are
+        not part of the property list of the style so we need to query the longhands which are the one added
+        in the list. Only if the longhands have equal priority the shorthand priority is known. I also renamed
+        getPropertyPriority (not the CSSOM exposed method) to something more consistent with WebKit naming guidelines.
+
+        Test: fast/css/shorthand-priority.html
+
+        * css/CSSMutableStyleDeclaration.cpp:
+        (WebCore::CSSMutableStyleDeclaration::propertyIsImportant):
+        (WebCore::CSSMutableStyleDeclaration::addParsedProperty):
+        (WebCore::CSSMutableStyleDeclaration::getPropertyPriority):
+        * css/CSSMutableStyleDeclaration.h:
+        (CSSMutableStyleDeclaration):
+        * editing/EditingStyle.cpp:
+        (WebCore::EditingStyle::extractAndRemoveTextDirection):
+        (WebCore::EditingStyle::collapseTextDecorationProperties):
+        (WebCore::EditingStyle::conflictsWithInlineStyleOfElement):
+        (WebCore::setTextDecorationProperty):
+        * editing/RemoveCSSPropertyCommand.cpp:
+        (WebCore::RemoveCSSPropertyCommand::doApply):
+
 2012-02-01  Ryosuke Niwa  <rniwa@webkit.org>
 
         Crash in EventHandler::updateDragAndDrop

diff --git a/Source/WebCore/css/CSSMutableStyleDeclaration.cpp b/Source/WebCore/css/CSSMutableStyleDeclaration.cpp
@@ -658,10 +658,21 @@ void CSSMutableStyleDeclaration::setNeedsStyleRecalc()
     }
 }
 
-bool CSSMutableStyleDeclaration::getPropertyPriority(int propertyID) const
+bool CSSMutableStyleDeclaration::propertyIsImportant(int propertyID) const
 {
     const CSSProperty* property = findPropertyWithId(propertyID);
-    return property ? property->isImportant() : false;
+    if (property)
+        return property->isImportant();
+
+    CSSPropertyLonghand longhands = longhandForProperty(propertyID);
+    if (!longhands.length())
+        return false;
+
+    for (unsigned i = 0; i < longhands.length(); ++i) {
+        if (!propertyIsImportant(longhands.properties()[i]))
+            return false;
+    }
+    return true;
 }
 
 int CSSMutableStyleDeclaration::getPropertyShorthand(int propertyID) const
@@ -788,7 +799,7 @@ void CSSMutableStyleDeclaration::addParsedProperty(const CSSProperty& property)
 #endif
 
     // Only add properties that have no !important counterpart present
-    if (!getPropertyPriority(property.id()) || property.isImportant()) {
+    if (!propertyIsImportant(property.id()) || property.isImportant()) {
         removeProperty(property.id(), false, false);
         m_properties.append(property);
     }
@@ -1046,7 +1057,7 @@ String CSSMutableStyleDeclaration::getPropertyPriority(const String& propertyNam
     int propertyID = cssPropertyID(propertyName);
     if (!propertyID)
         return String();
-    return getPropertyPriority(propertyID) ? "important" : "";
+    return propertyIsImportant(propertyID) ? "important" : "";
 }
 
 String CSSMutableStyleDeclaration::getPropertyShorthand(const String& propertyName)

diff --git a/Source/WebCore/css/CSSMutableStyleDeclaration.h b/Source/WebCore/css/CSSMutableStyleDeclaration.h
@@ -64,7 +64,7 @@ class CSSMutableStyleDeclaration : public CSSStyleDeclaration {
 
     PassRefPtr<CSSValue> getPropertyCSSValue(int propertyID) const;
     String getPropertyValue(int propertyID) const;
-    bool getPropertyPriority(int propertyID) const;
+    bool propertyIsImportant(int propertyID) const;
     int getPropertyShorthand(int propertyID) const;
     bool isPropertyImplicit(int propertyID) const;
 

diff --git a/Source/WebCore/editing/EditingStyle.cpp b/Source/WebCore/editing/EditingStyle.cpp
@@ -521,9 +521,9 @@ PassRefPtr<EditingStyle> EditingStyle::extractAndRemoveTextDirection()
 {
     RefPtr<EditingStyle> textDirection = EditingStyle::create();
     textDirection->m_mutableStyle = CSSMutableStyleDeclaration::create();
-    textDirection->m_mutableStyle->setProperty(CSSPropertyUnicodeBidi, CSSValueEmbed, m_mutableStyle->getPropertyPriority(CSSPropertyUnicodeBidi));
+    textDirection->m_mutableStyle->setProperty(CSSPropertyUnicodeBidi, CSSValueEmbed, m_mutableStyle->propertyIsImportant(CSSPropertyUnicodeBidi));
     textDirection->m_mutableStyle->setProperty(CSSPropertyDirection, m_mutableStyle->getPropertyValue(CSSPropertyDirection),
-        m_mutableStyle->getPropertyPriority(CSSPropertyDirection));
+        m_mutableStyle->propertyIsImportant(CSSPropertyDirection));
 
     m_mutableStyle->removeProperty(CSSPropertyUnicodeBidi);
     m_mutableStyle->removeProperty(CSSPropertyDirection);
@@ -579,7 +579,7 @@ void EditingStyle::collapseTextDecorationProperties()
         return;
 
     if (textDecorationsInEffect->isValueList())
-        m_mutableStyle->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText(), m_mutableStyle->getPropertyPriority(CSSPropertyTextDecoration));
+        m_mutableStyle->setProperty(CSSPropertyTextDecoration, textDecorationsInEffect->cssText(), m_mutableStyle->propertyIsImportant(CSSPropertyTextDecoration));
     else
         m_mutableStyle->removeProperty(CSSPropertyTextDecoration);
     m_mutableStyle->removeProperty(CSSPropertyWebkitTextDecorationsInEffect);
@@ -665,7 +665,7 @@ bool EditingStyle::conflictsWithInlineStyleOfElement(StyledElement* element, Edi
                 return true;
             conflictingProperties->append(CSSPropertyTextDecoration);
             if (extractedStyle)
-                extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->getPropertyPriority(CSSPropertyTextDecoration));
+                extractedStyle->setProperty(CSSPropertyTextDecoration, inlineStyle->getPropertyValue(CSSPropertyTextDecoration), inlineStyle->propertyIsImportant(CSSPropertyTextDecoration));
             continue;
         }
 
@@ -677,7 +677,7 @@ bool EditingStyle::conflictsWithInlineStyleOfElement(StyledElement* element, Edi
                 return true;
             conflictingProperties->append(CSSPropertyDirection);
             if (extractedStyle)
-                extractedStyle->setProperty(propertyID, inlineStyle->getPropertyValue(propertyID), inlineStyle->getPropertyPriority(propertyID));
+                extractedStyle->setProperty(propertyID, inlineStyle->getPropertyValue(propertyID), inlineStyle->propertyIsImportant(propertyID));
         }
 
         if (!conflictingProperties)
@@ -686,7 +686,7 @@ bool EditingStyle::conflictsWithInlineStyleOfElement(StyledElement* element, Edi
         conflictingProperties->append(propertyID);
 
         if (extractedStyle)
-            extractedStyle->setProperty(propertyID, inlineStyle->getPropertyValue(propertyID), inlineStyle->getPropertyPriority(propertyID));
+            extractedStyle->setProperty(propertyID, inlineStyle->getPropertyValue(propertyID), inlineStyle->propertyIsImportant(propertyID));
     }
 
     return conflictingProperties && !conflictingProperties->isEmpty();
@@ -1221,10 +1221,10 @@ StyleChange::StyleChange(EditingStyle* style, const Position& position)
 static void setTextDecorationProperty(CSSMutableStyleDeclaration* style, const CSSValueList* newTextDecoration, int propertyID)
 {
     if (newTextDecoration->length())
-        style->setProperty(propertyID, newTextDecoration->cssText(), style->getPropertyPriority(propertyID));
+        style->setProperty(propertyID, newTextDecoration->cssText(), style->propertyIsImportant(propertyID));
     else {
         // text-decoration: none is redundant since it does not remove any text decorations.
-        ASSERT(!style->getPropertyPriority(propertyID));
+        ASSERT(!style->propertyIsImportant(propertyID));
         style->removeProperty(propertyID);
     }
 }

diff --git a/Source/WebCore/editing/RemoveCSSPropertyCommand.cpp b/Source/WebCore/editing/RemoveCSSPropertyCommand.cpp
@@ -44,7 +44,7 @@ void RemoveCSSPropertyCommand::doApply()
 {
     CSSMutableStyleDeclaration* style = m_element->inlineStyleDecl();
     m_oldValue = style->getPropertyValue(m_property);
-    m_important = style->getPropertyPriority(m_property);
+    m_important = style->propertyIsImportant(m_property);
     style->removeProperty(m_property);
 }
 

diff --git a/Source/WebKit/qt/Api/qwebelement.cpp b/Source/WebKit/qt/Api/qwebelement.cpp
@@ -850,7 +850,7 @@ QString QWebElement::styleProperty(const QString &name, StyleResolveStrategy str
         return style->getPropertyValue(propID);
 
     if (strategy == CascadedStyle) {
-        if (style->getPropertyPriority(propID))
+        if (style->propertyIsImportant(propID))
             return style->getPropertyValue(propID);
 
         // We are going to resolve the style property by walking through the
@@ -866,7 +866,7 @@ QString QWebElement::styleProperty(const QString &name, StyleResolveStrategy str
             for (int i = rules->length(); i > 0; --i) {
                 CSSStyleRule* rule = static_cast<CSSStyleRule*>(rules->item(i - 1));
 
-                if (rule->declaration()->getPropertyPriority(propID))
+                if (rule->declaration()->propertyIsImportant(propID))
                     return rule->declaration()->getPropertyValue(propID);
 
                 if (style->getPropertyValue(propID).isEmpty())

diff --git a/Source/WebKit/qt/ChangeLog b/Source/WebKit/qt/ChangeLog
@@ -1,3 +1,15 @@
+2012-02-01  Alexis Menard  <alexis.menard@openbossa.org>
+
+        CSSStyleDeclaration.getPropertyPriority() fails for CSS shorthand properties with 'important' priority
+        https://bugs.webkit.org/show_bug.cgi?id=49058
+
+        Reviewed by Andreas Kling.
+
+        Update the code as getPropertyPriority has been renamed to propertyIsImportant.
+
+        * Api/qwebelement.cpp:
+        (QWebElement::styleProperty):
+
 2012-01-31  Antti Koivisto  <antti@apple.com>
 
         Try to fix Qt build.