From 619921f732975803642b1cecc0acfcd2de315a9f Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 29 Aug 2022 14:23:09 -0700 Subject: [PATCH] Fix a bug with `@media` query bubbling Closes #1791 --- CHANGELOG.md | 6 ++++ lib/src/ast/css/media_query.dart | 2 ++ lib/src/visitor/async_evaluate.dart | 43 +++++++++++++++++++++++------ lib/src/visitor/evaluate.dart | 42 ++++++++++++++++++++++------ pkg/sass_api/CHANGELOG.md | 4 +++ pkg/sass_api/pubspec.yaml | 4 +-- pubspec.yaml | 2 +- 7 files changed, 82 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2535b049f..42e01c3d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 1.54.6 + +* Fix a bug where a `@media` query could be incorrectly omitted from a + stylesheet if it had multiple levels of nested `@media` queries within it + *and* the inner queries were mergeable but the outer query was not. + ## 1.54.5 * Properly consider `a ~ c` to be a superselector of `a ~ b ~ c` and `a + b + diff --git a/lib/src/ast/css/media_query.dart b/lib/src/ast/css/media_query.dart index 7a9e56fc1..8a095622d 100644 --- a/lib/src/ast/css/media_query.dart +++ b/lib/src/ast/css/media_query.dart @@ -221,4 +221,6 @@ class MediaQuerySuccessfulMergeResult implements MediaQueryMergeResult { final CssMediaQuery query; MediaQuerySuccessfulMergeResult._(this.query); + + String toString() => query.toString(); } diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 50e62b723..3c58db12a 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -179,6 +179,13 @@ class _EvaluateVisitor /// The current media queries, if any. List? _mediaQueries; + /// The set of media queries that were merged together to create + /// [_mediaQueries]. + /// + /// This will be non-null if and only if [_mediaQueries] is non-null, but it + /// will be empty if [_mediaQueries] isn't the result of a merge. + Set? _mediaQuerySources; + /// The current parent node in the output CSS tree. ModifiableCssParentNode get _parent => _assertInModule(__parent, "__parent"); set _parent(ModifiableCssParentNode value) => __parent = value; @@ -1059,7 +1066,8 @@ class _EvaluateVisitor if (_mediaQueries != null && query.excludesName('media')) { var innerScope = scope; - scope = (callback) => _withMediaQueries(null, () => innerScope(callback)); + scope = (callback) => + _withMediaQueries(null, null, () => innerScope(callback)); } if (_inKeyframes && query.excludesName('keyframes')) { @@ -1778,9 +1786,14 @@ class _EvaluateVisitor .andThen((mediaQueries) => _mergeMediaQueries(mediaQueries, queries)); if (mergedQueries != null && mergedQueries.isEmpty) return null; + var mergedSources = mergedQueries == null + ? const {} + : {..._mediaQuerySources!, ..._mediaQueries!, ...queries}; + await _withParent( ModifiableCssMediaRule(mergedQueries ?? queries, node.span), () async { - await _withMediaQueries(mergedQueries ?? queries, () async { + await _withMediaQueries(mergedQueries ?? queries, mergedSources, + () async { var styleRule = _styleRule; if (styleRule == null) { for (var child in node.children) { @@ -1802,7 +1815,9 @@ class _EvaluateVisitor }, through: (node) => node is CssStyleRule || - (mergedQueries != null && node is CssMediaRule), + (mergedSources.isNotEmpty && + node is CssMediaRule && + node.queries.every(mergedSources.contains)), scopeWhen: node.hasDeclarations); return null; @@ -3018,10 +3033,15 @@ class _EvaluateVisitor (mediaQueries) => _mergeMediaQueries(mediaQueries, node.queries)); if (mergedQueries != null && mergedQueries.isEmpty) return; + var mergedSources = mergedQueries == null + ? const {} + : {..._mediaQuerySources!, ..._mediaQueries!, ...node.queries}; + await _withParent( ModifiableCssMediaRule(mergedQueries ?? node.queries, node.span), () async { - await _withMediaQueries(mergedQueries ?? node.queries, () async { + await _withMediaQueries(mergedQueries ?? node.queries, mergedSources, + () async { var styleRule = _styleRule; if (styleRule == null) { for (var child in node.children) { @@ -3041,9 +3061,7 @@ class _EvaluateVisitor } }); }, - through: (node) => - node is CssStyleRule || - (mergedQueries != null && node is CssMediaRule), + through: (node) => node is CssStyleRule || mergedSources.contains(node), scopeWhen: false); } @@ -3301,12 +3319,19 @@ class _EvaluateVisitor } /// Runs [callback] with [queries] as the current media queries. - Future _withMediaQueries( - List? queries, Future callback()) async { + /// + /// This also sets [sources] as the current set of media queries that were + /// merged together to create [queries]. This is used to determine when it's + /// safe to bubble one query through another. + Future _withMediaQueries(List? queries, + Set? sources, Future callback()) async { var oldMediaQueries = _mediaQueries; + var oldSources = _mediaQuerySources; _mediaQueries = queries; + _mediaQuerySources = sources; var result = await callback(); _mediaQueries = oldMediaQueries; + _mediaQuerySources = oldSources; return result; } diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index 1f74f7c5a..73d42238f 100644 --- a/lib/src/visitor/evaluate.dart +++ b/lib/src/visitor/evaluate.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_evaluate.dart. // See tool/grind/synchronize.dart for details. // -// Checksum: ad5691d0c80b924308f6ee03863fa8c7f5e25ca0 +// Checksum: 839ff689ee547ac6da772a97c5d70845a358f756 // // ignore_for_file: unused_import @@ -187,6 +187,13 @@ class _EvaluateVisitor /// The current media queries, if any. List? _mediaQueries; + /// The set of media queries that were merged together to create + /// [_mediaQueries]. + /// + /// This will be non-null if and only if [_mediaQueries] is non-null, but it + /// will be empty if [_mediaQueries] isn't the result of a merge. + Set? _mediaQuerySources; + /// The current parent node in the output CSS tree. ModifiableCssParentNode get _parent => _assertInModule(__parent, "__parent"); set _parent(ModifiableCssParentNode value) => __parent = value; @@ -1063,7 +1070,8 @@ class _EvaluateVisitor if (_mediaQueries != null && query.excludesName('media')) { var innerScope = scope; - scope = (callback) => _withMediaQueries(null, () => innerScope(callback)); + scope = (callback) => + _withMediaQueries(null, null, () => innerScope(callback)); } if (_inKeyframes && query.excludesName('keyframes')) { @@ -1775,9 +1783,13 @@ class _EvaluateVisitor .andThen((mediaQueries) => _mergeMediaQueries(mediaQueries, queries)); if (mergedQueries != null && mergedQueries.isEmpty) return null; + var mergedSources = mergedQueries == null + ? const {} + : {..._mediaQuerySources!, ..._mediaQueries!, ...queries}; + _withParent(ModifiableCssMediaRule(mergedQueries ?? queries, node.span), () { - _withMediaQueries(mergedQueries ?? queries, () { + _withMediaQueries(mergedQueries ?? queries, mergedSources, () { var styleRule = _styleRule; if (styleRule == null) { for (var child in node.children) { @@ -1799,7 +1811,9 @@ class _EvaluateVisitor }, through: (node) => node is CssStyleRule || - (mergedQueries != null && node is CssMediaRule), + (mergedSources.isNotEmpty && + node is CssMediaRule && + node.queries.every(mergedSources.contains)), scopeWhen: node.hasDeclarations); return null; @@ -2997,9 +3011,13 @@ class _EvaluateVisitor (mediaQueries) => _mergeMediaQueries(mediaQueries, node.queries)); if (mergedQueries != null && mergedQueries.isEmpty) return; + var mergedSources = mergedQueries == null + ? const {} + : {..._mediaQuerySources!, ..._mediaQueries!, ...node.queries}; + _withParent( ModifiableCssMediaRule(mergedQueries ?? node.queries, node.span), () { - _withMediaQueries(mergedQueries ?? node.queries, () { + _withMediaQueries(mergedQueries ?? node.queries, mergedSources, () { var styleRule = _styleRule; if (styleRule == null) { for (var child in node.children) { @@ -3019,9 +3037,7 @@ class _EvaluateVisitor } }); }, - through: (node) => - node is CssStyleRule || - (mergedQueries != null && node is CssMediaRule), + through: (node) => node is CssStyleRule || mergedSources.contains(node), scopeWhen: false); } @@ -3272,11 +3288,19 @@ class _EvaluateVisitor } /// Runs [callback] with [queries] as the current media queries. - T _withMediaQueries(List? queries, T callback()) { + /// + /// This also sets [sources] as the current set of media queries that were + /// merged together to create [queries]. This is used to determine when it's + /// safe to bubble one query through another. + T _withMediaQueries( + List? queries, Set? sources, T callback()) { var oldMediaQueries = _mediaQueries; + var oldSources = _mediaQuerySources; _mediaQueries = queries; + _mediaQuerySources = sources; var result = callback(); _mediaQueries = oldMediaQueries; + _mediaQuerySources = oldSources; return result; } diff --git a/pkg/sass_api/CHANGELOG.md b/pkg/sass_api/CHANGELOG.md index e1ae44fc0..ab1bf93b4 100644 --- a/pkg/sass_api/CHANGELOG.md +++ b/pkg/sass_api/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.0.1 + +* No user-visible chances. + ## 3.0.0 * **Breaking change:** Convert all visitor superclasses into mixins. This diff --git a/pkg/sass_api/pubspec.yaml b/pkg/sass_api/pubspec.yaml index 313036779..cd3889685 100644 --- a/pkg/sass_api/pubspec.yaml +++ b/pkg/sass_api/pubspec.yaml @@ -2,7 +2,7 @@ name: sass_api # Note: Every time we add a new Sass AST node, we need to bump the *major* # version because it's a breaking change for anyone who's implementing the # visitor interface(s). -version: 3.0.0 +version: 3.0.1 description: Additional APIs for Dart Sass. homepage: https://github.com/sass/dart-sass @@ -10,7 +10,7 @@ environment: sdk: ">=2.17.0 <3.0.0" dependencies: - sass: 1.54.5 + sass: 1.54.6 dev_dependencies: dartdoc: ^5.0.0 diff --git a/pubspec.yaml b/pubspec.yaml index 506a14088..812a9e311 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: sass -version: 1.54.5 +version: 1.54.6 description: A Sass implementation in Dart. homepage: https://github.com/sass/dart-sass