Bug 1422225: Allow parsing operators in media feature expressions. r?xidorn draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 15 Jun 2018 21:23:50 -0700
changeset 809282 bcca03533116e0ee752e4d8a54263085aa599976
parent 809247 c255e861f0a7fffb57866406010108b6bf7f8268
child 809283 632611f94be481742bfd821955de113b53635f4c
push id113617
push userbmo:emilio@crisal.io
push dateThu, 21 Jun 2018 17:44:14 +0000
reviewersxidorn
bugs1422225
milestone62.0a1
Bug 1422225: Allow parsing operators in media feature expressions. r?xidorn The only bit from the spec which I haven't implemented to my knowledge is the bit that allows you to swap the position of the media feature and the value, because it unnecessarily complicates parsing (we parse the value in terms of the feature), and I don't think it's useful given how easy it is to switch from, e.g., `(500px > width)` to `(width <= 500px)`. I filed https://github.com/w3c/csswg-drafts/issues/2791 about it. MozReview-Commit-ID: 6xrdVl87S9X
servo/components/style/gecko/media_queries.rs
--- a/servo/components/style/gecko/media_queries.rs
+++ b/servo/components/style/gecko/media_queries.rs
@@ -2,17 +2,17 @@
  * 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/. */
 
 //! Gecko's media-query device and expression representation.
 
 use app_units::AU_PER_PX;
 use app_units::Au;
 use context::QuirksMode;
-use cssparser::{BasicParseErrorKind, Parser, RGBA};
+use cssparser::{BasicParseErrorKind, Parser, RGBA, Token};
 use euclid::Size2D;
 use euclid::TypedScale;
 use gecko::values::{convert_nscolor_to_rgba, convert_rgba_to_nscolor};
 use gecko_bindings::bindings;
 use gecko_bindings::structs;
 use gecko_bindings::structs::{nsCSSKTableEntry, nsCSSKeyword, nsCSSUnit, nsCSSValue};
 use gecko_bindings::structs::{nsMediaFeature, nsMediaFeature_RangeType};
 use gecko_bindings::structs::{nsMediaFeature_ValueType, nsPresContext};
@@ -229,64 +229,106 @@ impl Device {
 
 /// The kind of matching that should be performed on a media feature value.
 #[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq)]
 pub enum Range {
     /// At least the specified value.
     Min,
     /// At most the specified value.
     Max,
-    /// Exactly the specified value.
+}
+
+/// The operator that was specified in this media feature.
+#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq)]
+enum Operator {
     Equal,
+    GreaterThan,
+    GreaterThanEqual,
+    LessThan,
+    LessThanEqual,
+}
+
+impl ToCss for Operator {
+    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: fmt::Write,
+    {
+        dest.write_str(match *self {
+            Operator::Equal => "=",
+            Operator::LessThan => "<",
+            Operator::LessThanEqual => "<=",
+            Operator::GreaterThan => ">",
+            Operator::GreaterThanEqual => ">=",
+        })
+    }
+}
+
+/// Either a `Range` or an `Operator`.
+///
+/// Ranged media features are not allowed with operations (that'd make no
+/// sense).
+#[derive(Clone, Copy, Debug, Eq, MallocSizeOf, PartialEq)]
+enum RangeOrOperator {
+    Range(Range),
+    Operator(Operator),
 }
 
 /// A expression for gecko contains a reference to the media feature, the value
 /// the media query contained, and the range to evaluate.
 #[derive(Clone, Debug, MallocSizeOf)]
 pub struct Expression {
     feature: &'static nsMediaFeature,
     value: Option<MediaExpressionValue>,
-    range: Range,
+    range_or_operator: Option<RangeOrOperator>,
 }
 
 impl ToCss for Expression {
     fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
     where
         W: fmt::Write,
     {
         dest.write_str("(")?;
 
         if (self.feature.mReqFlags & structs::nsMediaFeature_RequirementFlags_eHasWebkitPrefix) != 0
         {
             dest.write_str("-webkit-")?;
         }
-        match self.range {
-            Range::Min => dest.write_str("min-")?,
-            Range::Max => dest.write_str("max-")?,
-            Range::Equal => {},
+
+        if let Some(RangeOrOperator::Range(range)) = self.range_or_operator {
+            match range {
+                Range::Min => dest.write_str("min-")?,
+                Range::Max => dest.write_str("max-")?,
+            }
         }
 
         // NB: CssStringWriter not needed, feature names are under control.
         write!(dest, "{}", unsafe {
             Atom::from_static(*self.feature.mName)
         })?;
 
+        if let Some(RangeOrOperator::Operator(op)) = self.range_or_operator {
+            dest.write_char(' ')?;
+            op.to_css(dest)?;
+            dest.write_char(' ')?;
+        } else if self.value.is_some() {
+            dest.write_str(": ")?;
+        }
+
         if let Some(ref val) = self.value {
-            dest.write_str(": ")?;
             val.to_css(dest, self)?;
         }
 
         dest.write_str(")")
     }
 }
 
 impl PartialEq for Expression {
     fn eq(&self, other: &Expression) -> bool {
         self.feature.mName == other.feature.mName && self.value == other.value &&
-            self.range == other.range
+            self.range_or_operator == other.range_or_operator
     }
 }
 
 /// A value found or expected in a media expression.
 ///
 /// FIXME(emilio): How should calc() serialize in the Number / Integer /
 /// BoolInteger / IntRatio case, as computed or as specified value?
 ///
@@ -511,27 +553,63 @@ fn parse_feature_value<'i, 't>(
         nsMediaFeature_ValueType::eIdent => {
             MediaExpressionValue::Ident(Atom::from(input.expect_ident()?.as_ref()))
         },
     };
 
     Ok(value)
 }
 
+/// Consumes an operation or a colon, or returns an error.
+fn consume_operation_or_colon(
+    input: &mut Parser,
+) -> Result<Option<Operator>, ()> {
+    let first_delim = {
+        let next_token = match input.next() {
+            Ok(t) => t,
+            Err(..) => return Err(()),
+        };
+
+        match *next_token {
+            Token::Colon => return Ok(None),
+            Token::Delim(oper) => oper,
+            _ => return Err(()),
+        }
+    };
+    Ok(Some(match first_delim {
+        '=' => Operator::Equal,
+        '>' => {
+            if input.try(|i| i.expect_delim('=')).is_ok() {
+                Operator::GreaterThanEqual
+            } else {
+                Operator::GreaterThan
+            }
+        }
+        '<' => {
+            if input.try(|i| i.expect_delim('=')).is_ok() {
+                Operator::LessThanEqual
+            } else {
+                Operator::LessThan
+            }
+        }
+        _ => return Err(()),
+    }))
+}
+
 impl Expression {
     /// Trivially construct a new expression.
     fn new(
         feature: &'static nsMediaFeature,
         value: Option<MediaExpressionValue>,
-        range: Range,
+        range_or_operator: Option<RangeOrOperator>,
     ) -> Self {
         Self {
             feature,
             value,
-            range,
+            range_or_operator,
         }
     }
 
     /// Parse a media expression of the form:
     ///
     /// ```
     /// (media-feature: media-value)
     /// ```
@@ -582,22 +660,22 @@ impl Expression {
                             structs::StaticPrefs_sVarCache_layout_css_prefixes_device_pixel_ratio_webkit
                         } {
                             flags |= structs::nsMediaFeature_RequirementFlags_eWebkitDevicePixelRatioPrefEnabled;
                         }
                     }
 
                     let range = if starts_with_ignore_ascii_case(feature_name, "min-") {
                         feature_name = &feature_name[4..];
-                        Range::Min
+                        Some(Range::Min)
                     } else if starts_with_ignore_ascii_case(feature_name, "max-") {
                         feature_name = &feature_name[4..];
-                        Range::Max
+                        Some(Range::Max)
                     } else {
-                        Range::Equal
+                        None
                     };
 
                     let atom = Atom::from(string_as_ascii_lowercase(feature_name));
                     match find_feature(|f| atom.as_ptr() == unsafe { *f.mName as *mut _ }) {
                         Some(f) => Ok((f, range)),
                         None => Err(()),
                     }
                 };
@@ -615,44 +693,83 @@ impl Expression {
                 }
 
                 if (feature.mReqFlags & !flags) != 0 {
                     return Err(location.new_custom_error(
                         StyleParseErrorKind::MediaQueryExpectedFeatureName(ident.clone()),
                     ));
                 }
 
-                if range != Range::Equal &&
+                if range.is_some() &&
                     feature.mRangeType != nsMediaFeature_RangeType::eMinMaxAllowed
                 {
                     return Err(location.new_custom_error(
                         StyleParseErrorKind::MediaQueryExpectedFeatureName(ident.clone()),
                     ));
                 }
             }
 
-            // If there's no colon, this is a media query of the form
-            // '(<feature>)', that is, there's no value specified.
-            //
-            // Gecko doesn't allow ranged expressions without a value, so just
-            // reject them here too.
-            if input.try(|i| i.expect_colon()).is_err() {
-                if range != Range::Equal {
-                    return Err(input.new_custom_error(StyleParseErrorKind::RangedExpressionWithNoValue));
+            let feature_allows_ranges =
+                feature.mRangeType == nsMediaFeature_RangeType::eMinMaxAllowed;
+
+            let operator = input.try(consume_operation_or_colon);
+            let operator = match operator {
+                Err(..) => {
+                    // If there's no colon, this is a media query of the
+                    // form '(<feature>)', that is, there's no value
+                    // specified.
+                    //
+                    // Gecko doesn't allow ranged expressions without a
+                    // value, so just reject them here too.
+                    if range.is_some() {
+                        return Err(input.new_custom_error(
+                            StyleParseErrorKind::RangedExpressionWithNoValue
+                        ));
+                    }
+
+                    return Ok(Expression::new(
+                        feature,
+                        None,
+                        None,
+                    ));
                 }
-                return Ok(Expression::new(feature, None, range));
-            }
+                Ok(operator) => operator,
+            };
+
+            let range_or_operator = match range {
+                Some(range) => {
+                    if operator.is_some() {
+                        return Err(input.new_custom_error(
+                            StyleParseErrorKind::MediaQueryExpectedFeatureValue
+                        ));
+                    }
+                    Some(RangeOrOperator::Range(range))
+                }
+                None => {
+                    match operator {
+                        Some(operator) => {
+                            if !feature_allows_ranges {
+                                return Err(input.new_custom_error(
+                                    StyleParseErrorKind::MediaQueryExpectedFeatureValue
+                                ));
+                            }
+                            Some(RangeOrOperator::Operator(operator))
+                        }
+                        None => None,
+                    }
+                }
+            };
 
             let value =
                 parse_feature_value(feature, feature.mValueType, context, input).map_err(|err| {
                     err.location
                         .new_custom_error(StyleParseErrorKind::MediaQueryExpectedFeatureValue)
                 })?;
 
-            Ok(Expression::new(feature, Some(value), range))
+            Ok(Expression::new(feature, Some(value), range_or_operator))
         })
     }
 
     /// Returns whether this media query evaluates to true for the given device.
     pub fn matches(&self, device: &Device, quirks_mode: QuirksMode) -> bool {
         let mut css_value = nsCSSValue::null();
         unsafe {
             (self.feature.mGetter.unwrap())(
@@ -678,18 +795,18 @@ impl Expression {
         device: &Device,
         actual_value: &MediaExpressionValue,
         quirks_mode: QuirksMode,
     ) -> bool {
         use self::MediaExpressionValue::*;
         use std::cmp::Ordering;
 
         debug_assert!(
-            self.range == Range::Equal ||
-                self.feature.mRangeType == nsMediaFeature_RangeType::eMinMaxAllowed,
+            self.feature.mRangeType == nsMediaFeature_RangeType::eMinMaxAllowed ||
+            self.range_or_operator.is_none(),
             "Whoops, wrong range"
         );
 
         // http://dev.w3.org/csswg/mediaqueries3/#units
         // em units are relative to the initial font-size.
         let required_value = match self.value {
             Some(ref v) => v,
             None => {
@@ -704,17 +821,17 @@ impl Expression {
                         |context| l.to_computed_value(&context).px() != 0.,
                     ),
                     _ => true,
                 };
             },
         };
 
         // FIXME(emilio): Handle the possible floating point errors?
-        let cmp = match (required_value, actual_value) {
+        let cmp = match (actual_value, required_value) {
             (&Length(ref one), &Length(ref other)) => {
                 computed::Context::for_media_query_evaluation(device, quirks_mode, |context| {
                     one.to_computed_value(&context)
                         .to_i32_au()
                         .cmp(&other.to_computed_value(&context).to_i32_au())
                 })
             },
             (&Integer(one), &Integer(ref other)) => one.cmp(other),
@@ -724,21 +841,21 @@ impl Expression {
                 // Extend to avoid overflow.
                 (one_num as u64 * other_den as u64).cmp(&(other_num as u64 * one_den as u64))
             },
             (&Resolution(ref one), &Resolution(ref other)) => {
                 let actual_dpi = unsafe {
                     if (*device.pres_context).mOverrideDPPX > 0.0 {
                         self::Resolution::Dppx((*device.pres_context).mOverrideDPPX).to_dpi()
                     } else {
-                        other.to_dpi()
+                        one.to_dpi()
                     }
                 };
 
-                one.to_dpi().partial_cmp(&actual_dpi).unwrap()
+                actual_dpi.partial_cmp(&other.to_dpi()).unwrap()
             },
             (&Ident(ref one), &Ident(ref other)) => {
                 debug_assert_ne!(
                     self.feature.mRangeType,
                     nsMediaFeature_RangeType::eMinMaxAllowed
                 );
                 return one == other;
             },
@@ -747,15 +864,36 @@ impl Expression {
                     self.feature.mRangeType,
                     nsMediaFeature_RangeType::eMinMaxAllowed
                 );
                 return one == other;
             },
             _ => unreachable!(),
         };
 
-        cmp == Ordering::Equal || match self.range {
-            Range::Min => cmp == Ordering::Less,
-            Range::Equal => false,
-            Range::Max => cmp == Ordering::Greater,
+        let range_or_op = match self.range_or_operator {
+            Some(r) => r,
+            None => return cmp == Ordering::Equal,
+        };
+
+        match range_or_op {
+            RangeOrOperator::Range(range) => {
+                cmp == Ordering::Equal || match range {
+                    Range::Min => cmp == Ordering::Greater,
+                    Range::Max => cmp == Ordering::Less,
+                }
+            }
+            RangeOrOperator::Operator(op) => {
+                match op {
+                    Operator::Equal => cmp == Ordering::Equal,
+                    Operator::GreaterThan => cmp == Ordering::Greater,
+                    Operator::GreaterThanEqual => {
+                        cmp == Ordering::Equal || cmp == Ordering::Greater
+                    }
+                    Operator::LessThan => cmp == Ordering::Less,
+                    Operator::LessThanEqual => {
+                        cmp == Ordering::Equal || cmp == Ordering::Less
+                    }
+                }
+            }
         }
     }
 }