Bug 1453148: Let overflow parse two values. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 10 Apr 2018 18:40:22 +0200
changeset 780958 6642d345b9bad51adb96207dc291cfa39dac7a69
parent 780951 d194a40ad5bc0a0e6f7b47d121eab99fac2893cd
push id106167
push userbmo:emilio@crisal.io
push dateThu, 12 Apr 2018 09:26:05 +0000
reviewersxidorn
bugs1453148
milestone61.0a1
Bug 1453148: Let overflow parse two values. r?xidorn Per https://github.com/w3c/csswg-drafts/issues/2484. MozReview-Commit-ID: D7M3PhnTtD2
layout/style/nsComputedDOMStyle.cpp
layout/style/test/property_database.js
layout/style/test/test_bug319381.html
servo/components/style/properties/shorthand/box.mako.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/meta/css/cssom/index-002.html.ini
testing/web-platform/tests/css/css-overflow/overflow-shorthand-001.html
--- a/layout/style/nsComputedDOMStyle.cpp
+++ b/layout/style/nsComputedDOMStyle.cpp
@@ -5108,26 +5108,32 @@ nsComputedDOMStyle::DoGetWillChange()
   return valueList.forget();
 }
 
 already_AddRefed<CSSValue>
 nsComputedDOMStyle::DoGetOverflow()
 {
   const nsStyleDisplay* display = StyleDisplay();
 
-  if (display->mOverflowX != display->mOverflowY) {
-    // No value to return.  We can't express this combination of
-    // values as a shorthand.
-    return nullptr;
-  }
-
-  RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
-  val->SetIdent(nsCSSProps::ValueToKeywordEnum(display->mOverflowX,
-                                               nsCSSProps::kOverflowKTable));
-  return val.forget();
+  RefPtr<nsROCSSPrimitiveValue> overflowX = new nsROCSSPrimitiveValue;
+  overflowX->SetIdent(
+    nsCSSProps::ValueToKeywordEnum(display->mOverflowX,
+                                   nsCSSProps::kOverflowKTable));
+  if (display->mOverflowX == display->mOverflowY) {
+    return overflowX.forget();
+  }
+  RefPtr<nsDOMCSSValueList> valueList = GetROCSSValueList(false);
+  valueList->AppendCSSValue(overflowX.forget());
+
+  RefPtr<nsROCSSPrimitiveValue> overflowY= new nsROCSSPrimitiveValue;
+  overflowY->SetIdent(
+    nsCSSProps::ValueToKeywordEnum(display->mOverflowY,
+                                   nsCSSProps::kOverflowKTable));
+  valueList->AppendCSSValue(overflowY.forget());
+  return valueList.forget();
 }
 
 already_AddRefed<CSSValue>
 nsComputedDOMStyle::DoGetOverflowX()
 {
   RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
   val->SetIdent(
     nsCSSProps::ValueToKeywordEnum(StyleDisplay()->mOverflowX,
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@@ -4203,18 +4203,20 @@ var gCSSProperties = {
   },
   "overflow": {
     domProp: "overflow",
     inherited: false,
     type: CSS_TYPE_SHORTHAND_AND_LONGHAND,
     prerequisites: { "display": "block", "contain": "none" },
     subproperties: [ "overflow-x", "overflow-y" ],
     initial_values: [ "visible" ],
-    other_values: [ "auto", "scroll", "hidden", "-moz-hidden-unscrollable", "-moz-scrollbars-none" ],
-    invalid_values: []
+    other_values: [ "auto", "scroll", "hidden", "-moz-hidden-unscrollable", "-moz-scrollbars-none",
+                    "auto auto", "auto scroll", "hidden scroll", "auto hidden",
+                    "-moz-hidden-unscrollable -moz-hidden-unscrollable" ],
+    invalid_values: [ "-moz-hidden-unscrollable -moz-scrollbars-none" ]
   },
   "overflow-x": {
     domProp: "overflowX",
     inherited: false,
     type: CSS_TYPE_LONGHAND,
     // No applies_to_placeholder because we have a !important rule in forms.css.
     prerequisites: { "display": "block", "overflow-y": "visible", "contain": "none" },
     initial_values: [ "visible" ],
--- a/layout/style/test/test_bug319381.html
+++ b/layout/style/test/test_bug319381.html
@@ -38,51 +38,53 @@ for (i = 0; i < vals.length; ++i) {
   is($('t').style.overflow, vals[i], "Roundtrip");
   is(c(), vals[i], "Simple property set");
   isnot(cval(), null, "Simple property as CSSValue");
 }
 
 $('t').style.overflow = mozVals[0];
 is($('t').style.getPropertyValue("overflow-y"), "scroll", "Roundtrip");
 is($('t').style.getPropertyValue("overflow-x"), "hidden", "Roundtrip");
-is($('t').style.overflow, "", "Shorthand read directly");
-is(c(), "", "Shorthand computed");
-is(cval(), null, "Shorthand as CSSValue");
+is($('t').style.overflow, "hidden scroll", "Shorthand read directly");
+is(c(), "hidden scroll", "Shorthand computed");
+isnot(cval(), null, "Shorthand as CSSValue");
 
 $('t').style.overflow = mozVals[1];
 is($('t').style.getPropertyValue("overflow-x"), "scroll", "Roundtrip");
 is($('t').style.getPropertyValue("overflow-y"), "hidden", "Roundtrip");
-is($('t').style.overflow, "", "Shorthand read directly");
-is(c(), "", "Shorthand computed");
-is(cval(), null, "Shorthand as CSSValue");
+is($('t').style.overflow, "scroll hidden", "Shorthand read directly");
+is(c(), "scroll hidden", "Shorthand computed");
+isnot(cval(), null, "Shorthand as CSSValue");
 
 for (i = 0; i < vals.length; ++i) {
   for (j = 0; j < vals.length; ++j) {
     $('t').setAttribute("style",
                         "overflow-x: " + vals[i] + "; overflow-y: " + vals[j]);
     is($('t').style.getPropertyValue("overflow-x"), vals[i], "Roundtrip");
     is($('t').style.getPropertyValue("overflow-y"), vals[j], "Roundtrip");
 
     if (i == j) {
       is($('t').style.overflow, vals[i], "Shorthand serialization");
     } else {
-      is($('t').style.overflow, "", "Shorthand serialization");
+      is($('t').style.overflow, vals[i] + " " + vals[j], "Shorthand serialization");
     }
 
     // "visible" overflow-x and overflow-y become "auto" in computed style if
     // the other direction is not also "visible".
     if (i == j || (vals[i] == "visible" && vals[j] == "auto")) {
       is(c(), vals[j], "Shorthand computation");
       isnot(cval(), null, "Shorthand computation as CSSValue");
     } else if (vals[j] == "visible" && vals[i] == "auto") {
       is(c(), vals[i], "Shorthand computation");
-      isnot(cval(), null, "Shorthand computation as CSSValue");    
+      isnot(cval(), null, "Shorthand computation as CSSValue");
     } else {
-      is(c(), "", "Shorthand computation");
-      is(cval(), null, "Shorthand computation as CSSValue");
+      let x = vals[i] == "visible" ? "auto" : vals[i];
+      let y = vals[j] == "visible" ? "auto" : vals[j];
+      is(c(), x + " " + y, "Shorthand computation");
+      isnot(cval(), null, "Shorthand computation as CSSValue");
     }
   }
 }
 
 </script>
 </pre>
 </body>
 </html>
--- a/servo/components/style/properties/shorthand/box.mako.rs
+++ b/servo/components/style/properties/shorthand/box.mako.rs
@@ -1,16 +1,19 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
-<%helpers:shorthand name="overflow" sub_properties="overflow-x overflow-y"
-                    spec="https://drafts.csswg.org/css-overflow/#propdef-overflow">
+<%helpers:shorthand
+    name="overflow"
+    sub_properties="overflow-x overflow-y"
+    spec="https://drafts.csswg.org/css-overflow/#propdef-overflow"
+>
     use properties::longhands::overflow_x::parse as parse_overflow;
     % if product == "gecko":
         use properties::longhands::overflow_x::SpecifiedValue;
     % endif
 
     pub fn parse_value<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
@@ -37,30 +40,33 @@
                         })
                     }
                 }
             });
             if moz_kw_found.is_ok() {
                 return moz_kw_found
             }
         % endif
-        let overflow = parse_overflow(context, input)?;
+        let overflow_x = parse_overflow(context, input)?;
+        let overflow_y =
+            input.try(|i| parse_overflow(context, i)).unwrap_or(overflow_x);
         Ok(expanded! {
-            overflow_x: overflow,
-            overflow_y: overflow,
+            overflow_x: overflow_x,
+            overflow_y: overflow_y,
         })
     }
 
     impl<'a> ToCss for LonghandsToSerialize<'a>  {
         fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result where W: fmt::Write {
-            if self.overflow_x == self.overflow_y {
-                self.overflow_x.to_css(dest)
-            } else {
-                Ok(())
+            self.overflow_x.to_css(dest)?;
+            if self.overflow_x != self.overflow_y {
+                dest.write_char(' ')?;
+                self.overflow_y.to_css(dest)?;
             }
+            Ok(())
         }
     }
 </%helpers:shorthand>
 
 <%helpers:shorthand
     name="overflow-clip-box"
     sub_properties="overflow-clip-box-block overflow-clip-box-inline"
     enabled_in="ua"
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -313618,16 +313618,22 @@
     ]
    ],
    "css/css-multicol/multicol-gap-percentage-001.html": [
     [
      "/css/css-multicol/multicol-gap-percentage-001.html",
      {}
     ]
    ],
+   "css/css-overflow/overflow-shorthand-001.html": [
+    [
+     "/css/css-overflow/overflow-shorthand-001.html",
+     {}
+    ]
+   ],
    "css/css-position/position-sticky-bottom.html": [
     [
      "/css/css-position/position-sticky-bottom.html",
      {}
     ]
    ],
    "css/css-position/position-sticky-get-bounding-client-rect.html": [
     [
@@ -503525,16 +503531,20 @@
   "css/css-namespaces/syntax-016.xml": [
    "0cba1aed016d08e4706bffb8a4f4169c9cfd2108",
    "visual"
   ],
   "css/css-overflow/input-scrollable-region-001.html": [
    "f51bc673da28b0471018cdf945b4449ab00ce717",
    "reftest"
   ],
+  "css/css-overflow/overflow-shorthand-001.html": [
+   "6409ee499d3e853d8f4933f1b532e12ed9ab406b",
+   "testharness"
+  ],
   "css/css-overflow/reference/input-scrollable-region-001-ref.html": [
    "31e24bb1a2cb6f42703cc05e055fcb345c770a22",
    "support"
   ],
   "css/css-page/OWNERS": [
    "d4ee6029cc9c064e0e8b2c76becf3b59c4f7b62b",
    "support"
   ],
--- a/testing/web-platform/meta/css/cssom/index-002.html.ini
+++ b/testing/web-platform/meta/css/cssom/index-002.html.ini
@@ -21,17 +21,14 @@
     expected: FAIL
 
   [border is expected to be border-width: 1px; border-top-color: red;]
     expected: FAIL
 
   [border is expected to be border: dotted;]
     expected: FAIL
 
-  [overflow is expected to be overflow: scroll hidden;]
-    expected: FAIL
-
   [outline is expected to be outline: blue dotted 2px;]
     expected: FAIL
 
   [list is expected to be list-style: circle inside;]
     expected: FAIL
 
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-overflow/overflow-shorthand-001.html
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Overflow Test: Overflow longhand accepts two values</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" title="Emilio Cobos Álvarez <emilio@crisal.io>">
+<link rel="help" href="https://drafts.csswg.org/css-overflow/#propdef-overflow">
+<div id="test-div"></div>
+<script>
+let div = document.getElementById("test-div");
+function testOverflowShorthand(x, y) {
+  test(function() {
+    div.style.overflowX = x;
+    div.style.overflowY = y;
+
+    let expectedX = getComputedStyle(div).overflowX;
+    let expectedY = getComputedStyle(div).overflowY;
+    let expectedComputedSerialization = expectedX == expectedY ? expectedX : `${expectedX} ${expectedY}`;
+    let expectedSpecifiedSerialization = x == y ? x : `${x} ${y}`;
+
+    assert_equals(div.style.overflow, expectedSpecifiedSerialization);
+    assert_equals(getComputedStyle(div).overflow, expectedComputedSerialization);
+
+    div.style.overflowX = "";
+    div.style.overflowY = "";
+    assert_equals(div.style.overflow, "");
+
+    div.style.overflow = `${x} ${y}`;
+    assert_equals(div.style.overflow, expectedSpecifiedSerialization);
+    assert_equals(div.style.overflowX, x);
+    assert_equals(div.style.overflowY, y);
+    assert_equals(getComputedStyle(div).overflow, expectedComputedSerialization);
+    assert_equals(getComputedStyle(div).overflowX, expectedX);
+    assert_equals(getComputedStyle(div).overflowY, expectedY);
+  }, `overflow: ${x} ${y} works`);
+}
+
+let OVERFLOW_VALUES = [ "auto", "hidden", "scroll", "visible" ];
+for (let x of OVERFLOW_VALUES)
+  for (let y of OVERFLOW_VALUES)
+    testOverflowShorthand(x, y);
+</script>