Bug 1340341 - Set immutablity in GetCSSDeclaration instead of ToString. r?dbaron draft
authorXidorn Quan <me@upsuper.org>
Fri, 17 Feb 2017 18:15:01 +1100
changeset 485798 66b70cf39139d9983aebed86e71c5ad8c28b3b76
parent 483483 f2c682c8b1734ba9d17f55d1ee3c8fcddd071f28
child 546127 382b2bcb938446cc257cba49956593c8ed91ef02
push id45854
push userxquan@mozilla.com
push dateFri, 17 Feb 2017 07:53:23 +0000
reviewersdbaron
bugs1340341
milestone54.0a1
Bug 1340341 - Set immutablity in GetCSSDeclaration instead of ToString. r?dbaron MozReview-Commit-ID: 2aDsVJMJG8T
layout/style/Declaration.cpp
layout/style/DeclarationBlock.h
layout/style/nsDOMCSSAttrDeclaration.cpp
layout/style/test/mochitest.ini
layout/style/test/test_style_attr_listener.html
--- a/layout/style/Declaration.cpp
+++ b/layout/style/Declaration.cpp
@@ -1696,20 +1696,16 @@ Declaration::AppendVariableAndValueToStr
     aResult.AppendLiteral("!important");
   }
   aResult.AppendLiteral("; ");
 }
 
 void
 Declaration::ToString(nsAString& aString) const
 {
-  // Someone cares about this declaration's contents, so don't let it
-  // change from under them.  See e.g. bug 338679.
-  SetImmutable();
-
   nsCSSCompressedDataBlock *systemFontData =
     GetPropertyIsImportantByID(eCSSProperty__x_system_font) ? mImportantData
                                                             : mData;
   const nsCSSValue *systemFont =
     systemFontData->ValueFor(eCSSProperty__x_system_font);
   const bool haveSystemFont = systemFont &&
                                 systemFont->GetUnit() != eCSSUnit_None &&
                                 systemFont->GetUnit() != eCSSUnit_Null;
--- a/layout/style/DeclarationBlock.h
+++ b/layout/style/DeclarationBlock.h
@@ -58,17 +58,17 @@ public:
   void AssertMutable() const {
     MOZ_ASSERT(IsMutable(), "someone forgot to call EnsureMutable");
   }
 
   /**
    * Mark this declaration as unmodifiable.  It's 'const' so it can
    * be called from ToString.
    */
-  void SetImmutable() const { mImmutable = true; }
+  void SetImmutable() { mImmutable = true; }
 
   /**
    * Copy |this|, if necessary to ensure that it can be modified.
    */
   inline already_AddRefed<DeclarationBlock> EnsureMutable();
 
   void SetOwningRule(css::Rule* aRule) {
     MOZ_ASSERT(!mContainer.mOwningRule || !aRule,
@@ -130,17 +130,16 @@ private:
     css::Rule* mOwningRule;
 
     // The nsHTMLCSSStyleSheet that is responsible for this declaration.
     // Only non-null for style attributes.
     nsHTMLCSSStyleSheet* mHTMLCSSStyleSheet;
   } mContainer;
 
   // set when declaration put in the rule tree;
-  // also by ToString (hence the 'mutable').
-  mutable bool mImmutable;
+  bool mImmutable;
 
   const StyleBackendType mType;
 };
 
 } // namespace mozilla
 
 #endif // mozilla_DeclarationBlock_h
--- a/layout/style/nsDOMCSSAttrDeclaration.cpp
+++ b/layout/style/nsDOMCSSAttrDeclaration.cpp
@@ -7,17 +7,19 @@
 
 #include "nsDOMCSSAttrDeclaration.h"
 
 #include "mozilla/css/Declaration.h"
 #include "mozilla/css/StyleRule.h"
 #include "mozilla/DeclarationBlock.h"
 #include "mozilla/DeclarationBlockInlines.h"
 #include "mozilla/dom/Element.h"
+#include "mozilla/InternalMutationEvent.h"
 #include "mozilla/ServoDeclarationBlock.h"
+#include "nsContentUtils.h"
 #include "nsIDocument.h"
 #include "nsIDOMMutationEvent.h"
 #include "nsIURI.h"
 #include "nsNodeUtils.h"
 #include "nsWrapperCacheInlines.h"
 #include "nsIFrame.h"
 #include "ActiveLayerTracker.h"
 
@@ -114,16 +116,25 @@ nsDOMCSSAttributeDeclaration::GetCSSDecl
        (aOperation == eOperation_RemoveProperty && declaration))) {
     nsNodeUtils::AttributeWillChange(mElement, kNameSpaceID_None,
                                      nsGkAtoms::style,
                                      nsIDOMMutationEvent::MODIFICATION,
                                      nullptr);
   }
 
   if (declaration) {
+    if (aOperation != eOperation_Read &&
+        nsContentUtils::HasMutationListeners(
+          mElement, NS_EVENT_BITS_MUTATION_ATTRMODIFIED, mElement)) {
+      // If there is any mutation listener on the element, we need to
+      // ensure that any change would create a new declaration so that
+      // nsStyledElement::SetInlineStyleDeclaration can generate the
+      // correct old value.
+      declaration->SetImmutable();
+    }
     return declaration;
   }
 
   if (aOperation != eOperation_Modify) {
     return nullptr;
   }
 
   // cannot fail
--- a/layout/style/test/mochitest.ini
+++ b/layout/style/test/mochitest.ini
@@ -251,16 +251,17 @@ support-files = redundant_font_download.
 [test_rule_serialization.html]
 [test_rules_out_of_sheets.html]
 [test_selectors.html]
 skip-if = toolkit == 'android' #bug 775227
 [test_selectors_on_anonymous_content.html]
 [test_setPropertyWithNull.html]
 [test_shorthand_property_getters.html]
 [test_specified_value_serialization.html]
+[test_style_attr_listener.html]
 [test_style_attribute_quirks.html]
 [test_style_attribute_standards.html]
 [test_style_struct_copy_constructors.html]
 [test_supports_rules.html]
 [test_system_font_serialization.html]
 [test_text_decoration_shorthands.html]
 [test_transitions_and_reframes.html]
 [test_transitions_and_restyles.html]
new file mode 100644
--- /dev/null
+++ b/layout/style/test/test_style_attr_listener.html
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <title>Test for Bug 338679 (from bug 1340341)</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<div style=""></div>
+<script>
+let div = document.querySelector('div');
+let expectation;
+let count = 0;
+div.style.color = "red";
+div.addEventListener('DOMAttrModified', function(evt) {
+  count++;
+  is(evt.prevValue, expectation.prevValue, "Previous value for event ${count}");
+  is(evt.newValue, expectation.newValue, "New value for event ${count}");
+});
+expectation = { prevValue: 'color: red;', newValue: 'color: green;' };
+div.style.color = "green";
+expectation = { prevValue: 'color: green;', newValue: '' };
+div.style.color = '';
+</script>
+</body>
+</html>