From 4d02451b444e665ab532345b527e86c05adb5845 Mon Sep 17 00:00:00 2001 From: Sipho Nkebe Date: Mon, 15 Jun 2026 08:19:51 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(button):=20canonical-reference=20audit?= =?UTF-8?q?=20=E2=80=94=20crash=20fix,=20a11y,=20shadcn=20v4=20alignment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep adversarial audit of Button (the canonical component template ~40 components copy) across three lenses (shadcn/vision/API, a11y/mobile, Flutter-correctness). Fixes the findings so the reference is itself sound: CRITICAL - Crash fix (empirically reproduced): disabling a button WHILE pressed (async- submit flow) disposed the tap recognizer mid-gesture → onTapCancel→setState during build → assert crash. Clear callbacks are now always-wired; onTapDown guards internally. + regression test. - Touch target: visual sizes were 36-40px (below the mobile-first ≥44px rule, §4). Added MinTapTarget (a shared _utils RenderShiftedBox, Material's tap-target idiom) giving a ≥48px hit area decoupled from the visual size. + test. - Dynamic Type: fixed .h() heights silently CLIPPED labels at large OS text scales. Switched to minH + symmetric py so the button grows with scaled text (and py vertically centers). + growth test. DESIGN (user-approved) - Opaque hover: filled-variant hover now composites shadcn's /90 over the `background` token (opaque) instead of translucent alpha that bled cards/popovers. - Sizes aligned to current shadcn v4: sm h-8, md h-9, lg h-10, icon size-9. - Icon+text: new leading/trailing slots compose an FwRow(gap-2); icons render at 16px (shadcn [&_svg]:size-4) via IconTheme. + test + gallery/golden demo. - Kept the variant: enum API (consistent across the catalog); fixed the structure spec's stray Button.ghost(...) reference to Button(variant: ButtonVariant.ghost). POLISH - autofocus pass-through; palette via a record switch-expression (no `late`); doc-comments: semanticLabel override behavior, icon-button labeling requirement, the engine-fit rationale (don't convert to .tw state layers), known limitations. Goldens regenerated locally (Windows) + eyeball-verified; CI Linux re-baseline to follow (authoritative platform, §9). Full suite (14 tests) + analyze + format clean. Co-Authored-By: Claude Opus 4.8 --- .../lib/components/ui/_utils/tap_target.dart | 89 ++++++ apps/gallery/lib/components/ui/button.dart | 257 ++++++++++++------ apps/gallery/lib/main.dart | 43 ++- apps/gallery/test/button_behavior_test.dart | 72 +++++ apps/gallery/test/button_golden_test.dart | 43 ++- .../gallery/test/goldens/button_grid_dark.png | Bin 6103 -> 9794 bytes .../test/goldens/button_grid_light.png | Bin 6360 -> 10378 bytes apps/gallery/test/goldens/button_grid_rtl.png | Bin 6398 -> 10389 bytes ...lutterbits-structure-and-routing-design.md | 2 +- 9 files changed, 413 insertions(+), 93 deletions(-) create mode 100644 apps/gallery/lib/components/ui/_utils/tap_target.dart diff --git a/apps/gallery/lib/components/ui/_utils/tap_target.dart b/apps/gallery/lib/components/ui/_utils/tap_target.dart new file mode 100644 index 0000000..c08226e --- /dev/null +++ b/apps/gallery/lib/components/ui/_utils/tap_target.dart @@ -0,0 +1,89 @@ +import 'dart:math' as math; + +import 'package:flutter/rendering.dart'; +import 'package:flutter/widgets.dart'; + +/// Guarantees a minimum interactive hit area around [child] **without enlarging +/// the child's painted size** — the Material-free analog of Material's tap-target +/// padding (Apple HIG 44pt / Android 48dp; flutterbits is mobile-first, charter +/// §4: touch targets ≥ ~44px). +/// +/// The child paints at its natural size, centered; the extra area is transparent +/// and only extends layout + hit-testing. Use it to wrap the *gesture* layer of an +/// interactive component so a visually-small control (an icon button, a compact +/// `sm` button) is still comfortably tappable. +/// +/// This is a shared `_utils` helper, **not** copy-pasted per component — every +/// interactive component (Button, IconButton, Switch, Checkbox…) wraps its hit +/// layer in one of these, so the touch-target rule is solved once. +class MinTapTarget extends SingleChildRenderObjectWidget { + /// Wraps [child] so the laid-out + hit-tested area is at least [minSize] on + /// each axis (default 48 — Android's `mdpi` target, which also satisfies HIG 44). + const MinTapTarget({super.key, this.minSize = 48.0, required Widget super.child}) + : assert(minSize >= 0, 'flutterbits: minSize must be >= 0 (got $minSize).'); + + /// The minimum width and height of the hit area, in logical px. + final double minSize; + + @override + RenderObject createRenderObject(BuildContext context) => _RenderMinTapTarget(minSize); + + @override + void updateRenderObject(BuildContext context, RenderObject renderObject) { + (renderObject as _RenderMinTapTarget).minSize = minSize; + } +} + +class _RenderMinTapTarget extends RenderShiftedBox { + _RenderMinTapTarget(this._minSize) : super(null); + + double _minSize; + set minSize(double value) { + if (value != _minSize) { + _minSize = value; + markNeedsLayout(); + } + } + + Size _computeSize(BoxConstraints constraints, ChildLayouter layoutChild) { + final RenderBox? child = this.child; + if (child == null) { + return constraints.constrain(Size(_minSize, _minSize)); + } + // Loosen so the child takes its natural size; we only *expand* around it. + final Size childSize = layoutChild(child, constraints.loosen()); + return constraints.constrain( + Size(math.max(childSize.width, _minSize), math.max(childSize.height, _minSize)), + ); + } + + @override + double computeMinIntrinsicWidth(double height) => + math.max(super.computeMinIntrinsicWidth(height), _minSize); + + @override + double computeMaxIntrinsicWidth(double height) => + math.max(super.computeMaxIntrinsicWidth(height), _minSize); + + @override + double computeMinIntrinsicHeight(double width) => + math.max(super.computeMinIntrinsicHeight(width), _minSize); + + @override + double computeMaxIntrinsicHeight(double width) => + math.max(super.computeMaxIntrinsicHeight(width), _minSize); + + @override + Size computeDryLayout(BoxConstraints constraints) => + _computeSize(constraints, ChildLayoutHelper.dryLayoutChild); + + @override + void performLayout() { + size = _computeSize(constraints, ChildLayoutHelper.layoutChild); + final RenderBox? child = this.child; + if (child != null) { + final BoxParentData childParentData = child.parentData! as BoxParentData; + childParentData.offset = Alignment.center.alongOffset(size - child.size as Offset); + } + } +} diff --git a/apps/gallery/lib/components/ui/button.dart b/apps/gallery/lib/components/ui/button.dart index f281413..680fadc 100644 --- a/apps/gallery/lib/components/ui/button.dart +++ b/apps/gallery/lib/components/ui/button.dart @@ -2,20 +2,48 @@ import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; import 'package:flutterwindcss/flutterwindcss.dart'; -/// shadcn's button variants. `primary` is shadcn's `default` (`default` is a -/// Dart reserved word, so it cannot be an enum constant). +import '_utils/tap_target.dart'; + +/// shadcn's button variants. `primary` is shadcn's `default` (`default` is a Dart +/// reserved word, so it cannot be an enum constant). enum ButtonVariant { primary, secondary, destructive, outline, ghost, link } /// shadcn's button sizes. `md` is shadcn's `default` size (same reserved-word -/// reason as above). +/// reason). Heights track current shadcn (Tailwind v4): sm `h-8`, md `h-9`, +/// lg `h-10`, icon `size-9`. enum ButtonSize { sm, md, lg, icon } /// A Material-free, themeable button — shadcn parity. Copy-paste source you own. /// -/// Sources its own interaction states (hover/focus/pressed/disabled) via a -/// [FocusableActionDetector] and wires keyboard activation (Enter / Space) to -/// [ActivateIntent] → [onPressed]. Visual styling of all variant/size/state -/// combinations is applied through a single `.tw` chain in [_ButtonState._styled]. +/// **Intention-revealing:** `Button(onPressed: …, child: Text('Save'))` reads as +/// the artifact. Style comes entirely from `flutterwindcss` semantic tokens +/// (`context.fw.colors.*`), so any pasted theme reskins it. +/// +/// **Canonical state pattern (don't "improve" this into `.tw` state layers).** An +/// action-bearing component owns its interaction states: this widget sources +/// hover/focus/pressed/disabled via [FocusableActionDetector] + a tap detector and +/// branches the *visual* in Dart (a `switch` per variant), then applies a single +/// flat `.tw` chain. That is correct per AGENTS.md §6 — and deliberately different +/// from the engine's visual-only `.tw` state sourcing. Because the chain declares +/// no `.hover()/.focus()/.pressed()` layers, the engine adds no second +/// `MouseRegion`/`Focus`/`Listener` (no double sourcing). A `.tw` state layer +/// overlays a whole field last-wins; it cannot express "swap *which* token I read +/// per variant×state" (primary→primary/90 vs outline→accent+accentForeground), so +/// the Dart `switch` is both more legible and the right tool here. +/// +/// **Accessibility:** wrapped in [Semantics] (button role + enabled), keyboard +/// activation (Enter/Space → [ActivateIntent]), a focus-*visible* ring (keyboard +/// focus only — see [_styled]), and a ≥48px touch target via [MinTapTarget] +/// independent of the (smaller) visual size. **Icon-only buttons have no text, so +/// they MUST pass [semanticLabel]** or a screen reader announces an unlabeled +/// button. +/// +/// **Known limitations (engine, not bugs):** the hover dim composites over the +/// `background` token (see [_styled]) — on a non-`background` surface the hover +/// tint is computed against `background`, not the actual surface. `link`'s +/// underline has no offset control (the engine's `.underline` has none). A +/// `loading` state and a `tooltip` are not yet built (they need the Spinner / +/// Tooltip components). class Button extends StatefulWidget { const Button({ super.key, @@ -23,26 +51,42 @@ class Button extends StatefulWidget { this.onPressed, this.variant = ButtonVariant.primary, this.size = ButtonSize.md, + this.leading, + this.trailing, this.semanticLabel, this.focusNode, + this.autofocus = false, }); - /// The button's content (a `Text`, an icon widget, or a row of both). + /// The button's label (typically a `Text`). For an icon-only button, pass the + /// icon as [child] with `size: ButtonSize.icon` **and** a [semanticLabel]. final Widget child; - /// Tapped/activated callback. `null` disables the button. + /// Tapped/activated callback. `null` disables the button (the Flutter idiom). final VoidCallback? onPressed; final ButtonVariant variant; final ButtonSize size; - /// Optional accessibility label (defaults to the child's own semantics). + /// Optional icon before the label (composed with a gap, shadcn's `inline-flex + /// gap-2`). Icons render at 16px (shadcn `[&_svg]:size-4`). + final Widget? leading; + + /// Optional icon after the label. See [leading]. + final Widget? trailing; + + /// Screen-reader label. When provided it **replaces** the child's semantics + /// (like `aria-label`); omit it to announce the child's own text. **Required in + /// practice for icon-only buttons**, whose child carries no text. final String? semanticLabel; - /// Optional external [FocusNode]. When provided, the caller controls focus. - /// Useful in tests and for programmatic focus management. + /// External focus control (useful for forms, tests, programmatic focus). When + /// null, the detector manages its own node. final FocusNode? focusNode; + /// Whether to focus this button on first build. + final bool autofocus; + /// Whether the button is interactive. bool get enabled => onPressed != null; @@ -51,61 +95,61 @@ class Button extends StatefulWidget { } class _ButtonState extends State