From c8ba7407b2efa1e18156f7fdaed99a9bddd30dc1 Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Mon, 21 Sep 2026 17:21:33 -0400 Subject: [PATCH 1/2] fix(tags): show a tag in its own colour, not a tint of it A tag's colour in Settings > Manage > Tags was not the colour the tag showed anywhere else. The Manage page paints the stored colour; every chip painted `tag.color.withValues(alpha: 0.15)` or `0.2` over whatever surface happened to be behind it. A translucent fill is not a colour but a recipe, so the amber tag `#F59E0B` resolved to F7E7CF on a detail card, F2E8D7 on a list row and C4C2B7 on a selected row, where the row's blue-grey bled through and the tag stopped reading as amber at all. Every tag now renders through one TagChip, filled with Tag.color at full opacity. The label colour is chosen per chip by WCAG contrast ratio, so the pale yellow and the near-black slate of the palette both stay readable. The recipe was duplicated across eight call sites, which is why each one drifted on its own; they now share the widget. The dive filter sheet and the search page keep FilterChip's selection semantics and gain the exact-colour avatar dot that the site and equipment filter sheets already used, replacing their tinted selectedColor. Closes #2254 --- .../presentation/pages/dive_detail_page.dart | 11 +- .../presentation/pages/dive_search_page.dart | 13 +- .../widgets/dive_filter_sheet.dart | 18 +-- .../presentation/widgets/site_list_tile.dart | 13 +- .../presentation/widgets/site_tags_card.dart | 11 +- .../widgets/equipment_tag_chips.dart | 11 +- .../widgets/import_tags_field.dart | 18 +-- .../tags/presentation/tag_color_contrast.dart | 30 ++++ .../tags/presentation/widgets/tag_chip.dart | 144 ++++++++++++++++++ .../widgets/tag_input_widget.dart | 27 +--- .../pages/bulk_membership_wiring_test.dart | 3 +- .../pages/dive_edit_tag_picker_test.dart | 3 +- .../pages/site_detail_page_test.dart | 3 +- .../widgets/site_tags_card_test.dart | 5 +- .../pages/equipment_edit_tags_test.dart | 9 +- .../widgets/import_tags_field_test.dart | 9 +- .../presentation/tag_color_contrast_test.dart | 45 ++++++ .../presentation/widgets/tag_chip_test.dart | 113 ++++++++++++++ 18 files changed, 395 insertions(+), 91 deletions(-) create mode 100644 lib/features/tags/presentation/tag_color_contrast.dart create mode 100644 lib/features/tags/presentation/widgets/tag_chip.dart create mode 100644 test/features/tags/presentation/tag_color_contrast_test.dart create mode 100644 test/features/tags/presentation/widgets/tag_chip_test.dart diff --git a/lib/features/dive_log/presentation/pages/dive_detail_page.dart b/lib/features/dive_log/presentation/pages/dive_detail_page.dart index 906bc80d75..9eb5efcabc 100644 --- a/lib/features/dive_log/presentation/pages/dive_detail_page.dart +++ b/lib/features/dive_log/presentation/pages/dive_detail_page.dart @@ -13,6 +13,7 @@ import 'package:latlong2/latlong.dart'; import 'package:libdivecomputer_plugin/libdivecomputer_plugin.dart' as pigeon; import 'package:submersion/features/equipment/data/services/sensor_summary_scheduler.dart'; import 'package:submersion/features/equipment/presentation/widgets/observation_status_chip.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/shared/widgets/profile_photo/profile_avatar.dart'; import 'package:submersion/core/constants/dive_detail_layout.dart'; import 'package:submersion/core/constants/dive_detail_section_pairs.dart'; @@ -4153,14 +4154,10 @@ class _DiveDetailPageState extends ConsumerState { runSpacing: 8, children: dive.tags .map( - (tag) => ActionChip( - label: Text(tag.name), + (tag) => TagChip( + tag: tag, tooltip: context.l10n.tags_action_showDives(tag.name), - backgroundColor: tag.color.withValues(alpha: 0.2), - side: BorderSide(color: tag.color), - labelStyle: TextStyle(color: tag.color), - visualDensity: VisualDensity.compact, - onPressed: () => openDivesWithTag(context, ref, tag.id), + onTap: () => openDivesWithTag(context, ref, tag.id), ), ) .toList(), diff --git a/lib/features/dive_log/presentation/pages/dive_search_page.dart b/lib/features/dive_log/presentation/pages/dive_search_page.dart index 6b65d4265c..948edc9795 100644 --- a/lib/features/dive_log/presentation/pages/dive_search_page.dart +++ b/lib/features/dive_log/presentation/pages/dive_search_page.dart @@ -887,13 +887,16 @@ class _DiveSearchPageState extends ConsumerState { children: allTags.map((tag) { final isSelected = _selectedTagIds.contains(tag.id); return FilterChip( + // The dot carries the tag's own colour, exactly as the + // site and equipment filter sheets show it. A tinted + // chip body would report a different colour on every + // surface (issue #2254). + avatar: CircleAvatar( + backgroundColor: tag.color, + radius: 6, + ), label: Text(tag.name), selected: isSelected, - selectedColor: tag.color.withValues(alpha: 0.3), - checkmarkColor: tag.color, - side: BorderSide( - color: isSelected ? tag.color : Colors.grey.shade300, - ), onSelected: (selected) { setState(() { if (selected) { diff --git a/lib/features/dive_log/presentation/widgets/dive_filter_sheet.dart b/lib/features/dive_log/presentation/widgets/dive_filter_sheet.dart index 217f2ce915..80ad0a4916 100644 --- a/lib/features/dive_log/presentation/widgets/dive_filter_sheet.dart +++ b/lib/features/dive_log/presentation/widgets/dive_filter_sheet.dart @@ -783,17 +783,17 @@ class _DiveFilterSheetState extends ConsumerState { tag.id, ); return FilterChip( + // The dot carries the tag's own colour, + // exactly as the site and equipment + // filter sheets show it. A tinted chip + // body would report a different colour on + // every surface (issue #2254). + avatar: CircleAvatar( + backgroundColor: tag.color, + radius: 6, + ), label: Text(tag.name), selected: isSelected, - selectedColor: tag.color.withValues( - alpha: 0.3, - ), - checkmarkColor: tag.color, - side: BorderSide( - color: isSelected - ? tag.color - : Colors.grey.shade300, - ), onSelected: (selected) { setState(() { if (selected) { diff --git a/lib/features/dive_sites/presentation/widgets/site_list_tile.dart b/lib/features/dive_sites/presentation/widgets/site_list_tile.dart index fc8656058f..f59a047347 100644 --- a/lib/features/dive_sites/presentation/widgets/site_list_tile.dart +++ b/lib/features/dive_sites/presentation/widgets/site_list_tile.dart @@ -16,6 +16,7 @@ import 'package:submersion/features/maps/presentation/widgets/trackpad_zoom_map. import 'package:submersion/features/settings/presentation/providers/settings_providers.dart'; import 'package:submersion/features/site_scape/presentation/site_feature_glyph.dart'; import 'package:submersion/features/site_scape/presentation/site_feature_sheet.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/l10n_extension.dart'; import 'package:submersion/shared/selection/selection_inset.dart'; import 'package:submersion/shared/selection/selection_leading.dart'; @@ -163,14 +164,10 @@ class _SiteListTileState extends ConsumerState { color: SiteFeatureGlyph.styleFor(typeName).$2, textColor: chipTextColor, ), - // Tags (issue #1765): the first three, then a count of the rest. - for (final tag in shownTags) - _SiteChip( - icon: Icons.sell_outlined, - label: tag.name, - color: tag.color, - textColor: chipTextColor, - ), + // Tags (issue #1765): the first three, then a count of the rest. A tag + // carries the diver's own colour, so it is filled with it rather than + // outlined like the type and feature chips (issue #2254). + for (final tag in shownTags) TagChip(tag: tag, dense: true), if (hiddenTagCount > 0) _SiteChip( icon: Icons.sell_outlined, diff --git a/lib/features/dive_sites/presentation/widgets/site_tags_card.dart b/lib/features/dive_sites/presentation/widgets/site_tags_card.dart index de3a97acf2..7e17b7a854 100644 --- a/lib/features/dive_sites/presentation/widgets/site_tags_card.dart +++ b/lib/features/dive_sites/presentation/widgets/site_tags_card.dart @@ -3,6 +3,7 @@ import 'package:flutter/material.dart'; import 'package:submersion/core/providers/provider.dart'; import 'package:submersion/features/dive_sites/presentation/providers/site_providers.dart'; import 'package:submersion/features/dive_sites/presentation/site_tag_navigation.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/l10n_extension.dart'; /// The Tags card on site detail (issue #1765), built like the dive detail @@ -52,14 +53,10 @@ class SiteTagsCard extends ConsumerWidget { runSpacing: 8, children: [ for (final tag in tags) - ActionChip( - label: Text(tag.name), + TagChip( + tag: tag, tooltip: l10n.diveSites_detail_showSitesWith(tag.name), - backgroundColor: tag.color.withValues(alpha: 0.2), - side: BorderSide(color: tag.color), - labelStyle: TextStyle(color: tag.color), - visualDensity: VisualDensity.compact, - onPressed: () => openSitesWithTag(context, ref, tag.id), + onTap: () => openSitesWithTag(context, ref, tag.id), ), ], ), diff --git a/lib/features/equipment/presentation/widgets/equipment_tag_chips.dart b/lib/features/equipment/presentation/widgets/equipment_tag_chips.dart index 7f25b08399..4384cf5ac5 100644 --- a/lib/features/equipment/presentation/widgets/equipment_tag_chips.dart +++ b/lib/features/equipment/presentation/widgets/equipment_tag_chips.dart @@ -3,6 +3,7 @@ import 'package:flutter/material.dart'; import 'package:submersion/core/providers/provider.dart'; import 'package:submersion/features/equipment/presentation/equipment_tag_navigation.dart'; import 'package:submersion/features/equipment/presentation/providers/equipment_tag_providers.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/l10n_extension.dart'; /// An equipment item's tags as colored chips, under the name in the detail @@ -27,16 +28,12 @@ class EquipmentTagChips extends ConsumerWidget { runSpacing: 8, children: [ for (final tag in tags) - ActionChip( - label: Text(tag.name), + TagChip( + tag: tag, tooltip: context.l10n.equipment_detail_showEquipmentWith( tag.name, ), - backgroundColor: tag.color.withValues(alpha: 0.2), - side: BorderSide(color: tag.color), - labelStyle: TextStyle(color: tag.color), - visualDensity: VisualDensity.compact, - onPressed: () => openEquipmentWithTag(context, ref, tag.id), + onTap: () => openEquipmentWithTag(context, ref, tag.id), ), ], ), diff --git a/lib/features/import_wizard/presentation/widgets/import_tags_field.dart b/lib/features/import_wizard/presentation/widgets/import_tags_field.dart index 1d6de97341..9978f02bb5 100644 --- a/lib/features/import_wizard/presentation/widgets/import_tags_field.dart +++ b/lib/features/import_wizard/presentation/widgets/import_tags_field.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:submersion/features/import_wizard/domain/models/tag_selection.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/l10n_extension.dart'; import 'package:submersion/shared/widgets/forms/autocomplete_options_list.dart'; @@ -138,18 +139,11 @@ class _ImportTagsFieldState extends State { ], ), for (var i = 0; i < widget.tags.length; i++) - () { - final tagColor = _resolveColor(widget.tags[i]); - return Chip( - label: Text(widget.tags[i].name), - backgroundColor: tagColor.withValues(alpha: 0.2), - side: BorderSide(color: tagColor), - labelStyle: TextStyle(color: tagColor), - deleteIcon: Icon(Icons.close, size: 18, color: tagColor), - onDeleted: () => widget.onRemove(i), - visualDensity: VisualDensity.compact, - ); - }(), + TagChip.unsaved( + name: widget.tags[i].name, + color: _resolveColor(widget.tags[i]), + onDeleted: () => widget.onRemove(i), + ), IntrinsicWidth( child: TextField( controller: controller, diff --git a/lib/features/tags/presentation/tag_color_contrast.dart b/lib/features/tags/presentation/tag_color_contrast.dart new file mode 100644 index 0000000000..1506a9c76c --- /dev/null +++ b/lib/features/tags/presentation/tag_color_contrast.dart @@ -0,0 +1,30 @@ +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 new file mode 100644 index 0000000000..7eed5aeb07 --- /dev/null +++ b/lib/features/tags/presentation/widgets/tag_chip.dart @@ -0,0 +1,144 @@ +import 'package:flutter/material.dart'; + +import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/tag_color_contrast.dart'; + +/// A tag shown in the colour the diver gave it (issue #2254). +/// +/// 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. +class TagChip extends StatelessWidget { + TagChip({ + super.key, + required Tag tag, + this.onTap, + this.onDeleted, + this.tooltip, + this.deleteTooltip, + this.dense = false, + }) : name = tag.name, + color = tag.color; + + /// A tag with no row of its own yet, such as one typed into the import + /// wizard before the import runs. + const TagChip.unsaved({ + super.key, + required this.name, + required this.color, + this.onTap, + this.onDeleted, + this.tooltip, + this.deleteTooltip, + this.dense = false, + }); + + final String name; + + /// The fill, painted opaque. [Tag.color] for a stored tag. + final Color color; + + /// Tapping the chip, usually to open what carries the tag. + final VoidCallback? onTap; + + /// Removing the tag. The chip grows a close button when this is set. + final VoidCallback? onDeleted; + + final String? tooltip; + final String? deleteTooltip; + + /// The tighter type and padding for a list row, where several chips share + /// a line under the dive's stats. + final bool dense; + + @override + Widget build(BuildContext context) { + final foreground = tagForegroundColor(color); + final borderRadius = BorderRadius.circular(dense ? 4 : 8); + final textStyle = Theme.of(context).textTheme.bodySmall?.copyWith( + color: foreground, + fontSize: dense ? 11 : null, + ); + + Widget chip = Material( + color: color, + borderRadius: borderRadius, + child: InkWell( + onTap: onTap, + borderRadius: borderRadius, + child: Padding( + padding: dense + ? const EdgeInsets.symmetric(horizontal: 6, vertical: 2) + : const EdgeInsets.symmetric(horizontal: 10, vertical: 4), + child: Row( + mainAxisSize: MainAxisSize.min, + children: [ + Flexible( + child: Text( + name, + style: textStyle, + maxLines: 1, + overflow: TextOverflow.ellipsis, + ), + ), + if (onDeleted != null) ...[ + const SizedBox(width: 4), + _DeleteButton( + color: foreground, + // 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. + tooltip: + deleteTooltip ?? + MaterialLocalizations.of(context).deleteButtonTooltip, + onPressed: onDeleted!, + ), + ], + ], + ), + ), + ), + ); + + if (tooltip != null) { + chip = Tooltip(message: tooltip!, child: chip); + } + return chip; + } +} + +/// The close button of a removable chip. It is an [InkResponse] rather than +/// an [IconButton] so the icon can stay chip sized while the tap target keeps +/// its own radius. +class _DeleteButton extends StatelessWidget { + const _DeleteButton({ + required this.color, + required this.onPressed, + this.tooltip, + }); + + final Color color; + final VoidCallback onPressed; + final String? tooltip; + + @override + Widget build(BuildContext context) { + final button = InkResponse( + onTap: onPressed, + radius: 16, + child: Icon(Icons.close, size: 16, color: color), + ); + return Semantics( + button: true, + label: tooltip, + child: tooltip == null + ? button + : Tooltip(message: tooltip!, child: button), + ); + } +} diff --git a/lib/features/tags/presentation/widgets/tag_input_widget.dart b/lib/features/tags/presentation/widgets/tag_input_widget.dart index b02d81cc4b..c7f04d1f3e 100644 --- a/lib/features/tags/presentation/widgets/tag_input_widget.dart +++ b/lib/features/tags/presentation/widgets/tag_input_widget.dart @@ -2,6 +2,7 @@ import 'package:flutter/material.dart'; import 'package:submersion/core/providers/provider.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/l10n_extension.dart'; import 'package:submersion/features/tags/presentation/providers/tag_providers.dart'; @@ -81,16 +82,9 @@ class _TagInputWidgetState extends ConsumerState { spacing: 8, runSpacing: 4, children: widget.selectedTags.map((tag) { - return Chip( - label: Text(tag.name), - backgroundColor: tag.color.withValues(alpha: 0.2), - side: BorderSide(color: tag.color), - deleteIcon: widget.enabled - ? Icon(Icons.close, size: 18, color: tag.color) - : null, + return TagChip( + tag: tag, onDeleted: widget.enabled ? () => _removeTag(tag) : null, - labelStyle: TextStyle(color: tag.color), - visualDensity: VisualDensity.compact, ); }).toList(), ), @@ -265,20 +259,7 @@ class TagChips extends StatelessWidget { spacing: 4, runSpacing: 2, children: [ - ...displayTags.map( - (tag) => Container( - padding: const EdgeInsets.symmetric(horizontal: 6, vertical: 2), - decoration: BoxDecoration( - color: tag.color.withValues(alpha: 0.15), - borderRadius: BorderRadius.circular(4), - border: Border.all(color: tag.color.withValues(alpha: 0.3)), - ), - child: Text( - tag.name, - style: TextStyle(fontSize: 11, color: tag.color), - ), - ), - ), + ...displayTags.map((tag) => TagChip(tag: tag, dense: true)), if (remaining > 0) Container( padding: const EdgeInsets.symmetric(horizontal: 6, vertical: 2), diff --git a/test/features/dive_log/presentation/pages/bulk_membership_wiring_test.dart b/test/features/dive_log/presentation/pages/bulk_membership_wiring_test.dart index 26bfe8a943..434561d181 100644 --- a/test/features/dive_log/presentation/pages/bulk_membership_wiring_test.dart +++ b/test/features/dive_log/presentation/pages/bulk_membership_wiring_test.dart @@ -12,6 +12,7 @@ import 'package:submersion/features/dive_log/data/repositories/dive_repository_i import 'package:submersion/features/dive_log/domain/entities/dive.dart'; import 'package:submersion/features/dive_log/presentation/pages/dive_edit_page.dart'; import 'package:submersion/features/dive_log/presentation/providers/dive_providers.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/shared/bulk_edit/bulk_membership_editor.dart'; import 'package:submersion/features/dive_log/presentation/widgets/pickers/equipment_picker_sheet.dart'; import 'package:submersion/features/dive_log/presentation/widgets/pickers/equipment_set_picker_sheet.dart'; @@ -659,7 +660,7 @@ void main() { // Back in the dialog with the browsed tag staged as a chip. expect(find.byType(TagPickerSheet), findsNothing); - expect(find.widgetWithText(Chip, 'Nitrox'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Nitrox'), findsOneWidget); await tester.tap( find.descendant( diff --git a/test/features/dive_log/presentation/pages/dive_edit_tag_picker_test.dart b/test/features/dive_log/presentation/pages/dive_edit_tag_picker_test.dart index 521427f264..e35fc72086 100644 --- a/test/features/dive_log/presentation/pages/dive_edit_tag_picker_test.dart +++ b/test/features/dive_log/presentation/pages/dive_edit_tag_picker_test.dart @@ -8,6 +8,7 @@ import 'package:submersion/features/divers/presentation/providers/diver_provider 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/providers/tag_providers.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/features/tags/presentation/widgets/tag_picker_sheet.dart'; import 'package:submersion/features/tank_presets/presentation/providers/tank_preset_providers.dart'; import 'package:submersion/l10n/arb/app_localizations.dart'; @@ -103,6 +104,6 @@ void main() { // Sheet dismissed, and the picked tag is now a chip on the form. expect(find.byType(TagPickerSheet), findsNothing); - expect(find.widgetWithText(Chip, 'Wreck'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Wreck'), findsOneWidget); }); } diff --git a/test/features/dive_sites/presentation/pages/site_detail_page_test.dart b/test/features/dive_sites/presentation/pages/site_detail_page_test.dart index 27eb3dc780..61346d6b33 100644 --- a/test/features/dive_sites/presentation/pages/site_detail_page_test.dart +++ b/test/features/dive_sites/presentation/pages/site_detail_page_test.dart @@ -25,6 +25,7 @@ import 'package:submersion/features/site_scape/presentation/site_scape_view.dart import 'package:submersion/features/site_scape/presentation/site_terrain_pane.dart'; import 'package:submersion/features/site_types/domain/entities/site_type_entity.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/arb/app_localizations.dart'; import '../../../../helpers/mock_providers.dart'; @@ -1359,7 +1360,7 @@ void main() { await tester.scrollUntilVisible(find.text('To try'), 200); expect(find.text('Tags'), findsOneWidget); - expect(find.widgetWithText(ActionChip, 'To try'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'To try'), findsOneWidget); }); }); }); diff --git a/test/features/dive_sites/presentation/widgets/site_tags_card_test.dart b/test/features/dive_sites/presentation/widgets/site_tags_card_test.dart index 3457caa88f..7a33458de7 100644 --- a/test/features/dive_sites/presentation/widgets/site_tags_card_test.dart +++ b/test/features/dive_sites/presentation/widgets/site_tags_card_test.dart @@ -5,6 +5,7 @@ import 'package:submersion/core/providers/provider.dart'; import 'package:submersion/features/dive_sites/presentation/providers/site_providers.dart'; import 'package:submersion/features/dive_sites/presentation/widgets/site_tags_card.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/arb/app_localizations.dart'; /// The Tags card on site detail (issue #1765), the twin of the dive one. @@ -57,8 +58,8 @@ void main() { expect(find.byType(Card), findsOneWidget); expect(find.text('Tags'), findsOneWidget); expect(find.text('2 tags'), findsOneWidget); - expect(find.widgetWithText(ActionChip, 'To try'), findsOneWidget); - expect(find.widgetWithText(ActionChip, 'Avoid'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'To try'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Avoid'), findsOneWidget); }); testWidgets('tapping a tag opens the site list filtered to it', ( diff --git a/test/features/equipment/presentation/pages/equipment_edit_tags_test.dart b/test/features/equipment/presentation/pages/equipment_edit_tags_test.dart index 01f09828ad..7bf0f3667c 100644 --- a/test/features/equipment/presentation/pages/equipment_edit_tags_test.dart +++ b/test/features/equipment/presentation/pages/equipment_edit_tags_test.dart @@ -12,6 +12,7 @@ import 'package:submersion/features/equipment/presentation/pages/equipment_edit_ import 'package:submersion/features/equipment/presentation/providers/equipment_providers.dart'; import 'package:submersion/features/equipment/presentation/providers/equipment_tag_providers.dart'; import 'package:submersion/features/tags/domain/entities/tag.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/l10n/arb/app_localizations.dart'; @@ -88,7 +89,7 @@ void main() { tester.getTopLeft(find.text('Tags')).dy, greaterThan(tester.getTopLeft(find.text('Notes')).dy), ); - expect(find.widgetWithText(Chip, 'Travel kit'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Travel kit'), findsOneWidget); }); testWidgets('removing a tag and saving writes the new set', (tester) async { @@ -100,7 +101,7 @@ void main() { await tester.tap( find.descendant( - of: find.widgetWithText(Chip, 'Travel kit'), + of: find.widgetWithText(TagChip, 'Travel kit'), matching: find.byIcon(Icons.close), ), ); @@ -146,7 +147,7 @@ void main() { await tester.pumpAndSettle(); expect(tagField, findsOneWidget); - expect(find.widgetWithText(Chip, 'Travel kit'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Travel kit'), findsOneWidget); }); testWidgets('a read that fails leaves the field locked and the stored ' @@ -208,7 +209,7 @@ void main() { await tester.enterText(tagField, 'Rental'); await tester.testTextInput.receiveAction(TextInputAction.done); await tester.pumpAndSettle(); - expect(find.widgetWithText(Chip, 'Rental'), findsOneWidget); + expect(find.widgetWithText(TagChip, 'Rental'), findsOneWidget); await tester.tap(find.text('Save')); await tester.pumpAndSettle(); diff --git a/test/features/import_wizard/presentation/widgets/import_tags_field_test.dart b/test/features/import_wizard/presentation/widgets/import_tags_field_test.dart index c570fd89a7..5e3482c80a 100644 --- a/test/features/import_wizard/presentation/widgets/import_tags_field_test.dart +++ b/test/features/import_wizard/presentation/widgets/import_tags_field_test.dart @@ -4,6 +4,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:submersion/features/import_wizard/domain/models/tag_selection.dart'; import 'package:submersion/features/import_wizard/presentation/widgets/import_tags_field.dart'; import 'package:submersion/features/tags/domain/entities/tag.dart'; +import 'package:submersion/features/tags/presentation/widgets/tag_chip.dart'; import 'package:submersion/l10n/arb/app_localizations.dart'; void main() { @@ -240,8 +241,8 @@ void main() { ); // Chip should render with the tag's color - final chip = tester.widget(find.byType(Chip)); - expect(chip.side?.color, equals(const Color(0xFFFF0000))); + final chip = tester.widget(find.byType(TagChip)); + expect(chip.color, equals(const Color(0xFFFF0000))); }); testWidgets('chip falls back to blue for new tags', (tester) async { @@ -261,11 +262,11 @@ void main() { ); // Falls back to theme's primary color (not hardcoded blue) - final chip = tester.widget(find.byType(Chip)); + final chip = tester.widget(find.byType(TagChip)); final primaryColor = Theme.of( tester.element(find.byType(ImportTagsField)), ).colorScheme.primary; - expect(chip.side?.color, equals(primaryColor)); + expect(chip.color, equals(primaryColor)); }); testWidgets('shows autocomplete suggestions matching query', ( diff --git a/test/features/tags/presentation/tag_color_contrast_test.dart b/test/features/tags/presentation/tag_color_contrast_test.dart new file mode 100644 index 0000000000..863c034da3 --- /dev/null +++ b/test/features/tags/presentation/tag_color_contrast_test.dart @@ -0,0 +1,45 @@ +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 new file mode 100644 index 0000000000..aebcf5197b --- /dev/null +++ b/test/features/tags/presentation/widgets/tag_chip_test.dart @@ -0,0 +1,113 @@ +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/widgets/tag_chip.dart'; + +/// TagChip paints a tag in the colour the diver chose (issue #2254). +/// +/// 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. +void main() { + final amber = Tag( + id: 'shore', + name: 'Shore', + colorHex: '#F59E0B', + createdAt: DateTime(2026), + updatedAt: DateTime(2026), + ); + + 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), + ), + ), + ), + ); + + /// 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!; + } + + testWidgets('fills with the exact stored colour, fully opaque', ( + 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. + 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', + ); + } + }); + + testWidgets('labels the chip with the contrasting colour', (tester) async { + await tester.pumpWidget(harness(TagChip(tag: amber))); + + final label = tester.widget(find.text('Shore')); + expect(label.style?.color, tagForegroundColor(amber.color)); + }); + + testWidgets('a colourless tag still fills opaquely', (tester) async { + final plain = amber.copyWith(colorHex: ''); + + await tester.pumpWidget(harness(TagChip(tag: plain))); + + expect(fillOf(tester), plain.color); + expect(fillOf(tester).a, 1.0); + }); + + testWidgets('reports a tap', (tester) async { + var taps = 0; + await tester.pumpWidget(harness(TagChip(tag: amber, onTap: () => taps++))); + + await tester.tap(find.byType(TagChip)); + await tester.pumpAndSettle(); + + expect(taps, 1); + }); + + testWidgets('reports a delete', (tester) async { + var deletes = 0; + await tester.pumpWidget( + harness(TagChip(tag: amber, onDeleted: () => deletes++)), + ); + + await tester.tap(find.byIcon(Icons.close)); + await tester.pumpAndSettle(); + + expect(deletes, 1); + }); + + testWidgets('the dense variant keeps the same fill', (tester) async { + await tester.pumpWidget(harness(TagChip(tag: amber, dense: true))); + + expect(fillOf(tester), const Color(0xFFF59E0B)); + }); +} From 3c45f945d650dc674f6549ec17b6efd39a3ee09d Mon Sep 17 00:00:00 2001 From: Eric Griffin Date: Mon, 21 Sep 2026 18:38:05 -0400 Subject: [PATCH 2/2] fix(tags): give the chip's close button a reachable tap target The close button was a 16 dp icon with an InkResponse splash radius, and a splash radius does not size a hit box: the target was 16 by 16, well under either platform's floor, which put removing a tag out of reach for touch and for assistive tech. It is an IconButton again, constrained to 48 dp where a finger points and 32 dp where a mouse does, with tapTargetSize shrinkWrap so the constraints alone decide the box. A removable chip drops its vertical padding and takes its height from that target, the way a Material chip with a delete button sizes itself, so the chip does not grow to 56 dp. The size is measured per platform in the tests rather than assumed, since this is the second time a chip-sized control in this app has quietly shrunk below its own floor. --- .../tags/presentation/widgets/tag_chip.dart | 62 ++++++++++++++----- .../presentation/widgets/tag_chip_test.dart | 53 ++++++++++++++++ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/lib/features/tags/presentation/widgets/tag_chip.dart b/lib/features/tags/presentation/widgets/tag_chip.dart index 7eed5aeb07..17bcdf604e 100644 --- a/lib/features/tags/presentation/widgets/tag_chip.dart +++ b/lib/features/tags/presentation/widgets/tag_chip.dart @@ -72,9 +72,18 @@ class TagChip extends StatelessWidget { onTap: onTap, borderRadius: borderRadius, child: Padding( - padding: dense - ? const EdgeInsets.symmetric(horizontal: 6, vertical: 2) - : const EdgeInsets.symmetric(horizontal: 10, vertical: 4), + // A removable chip drops its vertical padding and takes its height + // from the close button's tap target instead, which is how a + // Material chip with a delete button sizes itself. Padding on top + // of a 48 dp target would make the chip 56 dp tall. + padding: EdgeInsets.symmetric( + horizontal: dense ? 6 : 10, + vertical: onDeleted != null + ? 0 + : dense + ? 2 + : 4, + ), child: Row( mainAxisSize: MainAxisSize.min, children: [ @@ -112,9 +121,12 @@ class TagChip extends StatelessWidget { } } -/// The close button of a removable chip. It is an [InkResponse] rather than -/// an [IconButton] so the icon can stay chip sized while the tap target keeps -/// its own radius. +/// The close button of a removable chip. +/// +/// The tap target is measured, not assumed: a bare icon with a splash radius +/// leaves a 16 dp target, and `VisualDensity.compact` pulls an [IconButton] +/// below its own constraints floor. The icon stays chip sized while the +/// button is constrained to the platform's minimum. class _DeleteButton extends StatelessWidget { const _DeleteButton({ required this.color, @@ -126,19 +138,35 @@ 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 button = InkResponse( - onTap: onPressed, - radius: 16, - child: Icon(Icons.close, size: 16, color: color), - ); - return Semantics( - button: true, - label: tooltip, - child: tooltip == null - ? button - : Tooltip(message: tooltip!, child: button), + final target = targetFor(Theme.of(context).platform); + return IconButton( + onPressed: onPressed, + tooltip: tooltip, + icon: Icon(Icons.close, size: 16, color: color), + iconSize: 16, + padding: EdgeInsets.zero, + // shrinkWrap so the constraints alone decide the box: the padded + // setting would add its own 48 dp on top of them. + style: IconButton.styleFrom( + tapTargetSize: MaterialTapTargetSize.shrinkWrap, + minimumSize: Size(target, target), + maximumSize: Size(target, target), + ), + constraints: BoxConstraints(minWidth: target, minHeight: target), ); } } diff --git a/test/features/tags/presentation/widgets/tag_chip_test.dart b/test/features/tags/presentation/widgets/tag_chip_test.dart index aebcf5197b..378bee0899 100644 --- a/test/features/tags/presentation/widgets/tag_chip_test.dart +++ b/test/features/tags/presentation/widgets/tag_chip_test.dart @@ -105,6 +105,59 @@ void main() { expect(deletes, 1); }); + group('the delete control keeps a reachable tap target', () { + // 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. + 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( + MaterialApp( + theme: ThemeData(platform: platform), + home: Scaffold( + body: Center( + child: TagChip(tag: amber, onDeleted: () {}), + ), + ), + ), + ); + + final target = tester.getSize( + find.ancestor( + of: find.byIcon(Icons.close), + matching: find.byType(IconButton), + ), + ); + + expect(target.width, greaterThanOrEqualTo(floor)); + expect(target.height, greaterThanOrEqualTo(floor)); + }); + } + + testWidgets('and the whole chip stays no taller than that target', ( + tester, + ) async { + await tester.pumpWidget( + MaterialApp( + theme: ThemeData(platform: TargetPlatform.macOS), + home: Scaffold( + body: Center( + child: TagChip(tag: amber, onDeleted: () {}), + ), + ), + ), + ); + + expect(tester.getSize(find.byType(TagChip)).height, 32.0); + }); + }); + testWidgets('the dense variant keeps the same fill', (tester) async { await tester.pumpWidget(harness(TagChip(tag: amber, dense: true)));