Bug 1312918 - Do not overrider previous keyframes rule if the previous rule is non-prefixed rule and the new one is prefixed. r?dholbert draft
authorHiroyuki Ikezoe <hiikezoe@mozilla-japan.org>
Sat, 15 Apr 2017 20:30:21 +0900
changeset 563212 5589dd6517f46da765c8e52b6ec927c3e6d02c3b
parent 563120 9379831bb9c3d9abfea7dbf8dd06dbdab1d81dc4
child 563213 def23d92469c74445827b3748cc78c2e6b8a7cef
push id54233
push userhikezoe@mozilla.com
push dateSat, 15 Apr 2017 11:30:59 +0000
reviewersdholbert
bugs1312918
milestone55.0a1
Bug 1312918 - Do not overrider previous keyframes rule if the previous rule is non-prefixed rule and the new one is prefixed. r?dholbert MozReview-Commit-ID: 1XuHyznnw20
layout/style/nsCSSParser.cpp
layout/style/nsCSSRuleProcessor.cpp
layout/style/nsCSSRules.h
layout/style/test/mochitest.ini
layout/style/test/test_vendor_prefix_keyframes.html
--- a/layout/style/nsCSSParser.cpp
+++ b/layout/style/nsCSSParser.cpp
@@ -62,16 +62,17 @@
 #include "mozilla/dom/AnimationEffectReadOnlyBinding.h"
 #include "mozilla/dom/URL.h"
 #include "gfxFontFamilyList.h"
 
 using namespace mozilla;
 using namespace mozilla::css;
 
 typedef nsCSSProps::KTableEntry KTableEntry;
+typedef nsCSSKeyframesRule::VendorPrefixType VendorPrefixType;
 
 // pref-backed bool values (hooked up in nsCSSParser::Startup)
 static bool sOpentypeSVGEnabled;
 static bool sWebkitPrefixedAliasesEnabled;
 static bool sWebkitDevicePixelRatioEnabled;
 static bool sMozGradientsEnabled;
 static bool sControlCharVisibility;
 
@@ -689,16 +690,17 @@ protected:
   bool ParseFontFeatureValuesRule(RuleAppendFunc aAppendFunc,
                                   void* aProcessData);
   bool ParseFontFeatureValueSet(nsCSSFontFeatureValuesRule *aRule);
   bool ParseFontDescriptor(nsCSSFontFaceRule* aRule);
   bool ParseFontDescriptorValue(nsCSSFontDesc aDescID,
                                 nsCSSValue& aValue);
 
   bool ParsePageRule(RuleAppendFunc aAppendFunc, void* aProcessData);
+  template <VendorPrefixType VendorPrefix>
   bool ParseKeyframesRule(RuleAppendFunc aAppendFunc, void* aProcessData);
   already_AddRefed<nsCSSKeyframeRule> ParseKeyframeRule();
   bool ParseKeyframeSelectorList(InfallibleTArray<float>& aSelectorList);
 
   bool ParseSupportsRule(RuleAppendFunc aAppendFunc, void* aProcessData);
   bool ParseSupportsCondition(bool& aConditionMet);
   bool ParseSupportsConditionNegation(bool& aConditionMet);
   bool ParseSupportsConditionInParens(bool& aConditionMet);
@@ -3317,23 +3319,32 @@ CSSParserImpl::ParseAtRule(RuleAppendFun
   } else if (mToken.mIdent.LowerCaseEqualsLiteral("font-feature-values")) {
     parseFunc = &CSSParserImpl::ParseFontFeatureValuesRule;
     newSection = eCSSSection_General;
 
   } else if (mToken.mIdent.LowerCaseEqualsLiteral("page")) {
     parseFunc = &CSSParserImpl::ParsePageRule;
     newSection = eCSSSection_General;
 
-  } else if ((nsCSSProps::IsEnabled(eCSSPropertyAlias_MozAnimation,
+  } else if (nsCSSProps::IsEnabled(eCSSPropertyAlias_MozAnimation,
                                     EnabledState()) &&
-              mToken.mIdent.LowerCaseEqualsLiteral("-moz-keyframes")) ||
-             (nsCSSProps::IsEnabled(eCSSPropertyAlias_WebkitAnimation) &&
-              mToken.mIdent.LowerCaseEqualsLiteral("-webkit-keyframes")) ||
-             mToken.mIdent.LowerCaseEqualsLiteral("keyframes")) {
-    parseFunc = &CSSParserImpl::ParseKeyframesRule;
+             mToken.mIdent.LowerCaseEqualsLiteral("-moz-keyframes")) {
+    parseFunc =
+      &CSSParserImpl::ParseKeyframesRule<VendorPrefixType::Mozilla>;
+    newSection = eCSSSection_General;
+
+  } else if (nsCSSProps::IsEnabled(eCSSPropertyAlias_WebkitAnimation) &&
+             mToken.mIdent.LowerCaseEqualsLiteral("-webkit-keyframes")) {
+    parseFunc =
+      &CSSParserImpl::ParseKeyframesRule<VendorPrefixType::WebKit>;
+    newSection = eCSSSection_General;
+
+  } else if (mToken.mIdent.LowerCaseEqualsLiteral("keyframes")) {
+    parseFunc =
+      &CSSParserImpl::ParseKeyframesRule<VendorPrefixType::None>;
     newSection = eCSSSection_General;
 
   } else if (mToken.mIdent.LowerCaseEqualsLiteral("supports")) {
     parseFunc = &CSSParserImpl::ParseSupportsRule;
     newSection = eCSSSection_General;
 
   } else if (mToken.mIdent.LowerCaseEqualsLiteral("counter-style")) {
     parseFunc = &CSSParserImpl::ParseCounterStyleRule;
@@ -4343,16 +4354,17 @@ CSSParserImpl::ParseFontFeatureValueSet(
       break;
     }
  }
 
   aFeatureValuesRule->AddValueList(whichVariant, values);
   return true;
 }
 
+template <VendorPrefixType VendorPrefix>
 bool
 CSSParserImpl::ParseKeyframesRule(RuleAppendFunc aAppendFunc, void* aData)
 {
   uint32_t linenum, colnum;
   if (!GetNextTokenLocation(true, &linenum, &colnum) ||
       !GetToken(true)) {
     REPORT_UNEXPECTED_EOF(PEKeyframeNameEOF);
     return false;
@@ -4382,17 +4394,19 @@ CSSParserImpl::ParseKeyframesRule(RuleAp
   nsString name(mToken.mIdent);
 
   if (!ExpectSymbol('{', true)) {
     REPORT_UNEXPECTED_TOKEN(PEKeyframeBrace);
     return false;
   }
 
   RefPtr<nsCSSKeyframesRule> rule = new nsCSSKeyframesRule(name,
-                                                             linenum, colnum);
+                                                           VendorPrefix,
+                                                           linenum,
+                                                           colnum);
 
   while (!ExpectSymbol('}', true)) {
     RefPtr<nsCSSKeyframeRule> kid = ParseKeyframeRule();
     if (kid) {
       rule->AppendStyleRule(kid);
     } else {
       OUTPUT_ERROR();
       SkipRuleSet(true);
--- a/layout/style/nsCSSRuleProcessor.cpp
+++ b/layout/style/nsCSSRuleProcessor.cpp
@@ -3830,17 +3830,24 @@ nsCSSRuleProcessor::RefreshRuleCascade(n
             return; /* out of memory */
         }
       }
 
       // Build mKeyframesRuleTable.
       for (nsTArray<nsCSSKeyframesRule*>::size_type i = 0,
              iEnd = newCascade->mKeyframesRules.Length(); i < iEnd; ++i) {
         nsCSSKeyframesRule* rule = newCascade->mKeyframesRules[i];
-        newCascade->mKeyframesRuleTable.Put(rule->GetName(), rule);
+        nsCSSKeyframesRule* ruleWithTheSameName =
+          newCascade->mKeyframesRuleTable.Get(rule->GetName());
+        if (!ruleWithTheSameName ||
+            // Do not override the previous rule if the previous rule is
+            // non-vendor and the new rule is prefixed rules.
+            rule->VendorPrefix() <= ruleWithTheSameName->VendorPrefix()) {
+          newCascade->mKeyframesRuleTable.Put(rule->GetName(), rule);
+        }
       }
 
       // Build mCounterStyleRuleTable
       for (nsTArray<nsCSSCounterStyleRule*>::size_type i = 0,
            iEnd = newCascade->mCounterStyleRules.Length(); i < iEnd; ++i) {
         nsCSSCounterStyleRule* rule = newCascade->mCounterStyleRules[i];
         newCascade->mCounterStyleRuleTable.Put(rule->GetName(), rule);
       }
--- a/layout/style/nsCSSRules.h
+++ b/layout/style/nsCSSRules.h
@@ -321,20 +321,29 @@ private:
   // lazily created when needed:
   RefPtr<nsCSSKeyframeStyleDeclaration>    mDOMDeclaration;
 };
 
 class nsCSSKeyframesRule final : public mozilla::css::GroupRule,
                                  public nsIDOMCSSKeyframesRule
 {
 public:
+  // This enum is ordered by priority. None < Mozilla < WebKit.
+  enum class VendorPrefixType : uint8_t {
+    None = 0,
+    Mozilla = 1,
+    WebKit = 1,
+  };
+
   nsCSSKeyframesRule(const nsSubstring& aName,
+                     VendorPrefixType aPrefix,
                      uint32_t aLineNumber, uint32_t aColumnNumber)
     : mozilla::css::GroupRule(aLineNumber, aColumnNumber)
     , mName(aName)
+    , mPrefix(aPrefix)
   {
   }
 private:
   nsCSSKeyframesRule(const nsCSSKeyframesRule& aCopy);
   ~nsCSSKeyframesRule();
 public:
   NS_DECL_ISUPPORTS_INHERITED
 
@@ -359,26 +368,28 @@ public:
   // The XPCOM deleteRule is OK, since it never throws
   nsCSSKeyframeRule* FindRule(const nsAString& aKey);
 
   // rest of GroupRule
   virtual bool UseForPresentation(nsPresContext* aPresContext,
                                     nsMediaQueryResultCacheKey& aKey) override;
 
   const nsString& GetName() { return mName; }
+  VendorPrefixType VendorPrefix() { return mPrefix; }
 
   virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
 
   virtual JSObject* WrapObject(JSContext* aCx,
                                JS::Handle<JSObject*> aGivenProto) override;
 
 private:
   uint32_t FindRuleIndexForKey(const nsAString& aKey);
 
   nsString                                   mName;
+  VendorPrefixType mPrefix;
 };
 
 class nsCSSPageRule;
 
 class nsCSSPageStyleDeclaration final : public nsDOMCSSDeclaration
 {
 public:
   explicit nsCSSPageStyleDeclaration(nsCSSPageRule *aRule);
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -305,16 +305,17 @@ support-files = ../../reftests/fonts/mar
 skip-if = toolkit == 'android' # bug 775227 for android
 [test_value_computation.html]
 skip-if = toolkit == 'android'
 [test_value_storage.html]
 [test_variable_serialization_computed.html]
 [test_variable_serialization_specified.html]
 [test_variables.html]
 support-files = support/external-variable-url.css
+[test_vendor_prefix_keyframes.html]
 [test_video_object_fit.html]
 [test_viewport_units.html]
 [test_visited_image_loading.html]
 skip-if = (toolkit == 'android' || stylo) # TIMED_OUT for android, timeout bug 1328511 for stylo
 [test_visited_image_loading_empty.html]
 skip-if = (toolkit == 'android' || stylo) # TIMED_OUT for android, timeout bug 1328511 for stylo
 [test_visited_lying.html]
 skip-if = (toolkit == 'android' || stylo) # TIMED_OUT for android, timeout bug 1328511 for stylo
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_vendor_prefix_keyframes.html
@@ -0,0 +1,132 @@
+<!DOCTYPE html>
+<meta charset=utf-8>
+<title>Test for ...</title>
+<script src='/resources/testharness.js'></script>
+<script src='/resources/testharnessreport.js'></script>
+<div id='log'></div>
+<script>
+/**
+ * Appends a style div to the document head.
+ *
+ * @param t  The testharness.js Test object. If provided, this will be used
+ *           to register a cleanup callback to remove the style element
+ *           when the test finishes.
+ *
+ * @param rules  A dictionary object with selector names and rules to set on
+ *               the style sheet.
+ */
+function addStyle(t, rules) {
+  var extraStyle = document.createElement('style');
+  document.head.appendChild(extraStyle);
+  if (rules) {
+    var sheet = extraStyle.sheet;
+    for (var selector in rules) {
+      sheet.insertRule(selector + '{' + rules[selector] + '}',
+                       sheet.cssRules.length);
+    }
+  }
+
+  if (t && typeof t.add_cleanup === 'function') {
+    t.add_cleanup(function() {
+      extraStyle.remove();
+    });
+  }
+}
+
+/**
+ * Appends a div to the document body.
+ *
+ * @param t  The testharness.js Test object. If provided, this will be used
+ *           to register a cleanup callback to remove the div when the test
+ *           finishes.
+ *
+ * @param attrs  A dictionary object with attribute names and values to set on
+ *               the div.
+ */
+function addDiv(t, attrs) {
+  var div = document.createElement('div');
+  if (attrs) {
+    for (var attrName in attrs) {
+      div.setAttribute(attrName, attrs[attrName]);
+    }
+  }
+  document.body.appendChild(div);
+  if (t && typeof t.add_cleanup === 'function') {
+    t.add_cleanup(function() {
+      if (div.parentNode) {
+        div.parentNode.removeChild(div);
+      }
+    });
+  }
+  return div;
+}
+
+test(function(t) {
+  addStyle(t,
+    { '@keyframes anim':         'from,to { color: rgb(0, 255, 0); }',
+      '@-webkit-keyframes anim': 'from,to { color: rgb(255, 0, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, '-webkit-keyframes does not override non-prefix keyframes');
+
+test(function(t) {
+  addStyle(t,
+    { '@keyframes anim':      'from,to { color: rgb(0, 255, 0); }',
+      '@-moz-keyframes anim': 'from,to { color: rgb(255, 0, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, '-moz-keyframes does not override non-prefix keyframes');
+
+test(function(t) {
+  addStyle(t,
+    { '@-moz-keyframes anim': 'from,to { color: rgb(255, 0, 0); }',
+      '@keyframes anim':      'from,to { color: rgb(0, 255, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, 'non-prefix keyframes overrides -moz-keyframes');
+
+test(function(t) {
+  addStyle(t,
+    { '@-webkit-keyframes anim': 'from,to { color: rgb(255, 0, 0); }',
+      '@keyframes anim':         'from,to { color: rgb(0, 255, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, 'non-prefix keyframes overrides -webkit-keyframes');
+
+test(function(t) {
+  addStyle(t,
+    { '@-webkit-keyframes anim': 'from,to { color: rgb(255, 0, 0); }',
+      '@-moz-keyframes anim':    'from,to { color: rgb(0, 255, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+
+  addStyle(t,
+    { '@-moz-keyframes anim2':    'from,to { color: rgb(255, 0, 0); }',
+      '@-webkit-keyframes anim2': 'from,to { color: rgb(0, 255, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim2 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, 'last prefixed keyframes overrides previous prefixed keyframes');
+
+test(function(t) {
+  addStyle(t,
+    { '@keyframes anim': 'from,to { color: rgb(255, 0, 0); }',
+      '@keyframes anim': 'from,to { color: rgb(0, 255, 0); }' });
+
+  var div = addDiv(t, { style: 'animation: anim 100s' });
+
+  assert_equals(getComputedStyle(div).color, 'rgb(0, 255, 0)');
+}, 'later keyframes override the previous keyframes');
+
+</script>