Bug 1430622: Don't allow fallback alignment in place-content shorthand. r?xidorn,mats draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 15 Jan 2018 22:34:34 +0100
changeset 720863 316fefdd489cdbcac1de04995e4fa16b8ccdbbbc
parent 720621 d1aabb59379e5f7b81a84f140961bb05660ee52b
child 746174 3d6d96f8a0a4d7123f456b5c7812fdc4cfbb4b7c
push id95667
push userbmo:emilio@crisal.io
push dateTue, 16 Jan 2018 10:53:50 +0000
reviewersxidorn, mats
bugs1430622
milestone59.0a1
Bug 1430622: Don't allow fallback alignment in place-content shorthand. r?xidorn,mats MozReview-Commit-ID: sALBFJeqvr
servo/components/style/properties/shorthand/position.mako.rs
servo/components/style/values/specified/align.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-002.html.ini
testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-004.html.ini
testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-006.html.ini
testing/web-platform/tests/css/css-align/content-distribution/place-content-shorthand-007-ref.html
testing/web-platform/tests/css/css-align/content-distribution/place-content-shorthand-007.html
--- a/servo/components/style/properties/shorthand/position.mako.rs
+++ b/servo/components/style/properties/shorthand/position.mako.rs
@@ -606,27 +606,29 @@
             Ok(())
         }
     }
 </%helpers:shorthand>
 
 <%helpers:shorthand name="place-content" sub_properties="align-content justify-content"
                     spec="https://drafts.csswg.org/css-align/#propdef-place-content"
                     products="gecko">
-    use properties::longhands::align_content;
-    use properties::longhands::justify_content;
+    use values::specified::align::{AlignJustifyContent, FallbackAllowed};
 
-    pub fn parse_value<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>)
-                               -> Result<Longhands, ParseError<'i>> {
-        let align = align_content::parse(context, input)?;
+    pub fn parse_value<'i, 't>(
+        _: &ParserContext,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<Longhands, ParseError<'i>> {
+        let align = AlignJustifyContent::parse_with_fallback(input, FallbackAllowed::No)?;
         if align.has_extra_flags() {
             return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
         }
-        let justify = input.try(|input| justify_content::parse(context, input))
-                           .unwrap_or(justify_content::SpecifiedValue::from(align));
+        let justify =
+            input.try(|input| AlignJustifyContent::parse_with_fallback(input, FallbackAllowed::No))
+                .unwrap_or(align);
         if justify.has_extra_flags() {
             return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
         }
 
         Ok(expanded! {
             align_content: align,
             justify_content: justify,
         })
--- a/servo/components/style/values/specified/align.rs
+++ b/servo/components/style/values/specified/align.rs
@@ -114,16 +114,31 @@ const ALIGN_ALL_SHIFT: u32 = structs::NS
 /// <https://drafts.csswg.org/css-align/#content-distribution>
 ///
 /// The 16-bit field stores the primary value in its lower 8 bits, and the optional fallback value
 /// in its upper 8 bits.  This matches the representation of these properties in Gecko.
 #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq, ToComputedValue)]
 #[cfg_attr(feature = "servo", derive(Deserialize, Serialize))]
 pub struct AlignJustifyContent(u16);
 
+/// Whether fallback is allowed in align-content / justify-content parsing.
+///
+/// This is used for the place-content shorthand, until the resolutions from [1]
+/// are specified.
+///
+/// [1]: https://github.com/w3c/csswg-drafts/issues/1002
+#[derive(Clone, Copy, PartialEq)]
+pub enum FallbackAllowed {
+    /// Allow fallback alignment.
+    Yes,
+    /// Don't allow fallback alignment.
+    No,
+}
+
+
 impl AlignJustifyContent {
     /// The initial value 'normal'
     #[inline]
     pub fn normal() -> Self {
         Self::new(AlignFlags::NORMAL)
     }
 
     /// Construct a value with no fallback.
@@ -154,16 +169,48 @@ impl AlignJustifyContent {
             .expect("AlignJustifyContent must contain valid flags")
     }
 
     /// Whether this value has extra flags.
     #[inline]
     pub fn has_extra_flags(self) -> bool {
         self.primary().intersects(AlignFlags::FLAG_BITS) || self.fallback().intersects(AlignFlags::FLAG_BITS)
     }
+
+    /// Parse a value for align-content / justify-content, optionally allowing
+    /// fallback.
+    pub fn parse_with_fallback<'i, 't>(
+        input: &mut Parser<'i, 't>,
+        fallback_allowed: FallbackAllowed,
+    ) -> Result<Self, ParseError<'i>> {
+        // normal | <baseline-position>
+        if let Ok(value) = input.try(|input| parse_normal_or_baseline(input)) {
+            return Ok(AlignJustifyContent::new(value))
+        }
+
+        // <content-distribution> followed by optional <*-position>
+        if let Ok(value) = input.try(|input| parse_content_distribution(input)) {
+            if fallback_allowed == FallbackAllowed::Yes {
+                if let Ok(fallback) = input.try(|input| parse_overflow_content_position(input)) {
+                    return Ok(AlignJustifyContent::with_fallback(value, fallback))
+                }
+            }
+            return Ok(AlignJustifyContent::new(value))
+        }
+
+        // <*-position> followed by optional <content-distribution>
+        let fallback = parse_overflow_content_position(input)?;
+        if fallback_allowed == FallbackAllowed::Yes {
+            if let Ok(value) = input.try(|input| parse_content_distribution(input)) {
+                return Ok(AlignJustifyContent::with_fallback(value, fallback))
+            }
+        }
+
+        Ok(AlignJustifyContent::new(fallback))
+    }
 }
 
 impl ToCss for AlignJustifyContent {
     fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
         self.primary().to_css(dest)?;
         match self.fallback() {
             AlignFlags::AUTO => {}
             fallback => {
@@ -175,37 +222,17 @@ impl ToCss for AlignJustifyContent {
     }
 }
 
 
 impl Parse for AlignJustifyContent {
     // normal | <baseline-position> |
     // [ <content-distribution> || [ <overflow-position>? && <content-position> ] ]
     fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
-        // normal | <baseline-position>
-        if let Ok(value) = input.try(|input| parse_normal_or_baseline(input)) {
-            return Ok(AlignJustifyContent::new(value))
-        }
-
-        // <content-distribution> followed by optional <*-position>
-        if let Ok(value) = input.try(|input| parse_content_distribution(input)) {
-            if let Ok(fallback) = input.try(|input| parse_overflow_content_position(input)) {
-                return Ok(AlignJustifyContent::with_fallback(value, fallback))
-            }
-            return Ok(AlignJustifyContent::new(value))
-        }
-
-        // <*-position> followed by optional <content-distribution>
-        if let Ok(fallback) = input.try(|input| parse_overflow_content_position(input)) {
-            if let Ok(value) = input.try(|input| parse_content_distribution(input)) {
-                return Ok(AlignJustifyContent::with_fallback(value, fallback))
-            }
-            return Ok(AlignJustifyContent::new(fallback))
-        }
-        Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+        Self::parse_with_fallback(input, FallbackAllowed::Yes)
     }
 }
 
 /// Value of the `align-self` or `justify-self` property.
 ///
 /// <https://drafts.csswg.org/css-align/#self-alignment>
 #[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
 #[derive(Clone, Copy, Debug, Eq, PartialEq, ToComputedValue, ToCss)]
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -97750,16 +97750,28 @@
       [
        "/css/compositing/svg/reference/mix-blend-mode-svg-rectangle-ref.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-align/content-distribution/place-content-shorthand-007.html": [
+    [
+     "/css/css-align/content-distribution/place-content-shorthand-007.html",
+     [
+      [
+       "/css/css-align/content-distribution/place-content-shorthand-007-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-align/distribution-values/space-evenly-001.html": [
     [
      "/css/css-align/distribution-values/space-evenly-001.html",
      [
       [
        "/css/reference/ref-filled-green-100px-square.xht",
        "=="
       ]
@@ -231571,16 +231583,21 @@
      {}
     ]
    ],
    "css/compositing/text-with-svg-background-ref.html": [
     [
      {}
     ]
    ],
+   "css/css-align/content-distribution/place-content-shorthand-007-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/css-align/reference/ttwf-reftest-alignContent-ref.html": [
     [
      {}
     ]
    ],
    "css/css-align/resources/alignment-parsing-utils.js": [
     [
      {}
@@ -468884,16 +468901,24 @@
   "css/css-align/content-distribution/place-content-shorthand-005.html": [
    "6b3d7b9ae7d5b28510385cccaaade09268409cab",
    "testharness"
   ],
   "css/css-align/content-distribution/place-content-shorthand-006.html": [
    "016c2ff7902fc01d8368645b7177e3932aa64d42",
    "testharness"
   ],
+  "css/css-align/content-distribution/place-content-shorthand-007-ref.html": [
+   "6008fc2ca235c4b4c171ace405fd312647be3313",
+   "support"
+  ],
+  "css/css-align/content-distribution/place-content-shorthand-007.html": [
+   "5268627f55928b969e022a626961958ac8f92e05",
+   "reftest"
+  ],
   "css/css-align/default-alignment/justify-items-legacy-001.html": [
    "bcf17f709a9b87ef728262b658d1dfa65afc93bb",
    "testharness"
   ],
   "css/css-align/default-alignment/place-items-shorthand-001.html": [
    "cc69bbbee852e6cd203d3f39dac2a1e05a428361",
    "testharness"
   ],
@@ -493445,17 +493470,17 @@
    "a736f68dc602c0fccab56ec5cc6234cb3298c88d",
    "support"
   ],
   "css/css-scoping/shadow-cascade-order-001.html": [
    "46913ea7e47811b11be898de5c3bd0a330ea6637",
    "testharness"
   ],
   "css/css-scoping/slotted-invalidation.html": [
-   "92dadd5f67eb4305008db8429674f71c1a972763",
+   "b22e8258671a8709a3ce6fdc42501b43b866e946",
    "testharness"
   ],
   "css/css-scoping/slotted-parsing.html": [
    "6bac5b15011d7177a40f7ca3e3c5f7e410643920",
    "testharness"
   ],
   "css/css-scoping/slotted-with-pseudo-element-ref.html": [
    "48561a3dff973b7ad1bfa9702461e50fd4a67c2d",
@@ -582601,37 +582626,37 @@
    "b11caf0a1766818a168a7f91b01ccd6ae9a7e4f0",
    "testharness"
   ],
   "web-animations/interfaces/DocumentTimeline/idlharness.html": [
    "72cb7900f86611e9c2a1b0f4acd0f634555310b9",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/composite.html": [
-   "7dd18327d8da81914adaf443086891ba3646d882",
+   "12fc2e8e7bcfb1eab6e162b68731ff6fcb767438",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/constructor.html": [
-   "4a80ea073da0a9c62dcb9587676445a2fba234e1",
+   "2f6449cbf2b47ae457efb23fb52b8fd1709837ac",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/copy-constructor.html": [
    "6ef462ddc696269f132d596188ffd5e8da1e1164",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/idlharness.html": [
    "f05c9bd1cdee77ff6be143b0eb4f982c7218908b",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/iterationComposite.html": [
    "65cd746596a6770d1101b030769712be433bf6f3",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-001.html": [
-   "165b651cea12ab9e0825f4335e7f697ce1fc6247",
+   "f54c7c0da5728f88f37a067761af7ad815fea005",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/processing-a-keyframes-argument-002.html": [
    "e9237e244034845f6f902f8149a0e66e5b6164f2",
    "testharness"
   ],
   "web-animations/interfaces/KeyframeEffect/setKeyframes.html": [
    "a346e0e004010a6f51e06ffd30d0b6eddd45421d",
@@ -582645,17 +582670,17 @@
    "77f747cef865fd5eba6ea621881706f801c812c0",
    "support"
   ],
   "web-animations/resources/effect-tests.js": [
    "2eb26f4cb0e65282b8e82014ac8ebe87a4209c6a",
    "support"
   ],
   "web-animations/resources/keyframe-tests.js": [
-   "52ffc50c4ebf0326db8f4e1d0cc1234f6c860dc2",
+   "b31029042fdfa77ba8bf0e9370f63a423fbe0da9",
    "support"
   ],
   "web-animations/resources/keyframe-utils.js": [
    "08da0c81847809328bda0d6e0581711f7838916e",
    "support"
   ],
   "web-animations/resources/xhr-doc.py": [
    "de68c45fc1d38a49946f9046f34031e9278a1531",
--- a/testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-002.html.ini
+++ b/testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-002.html.ini
@@ -81,232 +81,8 @@
     expected: FAIL
 
   [Checking place-content: first baseline last baseline]
     expected: FAIL
 
   [Checking place-content: last baseline first baseline]
     expected: FAIL
 
-  [Checking place-content: start stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch left]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch right]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch center]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch flex-start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: stretch flex-end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around left]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around right]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around center]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around flex-start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-around flex-end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between left]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between right]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between center]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between flex-start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-between flex-end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly end]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly left]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly right]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly center]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly flex-start]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: space-evenly flex-end]
-    expected:
-      if stylo: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-004.html.ini
+++ /dev/null
@@ -1,5 +0,0 @@
-[place-content-shorthand-004.html]
-  [Verify fallback values are invalid]
-    expected:
-      if stylo: FAIL
-
--- a/testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-006.html.ini
+++ b/testing/web-platform/meta/css/css-align/content-distribution/place-content-shorthand-006.html.ini
@@ -84,120 +84,8 @@
     expected: FAIL
 
   [Checking place-content: first baseline last baseline]
     expected: FAIL
 
   [Checking place-content: last baseline first baseline]
     expected: FAIL
 
-  [Checking place-content: start stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: start space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: end space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: left space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: right space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: center space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-start space-evenly]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end stretch]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-around]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-between]
-    expected:
-      if stylo: FAIL
-
-  [Checking place-content: flex-end space-evenly]
-    expected:
-      if stylo: FAIL
-
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-align/content-distribution/place-content-shorthand-007-ref.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<style>
+div {
+  width: 400px;
+  height: 400px;
+  background: blue;
+  position: relative;
+}
+span {
+  background: green;
+  width: 200px;
+  height: 200px;
+  position: absolute;
+  bottom: 0;
+  left: 100px;
+}
+</style>
+Should see a green square centered and at the bottom of the blue square.
+<div><span></span></div>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-align/content-distribution/place-content-shorthand-007.html
@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>CSS Box Alignment: place-content shorthand with fallback</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-align/#propdef-place-content">
+<link rel="help" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1430622">
+<link rel="match" href="place-content-shorthand-007-ref.html">
+<style>
+div {
+  display: grid;
+  grid: 200px / 200px;
+  width: 400px;
+  height: 400px;
+  background: blue;
+  place-content: end space-evenly;
+}
+span {
+  background: green;
+}
+</style>
+Should see a green square centered and at the bottom of the blue square.
+<div><span></span></div>