Bug 556661: make nsDOMCSSDeclaration::SetProperty capable of removing !important. r=dbaron
authorZack Weinberg <zweinberg@mozilla.com>
Tue, 06 Apr 2010 15:52:02 -0700
changeset 40520 54d74759a24c9cf33d8b9794bf85db58cad8271e
parent 40519 1adc864a08ce2d6ffc7e9369f7912ea58d071bac
child 40521 a016ac686be0d126b9aa13c6d887306095539e7f
push id12644
push userzweinberg@mozilla.com
push dateTue, 06 Apr 2010 22:54:43 +0000
treeherdermozilla-central@54d74759a24c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs556661
milestone1.9.3a4pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
Bug 556661: make nsDOMCSSDeclaration::SetProperty capable of removing !important. r=dbaron
layout/reftests/bugs/556661-1-ref.html
layout/reftests/bugs/556661-1.html
layout/reftests/bugs/reftest.list
layout/style/nsCSSParser.cpp
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/556661-1-ref.html
@@ -0,0 +1,24 @@
+<!doctype html>
+<html><head><title>Dynamic manipulation of !important</title>
+<style>
+div { float: left; width: 50px; height: 50px; margin: 5px;
+      background-color: green }
+div#control { width: 230px }
+p { clear: left }
+</style>
+<body>
+<div></div>
+<div></div>
+<div></div>
+<div></div>
+<p></p>
+<div></div>
+<div></div>
+<div></div>
+<div></div>
+<p></p>
+<div id="control"></div>
+<p>There should be two rows of four green squares and one solid green
+   bar above.</p>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/bugs/556661-1.html
@@ -0,0 +1,63 @@
+<!doctype html>
+<html><head><title>Dynamic manipulation of !important</title>
+<style>
+div { float: left; width: 50px; height: 50px; margin: 5px }
+div#control {
+  width: 230px;
+  background-color: green !important;
+  background-color: red;
+}
+div#a { background-color: green }
+div#b { background-color: orange }
+div.c { background-color: orange }
+div#d { background-color: orange }
+div#e { background-color: green }
+div#f { background-color: orange }
+div.g { background-color: orange }
+div#h { background-color: orange }
+p { clear: left }
+</style>
+<style>
+div.a { background-color: red !important }
+div.b { background-color: red !important }
+div#c { background-color: red }
+div.d { background-color: red }
+div.e { background-color: red !important }
+div.f { background-color: red !important }
+div#g { background-color: red }
+div.h { background-color: red }
+</style>
+<script>
+window.onload = function() {
+  var r = document.styleSheets[1].cssRules;
+  r[0].style.setProperty("background-color", "yellow", "");
+  r[1].style.setProperty("background-color", "green", "important");
+  r[2].style.setProperty("background-color", "green", "");
+  r[3].style.setProperty("background-color", "green", "important");
+
+  r[4].style.removeProperty("background-color");
+  r[4].style.setProperty("background-color", "yellow", "");
+  r[5].style.removeProperty("background-color");
+  r[5].style.setProperty("background-color", "green", "important");
+  r[6].style.removeProperty("background-color");
+  r[6].style.setProperty("background-color", "green", "");
+  r[7].style.removeProperty("background-color");
+  r[7].style.setProperty("background-color", "green", "important");
+}
+</script>
+<body>
+<div class="a" id="a"></div>
+<div class="b" id="b"></div>
+<div class="c" id="c"></div>
+<div class="d" id="d"></div>
+<p></p>
+<div class="e" id="e"></div>
+<div class="f" id="f"></div>
+<div class="g" id="g"></div>
+<div class="h" id="h"></div>
+<p></p>
+<div id="control"></div>
+<p>There should be two rows of four green squares and one solid green
+   bar above.</p>
+</body>
+</html>
--- a/layout/reftests/bugs/reftest.list
+++ b/layout/reftests/bugs/reftest.list
@@ -1414,10 +1414,11 @@ fails-if(http.oscpu.match(/Mac\x20OS\x20
 random-if(!haveTestPlugin) == 541406-1.html 541406-1-ref.html
 == 542620-1.html 542620-1-ref.html
 == 545049-1.html 545049-1-ref.html
 == 546033-1.html 546033-1-ref.html
 random-if(!haveTestPlugin) == 546071-1.html 546071-1-ref.html
 == 549184-1.html 549184-1-ref.html
 == 550716-1.html 550716-1-ref.html
 == 551463-1.html 551463-1-ref.html
+== 551699-1.html 551699-1-ref.html
 == 552334-1.html 552334-1-ref.html
-== 551699-1.html 551699-1-ref.html
+== 556661-1.html 556661-1-ref.html
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -398,23 +398,29 @@ protected:
                           PRBool* aChanged);
   // After a parse error parsing |aPropID|, clear the data in
   // |mTempData|.
   void ClearTempData(nsCSSProperty aPropID);
   // After a successful parse of |aPropID|, transfer data from
   // |mTempData| to |mData|.  Set |*aChanged| to true if something
   // changed, but leave it unmodified otherwise.  If aMustCallValueAppended
   // is false, will not call ValueAppended on aDeclaration if the property
-  // is already set in it.
+  // is already set in it.  If aOverrideImportant is true, new data will
+  // replace old settings of the same properties, even if the old settings
+  // are !important and the new data aren't.
   void TransferTempData(nsCSSDeclaration* aDeclaration,
-                        nsCSSProperty aPropID, PRBool aIsImportant,
+                        nsCSSProperty aPropID,
+                        PRBool aIsImportant,
+                        PRBool aOverrideImportant,
                         PRBool aMustCallValueAppended,
                         PRBool* aChanged);
   void DoTransferTempData(nsCSSDeclaration* aDeclaration,
-                          nsCSSProperty aPropID, PRBool aIsImportant,
+                          nsCSSProperty aPropID,
+                          PRBool aIsImportant,
+                          PRBool aOverrideImportant,
                           PRBool aMustCallValueAppended,
                           PRBool* aChanged);
   // Used to do a fast copy of a property value from source location to
   // destination location.  It's the caller's responsibility to make sure that
   // the source and destination locations point to the right kind of objects
   // for the property id.  This can only be used for non-shorthand properties.
   void CopyValue(void *aSource, void *aDest, nsCSSProperty aPropID,
                  PRBool* aChanged);
@@ -1139,17 +1145,18 @@ CSSParserImpl::ParseProperty(const nsCSS
   }
   nsresult result = NS_OK;
   PRBool parsedOK = ParseProperty(aPropID);
   if (parsedOK && !GetToken(PR_TRUE)) {
     if (valueSlot) {
       CopyValue(mTempData.PropertyAt(aPropID), valueSlot, aPropID, aChanged);
       mTempData.ClearPropertyBit(aPropID);
     } else {
-      TransferTempData(aDeclaration, aPropID, aIsImportant, PR_FALSE, aChanged);
+      TransferTempData(aDeclaration, aPropID, aIsImportant,
+                       PR_TRUE, PR_FALSE, aChanged);
     }
   } else {
     if (parsedOK) {
       // Junk at end of property value.
       REPORT_UNEXPECTED_TOKEN(PEExpectEndValue);
     }
     NS_ConvertASCIItoUTF16 propName(nsCSSProps::GetStringValue(aPropID));
     const PRUnichar *params[] = {
@@ -3978,17 +3985,17 @@ CSSParserImpl::ParseDeclaration(nsCSSDec
     return PR_FALSE;
   }
   CLEAR_ERROR();
 
   // See if the declaration is followed by a "!important" declaration
   PRBool isImportant = PR_FALSE;
   if (!GetToken(PR_TRUE)) {
     // EOF is a perfectly good way to end a declaration and declaration block
-    TransferTempData(aDeclaration, propID, isImportant,
+    TransferTempData(aDeclaration, propID, isImportant, PR_FALSE,
                      aMustCallValueAppended, aChanged);
     return PR_TRUE;
   }
 
   if (eCSSToken_Symbol == tk->mType && '!' == tk->mSymbol) {
     // Look for important ident
     if (!GetToken(PR_TRUE)) {
       // Premature eof is not ok
@@ -4011,31 +4018,31 @@ CSSParserImpl::ParseDeclaration(nsCSSDec
     UngetToken();
   }
 
   // Make sure valid property declaration is terminated with either a
   // semicolon, EOF or a right-curly-brace (this last only when
   // aCheckForBraces is true).
   if (!GetToken(PR_TRUE)) {
     // EOF is a perfectly good way to end a declaration and declaration block
-    TransferTempData(aDeclaration, propID, isImportant,
+    TransferTempData(aDeclaration, propID, isImportant, PR_FALSE,
                      aMustCallValueAppended, aChanged);
     return PR_TRUE;
   }
   if (eCSSToken_Symbol == tk->mType) {
     if (';' == tk->mSymbol) {
-      TransferTempData(aDeclaration, propID, isImportant,
+      TransferTempData(aDeclaration, propID, isImportant, PR_FALSE,
                        aMustCallValueAppended, aChanged);
       return PR_TRUE;
     }
     if (aCheckForBraces && '}' == tk->mSymbol) {
       // Unget the '}' so we'll be able to tell that this is the end
       // of the declaration block when we unwind from here.
       UngetToken();
-      TransferTempData(aDeclaration, propID, isImportant,
+      TransferTempData(aDeclaration, propID, isImportant, PR_FALSE,
                        aMustCallValueAppended, aChanged);
       return PR_TRUE;
     }
   }
   if (aCheckForBraces)
     REPORT_UNEXPECTED_TOKEN(PEBadDeclOrRuleEnd2);
   else
     REPORT_UNEXPECTED_TOKEN(PEBadDeclEnd);
@@ -4055,50 +4062,63 @@ CSSParserImpl::ClearTempData(nsCSSProper
   } else {
     mTempData.ClearProperty(aPropID);
   }
   mTempData.AssertInitialState();
 }
 
 void
 CSSParserImpl::TransferTempData(nsCSSDeclaration* aDeclaration,
-                                nsCSSProperty aPropID, PRBool aIsImportant,
+                                nsCSSProperty aPropID,
+                                PRBool aIsImportant,
+                                PRBool aOverrideImportant,
                                 PRBool aMustCallValueAppended,
                                 PRBool* aChanged)
 {
   if (nsCSSProps::IsShorthand(aPropID)) {
     CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(p, aPropID) {
-      DoTransferTempData(aDeclaration, *p, aIsImportant,
+      DoTransferTempData(aDeclaration, *p, aIsImportant, aOverrideImportant,
                          aMustCallValueAppended, aChanged);
     }
   } else {
-    DoTransferTempData(aDeclaration, aPropID, aIsImportant,
+    DoTransferTempData(aDeclaration, aPropID, aIsImportant, aOverrideImportant,
                        aMustCallValueAppended, aChanged);
   }
   mTempData.AssertInitialState();
 }
 
 // Perhaps the transferring code should be in nsCSSExpandedDataBlock, in
 // case some other caller wants to use it in the future (although I
 // can't think of why).
 void
 CSSParserImpl::DoTransferTempData(nsCSSDeclaration* aDeclaration,
-                                  nsCSSProperty aPropID, PRBool aIsImportant,
+                                  nsCSSProperty aPropID,
+                                  PRBool aIsImportant,
+                                  PRBool aOverrideImportant,
                                   PRBool aMustCallValueAppended,
                                   PRBool* aChanged)
 {
   NS_ASSERTION(mTempData.HasPropertyBit(aPropID), "oops");
   if (aIsImportant) {
     if (!mData.HasImportantBit(aPropID))
       *aChanged = PR_TRUE;
     mData.SetImportantBit(aPropID);
   } else {
     if (mData.HasImportantBit(aPropID)) {
-      mTempData.ClearProperty(aPropID);
-      return;
+      // When parsing a declaration block, an !important declaration
+      // is not overwritten by an ordinary declaration of the same
+      // property later in the block.  However, CSSOM manipulations
+      // come through here too, and in that case we do want to
+      // overwrite the property.
+      if (!aOverrideImportant) {
+        mTempData.ClearProperty(aPropID);
+        return;
+      }
+      *aChanged = PR_TRUE;
+      mData.ClearImportantBit(aPropID);
     }
   }
 
   if (aMustCallValueAppended || !mData.HasPropertyBit(aPropID)) {
     aDeclaration->ValueAppended(aPropID);
   }
 
   mData.SetPropertyBit(aPropID);