Preserve nested media queries when they can't be merged (#410)

See sass/sass#1831

CHANGELOG.md

@@ -1,4 +1,7 @@
-## 1.9.3
+## 1.10.0
+
+* When two `@media` rules' queries can't be merged, leave nested rules in place
+  for browsers that support them.
 
 * Fix a typo in an error message.
 

lib/src/ast/css/media_query.dart

@@ -45,55 +45,94 @@ class CssMediaQuery {
 
   /// Merges this with [other] to return a query that matches the intersection
   /// of both inputs.
-  CssMediaQuery merge(CssMediaQuery other) {
+  MediaQueryMergeResult merge(CssMediaQuery other) {
     var ourModifier = this.modifier?.toLowerCase();
     var ourType = this.type?.toLowerCase();
     var theirModifier = other.modifier?.toLowerCase();
     var theirType = other.type?.toLowerCase();
 
     if (ourType == null && theirType == null) {
-      return new CssMediaQuery.condition(
-          features.toList()..addAll(other.features));
+      return new MediaQuerySuccessfulMergeResult._(new CssMediaQuery.condition(
+          this.features.toList()..addAll(other.features)));
     }
 
     String modifier;
     String type;
+    List<String> features;
     if ((ourModifier == 'not') != (theirModifier == 'not')) {
-      if (ourType == theirType) return null;
+      if (ourType == theirType) {
+        var negativeFeatures =
+            ourModifier == 'not' ? this.features : other.features;
+        var positiveFeatures =
+            ourModifier == 'not' ? other.features : this.features;
+
+        // If the negative features are a subset of the positive features, the
+        // query is empty. For example, `not screen and (color)` has no
+        // intersection with `screen and (color) and (grid)`.
+        //
+        // However, `not screen and (color)` *does* intersect with `screen and
+        // (grid)`, because it means `not (screen and (color))` and so it allows
+        // a screen with no color but with a grid.
+        if (negativeFeatures.every(positiveFeatures.contains)) {
+          return MediaQueryMergeResult.empty;
+        } else {
+          return MediaQueryMergeResult.unrepresentable;
+        }
+      } else if (ourType == null || theirType == null) {
+        return MediaQueryMergeResult.unrepresentable;
+      }
 
       if (ourModifier == 'not') {
-        // The "not" would apply to the other query's features, which is not
-        // what we want.
-        if (other.features.isNotEmpty) return null;
         modifier = theirModifier;
         type = theirType;
+        features = other.features;
       } else {
-        if (this.features.isNotEmpty) return null;
         modifier = ourModifier;
         type = ourType;
+        features = this.features;
       }
     } else if (ourModifier == 'not') {
       assert(theirModifier == 'not');
       // CSS has no way of representing "neither screen nor print".
-      if (ourType == theirType) return null;
-      modifier = ourModifier; // "not"
-      type = ourType;
+      if (ourType != theirType) return MediaQueryMergeResult.unrepresentable;
+
+      var moreFeatures = this.features.length > other.features.length
+          ? this.features
+          : other.features;
+      var fewerFeatures = this.features.length > other.features.length
+          ? other.features
+          : this.features;
+
+      // If one set of features is a superset of the other, use those features
+      // because they're strictly narrower.
+      if (fewerFeatures.every(moreFeatures.contains)) {
+        modifier = ourModifier; // "not"
+        type = ourType;
+        features = moreFeatures;
+      } else {
+        // Otherwise, there's no way to represent the intersection.
+        return MediaQueryMergeResult.unrepresentable;
+      }
     } else if (ourType == null) {
       modifier = theirModifier;
       type = theirType;
+      features = this.features.toList()..addAll(other.features);
     } else if (theirType == null) {
       modifier = ourModifier;
       type = ourType;
+      features = this.features.toList()..addAll(other.features);
     } else if (ourType != theirType) {
-      return null;
+      return MediaQueryMergeResult.empty;
     } else {
       modifier = ourModifier ?? theirModifier;
       type = ourType;
+      features = this.features.toList()..addAll(other.features);
     }
 
-    return new CssMediaQuery(type == ourType ? this.type : other.type,
+    return new MediaQuerySuccessfulMergeResult._(new CssMediaQuery(
+        type == ourType ? this.type : other.type,
         modifier: modifier == ourModifier ? this.modifier : other.modifier,
-        features: features.toList()..addAll(other.features));
+        features: features));
   }
 
   bool operator ==(other) =>
@@ -115,3 +154,36 @@ class CssMediaQuery {
     return buffer.toString();
   }
 }
+
+/// The interface of possible return values of [CssMediaQuery.merge].
+///
+/// This is either the singleton values [empty] or [unrepresentable], or an
+/// instance of [MediaQuerySuccessfulMergeResult].
+abstract class MediaQueryMergeResult {
+  /// A singleton value indicating that there are no contexts that match both
+  /// input queries.
+  static const empty = const _SingletonCssMediaQueryMergeResult("empty");
+
+  /// A singleton value indicating that the contexts that match both input
+  /// queries can't be represented by a Level 3 media query.
+  static const unrepresentable =
+      const _SingletonCssMediaQueryMergeResult("unrepresentable");
+}
+
+/// The subclass [MediaQueryMergeResult] that represents singleton enum values.
+class _SingletonCssMediaQueryMergeResult implements MediaQueryMergeResult {
+  /// The name of the result type.
+  final String _name;
+
+  const _SingletonCssMediaQueryMergeResult(this._name);
+
+  String toString() => _name;
+}
+
+/// A successful result of [CssMediaQuery.merge].
+class MediaQuerySuccessfulMergeResult implements MediaQueryMergeResult {
+  /// The merged media query.
+  final CssMediaQuery query;
+
+  MediaQuerySuccessfulMergeResult._(this.query);
+}

lib/src/visitor/async_evaluate.dart

@@ -885,13 +885,14 @@ class _EvaluateVisitor
     }
 
     var queries = await _visitMediaQueries(node.query);
-    if (_mediaQueries != null) {
-      queries = _mergeMediaQueries(_mediaQueries, queries);
-      if (queries.isEmpty) return null;
-    }
+    var mergedQueries = _mediaQueries == null
+        ? null
+        : _mergeMediaQueries(_mediaQueries, queries);
+    if (mergedQueries != null && mergedQueries.isEmpty) return null;
 
-    await _withParent(new CssMediaRule(queries, node.span), () async {
-      await _withMediaQueries(queries, () async {
+    await _withParent(new CssMediaRule(mergedQueries ?? queries, node.span),
+        () async {
+      await _withMediaQueries(mergedQueries ?? queries, () async {
         if (!_inStyleRule) {
           for (var child in node.children) {
             await child.accept(this);
@@ -910,7 +911,9 @@ class _EvaluateVisitor
         }
       });
     },
-        through: (node) => node is CssStyleRule || node is CssMediaRule,
+        through: (node) =>
+            node is CssStyleRule ||
+            (mergedQueries != null && node is CssMediaRule),
         scopeWhen: node.hasDeclarations);
 
     return null;
@@ -928,13 +931,24 @@ class _EvaluateVisitor
         () => CssMediaQuery.parseList(resolved, logger: _logger));
   }
 
-  /// Returns a list of queries that selects for platforms that match both
+  /// Returns a list of queries that selects for contexts that match both
   /// [queries1] and [queries2].
+  ///
+  /// Returns the empty list if there are no contexts that match both [queries1]
+  /// and [queries2], or `null` if there are contexts that can't be represented
+  /// by media queries.
   List<CssMediaQuery> _mergeMediaQueries(
       Iterable<CssMediaQuery> queries1, Iterable<CssMediaQuery> queries2) {
-    return new List.unmodifiable(queries1.expand((query1) {
-      return queries2.map((query2) => query1.merge(query2));
-    }).where((query) => query != null));
+    var queries = <CssMediaQuery>[];
+    for (var query1 in queries1) {
+      for (var query2 in queries2) {
+        var result = query1.merge(query2);
+        if (result == MediaQueryMergeResult.empty) continue;
+        if (result == MediaQueryMergeResult.unrepresentable) return null;
+        queries.add((result as MediaQuerySuccessfulMergeResult).query);
+      }
+    }
+    return queries;
   }
 
   Future<Value> visitReturnRule(ReturnRule node) =>

lib/src/visitor/evaluate.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_evaluate.dart.
 // See tool/synchronize.dart for details.
 //
-// Checksum: e9558d77111c693319f39af55da8ce3797fe1807
+// Checksum: b774b5c88da1ce98a48ce71a245357a11b047ec3
 
 import 'async_evaluate.dart' show EvaluateResult;
 export 'async_evaluate.dart' show EvaluateResult;
@@ -883,13 +883,13 @@ class _EvaluateVisitor
     }
 
     var queries = _visitMediaQueries(node.query);
-    if (_mediaQueries != null) {
-      queries = _mergeMediaQueries(_mediaQueries, queries);
-      if (queries.isEmpty) return null;
-    }
+    var mergedQueries = _mediaQueries == null
+        ? null
+        : _mergeMediaQueries(_mediaQueries, queries);
+    if (mergedQueries != null && mergedQueries.isEmpty) return null;
 
-    _withParent(new CssMediaRule(queries, node.span), () {
-      _withMediaQueries(queries, () {
+    _withParent(new CssMediaRule(mergedQueries ?? queries, node.span), () {
+      _withMediaQueries(mergedQueries ?? queries, () {
         if (!_inStyleRule) {
           for (var child in node.children) {
             child.accept(this);
@@ -908,7 +908,9 @@ class _EvaluateVisitor
         }
       });
     },
-        through: (node) => node is CssStyleRule || node is CssMediaRule,
+        through: (node) =>
+            node is CssStyleRule ||
+            (mergedQueries != null && node is CssMediaRule),
         scopeWhen: node.hasDeclarations);
 
     return null;
@@ -924,13 +926,24 @@ class _EvaluateVisitor
         () => CssMediaQuery.parseList(resolved, logger: _logger));
   }
 
-  /// Returns a list of queries that selects for platforms that match both
+  /// Returns a list of queries that selects for contexts that match both
   /// [queries1] and [queries2].
+  ///
+  /// Returns the empty list if there are no contexts that match both [queries1]
+  /// and [queries2], or `null` if there are contexts that can't be represented
+  /// by media queries.
   List<CssMediaQuery> _mergeMediaQueries(
       Iterable<CssMediaQuery> queries1, Iterable<CssMediaQuery> queries2) {
-    return new List.unmodifiable(queries1.expand((query1) {
-      return queries2.map((query2) => query1.merge(query2));
-    }).where((query) => query != null));
+    var queries = <CssMediaQuery>[];
+    for (var query1 in queries1) {
+      for (var query2 in queries2) {
+        var result = query1.merge(query2);
+        if (result == MediaQueryMergeResult.empty) continue;
+        if (result == MediaQueryMergeResult.unrepresentable) return null;
+        queries.add((result as MediaQuerySuccessfulMergeResult).query);
+      }
+    }
+    return queries;
   }
 
   Value visitReturnRule(ReturnRule node) => node.expression.accept(this);

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.9.3-dev
+version: 1.10.0
 description: A Sass implementation in Dart.
 author: Dart Team <[email protected]>
 homepage: https://github.com/sass/dart-sass