From 82530d406d611fe7d16ed663b293950b6a6d1cb9 Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Mon, 10 Aug 2026 13:10:18 -0400 Subject: [PATCH] fix(mix): order variant merges by declared state dependencies mergeActiveVariants grouped active variants by class (is WidgetStateVariant) instead of by the declared semantics (widgetStateDependencies). FocusVisibleVariant declares {focused} but is not a WidgetStateVariant, so it always merged in the low-priority group and any widget-state variant silently overrode it on shared properties regardless of declaration order. Group by the declaration instead, so priority and state-tracking discovery read the same source of truth. FocusVisibleVariant, and NotVariant forwarding a state-driven inner variant, move to the high-priority group and compete by declaration order. Also replace the sort with a linear partition. List.sort falls back to an unstable quicksort at 32 elements, and with 40 equal-priority variants declared in order it resolved variant 26 as the winner rather than variant 40. Refs #967 --- packages/mix/CHANGELOG.md | 17 +++++ packages/mix/lib/src/core/style.dart | 68 +++++++++++-------- .../src/core/style_get_all_variants_test.dart | 55 +++++++++++++++ .../src/variants/context_variant_test.dart | 31 +++++++++ .../variants/focus_visible_variant_test.dart | 64 +++++++++++++++-- 5 files changed, 203 insertions(+), 32 deletions(-) diff --git a/packages/mix/CHANGELOG.md b/packages/mix/CHANGELOG.md index ee4f6c64e5..a2d772b8db 100644 --- a/packages/mix/CHANGELOG.md +++ b/packages/mix/CHANGELOG.md @@ -1,3 +1,20 @@ +## Unreleased + +### Fixes + +- **Variant merge priority follows declared state dependencies:** Priority now + groups active variants by whether they declare + `ContextVariant.widgetStateDependencies` rather than by whether they are a + `WidgetStateVariant`, so `onFocusVisible(...)` competes by declaration order + instead of always losing to any widget-state variant sharing a property, which + had been silently replacing focus rings. Variants built on + `ContextVariant.not(...)` move with their inner variant, so `onEnabled(...)` + now outranks an ambient variant such as `onDark(...)` declared after it. +- **Declaration order within a priority group is reliable:** Grouping is a + stable partition rather than a `List.sort`, which fell back to an unstable + quicksort at 32 elements and could reorder equal-priority variants in styles + that large. + ## 2.2.0-beta.3 ### New features diff --git a/packages/mix/lib/src/core/style.dart b/packages/mix/lib/src/core/style.dart index 450028f24e..00af940678 100644 --- a/packages/mix/lib/src/core/style.dart +++ b/packages/mix/lib/src/core/style.dart @@ -102,43 +102,57 @@ abstract class Style> extends Mix> /// Merges all active variants with their nested variants recursively. /// - /// This method evaluates which variants should be active based on the current - /// context and named variants, then recursively processes nested variants - /// within each active variant's style. The result is a fully merged style - /// with all applicable variants applied. + /// Evaluates which variants are active in [context] and against + /// [namedVariants], then recursively resolves the nested variants inside each + /// active variant's style. /// - /// Variant priority order (lowest to highest): - /// 1. ContextVariant and NamedVariant (applied first) - /// 2. StyleVariation (applied second) - /// 3. WidgetStateVariant (applied last, highest priority) + /// Active variants apply in two priority groups, lowest first: those that + /// declare no [ContextVariant.widgetStateDependencies], then those that do. + /// Within a group, the variant declared last merges last and so wins on the + /// properties they share. @visibleForTesting Style mergeActiveVariants( BuildContext context, { required Set namedVariants, }) { - // Filter variants that should be active in this context - final activeVariants = ($variants ?? []) - .where( - (variantAttr) => switch (variantAttr.variant) { - (ContextVariant variant) => variant.when(context), - (NamedVariant variant) => namedVariants.contains(variant), - (ContextVariantBuilder _) => true, - }, - ) - .toList(); - - // Sort by priority: WidgetStateVariant gets applied last (highest priority) - activeVariants.sort( - (a, b) => Comparable.compare( - a.variant is WidgetStateVariant ? 1 : 0, - b.variant is WidgetStateVariant ? 1 : 0, - ), - ); + final variants = $variants; + if (variants == null) return this; + + // Partition, don't sort: declaration order inside a group is load-bearing, + // and List.sort is only stable by accident of the insertion sort it falls + // back to below 32 elements. + final lowPriority = >[]; + final highPriority = >[]; + + for (final variantAttr in variants) { + final variant = variantAttr.variant; + + final isActive = switch (variant) { + ContextVariant() => variant.when(context), + NamedVariant() => namedVariants.contains(variant), + ContextVariantBuilder() => true, + }; + if (!isActive) continue; + + // Keyed off the declaration, not the class: FocusVisibleVariant is not a + // WidgetStateVariant yet reads WidgetState.focused just the same, and + // NotVariant forwards whatever its inner variant reads. + // + // The getter stays on ContextVariant rather than moving up to Variant + // because ContextVariant is also the only kind widgetStates walks, so a + // dependency declared on any other kind would never get tracking + // installed. + final readsWidgetState = + variant is ContextVariant && + variant.widgetStateDependencies.isNotEmpty; + + (readsWidgetState ? highPriority : lowPriority).add(variantAttr); + } // Extract the style from each active variant final stylesToMerge = <(Style, bool)>[]; // (style, isFromStyleVariation) - for (final variantAttr in activeVariants) { + for (final variantAttr in lowPriority.followedBy(highPriority)) { final result = switch (variantAttr.variant) { ContextVariantBuilder variant => ( variant.build(context) as Style, diff --git a/packages/mix/test/src/core/style_get_all_variants_test.dart b/packages/mix/test/src/core/style_get_all_variants_test.dart index 29a892ee12..1561704952 100644 --- a/packages/mix/test/src/core/style_get_all_variants_test.dart +++ b/packages/mix/test/src/core/style_get_all_variants_test.dart @@ -219,6 +219,61 @@ void main() { final spec = result.resolve(context); expect((spec.resolvedValue as Map)['width'], 400.0); }); + + testWidgets( + 'NotVariant inherits the priority of the variant it negates', + (tester) async { + final notHovered = ContextVariant.not( + ContextVariant.widgetState(WidgetState.hovered), + ); + final ambient = ContextVariant('ambient', (context) => true); + + final testAttribute = _MockSpecAttribute( + width: 50.0, + variants: [ + // Declared first, but forwards hovered as a dependency, so it + // still applies after the ambient variant. + VariantStyle(notHovered, _MockSpecAttribute(width: 100.0)), + VariantStyle(ambient, _MockSpecAttribute(width: 200.0)), + ], + ); + + await testVariantPriority( + tester, + testAttribute: testAttribute, + activeStates: {}, + namedVariants: {}, + expectedWidth: 100.0, + ); + }, + ); + + testWidgets('declaration order holds past the 32-element sort threshold', ( + tester, + ) async { + // List.sort switches from insertion sort to an unstable quicksort at 32 + // elements, so this size is the one that would catch a regression back + // to sorting. All variants sit in the same priority group, so the last + // declared must win. + final testAttribute = _MockSpecAttribute( + width: 0.0, + variants: [ + for (var i = 1; i <= 40; i++) + VariantStyle( + ContextVariant('context$i', (context) => true), + _MockSpecAttribute(width: i.toDouble()), + ), + ], + ); + + await testVariantPriority( + tester, + testAttribute: testAttribute, + activeStates: {}, + namedVariants: {}, + expectedWidth: 40.0, + ); + }); }); group('Variant resolution logic', () { diff --git a/packages/mix/test/src/variants/context_variant_test.dart b/packages/mix/test/src/variants/context_variant_test.dart index f6af1b6fc1..185ea39c59 100644 --- a/packages/mix/test/src/variants/context_variant_test.dart +++ b/packages/mix/test/src/variants/context_variant_test.dart @@ -148,6 +148,37 @@ void main() { ); }); + testWidgets('onEnabled outranks an ambient variant declared after it', ( + tester, + ) async { + // onEnabled is not(disabled), so it forwards a widget-state dependency + // and merges in the high-priority group. Pinned deliberately: it reads + // like base styling but does not behave like it. + await tester.pumpWidget( + MaterialApp( + home: MediaQuery( + data: const MediaQueryData(platformBrightness: Brightness.dark), + child: Box( + key: const Key('target'), + style: BoxStyler() + .size(50, 50) + .onEnabled(BoxStyler().color(Colors.blue)) + .onDark(BoxStyler().color(Colors.black)), + ), + ), + ), + ); + + final container = tester.widget( + find.descendant( + of: find.byKey(const Key('target')), + matching: find.byType(Container), + ), + ); + + expect((container.decoration as BoxDecoration?)?.color, Colors.blue); + }); + testWidgets('not variant inverts inner shouldApply result', (tester) async { final disabled = ContextVariant.widgetState(WidgetState.disabled); final enabled = ContextVariant.not(disabled); diff --git a/packages/mix/test/src/variants/focus_visible_variant_test.dart b/packages/mix/test/src/variants/focus_visible_variant_test.dart index f76c6987fe..d8fdddb879 100644 --- a/packages/mix/test/src/variants/focus_visible_variant_test.dart +++ b/packages/mix/test/src/variants/focus_visible_variant_test.dart @@ -22,20 +22,27 @@ void main() { return (container.decoration as BoxDecoration?)?.color; } - Widget buildWithController(WidgetStatesController controller) { + Widget buildStyle(BoxStyler style, WidgetStatesController controller) { return MaterialApp( home: StyleBuilder( controller: controller, - style: BoxStyler() - .size(50, 50) - .color(Colors.blue) - .onFocusVisible(BoxStyler().color(Colors.red)), + style: style, builder: (context, spec) => Container(key: const Key('target'), decoration: spec.decoration), ), ); } + Widget buildWithController(WidgetStatesController controller) { + return buildStyle( + BoxStyler() + .size(50, 50) + .color(Colors.blue) + .onFocusVisible(BoxStyler().color(Colors.red)), + controller, + ); + } + testWidgets('tracks highlight mode without a Pressable ancestor', ( tester, ) async { @@ -110,5 +117,52 @@ void main() { Colors.red, ); }); + + group('merge priority', () { + final selected = ContextVariant.widgetState(WidgetState.selected); + + WidgetStatesController selectedAndFocusVisible() { + FocusManager.instance.highlightStrategy = + FocusHighlightStrategy.alwaysTraditional; + final controller = WidgetStatesController(); + addTearDown(controller.dispose); + controller.selected = true; + controller.focused = true; + + return controller; + } + + testWidgets('beats a widget-state variant declared before it', ( + tester, + ) async { + await tester.pumpWidget( + buildStyle( + BoxStyler() + .size(50, 50) + .variant(selected, BoxStyler().color(Colors.red)) + .onFocusVisible(BoxStyler().color(Colors.blue)), + selectedAndFocusVisible(), + ), + ); + + expect(colorOf(tester), Colors.blue); + }); + + testWidgets('loses to a widget-state variant declared after it', ( + tester, + ) async { + await tester.pumpWidget( + buildStyle( + BoxStyler() + .size(50, 50) + .onFocusVisible(BoxStyler().color(Colors.blue)) + .variant(selected, BoxStyler().color(Colors.red)), + selectedAndFocusVisible(), + ), + ); + + expect(colorOf(tester), Colors.red); + }); + }); }); }