style: Restrict <baseline-position> and <content-position> depending on the axis in content distribution properties. draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 24 Jan 2018 13:23:19 +0100
changeset 747863 e99521508635c48527df4f7ffe887f8444cc8f23
parent 747862 e33105723cb17b21533073b5a199971ca5d7b2ac
child 747864 50b571cca1587aa4c7a4ec3c7bb50427801cfec0
push id97026
push userbmo:emilio@crisal.io
push dateFri, 26 Jan 2018 23:13:24 +0000
bugs1430817
milestone60.0a1
style: Restrict <baseline-position> and <content-position> depending on the axis in content distribution properties. This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1430817, and updates us to the current version of the css-align spec. MozReview-Commit-ID: LtBcdqYJeK
servo/components/style/properties/shorthand/position.mako.rs
servo/components/style/values/specified/align.rs
--- a/servo/components/style/properties/shorthand/position.mako.rs
+++ b/servo/components/style/properties/shorthand/position.mako.rs
@@ -629,21 +629,29 @@
             )?;
 
         let justify_content = input.try(|input| {
             ContentDistribution::parse(
                 input,
                 FallbackAllowed::No,
                 AxisDirection::Block,
             )
-        }).unwrap_or(align_content);
+        });
 
-        // NOTE(emilio): align-content parsing is more restrictive than
-        // justify-content parsing, so no need to do any extra check here for
-        // that.
+        let justify_content = match justify_content {
+            Ok(v) => v,
+            Err(err) => {
+                if !align_content.is_valid_on_both_axes() {
+                    return Err(err);
+                }
+
+                align_content
+            }
+        };
+
         if align_content.has_extra_flags() || justify_content.has_extra_flags() {
             return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
         }
 
         Ok(expanded! {
             align_content: AlignContent(align_content),
             justify_content: JustifyContent(justify_content),
         })
--- a/servo/components/style/values/specified/align.rs
+++ b/servo/components/style/values/specified/align.rs
@@ -173,16 +173,33 @@ impl ContentDistribution {
         Self::with_fallback(primary, fallback)
     }
 
     fn as_bits(&self) -> u16 {
         self.primary.bits() as u16 |
         ((self.fallback.bits() as u16) << ALIGN_ALL_SHIFT)
     }
 
+    /// Returns whether this value is valid for both axis directions.
+    pub fn is_valid_on_both_axes(&self) -> bool {
+        if self.primary.intersects(AlignFlags::BASELINE | AlignFlags::LAST_BASELINE) ||
+            self.fallback.intersects(AlignFlags::BASELINE | AlignFlags::LAST_BASELINE) {
+            // <baseline-position> is only allowed on the block axis.
+            return false;
+        }
+
+        if self.primary.intersects(AlignFlags::LEFT | AlignFlags::RIGHT) ||
+            self.fallback.intersects(AlignFlags::LEFT | AlignFlags::RIGHT) {
+            // left | right are only allowed on the inline axis.
+            return false;
+        }
+
+        true
+    }
+
     /// The primary alignment
     #[inline]
     pub fn primary(self) -> AlignFlags {
         self.primary
     }
 
     /// The fallback alignment
     #[inline]
@@ -197,35 +214,42 @@ impl ContentDistribution {
         self.fallback().intersects(AlignFlags::FLAG_BITS)
     }
 
     /// Parse a value for align-content / justify-content, optionally allowing
     /// fallback.
     pub fn parse<'i, 't>(
         input: &mut Parser<'i, 't>,
         fallback_allowed: FallbackAllowed,
-        _axis: AxisDirection,
+        axis: AxisDirection,
     ) -> Result<Self, ParseError<'i>> {
-        // normal | <baseline-position>
-        if let Ok(value) = input.try(|input| parse_normal_or_baseline(input)) {
-            return Ok(ContentDistribution::new(value))
+        // Try to parse normal first
+        if input.try(|i| i.expect_ident_matching("normal")).is_ok() {
+            return Ok(ContentDistribution::normal());
+        }
+
+        // Parse <baseline-position>, but only on the block axis.
+        if axis == AxisDirection::Block {
+            if let Ok(value) = input.try(parse_baseline) {
+                return Ok(ContentDistribution::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)) {
+                if let Ok(fallback) = input.try(|input| parse_overflow_content_position(input, axis)) {
                     return Ok(ContentDistribution::with_fallback(value, fallback))
                 }
             }
             return Ok(ContentDistribution::new(value))
         }
 
         // <*-position> followed by optional <content-distribution>
-        let fallback = parse_overflow_content_position(input)?;
+        let fallback = parse_overflow_content_position(input, axis)?;
         if fallback_allowed == FallbackAllowed::Yes {
             if let Ok(value) = input.try(|input| parse_content_distribution(input)) {
                 return Ok(ContentDistribution::with_fallback(value, fallback))
             }
         }
 
         Ok(ContentDistribution::new(fallback))
     }
@@ -330,17 +354,16 @@ impl SelfAlignment {
 
     /// Whether this value has extra flags.
     #[inline]
     pub fn has_extra_flags(self) -> bool {
         self.0.intersects(AlignFlags::FLAG_BITS)
     }
 }
 
-
 impl Parse for SelfAlignment {
     // auto | normal | stretch | <baseline-position> |
     // [ <overflow-position>? && <self-position> ]
     fn parse<'i, 't>(_: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {
         // auto | normal | stretch | <baseline-position>
         if let Ok(value) = input.try(parse_auto_normal_stretch_baseline) {
             return Ok(SelfAlignment(value))
         }
@@ -452,29 +475,18 @@ fn parse_normal_stretch_baseline<'i, 't>
     }
 
     try_match_ident_ignore_ascii_case! { input,
         "normal" => Ok(AlignFlags::NORMAL),
         "stretch" => Ok(AlignFlags::STRETCH),
     }
 }
 
-// normal | <baseline-position>
-fn parse_normal_or_baseline<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
-    if let Ok(baseline) = input.try(parse_baseline) {
-        return Ok(baseline);
-    }
-
-    input.expect_ident_matching("normal")?;
-    Ok(AlignFlags::NORMAL)
-}
-
 // <baseline-position>
 fn parse_baseline<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
-    // FIXME: remove clone() when lifetimes are non-lexical
     try_match_ident_ignore_ascii_case! { input,
         "baseline" => Ok(AlignFlags::BASELINE),
         "first" => {
             input.expect_ident_matching("baseline")?;
             Ok(AlignFlags::BASELINE)
         }
         "last" => {
             input.expect_ident_matching("baseline")?;
@@ -489,43 +501,47 @@ fn parse_content_distribution<'i, 't>(in
         "stretch" => Ok(AlignFlags::STRETCH),
         "space-between" => Ok(AlignFlags::SPACE_BETWEEN),
         "space-around" => Ok(AlignFlags::SPACE_AROUND),
         "space-evenly" => Ok(AlignFlags::SPACE_EVENLY),
     }
 }
 
 // [ <overflow-position>? && <content-position> ]
-fn parse_overflow_content_position<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
+fn parse_overflow_content_position<'i, 't>(
+    input: &mut Parser<'i, 't>,
+    axis: AxisDirection,
+) -> Result<AlignFlags, ParseError<'i>> {
     // <content-position> followed by optional <overflow-position>
-    if let Ok(mut content) = input.try(parse_content_position) {
+    if let Ok(mut content) = input.try(|input| parse_content_position(input, axis)) {
         if let Ok(overflow) = input.try(parse_overflow_position) {
             content |= overflow;
         }
         return Ok(content)
     }
+
     // <overflow-position> followed by required <content-position>
-    if let Ok(overflow) = parse_overflow_position(input) {
-        if let Ok(content) = parse_content_position(input) {
-            return Ok(overflow | content)
-        }
-    }
-    return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError))
+    let overflow = parse_overflow_position(input)?;
+    let content = parse_content_position(input, axis)?;
+    Ok(overflow | content)
 }
 
 // <content-position>
-fn parse_content_position<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
+fn parse_content_position<'i, 't>(
+    input: &mut Parser<'i, 't>,
+    axis: AxisDirection,
+) -> Result<AlignFlags, ParseError<'i>> {
     try_match_ident_ignore_ascii_case! { input,
         "start" => Ok(AlignFlags::START),
         "end" => Ok(AlignFlags::END),
         "flex-start" => Ok(AlignFlags::FLEX_START),
         "flex-end" => Ok(AlignFlags::FLEX_END),
         "center" => Ok(AlignFlags::CENTER),
-        "left" => Ok(AlignFlags::LEFT),
-        "right" => Ok(AlignFlags::RIGHT),
+        "left" if axis == AxisDirection::Inline => Ok(AlignFlags::LEFT),
+        "right" if axis == AxisDirection::Inline => Ok(AlignFlags::RIGHT),
     }
 }
 
 // <overflow-position>
 fn parse_overflow_position<'i, 't>(input: &mut Parser<'i, 't>) -> Result<AlignFlags, ParseError<'i>> {
     try_match_ident_ignore_ascii_case! { input,
         "safe" => Ok(AlignFlags::SAFE),
         "unsafe" => Ok(AlignFlags::UNSAFE),