From 34d05f4a5746a562ac28b717dc1c4605c1324e58 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Tue, 22 Sep 2026 12:21:30 -0400 Subject: [PATCH 1/3] fix(tags): show the tint a tag displays, in the app and in Settings Tags are a quiet tint of the diver's colour again, the way they looked before #2255 flooded the chip with that colour outright, and the colour swatches in Settings > Manage > Tags now offer that tint rather than the raw hex the tag is stored as. #2255 answered issue #2254, that Settings promised a colour the app never painted, by moving the app to the stored colour. The complaint is better answered the other way round: keep the quiet chips and correct what Settings promises. The tint has to become a value for that to be possible at all. A 15 per cent translucent fill is not a colour but a recipe whose result depends on the surface behind it, which is what #2254 measured: amber #F59E0B came out F7E7CF on a detail card, F2E8D7 on a dive row and C4C2B7 on a selected one, where the row's blue-grey bled through. A swatch cannot represent a display colour that does not exist. tagChipColors resolves the tint once against the theme surface and returns it opaque, so the pixel on a plain card is what it always was and the drift onto selected rows is gone. One tint, not the 0.15 of a dive row and the 0.2 of a detail card, since two tints would give a tag two display colours again. The label keeps the tag's hue and its saturation and gives up only lightness, moving away from the fill until it clears WCAG AA. The pre-#2255 label was the raw colour, about 1.7:1 on its own tint at the pale end of the palette. Darkening in HSL rather than blending towards a dark neutral is deliberate: an RGB blend compresses the channel spread and cost the saturated mid tones such as #A855F7 nearly half their saturation. The direction follows the fill, so one rule serves both themes. Every TagColorPicker swatch is now a real TagChip drawn by that same function, carrying the name as it is typed, in the Add and Edit dialogs and in the merge sheet. The Manage list row's solid CircleAvatar becomes the tag's own chip, which was the last place in the app still claiming the stored hex was what a tag looks like. The 48/32 dp close-button tap target from #2255 stays. It is an accessibility floor and has nothing to do with the fill it arrived with. Tests were written first and mutation tested: restoring the opaque fill, the translucent fill or the 0.2 tint each turns them red. Closes #2269 --- .../presentation/pages/tag_manage_page.dart | 20 ++- .../tags/presentation/tag_chip_colors.dart | 88 +++++++++++ .../tags/presentation/tag_color_contrast.dart | 30 ---- .../tags/presentation/widgets/tag_chip.dart | 37 +++-- .../widgets/tag_input_widget.dart | 139 ++++++++++++----- .../presentation/widgets/tag_merge_sheet.dart | 3 + .../pages/tag_manage_page_test.dart | 60 +++++++- .../presentation/tag_chip_colors_test.dart | 123 +++++++++++++++ .../presentation/tag_color_contrast_test.dart | 45 ------ .../presentation/widgets/tag_chip_test.dart | 145 +++++++++++------- .../widgets/tag_color_picker_test.dart | 134 ++++++++++++++++ 11 files changed, 639 insertions(+), 185 deletions(-) create mode 100644 lib/features/tags/presentation/tag_chip_colors.dart delete mode 100644 lib/features/tags/presentation/tag_color_contrast.dart create mode 100644 test/features/tags/presentation/tag_chip_colors_test.dart delete mode 100644 test/features/tags/presentation/tag_color_contrast_test.dart create mode 100644 test/features/tags/presentation/widgets/tag_color_picker_test.dart 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..05ba6499d2 --- /dev/null +++ b/lib/features/tags/presentation/tag_chip_colors.dart @@ -0,0 +1,88 @@ +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. +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 follows the fill, so the one rule serves both themes: the +/// label darkens on a pale chip and lightens on a dark one. +Color _labelOn(Color fill, Color seed) { + if (tagContrastRatio(seed, fill) >= _minLabelContrast) return seed; + + final hsl = HSLColor.fromColor(seed); + final darken = fill.computeLuminance() > seed.computeLuminance(); + + for (var step = 1; step <= _labelSteps; step++) { + final t = step / _labelSteps; + final lightness = darken + ? hsl.lightness * (1 - t) + : hsl.lightness + (1 - hsl.lightness) * t; + final candidate = hsl.withLightness(lightness.clamp(0.0, 1.0)).toColor(); + if (tagContrastRatio(candidate, fill) >= _minLabelContrast) { + return candidate; + } + } + return darken ? Colors.black : Colors.white; +} + +/// 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..68f6e31900 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. diff --git a/lib/features/tags/presentation/widgets/tag_input_widget.dart b/lib/features/tags/presentation/widgets/tag_input_widget.dart index c7f04d1f3e..da35ea18b3 100644 --- a/lib/features/tags/presentation/widgets/tag_input_widget.dart +++ b/lib/features/tags/presentation/widgets/tag_input_widget.dart @@ -277,57 +277,124 @@ 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), - child: Container( - width: 28, - height: 28, - 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, + 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; + + return Semantics( + button: true, + label: 'Select color $hex', + selected: isSelected, + child: GestureDetector( + onTap: onTap, + child: Container( + padding: const EdgeInsets.all(3), + decoration: BoxDecoration( + 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_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..407326f244 --- /dev/null +++ b/test/features/tags/presentation/tag_chip_colors_test.dart @@ -0,0 +1,123 @@ +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('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..1103687ed2 --- /dev/null +++ b/test/features/tags/presentation/widgets/tag_color_picker_test.dart @@ -0,0 +1,134 @@ +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, + }) => MaterialApp( + theme: theme, + 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)); + }); +} From 950206050113aae9ec10282a112ae2228cb91661 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Tue, 22 Sep 2026 13:13:06 -0400 Subject: [PATCH 2/3] fix(tags): read the label's direction off the fill, not off the seed The one line Codecov flagged as uncovered was uncovered because it only ran in a broken case. _labelOn chose which way to move the label by comparing the fill's luminance to the seed's, walked that way, exhausted its steps and fell through to a black or white that had already failed. The comparison is wrong at the ends of the range. A tag stored near black on a dark theme has a fill LIGHTER than itself, since the fill is mostly surface, so the label walked towards black, into the fill, and came out at 1.1:1. A white tag on a light theme was worse: white on white, 1.0:1. Tag.colorHex is free-form, so an imported tag can carry either. The direction is now read off the fill alone, taking whichever end of the lightness range has contrast room against it. Black clears AA for any fill above about 0.175 luminance and white for any below about 0.183, so the ranges overlap and some end always works, which makes the walk terminate by construction and retires the fallback. Output is unchanged for all twenty palette colours: the lerp is algebraically what it was, and only the choice of endpoint moved. Covered by two tests, a near-black tag on a dark surface and a near-white tag on a light one, each of which fails at about 1:1 without the fix. Patch coverage is now 100 per cent. Refs #2269 --- .../tags/presentation/tag_chip_colors.dart | 33 ++++++++++++++----- .../presentation/tag_chip_colors_test.dart | 33 +++++++++++++++++++ 2 files changed, 57 insertions(+), 9 deletions(-) diff --git a/lib/features/tags/presentation/tag_chip_colors.dart b/lib/features/tags/presentation/tag_chip_colors.dart index 05ba6499d2..b550cf9513 100644 --- a/lib/features/tags/presentation/tag_chip_colors.dart +++ b/lib/features/tags/presentation/tag_chip_colors.dart @@ -56,25 +56,40 @@ TagChipColors tagChipColorsFor({required Color seed, required Color surface}) { /// compresses the channel spread, which cost the saturated mid tones such as /// `#A855F7` nearly half their saturation on the way down. /// -/// The direction follows the fill, so the one rule serves both themes: the -/// label darkens on a pale chip and lightens on a dark one. +/// 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); - final darken = fill.computeLuminance() > seed.computeLuminance(); - for (var step = 1; step <= _labelSteps; step++) { + // 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 lightness = darken - ? hsl.lightness * (1 - t) - : hsl.lightness + (1 - hsl.lightness) * t; - final candidate = hsl.withLightness(lightness.clamp(0.0, 1.0)).toColor(); + final candidate = hsl + .withLightness(hsl.lightness + (endpoint - hsl.lightness) * t) + .toColor(); if (tagContrastRatio(candidate, fill) >= _minLabelContrast) { return candidate; } } - return darken ? Colors.black : Colors.white; + return hsl.withLightness(endpoint).toColor(); } /// WCAG 2.1 contrast ratio between two opaque colours, from 1 (identical) to diff --git a/test/features/tags/presentation/tag_chip_colors_test.dart b/test/features/tags/presentation/tag_chip_colors_test.dart index 407326f244..b8dc84b2f0 100644 --- a/test/features/tags/presentation/tag_chip_colors_test.dart +++ b/test/features/tags/presentation/tag_chip_colors_test.dart @@ -97,6 +97,39 @@ void main() { } }); + 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 From 9ce90c95ae2c09e78d57e366a62c3b0aa1d69ee5 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Tue, 22 Sep 2026 14:14:13 -0400 Subject: [PATCH 3/3] fix(tags): give a colour swatch the tap target of the control it is The swatches in Settings > Manage > Tags measured 79 by 29 on every platform, under the 48 dp touch floor and under the 32 dp pointer floor alike. The dots they replaced were 28 dp and cleared neither either, so this is an old gap rather than a new one, but it sat oddly beside a change that had just kept the chip's close button at its own floor. The 48/32 policy moves out of _DeleteButton to a shared tagTapTarget, so the close button and the swatch answer to one rule rather than two copies of it, and each swatch now reserves that box around its chip and hit tests opaquely across it. The chip itself is untouched at 29 dp: only the reachable area grew, from 177 to 272 dp of picker on touch and to 192 on a pointer, which the dialog's existing scroll view carries. Three scope tests and one merge test tapped controls that the taller picker had pushed past the fold. The merge one matters most: it asserts that an empty name does NOT merge, so a tap that missed would have passed it without ever reaching the guard. All four scroll their target into view first, and the tags suite now reports no missed taps at all. While measuring, the choice of ColorScheme.surface as the tint's fixed reference is now written down in tagChipColors. A chip on a card sits on surfaceContainerLow or higher, and tinting against whichever container the chip happens to occupy would hand back a colour per surface, which is the bug the function exists to prevent. Refs #2269 --- .../tags/presentation/tag_chip_colors.dart | 9 +++ .../tags/presentation/widgets/tag_chip.dart | 25 ++++---- .../widgets/tag_input_widget.dart | 59 +++++++++++-------- .../pages/tag_manage_page_scope_test.dart | 19 ++++-- .../widgets/tag_color_picker_test.dart | 27 ++++++++- .../widgets/tag_merge_sheet_test.dart | 4 ++ 6 files changed, 102 insertions(+), 41 deletions(-) diff --git a/lib/features/tags/presentation/tag_chip_colors.dart b/lib/features/tags/presentation/tag_chip_colors.dart index b550cf9513..10f7ad558d 100644 --- a/lib/features/tags/presentation/tag_chip_colors.dart +++ b/lib/features/tags/presentation/tag_chip_colors.dart @@ -30,6 +30,15 @@ const int _labelSteps = 20; /// 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, diff --git a/lib/features/tags/presentation/widgets/tag_chip.dart b/lib/features/tags/presentation/widgets/tag_chip.dart index 68f6e31900..891a375d2d 100644 --- a/lib/features/tags/presentation/widgets/tag_chip.dart +++ b/lib/features/tags/presentation/widgets/tag_chip.dart @@ -130,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 @@ -147,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 da35ea18b3..95e31c9f74 100644 --- a/lib/features/tags/presentation/widgets/tag_input_widget.dart +++ b/lib/features/tags/presentation/widgets/tag_input_widget.dart @@ -360,6 +360,7 @@ class _ColorSwatch extends StatelessWidget { @override Widget build(BuildContext context) { final scheme = Theme.of(context).colorScheme; + final target = tagTapTarget(Theme.of(context).platform); return Semantics( button: true, @@ -367,29 +368,41 @@ class _ColorSwatch extends StatelessWidget { selected: isSelected, child: GestureDetector( onTap: onTap, - child: Container( - padding: const EdgeInsets.all(3), - decoration: BoxDecoration( - 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, + // 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( + padding: const EdgeInsets.all(3), + decoration: BoxDecoration( + 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, + ), + ), ), ), ), 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/widgets/tag_color_picker_test.dart b/test/features/tags/presentation/widgets/tag_color_picker_test.dart index 1103687ed2..e39d34d470 100644 --- a/test/features/tags/presentation/widgets/tag_color_picker_test.dart +++ b/test/features/tags/presentation/widgets/tag_color_picker_test.dart @@ -24,8 +24,9 @@ void main() { String? selectedColor, TextEditingController? nameController, void Function(String)? onColorSelected, + TargetPlatform? platform, }) => MaterialApp( - theme: theme, + theme: platform == null ? theme : theme.copyWith(platform: platform), home: Scaffold( body: SingleChildScrollView( child: SizedBox( @@ -131,4 +132,28 @@ void main() { 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();