Bug 1473180 part 3 - Use the new algorithm for setting property. r=emilio

MozReview-Commit-ID: HQsVwWAGPBL

--HG--
extra : rebase_source : 7280d1a0278e698ebc2fb664874aea53a19a3d3f
extra : source : 08a40cf9746a83fceb124dd148d02ccb0d2e4864

modules/libpref/init/StaticPrefList.h

@@ -333,24 +333,6 @@ VARCACHE_PREF(
 )
 #undef PREF_VALUE
 
-// When the pref is true, CSSStyleDeclaration.setProperty always appends
-// new declarations (and discards old ones if they exist), otherwise, it
-// will update in-place when given property exists in the block, and
-// avoid updating at all when the existing property declaration is
-// identical to the new one.
-// See bug 1415330, bug 1460295, and bug 1461285 for some background.
-#ifdef RELEASE_OR_BETA
-# define PREF_VALUE false
-#else
-# define PREF_VALUE true
-#endif
-VARCACHE_PREF(
-  "layout.css.property-append-only",
-   layout_css_property_append_only,
-   bool, PREF_VALUE
-)
-#undef PREF_VALUE
-
 // Should the :visited selector ever match (otherwise :link matches instead)?
 VARCACHE_PREF(
   "layout.css.visited_links_enabled",

servo/ports/geckolib/glue.rs

@@ -3584,31 +3584,18 @@ fn set_property(
     }
 
     let importance = if is_important { Importance::Important } else { Importance::Normal };
-    let append_only = unsafe {
-        structs::StaticPrefs_sVarCache_layout_css_property_append_only
-    };
-    let mode = if append_only {
-        DeclarationPushMode::Append
-    } else {
-        let will_change = read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
-            decls.will_change_in_update_mode(&source_declarations, importance)
-        });
-        if !will_change {
-            return false;
-        }
-        DeclarationPushMode::Update
-    };
+    let mut updates = Default::default();
+    let will_change = read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
+        decls.prepare_for_update(&source_declarations, importance, &mut updates)
+    });
+    if !will_change {
+        return false;
+    }
 
     before_change_closure.invoke();
-
-    let result = write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.extend(
-            source_declarations.drain(),
-            importance,
-            mode,
-        )
+    write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
+        decls.update(source_declarations.drain(), importance, &mut updates)
     });
-    debug_assert!(result);
     true
 }
 

testing/web-platform/meta/MANIFEST.json

@@ -327173,9 +327173,15 @@
      {}
     ]
    ],
-   "css/cssom/cssstyledeclaration-setter-order.html": [
+   "css/cssom/cssstyledeclaration-setter-declarations.html": [
     [
-     "/css/cssom/cssstyledeclaration-setter-order.html",
+     "/css/cssom/cssstyledeclaration-setter-declarations.html",
+     {}
+    ]
+   ],
+   "css/cssom/cssstyledeclaration-setter-logical.html": [
+    [
+     "/css/cssom/cssstyledeclaration-setter-logical.html",
      {}
     ]
    ],
@@ -553875,7 +553881,7 @@
    "testharness"
   ],
   "css/cssom/cssstyledeclaration-mutationrecord-001.html": [
-   "5d455757e4c80b4781ea4263fa78bced1d6b8632",
+   "0ed8cb2c41f371fdb509731f2ad1cf11e047d46f",
    "testharness"
   ],
   "css/cssom/cssstyledeclaration-mutationrecord-002.html": [
@@ -553890,8 +553896,12 @@
    "958b71b8f1c58a809590459e6f085f3e1217e9c7",
    "testharness"
   ],
-  "css/cssom/cssstyledeclaration-setter-order.html": [
-   "3e0e768c466011bb3d91b3f0eff55e029a2aec0f",
+  "css/cssom/cssstyledeclaration-setter-declarations.html": [
+   "e530f6b573bfb9774dd732f7289156117fc4bd94",
+   "testharness"
+  ],
+  "css/cssom/cssstyledeclaration-setter-logical.html": [
+   "c454a34b964e2aa05790831cc2de20e169162dd5",
    "testharness"
   ],
   "css/cssom/escape.html": [

testing/web-platform/meta/css/cssom/cssstyledeclaration-mutationrecord-001.html.ini

@@ -1,2 +0,0 @@
-[cssstyledeclaration-mutationrecord-001.html]
-  prefs: [layout.css.property-append-only:true]

testing/web-platform/meta/css/cssom/cssstyledeclaration-setter-order.html.ini

@@ -1,2 +0,0 @@
-[cssstyledeclaration-setter-order.html]
-  prefs: [layout.css.property-append-only:true]

testing/web-platform/tests/css/cssom/cssstyledeclaration-mutationrecord-001.html

@@ -1,6 +1,6 @@
 <!doctype html>
 <meta charset="utf-8">
-<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when not actually mutated</title>
+<title>CSSOM: CSSStyleDeclaration.setPropertyValue queues a mutation record when changed</title>
 <link rel="help" href="https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-setpropertyvalue">
 <link rel="help" href="https://drafts.csswg.org/cssom/#update-style-attribute-for">
 <link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
@@ -9,12 +9,12 @@
 <script>
   document.documentElement.style.top = "0px";
 
-  let test = async_test("CSSStyleDeclaration.setPropertyValue queues a mutation record, even if not mutated");
+  let test = async_test("CSSStyleDeclaration.setPropertyValue queues a mutation record when serialization is changed");
   let m = new MutationObserver(function(r) {
     assert_equals(r.length, 1);
     test.done();
   });
 
   m.observe(document.documentElement,  { attributes: true });
-  document.documentElement.style.top = "0px";
+  document.documentElement.style.top = "1px";
 </script>

testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-declarations.html

@@ -0,0 +1,160 @@
+<!DOCTYPE html>
+<title>CSSOM test: declaration block after setting via CSSOM</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#set-a-css-declaration-value">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="log"></div>
+<script>
+  function generateCSSDeclBlock(props) {
+    let elem = document.createElement("div");
+    let cssText = props.map(({name, value, priority}) => {
+      let longhand = `${name}: ${value}`;
+      if (priority) {
+        longhand += "!" + priority;
+      }
+      return longhand + ";";
+    }).join(" ");
+    elem.setAttribute("style", cssText);
+    return elem.style;
+  }
+  function compareByName(a, b) {
+    if (a.name < b.name) return -1;
+    if (a.name > b.name) return 1;
+    return 0;
+  }
+  function checkDeclarationsAnyOrder(block, props, msg) {
+    let actual = [];
+    for (let name of block) {
+      let value = block.getPropertyValue(name);
+      let priority = block.getPropertyPriority(name);
+      actual.push({name, value, priority});
+    }
+    actual.sort(compareByName);
+    let expected = Array.from(props);
+    expected.sort(compareByName);
+    assert_object_equals(actual, expected, "Declaration block content should match " + msg);
+  }
+  function longhand(name, value, priority="") {
+    return {name, value, priority};
+  }
+  function* shorthand(name, value, priority="") {
+    for (let subprop of SUBPROPS[name]) {
+      yield longhand(subprop, value, priority);
+    }
+  }
+
+  const SUBPROPS = {
+    "margin": ["margin-top", "margin-right", "margin-bottom", "margin-left"],
+    "padding": ["padding-top", "padding-right", "padding-bottom", "padding-left"],
+  };
+
+  test(function() {
+    let expectedDecls = [
+      longhand("top", "1px"),
+      longhand("bottom", "2px"),
+      longhand("left", "3px", "important"),
+      longhand("right", "4px"),
+    ];
+    let block = generateCSSDeclBlock(expectedDecls);
+    checkDeclarationsAnyOrder(block, expectedDecls, "in initial block");
+
+    block.setProperty("top", "5px", "important");
+    expectedDecls[0] = longhand("top", "5px", "important");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property");
+
+    block.setProperty("bottom", "2px");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property with identical value");
+
+    block.setProperty("left", "3px");
+    expectedDecls[2].priority = "";
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property with different priority");
+
+    block.setProperty("float", "none");
+    expectedDecls.push(longhand("float", "none"));
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting non-existing property");
+  }, "setProperty with longhand should update only the declaration being set");
+
+  test(function() {
+    let expectedDecls = [
+      longhand("top", "1px"),
+      longhand("bottom", "2px"),
+      longhand("left", "3px", "important"),
+      longhand("right", "4px"),
+    ];
+    let block = generateCSSDeclBlock(expectedDecls);
+    checkDeclarationsAnyOrder(block, expectedDecls, "in initial block");
+
+    block.top = "5px";
+    expectedDecls[0] = longhand("top", "5px");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property");
+
+    block.bottom = "2px";
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property with identical value");
+
+    block.left = "3px";
+    expectedDecls[2].priority = "";
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting existing property with different priority");
+
+    block.float = "none";
+    expectedDecls.push(longhand("float", "none"));
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting non-existing property");
+  }, "property setter should update only the declaration being set");
+
+  test(function() {
+    let expectedDecls = [
+      ...shorthand("margin", "1px"),
+      longhand("top", "2px"),
+      ...shorthand("padding", "3px", "important"),
+    ];
+    let block = generateCSSDeclBlock(expectedDecls);
+    checkDeclarationsAnyOrder(block, expectedDecls, "in initial block");
+
+    block.setProperty("margin", "4px");
+    for (let i = 0; i < 4; i++) {
+      expectedDecls[i].value = "4px";
+    }
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand");
+
+    block.setProperty("margin", "4px");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand with identical value");
+
+    block.setProperty("padding", "3px");
+    for (let i = 5; i < 9; i++) {
+      expectedDecls[i].priority = "";
+    }
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand with different priority");
+
+    block.setProperty("margin-bottom", "5px", "important");
+    expectedDecls[2] = longhand("margin-bottom", "5px", "important");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting a longhand in an existing shorthand");
+  }, "setProperty with shorthand should update only the declarations being set");
+
+  test(function() {
+    let expectedDecls = [
+      ...shorthand("margin", "1px"),
+      longhand("top", "2px"),
+      ...shorthand("padding", "3px", "important"),
+    ];
+    let block = generateCSSDeclBlock(expectedDecls);
+    checkDeclarationsAnyOrder(block, expectedDecls, "in initial block");
+
+    block.margin = "4px";
+    for (let i = 0; i < 4; i++) {
+      expectedDecls[i].value = "4px";
+    }
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand");
+
+    block.margin = "4px";
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand with identical value");
+
+    block.padding = "3px";
+    for (let i = 5; i < 9; i++) {
+      expectedDecls[i].priority = "";
+    }
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting an existing shorthand with different priority");
+
+    block.marginBottom = "5px";
+    expectedDecls[2] = longhand("margin-bottom", "5px");
+    checkDeclarationsAnyOrder(block, expectedDecls, "after setting a longhand in an existing shorthand");
+  }, "longhand property setter should update only the decoarations being set");
+</script>

testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-logical.html

@@ -0,0 +1,73 @@
+<!DOCTYPE html>
+<title>CSSOM test: declaration block after setting via CSSOM</title>
+<link rel="help" href="https://drafts.csswg.org/cssom/#set-a-css-declaration-value">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="log"></div>
+<div id="test"></div>
+<script>
+  test(function() {
+    const PHYSICAL_PROPS = ["padding-top", "padding-right",
+                            "padding-bottom", "padding-left"];
+    const LOGICAL_PROPS = ["padding-block-start", "padding-block-end",
+                           "padding-inline-start", "padding-inline-end"];
+    let elem = document.getElementById("test");
+    let decls = new Map;
+
+    {
+      let css = []
+      let i = 0;
+      for (let name of [...PHYSICAL_PROPS, ...LOGICAL_PROPS]) {
+        let value = `${i++}px`;
+        css.push(`${name}: ${value};`);
+        decls.set(name, value);
+      }
+      elem.setAttribute("style", css.join(" "));
+    }
+
+    let style = elem.style;
+    function indexOfProperty(prop) {
+      return Array.prototype.indexOf.apply(style, [prop]);
+    }
+    function assertPropAfterProps(prop, props, msg) {
+      let index = indexOfProperty(prop);
+      for (let p of props) {
+        assert_less_than(indexOfProperty(p), index, `${prop} should be after ${p} ${msg}`);
+      }
+    }
+    // We are not changing any value in this test, just order.
+    function assertValueUnchanged() {
+      for (let [name, value] of decls.entries()) {
+        assert_equals(style.getPropertyValue(name), value,
+                      `value of ${name} shouldn't be changed`);
+      }
+    }
+
+    assertValueUnchanged();
+    // Check that logical properties are all after physical properties
+    // at the beginning.
+    for (let p of LOGICAL_PROPS) {
+      assertPropAfterProps(p, PHYSICAL_PROPS, "initially");
+    }
+
+    // Try setting a longhand.
+    style.setProperty("padding-top", "0px");
+    assertValueUnchanged();
+    // Check that padding-top is after logical properties, but other
+    // physical properties are still before logical properties.
+    assertPropAfterProps("padding-top", LOGICAL_PROPS, "after setting padding-top");
+    for (let p of LOGICAL_PROPS) {
+      assertPropAfterProps(p, PHYSICAL_PROPS.filter(prop => prop != "padding-top"),
+                           "after setting padding-top");
+    }
+
+    // Try setting a shorthand.
+    style.setProperty("padding", "0px 1px 2px 3px");
+    assertValueUnchanged();
+    // Check that all physical properties are now after logical properties.
+    for (let p of PHYSICAL_PROPS) {
+      assertPropAfterProps(p, LOGICAL_PROPS, "after setting padding shorthand");
+    }
+  }, "newly set declaration should be after all declarations " +
+  "in the same logical property group but have different logical kind");
+</script>

testing/web-platform/tests/css/cssom/cssstyledeclaration-setter-order.html

@@ -1,108 +0,0 @@
-<!DOCTYPE html>
-<title>CSSOM test: order of declarations after setting via CSSOM</title>
-<link rel="help" href="https://drafts.csswg.org/cssom/#set-a-css-declaration-value">
-<script src="/resources/testharness.js"></script>
-<script src="/resources/testharnessreport.js"></script>
-<div id="log"></div>
-<script>
-  function generateCSSDeclBlock(props) {
-    let elem = document.createElement("div");
-    let cssText = props.map(([prop, value]) => `${prop}: ${value};`).join(" ");
-    elem.setAttribute("style", cssText);
-    return elem.style;
-  }
-  function checkOrder(block, props, msg) {
-    assert_array_equals(Array.from(block), props, `Property order should match ${msg}`);
-  }
-  function arrayWithItemsAtEnd(array, items) {
-    let result = array.filter(item => !items.includes(item));
-    return result.concat(items);
-  }
-
-  const SUBPROPS = {
-    "margin": ["margin-top", "margin-right", "margin-bottom", "margin-left"],
-    "padding": ["padding-top", "padding-right", "padding-bottom", "padding-left"],
-  };
-
-  test(function() {
-    let block = generateCSSDeclBlock([
-      ["top", "1px"],
-      ["bottom", "2px"],
-      ["left", "3px"],
-      ["right", "4px"],
-    ]);
-    let expectedOrder = ["top", "bottom", "left", "right"];
-    checkOrder(block, expectedOrder, "in initial block");
-
-    block.setProperty("top", "5px");
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["top"]);
-    checkOrder(block, expectedOrder, "after setting existing property");
-
-    block.setProperty("bottom", "2px");
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["bottom"]);
-    checkOrder(block, expectedOrder, "after setting existing property with identical value");
-  }, "setProperty with existing longhand should change order");
-
-  test(function() {
-    let block = generateCSSDeclBlock([
-      ["top", "1px"],
-      ["bottom", "2px"],
-      ["left", "3px"],
-      ["right", "4px"],
-    ]);
-    let expectedOrder = ["top", "bottom", "left", "right"];
-    checkOrder(block, expectedOrder, "in initial block");
-
-    block.top = "5px";
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["top"]);
-    checkOrder(block, expectedOrder, "after setting existing property");
-
-    block.bottom = "2px";
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["bottom"]);
-    checkOrder(block, expectedOrder, "after setting existing property with identical value");
-  }, "invoke property setter with existing longhand should change order");
-
-  test(function() {
-    let block = generateCSSDeclBlock([
-      ["margin", "1px"],
-      ["top", "2px"],
-      ["padding", "3px"],
-    ]);
-    let expectedOrder = SUBPROPS["margin"].concat(["top"]).concat(SUBPROPS["padding"]);
-    checkOrder(block, expectedOrder, "in initial block");
-
-    block.setProperty("margin", "4px");
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, SUBPROPS["margin"]);
-    checkOrder(block, expectedOrder, "after setting an existing shorthand");
-
-    block.setProperty("padding", "3px");
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, SUBPROPS["padding"]);
-    checkOrder(block, expectedOrder, "after setting an existing shorthand with identical value");
-
-    block.setProperty("margin-bottom", "5px");
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["margin-bottom"]);
-    checkOrder(block, expectedOrder, "after setting a longhand in an existing shorthand");
-  }, "setProperty with existing shorthand should change order");
-
-  test(function() {
-    let block = generateCSSDeclBlock([
-      ["margin", "1px"],
-      ["top", "2px"],
-      ["padding", "3px"],
-    ]);
-    let expectedOrder = SUBPROPS["margin"].concat(["top"]).concat(SUBPROPS["padding"]);
-    checkOrder(block, expectedOrder, "in initial block");
-
-    block.margin = "4px";
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, SUBPROPS["margin"]);
-    checkOrder(block, expectedOrder, "after setting an existing shorthand");
-
-    block.padding = "3px";
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, SUBPROPS["padding"]);
-    checkOrder(block, expectedOrder, "after setting an existing shorthand with identical value");
-
-    block.marginBottom = "5px";
-    expectedOrder = arrayWithItemsAtEnd(expectedOrder, ["margin-bottom"]);
-    checkOrder(block, expectedOrder, "after setting a longhand in an existing shorthand");
-  }, "invoke property setter with existing shorthand should change order");
-</script>