Bug 1570759 - Enable the optimization to not flush for fixed margin values. r=jwatt
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 30 Aug 2019 14:45:20 +0000
changeset 490860 7fa833ff0b1c7880dbba9cd9b6014ad88bed5b67
parent 490859 ec88018fd94e5e6d013b722186c95f20ecbb615b
child 490861 11104935922576fa9d6c2e7ae73a1b09ebab5477
push id36514
push usermalexandru@mozilla.com
push dateFri, 30 Aug 2019 21:54:33 +0000
treeherdermozilla-central@cae93ef1993e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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 1570759 - Enable the optimization to not flush for fixed margin values. r=jwatt As noted this changes behavior, but it's unclear per https://github.com/w3c/csswg-drafts/issues/2328 what behavior is correct, and our behavior is inconsistent depending on whether there's any percentage involved. This matches other browsers so it's pretty low risk I'd say. The test starts passing without changes to the test, but given the CSSWG issue I made the test not rely on the optimization. Differential Revision: https://phabricator.services.mozilla.com/D43754
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -894,27 +894,20 @@ bool nsComputedDOMStyle::NeedsToFlushLay
       // layout for themed frames.
       return PaddingNeedsUsedValue(style->StylePadding()->mPadding.Get(side),
     case eCSSProperty_margin_top:
     case eCSSProperty_margin_right:
     case eCSSProperty_margin_bottom:
     case eCSSProperty_margin_left: {
-      // NOTE(emilio): We could do the commented out thing below, but it is not
-      // clear whether it's correct. It does change behavior and cause a new
-      // test to _pass_. But it's unclear whether it is the right thing, see:
-      //
-      // https://github.com/w3c/csswg-drafts/issues/2328
-      //
-      // Bug 1570759 tracks maybe changing the behavior here.
-      //
-      // Side side = SideForPaddingOrMarginOrInsetProperty(aPropID);
-      // return !style->StyleMargin()->mMargin.Get(side).ConvertsToLength();
-      return true;
+      // NOTE(emilio): This is dubious, but matches other browsers.
+      // See https://github.com/w3c/csswg-drafts/issues/2328
+      Side side = SideForPaddingOrMarginOrInsetProperty(aPropID);
+      return !style->StyleMargin()->mMargin.Get(side).ConvertsToLength();
       return false;
 void nsComputedDOMStyle::Flush(Document& aDocument, FlushType aFlushType) {
--- a/testing/web-platform/meta/css/css-box/parsing/margin-computed.html.ini
+++ b/testing/web-platform/meta/css/css-box/parsing/margin-computed.html.ini
@@ -1,15 +1,12 @@
   [Property margin value '10px' computes to '10px']
     expected: FAIL
-  [Property margin-right value '20px' computes to '20px']
-    expected: FAIL
   [Property margin value '30%' computes to '60px']
     expected: FAIL
   [Property margin value '10px 20px 30px 40px' computes to '10px 20px 30px 40px']
     expected: FAIL
   [Property margin value 'calc(0.5em + 10px)' computes to '30px']
     expected: FAIL
--- a/testing/web-platform/tests/css/css-box/parsing/margin-computed.html
+++ b/testing/web-platform/tests/css/css-box/parsing/margin-computed.html
@@ -18,20 +18,29 @@
 <div id="parent">
   <div id="target"></div>
+const target = document.getElementById("target");
 test_computed_value("margin", "10px");
 test_computed_value("margin", "10px 20px 30px 40px");
 test_computed_value("margin", "calc(0.5em + 10px)", "30px");
 test_computed_value("margin", "30%", "60px");
+// Since what should the margin be in presence of other margins is a bit
+// unclear (https://github.com/w3c/csswg-drafts/issues/2328), reset the margin
+// before testing.
+target.style.margin = "0";
 test_computed_value("margin-top", "10px");
+target.style.margin = "0";
 test_computed_value("margin-right", "20px");
+target.style.margin = "0";
 test_computed_value("margin-bottom", "30px");
+target.style.margin = "0";
 test_computed_value("margin-left", "40px");