diff --git a/lib/features/tags/presentation/pages/tag_manage_page.dart b/lib/features/tags/presentation/pages/tag_manage_page.dart index 364610d670..71491f2136 100644 --- a/lib/features/tags/presentation/pages/tag_manage_page.dart +++ b/lib/features/tags/presentation/pages/tag_manage_page.dart @@ -11,6 +11,7 @@ import 'package:submersion/features/tags/domain/entities/tag.dart'; import 'package:submersion/features/tags/presentation/providers/tag_providers.dart'; import 'package:submersion/features/tags/presentation/tag_scope_labels.dart'; import 'package:submersion/features/tags/presentation/tag_usage_messages.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/features/tags/presentation/widgets/tag_input_widget.dart'; import 'package:submersion/features/tags/presentation/widgets/tag_merge_sheet.dart'; import 'package:submersion/l10n/l10n_extension.dart'; @@ -226,13 +227,22 @@ class _TagManagePageState extends ConsumerState { final isSelected = _selectedIds.contains(tag.id); return ListTile( + // Nothing but the checkbox column: the tag itself is the title, since + // a chip carries the name and the colour together (issue #2269). A + // solid dot here was the last place in the app still claiming the + // stored hex was what a tag looks like. leading: SelectionLeading( isSelectionMode: _isSelectionMode, isChecked: isSelected, onChanged: (_) => _toggleSelection(tag.id), - child: CircleAvatar(radius: 16, backgroundColor: tag.color), + child: const SizedBox.shrink(), + ), + minLeadingWidth: 0, + horizontalTitleGap: 0, + title: Align( + alignment: AlignmentDirectional.centerStart, + child: TagChip(tag: tag), ), - title: Text(tag.name), // Where the tag is offered (issues #1765, #1942). subtitle: Text( [ @@ -303,6 +313,9 @@ class _TagManagePageState extends ConsumerState { const SizedBox(height: 8), TagColorPicker( selectedColor: selectedColor, + // Each swatch previews this tag by name, so the colour + // being chosen is the colour that will be seen. + nameController: controller, onColorSelected: (color) => setDialogState(() => selectedColor = color), ), @@ -395,6 +408,9 @@ class _TagManagePageState extends ConsumerState { const SizedBox(height: 8), TagColorPicker( selectedColor: selectedColor, + // Each swatch previews this tag by name, so the colour + // being chosen is the colour that will be seen. + nameController: controller, onColorSelected: (color) => setDialogState(() => selectedColor = color), ), diff --git a/lib/features/tags/presentation/tag_chip_colors.dart b/lib/features/tags/presentation/tag_chip_colors.dart new file mode 100644 index 0000000000..10f7ad558d --- /dev/null +++ b/lib/features/tags/presentation/tag_chip_colors.dart @@ -0,0 +1,112 @@ +import 'package:flutter/material.dart'; + +/// The three colours a tag chip is painted with (issue #2269). +typedef TagChipColors = ({Color fill, Color border, Color label}); + +/// How much of the tag's colour the fill carries. +/// +/// One value, not the 0.15 a dive row used and the 0.2 a detail card used: +/// two tints give a tag two display colours, and a swatch in Settings can +/// only tell the truth about one of them. +const double _fillTint = 0.15; + +/// WCAG 2.1 AA for body text. +const double _minLabelContrast = 4.5; + +/// How finely the label's lightness is searched. Enough steps that a colour +/// moves no further than it has to. +const int _labelSteps = 20; + +/// The colours a tag chip paints for a tag whose stored colour is [seed]. +/// +/// A tag chip is a quiet tint of the diver's colour rather than a chip +/// flooded with it, which is how tags looked before #2255 and how they look +/// again. The difference is that the tint is resolved here, once, against the +/// theme's own surface and returned opaque. +/// +/// That matters beyond tidiness. A translucent fill is not a colour but a +/// recipe whose result depends on whatever is painted behind it, so the same +/// tag used to come out four different colours across the app and a fifth on +/// a selected row, where the row's blue-grey bled through (issue #2254). +/// Resolving the tint here means a tag has exactly one display colour, which +/// is what lets the Settings swatches offer it. +/// +/// [ColorScheme.surface] is a deliberate fixed reference, not an oversight: +/// a chip on a card sits on `surfaceContainerLow` or higher, and tinting +/// against whichever container the chip happens to be in would hand back a +/// different colour per surface, which is the bug this exists to prevent. The +/// chip is meant to read as a chip against its background rather than as a +/// wash of it, and the full-strength border carries the identity either way. +/// `course_status_colors.dart` names its own surface for the same reason, and +/// names the one its card actually uses because that card has only one home. +TagChipColors tagChipColors(BuildContext context, Color seed) => + tagChipColorsFor( + seed: seed, + surface: Theme.of(context).colorScheme.surface, + ); + +/// [tagChipColors] without a [BuildContext], for callers that already hold +/// the scheme and for tests that need to name the surface they are checking. +TagChipColors tagChipColorsFor({required Color seed, required Color surface}) { + final fill = Color.alphaBlend(seed.withValues(alpha: _fillTint), surface); + return (fill: fill, border: seed, label: _labelOn(fill, seed)); +} + +/// The label colour for a chip filled with [fill] by a tag coloured [seed]. +/// +/// The pre-#2255 chips wrote the label in the raw [seed], which on its own +/// 15 per cent tint is about 1.7:1 for the pale end of the palette: legible +/// in theory, invisible in practice. #2255 answered that with black or white, +/// which reads but drops the tag's identity from the text. +/// +/// The seed instead keeps its hue and its saturation and gives up only +/// lightness, moving away from the fill until it clears AA. Working in HSL +/// rather than blending towards a dark neutral matters: an RGB blend +/// compresses the channel spread, which cost the saturated mid tones such as +/// `#A855F7` nearly half their saturation on the way down. +/// +/// The direction is read off the fill alone, so the one rule serves both +/// themes: the label darkens on a pale chip and lightens on a dark one. +/// +/// It is read off the fill rather than off the fill against the seed, which +/// is a distinction that only shows up at the ends of the range. A tag stored +/// near black on a dark theme has a fill LIGHTER than itself, because the +/// fill is mostly surface; comparing the two then walked the label towards +/// black, into the fill, and left it at 1.1:1. A white tag on a light theme +/// was worse still, white on white at 1.0:1. +Color _labelOn(Color fill, Color seed) { + if (tagContrastRatio(seed, fill) >= _minLabelContrast) return seed; + + final hsl = HSLColor.fromColor(seed); + + // Whichever end of the lightness range has room against this fill. Black + // clears AA for any fill above about 0.175 luminance and white for any + // below about 0.183, so the two ranges overlap: some end always works, + // whatever the fill, and the walk below always terminates. + final endpoint = + tagContrastRatio(Colors.black, fill) >= + tagContrastRatio(Colors.white, fill) + ? 0.0 + : 1.0; + + for (var step = 1; step < _labelSteps; step++) { + final t = step / _labelSteps; + final candidate = hsl + .withLightness(hsl.lightness + (endpoint - hsl.lightness) * t) + .toColor(); + if (tagContrastRatio(candidate, fill) >= _minLabelContrast) { + return candidate; + } + } + return hsl.withLightness(endpoint).toColor(); +} + +/// WCAG 2.1 contrast ratio between two opaque colours, from 1 (identical) to +/// 21 (black on white). +double tagContrastRatio(Color a, Color b) { + final la = a.computeLuminance(); + final lb = b.computeLuminance(); + final lighter = la > lb ? la : lb; + final darker = la > lb ? lb : la; + return (lighter + 0.05) / (darker + 0.05); +} diff --git a/lib/features/tags/presentation/tag_color_contrast.dart b/lib/features/tags/presentation/tag_color_contrast.dart deleted file mode 100644 index 1506a9c76c..0000000000 --- a/lib/features/tags/presentation/tag_color_contrast.dart +++ /dev/null @@ -1,30 +0,0 @@ -import 'package:flutter/material.dart'; - -/// The label colour for text written on [background] (issue #2254). -/// -/// A tag chip is filled with the diver's own colour, so it cannot take its -/// label colour from the theme: the palette runs from a pale yellow to a -/// near-black slate, and either extreme swallows one of the two. The choice -/// is made on WCAG 2.1 contrast ratio rather than a luminance threshold, -/// because the crossover point between black and white sits at a luminance -/// of about 0.18 and a threshold picked by eye lands on the wrong side of -/// the palette's mid tones. -/// -/// Both candidates are opaque, so nothing behind the chip shows through the -/// label. -Color tagForegroundColor(Color background) { - return _contrast(Colors.black, background) >= - _contrast(Colors.white, background) - ? Colors.black - : Colors.white; -} - -/// WCAG 2.1 contrast ratio between two opaque colours, from 1 (identical) to -/// 21 (black on white). -double _contrast(Color a, Color b) { - final la = a.computeLuminance(); - final lb = b.computeLuminance(); - final lighter = la > lb ? la : lb; - final darker = la > lb ? lb : la; - return (lighter + 0.05) / (darker + 0.05); -} diff --git a/lib/features/tags/presentation/widgets/tag_chip.dart b/lib/features/tags/presentation/widgets/tag_chip.dart index 17bcdf604e..891a375d2d 100644 --- a/lib/features/tags/presentation/widgets/tag_chip.dart +++ b/lib/features/tags/presentation/widgets/tag_chip.dart @@ -1,18 +1,21 @@ import 'package:flutter/material.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; -import 'package:submersion/features/tags/presentation/tag_color_contrast.dart'; +import 'package:submersion/features/tags/presentation/tag_chip_colors.dart'; -/// A tag shown in the colour the diver gave it (issue #2254). +/// A tag shown in the colour the diver gave it (issues #2254, #2269). /// /// Every tag chip in the app renders through this widget, so a tag looks the /// same in a dive list row, on a detail card, beside a site and beside a -/// piece of equipment. The fill is [Tag.color] at full opacity, never a -/// translucent tint: a tint is not a colour but a recipe, and the chips that -/// used one resolved to a different colour on every surface, turning an amber -/// tag grey-olive on a selected row. The label takes the colour that -/// contrasts with the fill, since the palette spans a pale yellow and a -/// near-black slate. +/// piece of equipment. The chip is a quiet tint of [Tag.color] outlined in +/// that colour, which is how a tag looked before #2255 flooded the chip with +/// the colour outright. +/// +/// The tint is a value, not a translucent overlay: [tagChipColors] resolves +/// it against the theme surface and hands back an opaque fill. A translucent +/// fill is a recipe rather than a colour, and the chips that used one came +/// out differently on every surface, turning an amber tag grey-olive on a +/// selected row. class TagChip extends StatelessWidget { TagChip({ super.key, @@ -40,7 +43,8 @@ class TagChip extends StatelessWidget { final String name; - /// The fill, painted opaque. [Tag.color] for a stored tag. + /// The tag's own colour, which the outline shows at full strength and the + /// fill shows as a tint. [Tag.color] for a stored tag. final Color color; /// Tapping the chip, usually to open what carries the tag. @@ -58,16 +62,21 @@ class TagChip extends StatelessWidget { @override Widget build(BuildContext context) { - final foreground = tagForegroundColor(color); + final colors = tagChipColors(context, color); final borderRadius = BorderRadius.circular(dense ? 4 : 8); final textStyle = Theme.of(context).textTheme.bodySmall?.copyWith( - color: foreground, + color: colors.label, fontSize: dense ? 11 : null, ); Widget chip = Material( - color: color, - borderRadius: borderRadius, + color: colors.fill, + // The outline is the one place the tag's colour is shown at full + // strength, which is what keeps a pale tint identifiable. + shape: RoundedRectangleBorder( + borderRadius: borderRadius, + side: BorderSide(color: colors.border), + ), child: InkWell( onTap: onTap, borderRadius: borderRadius, @@ -98,7 +107,7 @@ class TagChip extends StatelessWidget { if (onDeleted != null) ...[ const SizedBox(width: 4), _DeleteButton( - color: foreground, + color: colors.label, // Material's own chips label their delete button from the // same string, so a caller that has nothing better to say // keeps the wording screen readers already know. @@ -121,6 +130,17 @@ class TagChip extends StatelessWidget { } } +/// The floor for a tag control's tap target. +/// +/// A finger needs the 48 dp Material touch minimum; a pointer is precise, and +/// a 48 dp box inside a chip is out of scale on a desktop, so it takes the +/// 32 dp pointer minimum instead. Shared by the chip's close button and by +/// the colour swatches in Settings, which are chips the diver taps. +double tagTapTarget(TargetPlatform platform) => switch (platform) { + TargetPlatform.android || TargetPlatform.iOS || TargetPlatform.fuchsia => 48, + TargetPlatform.macOS || TargetPlatform.linux || TargetPlatform.windows => 32, +}; + /// The close button of a removable chip. /// /// The tap target is measured, not assumed: a bare icon with a splash radius @@ -138,21 +158,9 @@ class _DeleteButton extends StatelessWidget { final VoidCallback onPressed; final String? tooltip; - /// The floor for the tap target. A finger needs the 48 dp Material touch - /// minimum; a pointer is precise, and a 48 dp box inside a chip is out of - /// scale on a desktop, so it takes the 32 dp pointer minimum instead. - static double targetFor(TargetPlatform platform) => switch (platform) { - TargetPlatform.android || - TargetPlatform.iOS || - TargetPlatform.fuchsia => 48, - TargetPlatform.macOS || - TargetPlatform.linux || - TargetPlatform.windows => 32, - }; - @override Widget build(BuildContext context) { - final target = targetFor(Theme.of(context).platform); + final target = tagTapTarget(Theme.of(context).platform); return IconButton( onPressed: onPressed, tooltip: tooltip, diff --git a/lib/features/tags/presentation/widgets/tag_input_widget.dart b/lib/features/tags/presentation/widgets/tag_input_widget.dart index c7f04d1f3e..95e31c9f74 100644 --- a/lib/features/tags/presentation/widgets/tag_input_widget.dart +++ b/lib/features/tags/presentation/widgets/tag_input_widget.dart @@ -277,57 +277,137 @@ class TagChips extends StatelessWidget { } } -/// Color picker for tags +/// The colour choices for a tag, each shown as the chip the tag will become +/// (issue #2269). +/// +/// The swatches used to be solid 28 dp dots of the stored hex, which is a +/// colour a tag never actually displays: a chip is a tint of it. Offering the +/// raw hex here is what made Settings disagree with the rest of the app in +/// issue #2254. Every swatch is now a real [TagChip] drawn by the same +/// [tagChipColors], carrying the name being typed, so choosing a colour is +/// choosing a preview rather than a code. class TagColorPicker extends StatelessWidget { final String? selectedColor; final void Function(String color) onColorSelected; + /// The tag's name field, so each swatch previews this tag rather than a + /// generic one and keeps up as the name is typed. A null or empty + /// controller leaves the swatches unlabelled, which is what the Add dialog + /// shows before anything has been entered. + final TextEditingController? nameController; + const TagColorPicker({ super.key, this.selectedColor, required this.onColorSelected, + this.nameController, }); @override Widget build(BuildContext context) { - return Wrap( - spacing: 8, - runSpacing: 8, - children: TagColors.predefined.map((color) { - final isSelected = color == selectedColor; - return Semantics( - button: true, - label: 'Select color $color', - selected: isSelected, - child: GestureDetector( - onTap: () => onColorSelected(color), + final controller = nameController; + if (controller == null) return _swatches(''); + + return ValueListenableBuilder( + valueListenable: controller, + builder: (context, value, _) => _swatches(value.text), + ); + } + + Widget _swatches(String name) => Wrap( + spacing: 8, + runSpacing: 8, + children: [ + for (final hex in TagColors.predefined) + _ColorSwatch( + hex: hex, + name: name, + isSelected: hex == selectedColor, + onTap: () => onColorSelected(hex), + ), + ], + ); +} + +/// One colour choice, shown as the chip it produces. +class _ColorSwatch extends StatelessWidget { + const _ColorSwatch({ + required this.hex, + required this.name, + required this.isSelected, + required this.onTap, + }); + + final String hex; + final String name; + final bool isSelected; + final VoidCallback onTap; + + /// Enough of a long tag name to recognise the preview by, without one + /// swatch driving the grid down to a single column. Twenty labelled chips + /// wrap to several times the height of twenty dots, so the grid is kept + /// tight and the dialog's own scroll view carries the rest. + static const double _maxChipWidth = 96; + + /// A swatch with nothing to label it is still a chip, not a hairline. + static const double _minChipWidth = 32; + + /// The ring the chosen swatch wears. Reserved on every swatch and left + /// transparent where it is not worn, so choosing one does not reflow the + /// grid under the pointer. + static const double _ringWidth = 2; + + @override + Widget build(BuildContext context) { + final scheme = Theme.of(context).colorScheme; + final target = tagTapTarget(Theme.of(context).platform); + + return Semantics( + button: true, + label: 'Select color $hex', + selected: isSelected, + child: GestureDetector( + onTap: onTap, + // Opaque so the whole target answers the tap, not only the pixels the + // chip happens to cover. + behavior: HitTestBehavior.opaque, + child: ConstrainedBox( + // The swatch is a control the diver taps, so it owns the platform's + // tap target even though the chip it previews is 29 dp tall. The + // dots it replaced were 28 dp and cleared neither floor either. + constraints: BoxConstraints(minWidth: target, minHeight: target), + child: Center( + widthFactor: 1, child: Container( - width: 28, - height: 28, + padding: const EdgeInsets.all(3), decoration: BoxDecoration( - color: TagColors.fromHex(color), - shape: BoxShape.circle, - border: isSelected - ? Border.all(color: Colors.white, width: 2) - : null, - boxShadow: isSelected - ? [ - BoxShadow( - color: TagColors.fromHex( - color, - ).withValues(alpha: 0.5), - blurRadius: 6, - ), - ] - : null, + borderRadius: BorderRadius.circular(12), + border: Border.all( + color: isSelected ? scheme.primary : Colors.transparent, + width: _ringWidth, + ), + ), + child: ConstrainedBox( + constraints: const BoxConstraints( + minWidth: _minChipWidth, + maxWidth: _maxChipWidth, + ), + // The chip is this control's picture, not its description. + // Its label repeats the name field above on all twenty + // swatches, and announcing it once per swatch would bury the + // colour, which is the only thing that differs between them. + child: ExcludeSemantics( + child: TagChip.unsaved( + name: name, + color: TagColors.fromHex(hex), + dense: true, + ), + ), ), - child: isSelected - ? const Icon(Icons.check, size: 16, color: Colors.white) - : null, ), ), - ); - }).toList(), + ), + ), ); } } diff --git a/lib/features/tags/presentation/widgets/tag_merge_sheet.dart b/lib/features/tags/presentation/widgets/tag_merge_sheet.dart index d1d619751f..33e7dc8a3b 100644 --- a/lib/features/tags/presentation/widgets/tag_merge_sheet.dart +++ b/lib/features/tags/presentation/widgets/tag_merge_sheet.dart @@ -200,6 +200,9 @@ class _TagMergeSheetState extends ConsumerState { const SizedBox(height: 8), TagColorPicker( selectedColor: _selectedColor, + // The merged tag's own name, so the swatches preview the tag + // this sheet is about to produce. + nameController: _nameController, onColorSelected: (color) { setState(() { _selectedColor = color; diff --git a/test/features/tags/presentation/pages/tag_manage_page_scope_test.dart b/test/features/tags/presentation/pages/tag_manage_page_scope_test.dart index 8b23b02082..e6306eca7c 100644 --- a/test/features/tags/presentation/pages/tag_manage_page_scope_test.dart +++ b/test/features/tags/presentation/pages/tag_manage_page_scope_test.dart @@ -12,6 +12,17 @@ import '../../../../helpers/test_database.dart'; /// Tag scope on the Tags management page (issue #1765), against a real /// database so the narrowing path runs end to end. +/// Taps a scope checkbox, scrolling it into view first. +/// +/// The colour swatches above it are chips carrying a full tap target +/// (issue #2269), so the scope editor can sit past the dialog's fold. The +/// dialog has always scrolled; only the distance changed. +Future _tapScope(WidgetTester tester, String label) async { + final target = find.widgetWithText(CheckboxListTile, label); + await tester.ensureVisible(target); + await tester.tap(target); +} + void main() { late MockCurrentDiverIdNotifier diverIdNotifier; @@ -71,7 +82,7 @@ void main() { await tester.tap(find.byKey(const ValueKey('tag_edit_t1'))); await tester.pumpAndSettle(); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for sites')); + await _tapScope(tester, 'Use for sites'); await tester.pumpAndSettle(); await tester.tap(find.widgetWithText(TextButton, 'Save')); await tester.pumpAndSettle(); @@ -96,7 +107,7 @@ void main() { await tester.tap(find.byKey(const ValueKey('tag_edit_t1'))); await tester.pumpAndSettle(); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for sites')); + await _tapScope(tester, 'Use for sites'); await tester.pumpAndSettle(); await tester.tap(find.widgetWithText(TextButton, 'Save')); await tester.pumpAndSettle(); @@ -127,8 +138,8 @@ void main() { await tester.tap(find.byKey(const ValueKey('tag_edit_t1'))); await tester.pumpAndSettle(); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for dives')); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for sites')); + await _tapScope(tester, 'Use for dives'); + await _tapScope(tester, 'Use for sites'); await tester.pumpAndSettle(); expect( diff --git a/test/features/tags/presentation/pages/tag_manage_page_test.dart b/test/features/tags/presentation/pages/tag_manage_page_test.dart index 05a71f54d3..fa2799e4b0 100644 --- a/test/features/tags/presentation/pages/tag_manage_page_test.dart +++ b/test/features/tags/presentation/pages/tag_manage_page_test.dart @@ -13,6 +13,7 @@ import 'package:submersion/features/tags/data/repositories/tag_repository.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; import 'package:submersion/features/tags/presentation/pages/tag_manage_page.dart'; import 'package:submersion/features/tags/presentation/providers/tag_providers.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/features/tags/presentation/widgets/tag_merge_sheet.dart'; import 'package:submersion/l10n/arb/app_localizations.dart'; import 'package:submersion/shared/selection/selection_leading.dart'; @@ -1042,8 +1043,14 @@ void main() { await tester.tap(find.byKey(const ValueKey('tag_edit_tag1'))); await tester.pumpAndSettle(); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for sites')); - await tester.tap(find.widgetWithText(CheckboxListTile, 'Use for dives')); + // The colour swatches are chips rather than dots (issue #2269), so the + // scope editor below them can sit past the dialog's fold. + final sites = find.widgetWithText(CheckboxListTile, 'Use for sites'); + final dives = find.widgetWithText(CheckboxListTile, 'Use for dives'); + await tester.ensureVisible(sites); + await tester.tap(sites); + await tester.ensureVisible(dives); + await tester.tap(dives); await tester.pump(); await tester.tap(find.widgetWithText(TextButton, 'Save')); await tester.pump(); @@ -1419,6 +1426,55 @@ void main() { ); }); }); + group('a row shows the tag as the app shows it', () { + // The row's leading dot used to be a solid CircleAvatar of the stored + // hex, which is the exact spot issue #2254 measured as disagreeing with + // every chip in the app. Settings now offers the same chip the rest of + // the app paints, so there is nothing left claiming the raw hex is what + // a tag looks like (issue #2269). + testWidgets('renders each tag as its chip', (tester) async { + await tester.pumpWidget(_buildTestWidget(stats: _testStats)); + await tester.pumpAndSettle(); + + expect(find.byType(TagChip), findsNWidgets(_testStats.length)); + }); + + testWidgets('keeps no solid dot of the stored colour', (tester) async { + await tester.pumpWidget(_buildTestWidget(stats: _testStats)); + await tester.pumpAndSettle(); + + expect(find.byType(CircleAvatar), findsNothing); + }); + + testWidgets('paints the chip in the tint, not the stored hex', ( + tester, + ) async { + await tester.pumpWidget(_buildTestWidget(stats: _testStats)); + await tester.pumpAndSettle(); + + final fill = tester + .widget( + find + .descendant( + of: find.byType(TagChip).first, + matching: find.byType(Material), + ) + .first, + ) + .color; + + expect(fill, isNot(_testStats.first.tag.color)); + expect(fill?.a, 1.0); + }); + + testWidgets('names the tag exactly once in the row', (tester) async { + // The chip carries the name, so the ListTile title must not repeat it. + await tester.pumpWidget(_buildTestWidget(stats: _testStats)); + await tester.pumpAndSettle(); + + expect(find.text('Night Dive'), findsOneWidget); + }); + }); } /// A notifier whose single-tag delete fails, as a repository or sync failure diff --git a/test/features/tags/presentation/tag_chip_colors_test.dart b/test/features/tags/presentation/tag_chip_colors_test.dart new file mode 100644 index 0000000000..b8dc84b2f0 --- /dev/null +++ b/test/features/tags/presentation/tag_chip_colors_test.dart @@ -0,0 +1,156 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/tag_chip_colors.dart'; + +/// A tag chip is a pale tint of the diver's colour (issue #2269). +/// +/// The tint is computed once against the theme's own surface and painted +/// opaque, rather than laid over whatever happens to sit behind the chip. +/// That is what lets Settings offer the colour a tag really displays: a +/// translucent fill has no single colour, only a recipe whose result changes +/// per surface. +void main() { + const amber = Color(0xFFF59E0B); + const white = Colors.white; + + TagChipColors colorsFor(Color seed, {Color surface = white}) => + tagChipColorsFor(seed: seed, surface: surface); + + group('fill', () { + test('is opaque, so nothing behind the chip can change it', () { + expect(colorsFor(amber).fill.a, 1.0); + }); + + test('is the surface tinted 15 per cent by the tag colour', () { + // Amber over white is the pale sand the chips showed before #2255. + // Pinning the channels pins the tint: at the 0.2 the detail cards + // used, green and blue both land outside these bounds. + final fill = colorsFor(amber).fill; + + expect(fill.r, closeTo(0.994, 0.002)); + expect(fill.g, closeTo(0.943, 0.002)); + expect(fill.b, closeTo(0.856, 0.002)); + }); + + test('follows the theme surface, not the widget behind the chip', () { + // The same tag on a dark theme takes the dark surface's tint. What it + // must never do is take the blue-grey of a selected row, which is what + // the translucent fill did. + final onDark = colorsFor(amber, surface: const Color(0xFF121212)).fill; + + expect(onDark.a, 1.0); + expect(onDark.r, lessThan(0.3)); + }); + }); + + group('border', () { + test('is the stored colour at full strength', () { + expect(colorsFor(amber).border, amber); + expect(colorsFor(amber).border.a, 1.0); + }); + }); + + group('label', () { + test('clears WCAG AA against its own fill for every palette colour', () { + for (final hex in TagColors.predefined) { + final colors = colorsFor(TagColors.fromHex(hex)); + + expect( + tagContrastRatio(colors.label, colors.fill), + greaterThanOrEqualTo(4.5), + reason: '$hex label is unreadable on its own chip', + ); + } + }); + + test('gives up lightness only, never hue or saturation', () { + // #2255 wrote the label in black or white, which is legible but drops + // the tag's identity from the text. Darkening in HSL keeps both, which + // an RGB blend towards a dark neutral does not: that cost the + // saturated mid tones nearly half their saturation. + for (final hex in TagColors.predefined) { + final seed = HSLColor.fromColor(TagColors.fromHex(hex)); + final label = HSLColor.fromColor(colorsFor(seed.toColor()).label); + + // Only where there is a hue to keep. Stone and Zinc differ by under + // 12 of 255 between their strongest and weakest channel, so a + // one-unit rounding through 8-bit RGB swings their computed hue by + // degrees. That is the palette's arithmetic, not the label rule. + if (seed.saturation > 0.2) { + expect( + _hueGap(seed.hue, label.hue), + lessThan(2), + reason: '$hex label drifted off its own hue', + ); + } + expect( + label.saturation, + closeTo(seed.saturation, 0.05), + reason: '$hex label washed out towards a neutral', + ); + expect( + label.lightness, + lessThan(seed.lightness), + reason: '$hex label had to darken to be readable', + ); + } + }); + + test('stays readable when the tag is darker than its own fill', () { + // A tag stored near black, on a dark theme. The fill is the surface + // tinted, so it comes out LIGHTER than the seed, and picking the + // direction by comparing the two then walks the label towards black, + // into the fill, for about 1.1:1. The direction has to be read off the + // fill alone: whichever end of the lightness range has room against it. + const darkSurface = Color(0xFF121212); + + for (final seed in [Colors.black, const Color(0xFF050505)]) { + final colors = colorsFor(seed, surface: darkSurface); + + expect( + tagContrastRatio(colors.label, colors.fill), + greaterThanOrEqualTo(4.5), + reason: '$seed label is unreadable on its own chip', + ); + } + }); + + test('stays readable when the tag is lighter than its own fill', () { + // The mirror case on a light theme, where a near-white tag's fill is + // fractionally darker than the tag. + for (final seed in [Colors.white, const Color(0xFFFAFAFA)]) { + final colors = colorsFor(seed); + + expect( + tagContrastRatio(colors.label, colors.fill), + greaterThanOrEqualTo(4.5), + reason: '$seed label is unreadable on its own chip', + ); + } + }); + + test('darkens the pale colours that the raw hue could not carry', () { + // Amber on its own 15 per cent tint is about 1.7:1, which is why the + // pre-#2255 label was close to invisible on the pale end of the + // palette. The label has to move. + final colors = colorsFor(amber); + + expect(colors.label, isNot(amber)); + expect(tagContrastRatio(amber, colors.fill), lessThan(4.5)); + }); + }); + + group('tagContrastRatio', () { + test('runs from 1 for identical colours to 21 for black on white', () { + expect(tagContrastRatio(white, white), closeTo(1, 0.001)); + expect(tagContrastRatio(Colors.black, white), closeTo(21, 0.001)); + }); + }); +} + +/// The shorter way round the hue circle, in degrees. +double _hueGap(double a, double b) { + final gap = (a - b).abs() % 360; + return gap > 180 ? 360 - gap : gap; +} diff --git a/test/features/tags/presentation/tag_color_contrast_test.dart b/test/features/tags/presentation/tag_color_contrast_test.dart deleted file mode 100644 index 863c034da3..0000000000 --- a/test/features/tags/presentation/tag_color_contrast_test.dart +++ /dev/null @@ -1,45 +0,0 @@ -import 'package:flutter/material.dart'; -import 'package:flutter_test/flutter_test.dart'; -import 'package:submersion/features/tags/domain/entities/tag.dart'; -import 'package:submersion/features/tags/presentation/tag_color_contrast.dart'; - -/// The label colour a tag chip writes on its own colour (issue #2254). -/// -/// A chip filled with the exact tag colour has to choose its own text colour, -/// because the palette spans a pale yellow and a near-black slate. The choice -/// is made on contrast ratio, so every predefined colour stays readable. -void main() { - /// WCAG 2.1 relative luminance contrast between two opaque colours. - double ratio(Color a, Color b) { - final la = a.computeLuminance(); - final lb = b.computeLuminance(); - final lighter = la > lb ? la : lb; - final darker = la > lb ? lb : la; - return (lighter + 0.05) / (darker + 0.05); - } - - test('every predefined tag colour gets a label above WCAG AA', () { - for (final hex in TagColors.predefined) { - final background = TagColors.fromHex(hex); - final foreground = tagForegroundColor(background); - expect( - ratio(foreground, background), - greaterThanOrEqualTo(4.5), - reason: '$hex label contrast', - ); - } - }); - - test('a pale colour takes a dark label, a dark colour a light one', () { - // Amber, the colour in the issue's report. - expect(tagForegroundColor(const Color(0xFFF59E0B)).computeLuminance(), 0.0); - // Slate, the darkest entry of the palette. - expect(tagForegroundColor(const Color(0xFF64748B)).computeLuminance(), 1.0); - }); - - test('the label is opaque, so no surface shows through it', () { - for (final hex in TagColors.predefined) { - expect(tagForegroundColor(TagColors.fromHex(hex)).a, 1.0, reason: hex); - } - }); -} diff --git a/test/features/tags/presentation/widgets/tag_chip_test.dart b/test/features/tags/presentation/widgets/tag_chip_test.dart index 378bee0899..4b799f80ed 100644 --- a/test/features/tags/presentation/widgets/tag_chip_test.dart +++ b/test/features/tags/presentation/widgets/tag_chip_test.dart @@ -1,16 +1,16 @@ import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; -import 'package:submersion/features/tags/presentation/tag_color_contrast.dart'; +import 'package:submersion/features/tags/presentation/tag_chip_colors.dart'; import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; -/// TagChip paints a tag in the colour the diver chose (issue #2254). +/// TagChip paints a tag as a pale tint of the diver's colour (issue #2269). /// -/// The chips used to fill with `tag.color.withValues(alpha: 0.15)`, which is -/// not a colour but a recipe: the result depended on the surface behind the -/// chip, so one tag read as four different colours across the app and changed -/// again when its row was selected. These tests pin the fill to the stored -/// colour, opaque, whatever sits behind it. +/// #2255 filled the chip with the stored colour at full strength to make the +/// app agree with Settings. The agreement is wanted; the flooded chip is not. +/// The chip is quiet again, and the tint is resolved against the theme's own +/// surface and painted opaque, so a tag is still one colour everywhere +/// instead of the four that the translucent fill produced. void main() { final amber = Tag( id: 'shore', @@ -19,59 +19,89 @@ void main() { createdAt: DateTime(2026), updatedAt: DateTime(2026), ); + const amberSeed = Color(0xFFF59E0B); - Widget harness(Widget child, {Color? surface}) => MaterialApp( - home: Scaffold( - body: Center( - child: ColoredBox( - color: surface ?? Colors.white, - child: Padding(padding: const EdgeInsets.all(8), child: child), - ), - ), + final theme = ThemeData( + colorScheme: const ColorScheme.light().copyWith( + surface: Colors.white, + onSurface: const Color(0xFF1C1B1F), ), ); - /// The colour [TagChip] fills itself with. - Color fillOf(WidgetTester tester) { - final material = tester.widget( - find - .descendant(of: find.byType(TagChip), matching: find.byType(Material)) - .first, - ); - return material.color!; - } + Widget harness(Widget child, {Color? surface, ThemeData? withTheme}) => + MaterialApp( + theme: withTheme ?? theme, + home: Scaffold( + body: Center( + child: ColoredBox( + color: surface ?? Colors.white, + child: Padding(padding: const EdgeInsets.all(8), child: child), + ), + ), + ), + ); - testWidgets('fills with the exact stored colour, fully opaque', ( - tester, - ) async { + Material materialOf(WidgetTester tester) => tester.widget( + find + .descendant(of: find.byType(TagChip), matching: find.byType(Material)) + .first, + ); + + Color fillOf(WidgetTester tester) => materialOf(tester).color!; + + BorderSide borderOf(WidgetTester tester) => + (materialOf(tester).shape! as RoundedRectangleBorder).side; + + testWidgets('fills with a pale tint, not the stored colour', (tester) async { + await tester.pumpWidget(harness(TagChip(tag: amber))); + + final expected = tagChipColorsFor(seed: amberSeed, surface: Colors.white); + + expect(fillOf(tester), expected.fill); + expect(fillOf(tester), isNot(amberSeed)); + }); + + testWidgets('fills opaquely', (tester) async { await tester.pumpWidget(harness(TagChip(tag: amber))); - expect(fillOf(tester), const Color(0xFFF59E0B)); expect(fillOf(tester).a, 1.0); }); testWidgets('shows the same colour whatever surface is behind it', ( tester, ) async { - // The second is the blue-grey of a selected dive row. Composited under - // the old 15% tint the chip came out F2E8D7 on one and C4C2B7 on the - // other, so one tag read as two colours. + // The second is the blue-grey of a selected dive row. Under the old + // translucent tint the chip came out F2E8D7 on one and C4C2B7 on the + // other, so one tag read as two colours. The tint is resolved from the + // theme now, so what sits behind the chip cannot reach it. + final fills = []; for (final surface in [Colors.white, const Color(0xFFBBC8D6)]) { await tester.pumpWidget(harness(TagChip(tag: amber), surface: surface)); - - expect( - Color.alphaBlend(fillOf(tester), surface), - const Color(0xFFF59E0B), - reason: 'on $surface', - ); + fills.add(fillOf(tester)); } + + expect(fills.first, fills.last); }); - testWidgets('labels the chip with the contrasting colour', (tester) async { + testWidgets('outlines the chip in the stored colour at full strength', ( + tester, + ) async { + await tester.pumpWidget(harness(TagChip(tag: amber))); + + expect(borderOf(tester).color, amberSeed); + }); + + testWidgets('labels the chip in the tag hue, darkened to stay readable', ( + tester, + ) async { await tester.pumpWidget(harness(TagChip(tag: amber))); final label = tester.widget(find.text('Shore')); - expect(label.style?.color, tagForegroundColor(amber.color)); + final expected = tagChipColorsFor(seed: amberSeed, surface: Colors.white); + + expect(label.style?.color, expected.label); + expect(label.style?.color, isNot(Colors.black)); + expect(label.style?.color, isNot(Colors.white)); }); testWidgets('a colourless tag still fills opaquely', (tester) async { @@ -79,8 +109,8 @@ void main() { await tester.pumpWidget(harness(TagChip(tag: plain))); - expect(fillOf(tester), plain.color); expect(fillOf(tester).a, 1.0); + expect(borderOf(tester).color, plain.color); }); testWidgets('reports a tap', (tester) async { @@ -109,6 +139,8 @@ void main() { // A 16px icon with only a splash radius left a 16x16 target, well under // either platform's floor. Measured per platform, because a chip is not // allowed to shrink the target the way VisualDensity.compact once did. + // The colour of the chip has nothing to do with this floor, so the fix + // that introduced it outlives the fill that came with it. for (final (platform, floor) in [ (TargetPlatform.android, 48.0), (TargetPlatform.iOS, 48.0), @@ -118,13 +150,9 @@ void main() { ]) { testWidgets('$platform reaches $floor', (tester) async { await tester.pumpWidget( - MaterialApp( - theme: ThemeData(platform: platform), - home: Scaffold( - body: Center( - child: TagChip(tag: amber, onDeleted: () {}), - ), - ), + harness( + TagChip(tag: amber, onDeleted: () {}), + withTheme: theme.copyWith(platform: platform), ), ); @@ -144,13 +172,9 @@ void main() { tester, ) async { await tester.pumpWidget( - MaterialApp( - theme: ThemeData(platform: TargetPlatform.macOS), - home: Scaffold( - body: Center( - child: TagChip(tag: amber, onDeleted: () {}), - ), - ), + harness( + TagChip(tag: amber, onDeleted: () {}), + withTheme: theme.copyWith(platform: TargetPlatform.macOS), ), ); @@ -158,9 +182,18 @@ void main() { }); }); - testWidgets('the dense variant keeps the same fill', (tester) async { + testWidgets('the dense variant keeps the same fill and border', ( + tester, + ) async { + // One tint across the app, not the 0.15 of a list row and the 0.2 of a + // card: two tints would give a tag two display colours again, and the + // Settings swatch could only tell the truth about one of them. + await tester.pumpWidget(harness(TagChip(tag: amber))); + final normalFill = fillOf(tester); + await tester.pumpWidget(harness(TagChip(tag: amber, dense: true))); - expect(fillOf(tester), const Color(0xFFF59E0B)); + expect(fillOf(tester), normalFill); + expect(borderOf(tester).color, amberSeed); }); } diff --git a/test/features/tags/presentation/widgets/tag_color_picker_test.dart b/test/features/tags/presentation/widgets/tag_color_picker_test.dart new file mode 100644 index 0000000000..e39d34d470 --- /dev/null +++ b/test/features/tags/presentation/widgets/tag_color_picker_test.dart @@ -0,0 +1,159 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/tag_chip_colors.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_input_widget.dart'; + +/// The colour picker offers the colour a tag will actually display +/// (issue #2269). +/// +/// It used to paint a solid 28 dp dot of the stored hex, which is not a +/// colour the diver ever sees on a tag: the chip is a pale tint of it. Every +/// swatch is now the chip itself, drawn by the same colour function, so +/// Settings and the rest of the app cannot disagree again. +void main() { + final theme = ThemeData( + colorScheme: const ColorScheme.light().copyWith( + surface: Colors.white, + onSurface: const Color(0xFF1C1B1F), + ), + ); + + Widget harness({ + String? selectedColor, + TextEditingController? nameController, + void Function(String)? onColorSelected, + TargetPlatform? platform, + }) => MaterialApp( + theme: platform == null ? theme : theme.copyWith(platform: platform), + home: Scaffold( + body: SingleChildScrollView( + child: SizedBox( + width: 400, + child: TagColorPicker( + selectedColor: selectedColor, + nameController: nameController, + onColorSelected: onColorSelected ?? (_) {}, + ), + ), + ), + ), + ); + + testWidgets('offers one chip per palette colour', (tester) async { + await tester.pumpWidget(harness()); + + expect(find.byType(TagChip), findsNWidgets(TagColors.predefined.length)); + }); + + testWidgets('paints each swatch as the chip, not as the raw hex', ( + tester, + ) async { + await tester.pumpWidget(harness()); + + final expected = tagChipColorsFor( + seed: TagColors.fromHex(TagColors.predefined.first), + surface: Colors.white, + ); + + final material = tester.widget( + find + .descendant( + of: find.byType(TagChip).first, + matching: find.byType(Material), + ) + .first, + ); + + expect(material.color, expected.fill); + expect( + material.color, + isNot(TagColors.fromHex(TagColors.predefined.first)), + reason: 'a swatch showing the raw hex is the mismatch this fixes', + ); + }); + + testWidgets('shows the name being typed inside every swatch', (tester) async { + final controller = TextEditingController(text: 'Shore'); + addTearDown(controller.dispose); + + await tester.pumpWidget(harness(nameController: controller)); + + expect( + find.text('Shore'), + findsNWidgets(TagColors.predefined.length), + reason: 'the swatch is a preview of this tag, not of a generic one', + ); + }); + + testWidgets('follows the name as it is typed', (tester) async { + final controller = TextEditingController(text: 'Shore'); + addTearDown(controller.dispose); + + await tester.pumpWidget(harness(nameController: controller)); + controller.text = 'Night'; + await tester.pump(); + + expect(find.text('Night'), findsNWidgets(TagColors.predefined.length)); + expect(find.text('Shore'), findsNothing); + }); + + testWidgets('still offers every colour when no name has been typed', ( + tester, + ) async { + final controller = TextEditingController(); + addTearDown(controller.dispose); + + await tester.pumpWidget(harness(nameController: controller)); + + expect(find.byType(TagChip), findsNWidgets(TagColors.predefined.length)); + }); + + testWidgets('reports the hex of the swatch that was tapped', (tester) async { + String? picked; + await tester.pumpWidget(harness(onColorSelected: (hex) => picked = hex)); + + await tester.tap(find.byType(TagChip).at(2)); + await tester.pumpAndSettle(); + + expect(picked, TagColors.predefined[2]); + }); + + testWidgets('marks the chosen swatch as selected for assistive tech', ( + tester, + ) async { + final chosen = TagColors.predefined[3]; + await tester.pumpWidget(harness(selectedColor: chosen)); + + final semantics = tester + .widgetList(find.byType(Semantics)) + .where((s) => s.properties.selected == true); + + expect(semantics, hasLength(1)); + }); + + group('a swatch is a reachable tap target', () { + // The swatch is the control the diver taps to choose a colour, so it + // answers to the same floor as the chip's own close button: 48 dp where + // a finger points, 32 dp where a mouse does. The dots it replaced were + // 28 dp and the dense chip on its own is 29, so neither the old grid nor + // the new one cleared either floor until this was measured. + for (final (platform, floor) in [ + (TargetPlatform.android, 48.0), + (TargetPlatform.iOS, 48.0), + (TargetPlatform.macOS, 32.0), + (TargetPlatform.windows, 32.0), + (TargetPlatform.linux, 32.0), + ]) { + testWidgets('$platform reaches $floor', (tester) async { + await tester.pumpWidget(harness(platform: platform)); + + final swatch = tester.getSize(find.byType(GestureDetector).first); + + expect(swatch.height, greaterThanOrEqualTo(floor)); + expect(swatch.width, greaterThanOrEqualTo(floor)); + }); + } + }); +} diff --git a/test/features/tags/presentation/widgets/tag_merge_sheet_test.dart b/test/features/tags/presentation/widgets/tag_merge_sheet_test.dart index 27eb2e8dca..5380c121b4 100644 --- a/test/features/tags/presentation/widgets/tag_merge_sheet_test.dart +++ b/test/features/tags/presentation/widgets/tag_merge_sheet_test.dart @@ -180,6 +180,10 @@ void main() { // Tap the Merge button final mergeButton = find.widgetWithText(FilledButton, 'Merge'); expect(mergeButton, findsOneWidget); + // Scrolled into view first. This assertion is a negative one, so a tap + // that missed would pass it without ever exercising the empty-name + // guard, and the swatches above the button grew in issue #2269. + await tester.ensureVisible(mergeButton); await tester.tap(mergeButton); await tester.pumpAndSettle();