From 55844d96671b59d3236468fafc6a190369e189b2 Mon Sep 17 00:00:00 2001 From: Cameron McCormack Date: Fri, 10 Apr 2015 10:41:35 +1000 Subject: [PATCH] Bug 1149042 - Call AttributeWillChange before a style="" attribute gets created when touching element.style. r=smaug --- dom/base/test/test_mutationobservers.html | 87 ++++++++++++++++++++++- layout/style/StyleRule.cpp | 4 +- layout/style/nsCSSRules.cpp | 4 +- layout/style/nsCSSRules.h | 4 +- layout/style/nsComputedDOMStyle.cpp | 2 +- layout/style/nsComputedDOMStyle.h | 2 +- layout/style/nsDOMCSSAttrDeclaration.cpp | 35 +++++---- layout/style/nsDOMCSSAttrDeclaration.h | 2 +- layout/style/nsDOMCSSDeclaration.cpp | 24 +++---- layout/style/nsDOMCSSDeclaration.h | 24 +++++-- 10 files changed, 150 insertions(+), 38 deletions(-) diff --git a/dom/base/test/test_mutationobservers.html b/dom/base/test/test_mutationobservers.html index 657b4099edc..3f064790094 100644 --- a/dom/base/test/test_mutationobservers.html +++ b/dom/base/test/test_mutationobservers.html @@ -580,7 +580,7 @@ function testExpandos() { var m2 = new M(function(records, observer) { is(observer.expandoProperty, true); observer.disconnect(); - then(); + then(testStyleCreate); }); m2.expandoProperty = true; m2.observe(div, { attributes: true }); @@ -596,6 +596,91 @@ function testExpandos() { div.setAttribute("foo", "bar2"); } +function testStyleCreate() { + m = new M(function(records, observer) { + is(records.length, 1, "number of records"); + is(records[0].type, "attributes", "record.type"); + is(records[0].attributeName, "style", "record.attributeName"); + is(records[0].oldValue, null, "record.oldValue"); + isnot(div.getAttribute("style"), null, "style attribute after creation"); + observer.disconnect(); + m = null; + div.removeAttribute("style"); + then(testStyleModify); + }); + m.observe(div, { attributes: true, attributeOldValue: true }); + is(div.getAttribute("style"), null, "style attribute before creation"); + div.style.color = "blue"; +} + +function testStyleModify() { + div.style.color = "yellow"; + m = new M(function(records, observer) { + is(records.length, 1, "number of records"); + is(records[0].type, "attributes", "record.type"); + is(records[0].attributeName, "style", "record.attributeName"); + isnot(div.getAttribute("style"), null, "style attribute after modification"); + observer.disconnect(); + m = null; + div.removeAttribute("style"); + then(testStyleRead); + }); + m.observe(div, { attributes: true }); + isnot(div.getAttribute("style"), null, "style attribute before modification"); + div.style.color = "blue"; +} + +function testStyleRead() { + m = new M(function(records, observer) { + is(records.length, 1, "number of records"); + is(records[0].type, "attributes", "record.type"); + is(records[0].attributeName, "data-test", "record.attributeName"); + is(div.getAttribute("style"), null, "style attribute after read"); + observer.disconnect(); + div.removeAttribute("data-test"); + m = null; + then(testStyleRemoveProperty); + }); + m.observe(div, { attributes: true }); + is(div.getAttribute("style"), null, "style attribute before read"); + var value = div.style.color; // shouldn't generate any mutation records + div.setAttribute("data-test", "a"); +} + +function testStyleRemoveProperty() { + div.style.color = "blue"; + m = new M(function(records, observer) { + is(records.length, 1, "number of records"); + is(records[0].type, "attributes", "record.type"); + is(records[0].attributeName, "style", "record.attributeName"); + isnot(div.getAttribute("style"), null, "style attribute after successful removeProperty"); + observer.disconnect(); + m = null; + div.removeAttribute("style"); + then(testStyleRemoveProperty2); + }); + m.observe(div, { attributes: true }); + isnot(div.getAttribute("style"), null, "style attribute before successful removeProperty"); + div.style.removeProperty("color"); +} + +function testStyleRemoveProperty2() { + m = new M(function(records, observer) { + is(records.length, 1, "number of records"); + is(records[0].type, "attributes", "record.type"); + is(records[0].attributeName, "data-test", "record.attributeName"); + is(div.getAttribute("style"), null, "style attribute after unsuccessful removeProperty"); + observer.disconnect(); + m = null; + div.removeAttribute("data-test"); + then(); + }); + m.observe(div, { attributes: true }); + is(div.getAttribute("style"), null, "style attribute before unsuccessful removeProperty"); + div.style.removeProperty("color"); // shouldn't generate any mutation records + div.setAttribute("data-test", "a"); +} + SimpleTest.waitForExplicitFinish(); diff --git a/layout/style/StyleRule.cpp b/layout/style/StyleRule.cpp index b83fc45c8d0..729a6f0271f 100644 --- a/layout/style/StyleRule.cpp +++ b/layout/style/StyleRule.cpp @@ -1009,7 +1009,7 @@ public: NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; void DropReference(void); - virtual css::Declaration* GetCSSDeclaration(bool aAllocate) override; + virtual css::Declaration* GetCSSDeclaration(Operation aOperation) override; virtual nsresult SetCSSDeclaration(css::Declaration* aDecl) override; virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; virtual nsIDocument* DocToUpdate() override; @@ -1114,7 +1114,7 @@ DOMCSSDeclarationImpl::DropReference(void) } css::Declaration* -DOMCSSDeclarationImpl::GetCSSDeclaration(bool aAllocate) +DOMCSSDeclarationImpl::GetCSSDeclaration(Operation aOperation) { if (mRule) { return mRule->GetDeclaration(); diff --git a/layout/style/nsCSSRules.cpp b/layout/style/nsCSSRules.cpp index 646cc31d3ef..49f8eb4c1e8 100644 --- a/layout/style/nsCSSRules.cpp +++ b/layout/style/nsCSSRules.cpp @@ -2103,7 +2103,7 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsCSSKeyframeStyleDeclaration) NS_INTERFACE_MAP_END_INHERITING(nsDOMCSSDeclaration) css::Declaration* -nsCSSKeyframeStyleDeclaration::GetCSSDeclaration(bool aAllocate) +nsCSSKeyframeStyleDeclaration::GetCSSDeclaration(Operation aOperation) { if (mRule) { return mRule->Declaration(); @@ -2664,7 +2664,7 @@ NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsCSSPageStyleDeclaration) NS_INTERFACE_MAP_END_INHERITING(nsDOMCSSDeclaration) css::Declaration* -nsCSSPageStyleDeclaration::GetCSSDeclaration(bool aAllocate) +nsCSSPageStyleDeclaration::GetCSSDeclaration(Operation aOperation) { if (mRule) { return mRule->Declaration(); diff --git a/layout/style/nsCSSRules.h b/layout/style/nsCSSRules.h index 90e1fc11123..77efa70cf77 100644 --- a/layout/style/nsCSSRules.h +++ b/layout/style/nsCSSRules.h @@ -408,7 +408,7 @@ public: NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; void DropReference() { mRule = nullptr; } - virtual mozilla::css::Declaration* GetCSSDeclaration(bool aAllocate) override; + virtual mozilla::css::Declaration* GetCSSDeclaration(Operation aOperation) override; virtual nsresult SetCSSDeclaration(mozilla::css::Declaration* aDecl) override; virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; virtual nsIDocument* DocToUpdate() override; @@ -541,7 +541,7 @@ public: NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; void DropReference() { mRule = nullptr; } - virtual mozilla::css::Declaration* GetCSSDeclaration(bool aAllocate) override; + virtual mozilla::css::Declaration* GetCSSDeclaration(Operation aOperation) override; virtual nsresult SetCSSDeclaration(mozilla::css::Declaration* aDecl) override; virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; virtual nsIDocument* DocToUpdate() override; diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp index df4b0720e0c..e87dcd8547d 100644 --- a/layout/style/nsComputedDOMStyle.cpp +++ b/layout/style/nsComputedDOMStyle.cpp @@ -543,7 +543,7 @@ nsComputedDOMStyle::GetPresShellForContent(nsIContent* aContent) // on a nsComputedDOMStyle object, but must be defined to avoid // compile errors. css::Declaration* -nsComputedDOMStyle::GetCSSDeclaration(bool) +nsComputedDOMStyle::GetCSSDeclaration(Operation) { NS_RUNTIMEABORT("called nsComputedDOMStyle::GetCSSDeclaration"); return nullptr; diff --git a/layout/style/nsComputedDOMStyle.h b/layout/style/nsComputedDOMStyle.h index bb14ab265b8..549c05a482b 100644 --- a/layout/style/nsComputedDOMStyle.h +++ b/layout/style/nsComputedDOMStyle.h @@ -120,7 +120,7 @@ public: // nsDOMCSSDeclaration abstract methods which should never be called // on a nsComputedDOMStyle object, but must be defined to avoid // compile errors. - virtual mozilla::css::Declaration* GetCSSDeclaration(bool) override; + virtual mozilla::css::Declaration* GetCSSDeclaration(Operation) override; virtual nsresult SetCSSDeclaration(mozilla::css::Declaration*) override; virtual nsIDocument* DocToUpdate() override; virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; diff --git a/layout/style/nsDOMCSSAttrDeclaration.cpp b/layout/style/nsDOMCSSAttrDeclaration.cpp index 6701312be9a..3897a920304 100644 --- a/layout/style/nsDOMCSSAttrDeclaration.cpp +++ b/layout/style/nsDOMCSSAttrDeclaration.cpp @@ -91,23 +91,13 @@ nsDOMCSSAttributeDeclaration::SetCSSDeclaration(css::Declaration* aDecl) nsIDocument* nsDOMCSSAttributeDeclaration::DocToUpdate() { - // XXXbz this is a bit of a hack, especially doing it before the - // BeginUpdate(), but this is a good chokepoint where we know we - // plan to modify the CSSDeclaration, so need to notify - // AttributeWillChange if this is inline style. - if (!mIsSMILOverride) { - nsNodeUtils::AttributeWillChange(mElement, kNameSpaceID_None, - nsGkAtoms::style, - nsIDOMMutationEvent::MODIFICATION); - } - // We need OwnerDoc() rather than GetCurrentDoc() because it might // be the BeginUpdate call that inserts mElement into the document. return mElement->OwnerDoc(); } css::Declaration* -nsDOMCSSAttributeDeclaration::GetCSSDeclaration(bool aAllocate) +nsDOMCSSAttributeDeclaration::GetCSSDeclaration(Operation aOperation) { if (!mElement) return nullptr; @@ -118,10 +108,31 @@ nsDOMCSSAttributeDeclaration::GetCSSDeclaration(bool aAllocate) else cssRule = mElement->GetInlineStyleRule(); + // Notify observers that our style="" attribute is going to change + // unless: + // * this is a declaration that holds SMIL animation values (which + // aren't reflected in the DOM style="" attribute), or + // * we're getting the declaration for reading, or + // * we're getting it for property removal but we don't currently have + // a declaration. + + // XXXbz this is a bit of a hack, especially doing it before the + // BeginUpdate(), but this is a good chokepoint where we know we + // plan to modify the CSSDeclaration, so need to notify + // AttributeWillChange if this is inline style. + if (!mIsSMILOverride && + ((aOperation == eOperation_Modify) || + (aOperation == eOperation_RemoveProperty && cssRule))) { + nsNodeUtils::AttributeWillChange(mElement, kNameSpaceID_None, + nsGkAtoms::style, + nsIDOMMutationEvent::MODIFICATION); + } + if (cssRule) { return cssRule->GetDeclaration(); } - if (!aAllocate) { + + if (aOperation != eOperation_Modify) { return nullptr; } diff --git a/layout/style/nsDOMCSSAttrDeclaration.h b/layout/style/nsDOMCSSAttrDeclaration.h index 2f98d4ff755..681751843b9 100644 --- a/layout/style/nsDOMCSSAttrDeclaration.h +++ b/layout/style/nsDOMCSSAttrDeclaration.h @@ -31,7 +31,7 @@ public: // If GetCSSDeclaration returns non-null, then the decl it returns // is owned by our current style rule. - virtual mozilla::css::Declaration* GetCSSDeclaration(bool aAllocate) override; + virtual mozilla::css::Declaration* GetCSSDeclaration(Operation aOperation) override; virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; diff --git a/layout/style/nsDOMCSSDeclaration.cpp b/layout/style/nsDOMCSSDeclaration.cpp index 4e668fde33f..9adc003ce72 100644 --- a/layout/style/nsDOMCSSDeclaration.cpp +++ b/layout/style/nsDOMCSSDeclaration.cpp @@ -45,7 +45,7 @@ nsDOMCSSDeclaration::GetPropertyValue(const nsCSSProperty aPropID, NS_PRECONDITION(aPropID != eCSSProperty_UNKNOWN, "Should never pass eCSSProperty_UNKNOWN around"); - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); aValue.Truncate(); if (decl) { @@ -61,7 +61,7 @@ nsDOMCSSDeclaration::GetCustomPropertyValue(const nsAString& aPropertyName, MOZ_ASSERT(Substring(aPropertyName, 0, CSS_CUSTOM_NAME_PREFIX_LENGTH).EqualsLiteral("--")); - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); if (!decl) { aValue.Truncate(); return; @@ -89,7 +89,7 @@ nsDOMCSSDeclaration::SetPropertyValue(const nsCSSProperty aPropID, NS_IMETHODIMP nsDOMCSSDeclaration::GetCssText(nsAString& aCssText) { - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); aCssText.Truncate(); if (decl) { @@ -104,7 +104,7 @@ nsDOMCSSDeclaration::SetCssText(const nsAString& aCssText) { // We don't need to *do* anything with the old declaration, but we need // to ensure that it exists, or else SetCSSDeclaration may crash. - css::Declaration* olddecl = GetCSSDeclaration(true); + css::Declaration* olddecl = GetCSSDeclaration(eOperation_Modify); if (!olddecl) { return NS_ERROR_FAILURE; } @@ -139,7 +139,7 @@ nsDOMCSSDeclaration::SetCssText(const nsAString& aCssText) NS_IMETHODIMP nsDOMCSSDeclaration::GetLength(uint32_t* aLength) { - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); if (decl) { *aLength = decl->Count(); @@ -161,7 +161,7 @@ nsDOMCSSDeclaration::GetPropertyCSSValue(const nsAString& aPropertyName, ErrorRe void nsDOMCSSDeclaration::IndexedGetter(uint32_t aIndex, bool& aFound, nsAString& aPropName) { - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); aFound = decl && decl->GetNthProperty(aIndex, aPropName); } @@ -202,7 +202,7 @@ nsDOMCSSDeclaration::GetAuthoredPropertyValue(const nsAString& aPropertyName, return NS_OK; } - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); if (!decl) { return NS_ERROR_FAILURE; } @@ -215,7 +215,7 @@ NS_IMETHODIMP nsDOMCSSDeclaration::GetPropertyPriority(const nsAString& aPropertyName, nsAString& aReturn) { - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_Read); aReturn.Truncate(); if (decl && decl->GetValueIsImportant(aPropertyName)) { @@ -310,7 +310,7 @@ nsDOMCSSDeclaration::ParsePropertyValue(const nsCSSProperty aPropID, const nsAString& aPropValue, bool aIsImportant) { - css::Declaration* olddecl = GetCSSDeclaration(true); + css::Declaration* olddecl = GetCSSDeclaration(eOperation_Modify); if (!olddecl) { return NS_ERROR_FAILURE; } @@ -351,7 +351,7 @@ nsDOMCSSDeclaration::ParseCustomPropertyValue(const nsAString& aPropertyName, { MOZ_ASSERT(nsCSSProps::IsCustomPropertyName(aPropertyName)); - css::Declaration* olddecl = GetCSSDeclaration(true); + css::Declaration* olddecl = GetCSSDeclaration(eOperation_Modify); if (!olddecl) { return NS_ERROR_FAILURE; } @@ -391,7 +391,7 @@ nsDOMCSSDeclaration::ParseCustomPropertyValue(const nsAString& aPropertyName, nsresult nsDOMCSSDeclaration::RemoveProperty(const nsCSSProperty aPropID) { - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_RemoveProperty); if (!decl) { return NS_OK; // no decl, so nothing to remove } @@ -414,7 +414,7 @@ nsDOMCSSDeclaration::RemoveCustomProperty(const nsAString& aPropertyName) MOZ_ASSERT(Substring(aPropertyName, 0, CSS_CUSTOM_NAME_PREFIX_LENGTH).EqualsLiteral("--")); - css::Declaration* decl = GetCSSDeclaration(false); + css::Declaration* decl = GetCSSDeclaration(eOperation_RemoveProperty); if (!decl) { return NS_OK; // no decl, so nothing to remove } diff --git a/layout/style/nsDOMCSSDeclaration.h b/layout/style/nsDOMCSSDeclaration.h index b938c4ddf72..406da374664 100644 --- a/layout/style/nsDOMCSSDeclaration.h +++ b/layout/style/nsDOMCSSDeclaration.h @@ -100,10 +100,26 @@ public: virtual JSObject* WrapObject(JSContext* aCx, JS::Handle aGivenProto) override; protected: - // This method can return null regardless of the value of aAllocate; - // however, a null return should only be considered a failure - // if aAllocate is true. - virtual mozilla::css::Declaration* GetCSSDeclaration(bool aAllocate) = 0; + // The reason for calling GetCSSDeclaration. + enum Operation { + // We are calling GetCSSDeclaration so that we can read from it. Does not + // allocate a new declaration if we don't have one yet; returns nullptr in + // this case. + eOperation_Read, + + // We are calling GetCSSDeclaration so that we can set a property on it + // or re-parse the whole declaration. Allocates a new declaration if we + // don't have one yet and calls AttributeWillChange. A nullptr return value + // indicates an error allocating the declaration. + eOperation_Modify, + + // We are calling GetCSSDeclaration so that we can remove a property from + // it. Does not allocates a new declaration if we don't have one yet; + // returns nullptr in this case. If we do have a declaration, calls + // AttributeWillChange. + eOperation_RemoveProperty + }; + virtual mozilla::css::Declaration* GetCSSDeclaration(Operation aOperation) = 0; virtual nsresult SetCSSDeclaration(mozilla::css::Declaration* aDecl) = 0; // Document that we must call BeginUpdate/EndUpdate on around the // calls to SetCSSDeclaration and the style rule mutation that leads