Bug 1498445 - Stop creating a style attribute for invalid property values. r=smaug
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 12 Oct 2018 09:07:00 +0000
changeset 440868 ccdfe4ca1f881fea6751d8addbd6466d106ed3dd
parent 440867 a0fc8ecfa271f44bd24598786930a36bdecbd406
child 440869 d1adf0333d63a102d7e91c887586da7219c0d650
push id70851
push useremilio@crisal.io
push dateFri, 12 Oct 2018 11:57:42 +0000
treeherderautoland@ccdfe4ca1f88 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1498445
milestone64.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1498445 - Stop creating a style attribute for invalid property values. r=smaug This matches all other browsers, and the spec. Added an explicit test for this, and a test that tests what cssstyledeclaration-mutationrecord-002.html wanted to test, which is that changing an existing declaration doesn't generate a mutation record. Differential Revision: https://phabricator.services.mozilla.com/D8500
dom/base/nsStyledElement.cpp
layout/style/nsDOMCSSAttrDeclaration.cpp
layout/style/nsDOMCSSDeclaration.cpp
testing/web-platform/meta/css/cssom/cssstyledeclaration-mutationrecord-002.html.ini
testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-002.html
testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-005.html
testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-attr.html
--- a/dom/base/nsStyledElement.cpp
+++ b/dom/base/nsStyledElement.cpp
@@ -127,18 +127,17 @@ nsStyledElement::SetInlineStyleDeclarati
                                          this);
 
   nsAttrValue attrValue(do_AddRef(&aDeclaration), nullptr);
   SetMayHaveStyle();
 
   nsIDocument* document = GetComposedDoc();
   mozAutoDocUpdate updateBatch(document, true);
   return SetAttrAndNotify(kNameSpaceID_None, nsGkAtoms::style, nullptr,
-                          aData.mOldValue.isSome() ?
-                            aData.mOldValue.ptr() : nullptr,
+                          aData.mOldValue.ptrOr(nullptr),
                           attrValue, nullptr, aData.mModType,
                           hasListeners, true, kDontCallAfterSetAttr,
                           document, updateBatch);
 }
 
 // ---------------------------------------------------------------
 // Others and helpers
 
--- a/layout/style/nsDOMCSSAttrDeclaration.cpp
+++ b/layout/style/nsDOMCSSAttrDeclaration.cpp
@@ -73,22 +73,19 @@ nsDOMCSSAttributeDeclaration::SetCSSDecl
 {
   NS_ASSERTION(mElement, "Must have Element to set the declaration!");
 
   // Whenever changing element.style values, aClosureData must be non-null.
   // SMIL doesn't update Element's attribute values, so closure data isn't
   // needed.
   MOZ_ASSERT_IF(!mIsSMILOverride, aClosureData);
 
-  // If the closure hasn't been called because the declaration wasn't changed,
-  // we need to explicitly call it now to get InlineStyleDeclarationWillChange
-  // notification before SetInlineStyleDeclaration.
-  if (aClosureData && aClosureData->mClosure) {
-    aClosureData->mClosure(aClosureData);
-  }
+  // The closure needs to have been called by now, otherwise we shouldn't be
+  // getting here when the attribute hasn't changed.
+  MOZ_ASSERT_IF(aClosureData, !aClosureData->mClosure);
 
   aDecl->SetDirty();
   return mIsSMILOverride
     ? mElement->SetSMILOverrideStyleDeclaration(aDecl, true)
     : mElement->SetInlineStyleDeclaration(*aDecl, *aClosureData);
 }
 
 nsIDocument*
--- a/layout/style/nsDOMCSSDeclaration.cpp
+++ b/layout/style/nsDOMCSSDeclaration.cpp
@@ -122,22 +122,16 @@ nsDOMCSSDeclaration::SetCssText(const ns
   mozAutoDocUpdate autoUpdate(DocToUpdate(), true);
   DeclarationBlockMutationClosure closure = {};
   MutationClosureData closureData;
   GetPropertyChangeClosure(&closure, &closureData);
 
   ParsingEnvironment servoEnv =
     GetParsingEnvironment(aSubjectPrincipal);
   if (!servoEnv.mUrlExtraData) {
-    if (created) {
-      // In case we can't set a new declaration, but one was
-      // created for the old one, we need to set the old declaration to
-      // get right style attribute handling.
-      SetCSSDeclaration(olddecl, &closureData);
-    }
     aRv.Throw(NS_ERROR_NOT_AVAILABLE);
     return;
   }
 
   // Need to special case closure calling here, since parsing css text
   // doesn't modify any existing declaration and that is why the callback isn't
   // called implicitly.
   if (closureData.mClosure) {
@@ -282,32 +276,22 @@ nsDOMCSSDeclaration::ModifyDeclaration(n
   // rule (see stack in bug 209575).
   mozAutoDocUpdate autoUpdate(DocToUpdate(), true);
   RefPtr<DeclarationBlock> decl = olddecl->EnsureMutable();
 
   bool changed;
   ParsingEnvironment servoEnv =
     GetParsingEnvironment(aSubjectPrincipal);
   if (!servoEnv.mUrlExtraData) {
-    if (created) {
-      // In case we can't set a new declaration, but one was
-      // created for the old one, we need to set the old declaration to
-      // get right style attribute handling.
-      SetCSSDeclaration(olddecl, aClosureData);
-    }
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   changed = aFunc(decl, servoEnv);
 
   if (!changed) {
-    if (created) {
-      // See comment above about setting old declaration.
-      SetCSSDeclaration(olddecl, aClosureData);
-    }
     // Parsing failed -- but we don't throw an exception for that.
     return NS_OK;
   }
 
   return SetCSSDeclaration(decl, aClosureData);
 }
 
 nsresult
deleted file mode 100644
--- a/testing/web-platform/meta/css/cssom/cssstyledeclaration-mutationrecord-002.html.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[cssstyledeclaration-mutationrecord-002.html]
-  [CSSStyleDeclaration.setPropertyValue doesn't queue a mutation record when setting invalid values]
-    expected: FAIL
-
--- a/testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-002.html
+++ b/testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-002.html
@@ -1,12 +1,12 @@
 <!doctype html>
 <meta charset="utf-8">
 <title>CSSOM: CSSStyleDeclaration.setPropertyValue doesn't queue a mutation record for invalid values</title>
-<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue">
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty">
 <link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
 <script src=/resources/testharness.js></script>
 <script src=/resources/testharnessreport.js></script>
 <script>
   let test = async_test("CSSStyleDeclaration.setPropertyValue doesn't queue a mutation record when setting invalid values");
   let m = new MutationObserver(test.unreached_func("shouldn't queue a mutation record"));
   m.observe(document.documentElement,  { attributes: true });
 
copy from testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-002.html
copy to testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-005.html
--- a/testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-002.html
+++ b/testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-005.html
@@ -1,12 +1,13 @@
 <!doctype html>
+<html style="color: inherit">
 <meta charset="utf-8">
 <title>CSSOM: CSSStyleDeclaration.setPropertyValue doesn't queue a mutation record for invalid values</title>
-<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue">
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty">
 <link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
 <script src=/resources/testharness.js></script>
 <script src=/resources/testharnessreport.js></script>
 <script>
   let test = async_test("CSSStyleDeclaration.setPropertyValue doesn't queue a mutation record when setting invalid values");
   let m = new MutationObserver(test.unreached_func("shouldn't queue a mutation record"));
   m.observe(document.documentElement,  { attributes: true });
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-attr.html
@@ -0,0 +1,18 @@
+<!doctype html>
+<title>CSSOM test: declaration block after setting via CSSOM</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setproperty">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<script>
+test(function() {
+  let element = document.createElement("div");
+  element.style.setProperty("doesntexist", "0");
+  assert_false(element.hasAttribute("style"));
+}, "Setting an invalid property via the declaration setter doesn't create a declaration");
+test(function() {
+  let element = document.createElement("div");
+  element.style.setProperty("width", "-100");
+  assert_false(element.hasAttribute("style"));
+}, "Setting an invalid value via the declaration setter doesn't create a declaration");
+</script>