Bug 784466 - [css3-animations] Drop declarations in keyframe rules that have !important. r=dbaron
authorEmmanuele Bassi <ebassi@mozilla.com>
Tue, 16 Oct 2012 09:21:35 -0700
changeset 110549 71eacb57041d465e7a32ce2bb2f554ef17b11d63
parent 110548 532c0ef6588b0a15b17fd17d2a90773d7003765a
child 110550 741fb7f8e5cb52247b69fb17b7214ede8c83d8eb
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersdbaron
bugs784466
milestone19.0a1
Bug 784466 - [css3-animations] Drop declarations in keyframe rules that have !important. r=dbaron
layout/style/nsCSSParser.cpp
layout/style/nsCSSRules.cpp
layout/style/test/test_animations.html
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -391,21 +391,26 @@ protected:
 
   // If aStopChar is non-zero, the selector list is done when we hit
   // aStopChar.  Otherwise, it's done when we hit EOF.
   bool ParseSelectorList(nsCSSSelectorList*& aListHead,
                            PRUnichar aStopChar);
   bool ParseSelectorGroup(nsCSSSelectorList*& aListHead);
   bool ParseSelector(nsCSSSelectorList* aList, PRUnichar aPrevCombinator);
 
-  css::Declaration* ParseDeclarationBlock(bool aCheckForBraces);
+  enum {
+    eParseDeclaration_InBraces       = 1 << 0,
+    eParseDeclaration_AllowImportant = 1 << 1
+  };
+
+  css::Declaration* ParseDeclarationBlock(uint32_t aFlags);
   bool ParseDeclaration(css::Declaration* aDeclaration,
-                          bool aCheckForBraces,
-                          bool aMustCallValueAppended,
-                          bool* aChanged);
+                        uint32_t aFlags,
+                        bool aMustCallValueAppended,
+                        bool* aChanged);
 
   bool ParseProperty(nsCSSProperty aPropID);
   bool ParsePropertyByFunction(nsCSSProperty aPropID);
   bool ParseSingleValueProperty(nsCSSValue& aValue,
                                   nsCSSProperty aPropID);
 
   enum PriorityParsingStatus {
     ePriority_None,
@@ -982,17 +987,22 @@ CSSParserImpl::ParseStyleAttribute(const
     haveBraces = eCSSToken_Symbol == mToken.mType &&
                  '{' == mToken.mSymbol;
     UngetToken();
   }
   else {
     haveBraces = false;
   }
 
-  css::Declaration* declaration = ParseDeclarationBlock(haveBraces);
+  uint32_t parseFlags = eParseDeclaration_AllowImportant;
+  if (haveBraces) {
+    parseFlags |= eParseDeclaration_InBraces;
+  }
+
+  css::Declaration* declaration = ParseDeclarationBlock(parseFlags);
   if (declaration) {
     // Create a style rule for the declaration
     NS_ADDREF(*aResult = new css::StyleRule(nullptr, declaration));
   } else {
     *aResult = nullptr;
   }
 
   ReleaseScanner();
@@ -1021,17 +1031,18 @@ CSSParserImpl::ParseDeclarations(const n
   mData.AssertInitialState();
   aDeclaration->ClearData();
   // We could check if it was already empty, but...
   *aChanged = true;
 
   for (;;) {
     // If we cleared the old decl, then we want to be calling
     // ValueAppended as we parse.
-    if (!ParseDeclaration(aDeclaration, false, true, aChanged)) {
+    if (!ParseDeclaration(aDeclaration, eParseDeclaration_AllowImportant,
+                          true, aChanged)) {
       if (!SkipDeclaration(false)) {
         break;
       }
     }
   }
 
   aDeclaration->CompressFrom(&mData);
   ReleaseScanner();
@@ -2296,17 +2307,19 @@ already_AddRefed<nsCSSKeyframeRule>
 CSSParserImpl::ParseKeyframeRule()
 {
   InfallibleTArray<float> selectorList;
   if (!ParseKeyframeSelectorList(selectorList)) {
     REPORT_UNEXPECTED(PEBadSelectorKeyframeRuleIgnored);
     return nullptr;
   }
 
-  nsAutoPtr<css::Declaration> declaration(ParseDeclarationBlock(true));
+  // Ignore !important in keyframe rules
+  uint32_t parseFlags = eParseDeclaration_InBraces;
+  nsAutoPtr<css::Declaration> declaration(ParseDeclarationBlock(parseFlags));
   if (!declaration) {
     REPORT_UNEXPECTED(PEBadSelectorKeyframeRuleIgnored);
     return nullptr;
   }
 
   // Takes ownership of declaration, and steals contents of selectorList.
   nsRefPtr<nsCSSKeyframeRule> rule =
     new nsCSSKeyframeRule(selectorList, declaration);
@@ -2752,17 +2765,19 @@ CSSParserImpl::ParseRuleSet(RuleAppendFu
     OUTPUT_ERROR();
     SkipRuleSet(aInsideBraces);
     return false;
   }
   NS_ASSERTION(nullptr != slist, "null selector list");
   CLEAR_ERROR();
 
   // Next parse the declaration block
-  css::Declaration* declaration = ParseDeclarationBlock(true);
+  uint32_t parseFlags = eParseDeclaration_InBraces |
+                        eParseDeclaration_AllowImportant;
+  css::Declaration* declaration = ParseDeclarationBlock(parseFlags);
   if (nullptr == declaration) {
     delete slist;
     return false;
   }
 
 #if 0
   slist->Dump();
   fputs("{\n", stdout);
@@ -3872,36 +3887,37 @@ CSSParserImpl::ParseSelector(nsCSSSelect
     selector->mClassList = pseudoElementArgs.forget();
     selector->SetPseudoType(pseudoElementType);
   }
 
   return true;
 }
 
 css::Declaration*
-CSSParserImpl::ParseDeclarationBlock(bool aCheckForBraces)
-{
-  if (aCheckForBraces) {
+CSSParserImpl::ParseDeclarationBlock(uint32_t aFlags)
+{
+  bool checkForBraces = (aFlags & eParseDeclaration_InBraces) != 0;
+
+  if (checkForBraces) {
     if (!ExpectSymbol('{', true)) {
       REPORT_UNEXPECTED_TOKEN(PEBadDeclBlockStart);
       OUTPUT_ERROR();
       return nullptr;
     }
   }
   css::Declaration* declaration = new css::Declaration();
   mData.AssertInitialState();
   if (declaration) {
     for (;;) {
       bool changed;
-      if (!ParseDeclaration(declaration, aCheckForBraces,
-                            true, &changed)) {
-        if (!SkipDeclaration(aCheckForBraces)) {
+      if (!ParseDeclaration(declaration, aFlags, true, &changed)) {
+        if (!SkipDeclaration(checkForBraces)) {
           break;
         }
-        if (aCheckForBraces) {
+        if (checkForBraces) {
           if (ExpectSymbol('}', true)) {
             break;
           }
         }
         // Since the skipped declaration didn't end the block we parse
         // the next declaration.
       }
     }
@@ -4276,28 +4292,30 @@ CSSParserImpl::ParseTreePseudoElement(ns
   return true;
 }
 #endif
 
 //----------------------------------------------------------------------
 
 bool
 CSSParserImpl::ParseDeclaration(css::Declaration* aDeclaration,
-                                bool aCheckForBraces,
+                                uint32_t aFlags,
                                 bool aMustCallValueAppended,
                                 bool* aChanged)
 {
+  bool checkForBraces = (aFlags & eParseDeclaration_InBraces) != 0;
+
   mTempData.AssertInitialState();
 
   // Get property name
   nsCSSToken* tk = &mToken;
   nsAutoString propertyName;
   for (;;) {
     if (!GetToken(true)) {
-      if (aCheckForBraces) {
+      if (checkForBraces) {
         REPORT_UNEXPECTED_EOF(PEDeclEndEOF);
       }
       return false;
     }
     if (eCSSToken_Ident == tk->mType) {
       propertyName = tk->mIdent;
       // grab the ident before the ExpectSymbol trashes the token
       if (!ExpectSymbol(':', true)) {
@@ -4348,38 +4366,44 @@ CSSParserImpl::ParseDeclaration(css::Dec
     OUTPUT_ERROR();
     mTempData.ClearProperty(propID);
     mTempData.AssertInitialState();
     return false;
   }
   CLEAR_ERROR();
 
   // Look for "!important".
-  PriorityParsingStatus status = ParsePriority();
+  PriorityParsingStatus status;
+  if ((aFlags & eParseDeclaration_AllowImportant) != 0) {
+    status = ParsePriority();
+  }
+  else {
+    status = ePriority_None;
+  }
 
   // Look for a semicolon or close brace.
   if (status != ePriority_Error) {
     if (!GetToken(true)) {
       // EOF is always ok
     } else if (mToken.IsSymbol(';')) {
       // semicolon is always ok
     } else if (mToken.IsSymbol('}')) {
-      // brace is ok if aCheckForBraces, but don't eat it
+      // brace is ok if checkForBraces, but don't eat it
       UngetToken();
-      if (!aCheckForBraces) {
+      if (!checkForBraces) {
         status = ePriority_Error;
       }
     } else {
       UngetToken();
       status = ePriority_Error;
     }
   }
 
   if (status == ePriority_Error) {
-    if (aCheckForBraces) {
+    if (checkForBraces) {
       REPORT_UNEXPECTED_TOKEN(PEBadDeclOrRuleEnd2);
     } else {
       REPORT_UNEXPECTED_TOKEN(PEBadDeclEnd);
     }
     REPORT_UNEXPECTED(PEDeclDropped);
     OUTPUT_ERROR();
     mTempData.ClearProperty(propID);
     mTempData.AssertInitialState();
--- a/layout/style/nsCSSRules.cpp
+++ b/layout/style/nsCSSRules.cpp
@@ -1977,21 +1977,20 @@ IMPL_STYLE_RULE_INHERIT_GET_DOM_RULE_WEA
 
 /* virtual */ void
 nsCSSKeyframeRule::MapRuleInfoInto(nsRuleData* aRuleData)
 {
   // We need to implement MapRuleInfoInto because the animation manager
   // constructs a rule node pointing to us in order to compute the
   // styles it needs to animate.
 
-  // FIXME (spec): The spec doesn't say what to do with !important.
-  // We'll just map them.
-  if (mDeclaration->HasImportantData()) {
-    mDeclaration->MapImportantRuleInfoInto(aRuleData);
-  }
+  // The spec says that !important declarations should just be ignored
+  NS_ASSERTION(!mDeclaration->HasImportantData(),
+               "Keyframe rules has !important data");
+
   mDeclaration->MapNormalRuleInfoInto(aRuleData);
 }
 
 #ifdef DEBUG
 void
 nsCSSKeyframeRule::List(FILE* out, int32_t aIndent) const
 {
   // FIXME: WRITE ME
--- a/layout/style/test/test_animations.html
+++ b/layout/style/test/test_animations.html
@@ -107,16 +107,29 @@ https://bugzilla.mozilla.org/show_bug.cg
   }
 
   @keyframes primitives1 {
     from { -moz-transform: rotate(0deg) translateX(0px) scaleX(1)
       translate(0px) scale3d(1, 1, 1); }
     to { -moz-transform: rotate(270deg) translate3d(0px, 0px, 0px) scale(1)
       translateY(0px) scaleY(1); }
   }
+
+  @keyframes important1 {
+    from { margin-top: 50px; }
+    50%  { margin-top: 150px !important; } /* ignored */
+    to   { margin-top: 100px; }
+  }
+
+  @keyframes important2 {
+    from { margin-top: 50px;
+           margin-bottom: 100px; }
+    to   { margin-top: 150px !important; /* ignored */
+           margin-bottom: 50px; }
+  }
   </style>
 </head>
 <body>
 <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=435442">Mozilla Bug 435442</a>
 <div id="display"></div>
 <pre id="test">
 <script type="application/javascript">
 "use strict";
@@ -1417,14 +1430,30 @@ advance_clock(1000);
 is(cs.getPropertyValue("-moz-transform"),
    "matrix(-0.707107, 0.707107, -0.707107, -0.707107, 0, 0)",
    "primitives1 at 1s");
 advance_clock(1000);
 is(cs.getPropertyValue("-moz-transform"), "matrix(0, -1, 1, 0, 0, 0)",
     "primitives1 at 0s");
 done_div();
 
+new_div("animation: important1 1s linear forwards");
+is(cs.marginTop, "50px", "important1 test at 0s");
+advance_clock(500);
+is(cs.marginTop, "75px", "important1 test at 0.5s");
+advance_clock(500);
+is(cs.marginTop, "100px", "important1 test at 1s");
+done_div();
+
+new_div("animation: important2 1s linear forwards");
+is(cs.marginTop, "50px", "important2 (margin-top) test at 0s");
+is(cs.marginBottom, "100px", "important2 (margin-bottom) test at 0s");
+advance_clock(1000);
+is(cs.marginTop, "0px", "important2 (margin-top) test at 1s");
+is(cs.marginBottom, "50px", "important2 (margin-bottom) test at 1s");
+done_div();
+
 SpecialPowers.DOMWindowUtils.restoreNormalRefresh();
 
 </script>
 </pre>
 </body>
 </html>