Fix various problems with Chip delete button. (#90531)

packages/flutter/lib/src/material/chip.dart

@@ -11,7 +11,6 @@ import 'chip_theme.dart';
 import 'colors.dart';
 import 'constants.dart';
 import 'debug.dart';
-import 'feedback.dart';
 import 'icons.dart';
 import 'ink_well.dart';
 import 'material.dart';
@@ -1589,8 +1588,6 @@ class _RawChipState extends State<RawChip> with MaterialStateMixin, TickerProvid
   late Animation<double> enableAnimation;
   late Animation<double> selectionFade;
 
-  final GlobalKey deleteIconKey = GlobalKey();
-
   bool get hasDeleteButton => widget.onDeleted != null;
   bool get hasAvatar => widget.avatar != null;
 
@@ -1795,7 +1792,6 @@ class _RawChipState extends State<RawChip> with MaterialStateMixin, TickerProvid
     BuildContext context,
     ThemeData theme,
     ChipThemeData chipTheme,
-    GlobalKey deleteIconKey,
   ) {
     if (!hasDeleteButton) {
       return null;
@@ -1806,15 +1802,12 @@ class _RawChipState extends State<RawChip> with MaterialStateMixin, TickerProvid
       child: _wrapWithTooltip(
         widget.deleteButtonTooltipMessage ?? MaterialLocalizations.of(context).deleteButtonTooltip,
         widget.onDeleted,
-        GestureDetector(
-          key: deleteIconKey,
-          behavior: HitTestBehavior.opaque,
-          onTap: widget.isEnabled
-            ? () {
-                Feedback.forTap(context);
-                widget.onDeleted!();
-              }
-            : null,
+        InkWell(
+          // Radius should be slightly less than the full size of the chip.
+          radius: (_kChipHeight + (widget.padding?.vertical ?? 0.0)) * .45,
+          // Keeps the splash from being constrained to the icon alone.
+          splashFactory: _UnconstrainedInkSplashFactory(Theme.of(context).splashFactory),
+          onTap: widget.isEnabled ? widget.onDeleted : null,
           child: IconTheme(
             data: theme.iconTheme.copyWith(
               color: widget.deleteIconColor ?? chipTheme.deleteIconColor,
@@ -1878,11 +1871,6 @@ class _RawChipState extends State<RawChip> with MaterialStateMixin, TickerProvid
         onTapDown: canTap ? _handleTapDown : null,
         onTapCancel: canTap ? _handleTapCancel : null,
         onHover: canTap ? updateMaterialState(MaterialState.hovered) : null,
-        splashFactory: _LocationAwareInkRippleFactory(
-            hasDeleteButton,
-            context,
-            deleteIconKey,
-        ),
         customBorder: resolvedShape,
         child: AnimatedBuilder(
           animation: Listenable.merge(<Listenable>[selectController, enableController]),
@@ -1916,7 +1904,7 @@ class _RawChipState extends State<RawChip> with MaterialStateMixin, TickerProvid
                 deleteIcon: AnimatedSwitcher(
                   duration: _kDrawerDuration,
                   switchInCurve: Curves.fastOutSlowIn,
-                  child: _buildDeleteIcon(context, theme, chipTheme, deleteIconKey),
+                  child: _buildDeleteIcon(context, theme, chipTheme),
                 ),
                 brightness: chipTheme.brightness,
                 padding: (widget.padding ?? chipTheme.padding).resolve(textDirection),
@@ -2531,13 +2519,14 @@ class _RenderChip extends RenderBox {
     if (!size.contains(position)) {
       return false;
     }
-    final bool tapIsOnDeleteIcon = _tapIsOnDeleteIcon(
-      hasDeleteButton: deleteIcon != null,
+    final bool hitIsOnDeleteIcon = deleteIcon != null && _hitIsOnDeleteIcon(
+      padding: theme.padding,
       tapPosition: position,
       chipSize: size,
+      deleteButtonSize: deleteIcon!.size,
       textDirection: textDirection!,
     );
-    final RenderBox? hitTestChild = tapIsOnDeleteIcon
+    final RenderBox? hitTestChild = hitIsOnDeleteIcon
         ? (deleteIcon ?? label ?? avatar)
         : (label ?? avatar);
 
@@ -2924,16 +2913,10 @@ class _ChipSizes {
   final Offset densityAdjustment;
 }
 
-class _LocationAwareInkRippleFactory extends InteractiveInkFeatureFactory {
-  const _LocationAwareInkRippleFactory(
-    this.hasDeleteButton,
-    this.chipContext,
-    this.deleteIconKey,
-  );
+class _UnconstrainedInkSplashFactory extends InteractiveInkFeatureFactory {
+  const _UnconstrainedInkSplashFactory(this.parentFactory);
 
-  final bool hasDeleteButton;
-  final BuildContext chipContext;
-  final GlobalKey deleteIconKey;
+  final InteractiveInkFeatureFactory parentFactory;
 
   @override
   InteractiveInkFeature create({
@@ -2949,61 +2932,55 @@ class _LocationAwareInkRippleFactory extends InteractiveInkFeatureFactory {
     double? radius,
     VoidCallback? onRemoved,
   }) {
-
-    final bool tapIsOnDeleteIcon = _tapIsOnDeleteIcon(
-      hasDeleteButton: hasDeleteButton,
-      tapPosition: position,
-      chipSize: chipContext.size!,
-      textDirection: textDirection,
-    );
-
-    final BuildContext splashContext = tapIsOnDeleteIcon
-        ? deleteIconKey.currentContext!
-        : chipContext;
-
-    final InteractiveInkFeatureFactory splashFactory = Theme.of(splashContext).splashFactory;
-
-    if (tapIsOnDeleteIcon) {
-      final RenderBox currentBox = referenceBox;
-      referenceBox = deleteIconKey.currentContext!.findRenderObject()! as RenderBox;
-      position = referenceBox.globalToLocal(currentBox.localToGlobal(position));
-      containedInkWell = false;
-    }
-
-    return splashFactory.create(
+    return parentFactory.create(
       controller: controller,
       referenceBox: referenceBox,
       position: position,
       color: color,
-      textDirection: textDirection,
-      containedInkWell: containedInkWell,
+      containedInkWell: false,
       rectCallback: rectCallback,
       borderRadius: borderRadius,
       customBorder: customBorder,
       radius: radius,
       onRemoved: onRemoved,
+      textDirection: textDirection,
     );
   }
 }
 
-bool _tapIsOnDeleteIcon({
-  required bool hasDeleteButton,
+bool _hitIsOnDeleteIcon({
+  required EdgeInsetsGeometry padding,
   required Offset tapPosition,
   required Size chipSize,
+  required Size deleteButtonSize,
   required TextDirection textDirection,
 }) {
-  bool tapIsOnDeleteIcon;
-  if (!hasDeleteButton) {
-    tapIsOnDeleteIcon = false;
-  } else {
-    switch (textDirection) {
-      case TextDirection.ltr:
-        tapIsOnDeleteIcon = tapPosition.dx / chipSize.width > 0.66;
-        break;
-      case TextDirection.rtl:
-        tapIsOnDeleteIcon = tapPosition.dx / chipSize.width < 0.33;
-        break;
-    }
+  // The chipSize includes the padding, so we need to deflate the size and adjust the
+  // tap position to account for the padding.
+  final EdgeInsets resolvedPadding = padding.resolve(textDirection);
+  final Size deflatedSize = resolvedPadding.deflateSize(chipSize);
+  final Offset adjustedPosition = tapPosition - Offset(resolvedPadding.left, resolvedPadding.top);
+  // The delete button hit area should be at least the width of the delete button,
+  // but, if there's room, up to 24 pixels from the center of the delete icon
+  // (corresponding to part of a 48x48 square that Material would prefer for touch
+  // targets), but no more than half of the overall size of the chip when the chip is
+  // small.
+  //
+  // This isn't affected by materialTapTargetSize because it only applies to the
+  // width of the tappable region within the chip, not outside of the chip, which is
+  // handled elsewhere. Also because delete buttons aren't specified to be used on
+  // touch devices, only desktop devices.
+  final double accessibleDeleteButtonWidth = math.max(
+    deleteButtonSize.width,
+    math.min(
+      deflatedSize.width * 0.5,
+      24.0 + deleteButtonSize.width / 2.0,
+    ),
+  );
+  switch (textDirection) {
+    case TextDirection.ltr:
+      return adjustedPosition.dx >= deflatedSize.width - accessibleDeleteButtonWidth;
+    case TextDirection.rtl:
+      return adjustedPosition.dx <= accessibleDeleteButtonWidth;
   }
-  return tapIsOnDeleteIcon;
 }

packages/flutter/test/material/chip_test.dart

@@ -191,20 +191,23 @@ void _expectCheckmarkColor(Finder finder, Color color) {
   );
 }
 
+void _doNothing() {}
+
 Widget _chipWithOptionalDeleteButton({
-  UniqueKey? deleteButtonKey,
-  UniqueKey? labelKey,
+  Key? deleteButtonKey,
+  Key? labelKey,
   required bool deletable,
   TextDirection textDirection = TextDirection.ltr,
   bool hasDeleteButtonTooltip = true,
+  VoidCallback? onPressed = _doNothing,
 }) {
   return _wrapForChip(
     textDirection: textDirection,
     child: Wrap(
       children: <Widget>[
         RawChip(
-          onPressed: () {},
-          onDeleted: deletable ? () {} : null,
+          onPressed: onPressed,
+          onDeleted: deletable ? _doNothing : null,
           deleteIcon: Icon(Icons.close, key: deleteButtonKey),
           useDeleteButtonTooltip: hasDeleteButtonTooltip,
           label: Text(
@@ -526,6 +529,59 @@ void main() {
     },
   );
 
+  testWidgets('delete button tap target is the right proportion of the chip', (WidgetTester tester) async {
+    final UniqueKey deleteKey = UniqueKey();
+    bool calledDelete = false;
+    await tester.pumpWidget(
+      _wrapForChip(
+        child: Column(
+          children: <Widget>[
+            Chip(
+              label: const Text('Really Long Label'),
+              deleteIcon: Icon(Icons.delete, key: deleteKey),
+              onDeleted: () {
+                calledDelete = true;
+              },
+            ),
+          ],
+        ),
+      ),
+    );
+    await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(24.0, 0.0));
+    await tester.pump();
+    expect(calledDelete, isTrue);
+    calledDelete = false;
+
+    await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(25.0, 0.0));
+    await tester.pump();
+    expect(calledDelete, isFalse);
+    calledDelete = false;
+
+    await tester.pumpWidget(
+      _wrapForChip(
+        child: Column(
+          children: <Widget>[
+            Chip(
+              label: const SizedBox(), // Short label
+              deleteIcon: Icon(Icons.delete, key: deleteKey),
+              onDeleted: () {
+                calledDelete = true;
+              },
+            ),
+          ],
+        ),
+      ),
+    );
+    await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(12.0, 0.0));
+    await tester.pump();
+    expect(calledDelete, isTrue);
+    calledDelete = false;
+
+    await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(13.0, 0.0));
+    await tester.pump();
+    expect(calledDelete, isFalse);
+  });
+
   testWidgets('Chip elements are ordered horizontally for locale', (WidgetTester tester) async {
     final UniqueKey iconKey = UniqueKey();
     final Widget test = Overlay(
@@ -1022,7 +1078,6 @@ void main() {
   });
 
   testWidgets('Chip creates centered, unique ripple when label is tapped', (WidgetTester tester) async {
-    // Creates a chip with a delete button.
     final UniqueKey labelKey = UniqueKey();
     final UniqueKey deleteButtonKey = UniqueKey();
 
@@ -1045,10 +1100,6 @@ void main() {
     // Waits for 100 ms.
     await tester.pump(const Duration(milliseconds: 100));
 
-    // There should be exactly one ink-creating widget.
-    expect(find.byType(InkWell), findsOneWidget);
-    expect(find.byType(InkResponse), findsNothing);
-
     // There should be one unique, centered ink ripple.
     expect(box, ripplePattern(const Offset(163.0, 6.0), 20.9));
     expect(box, uniqueRipplePattern(const Offset(163.0, 6.0), 20.9));
@@ -1075,8 +1126,40 @@ void main() {
     await gesture.up();
   });
 
+  testWidgets('Delete button is focusable', (WidgetTester tester) async {
+    final GlobalKey labelKey = GlobalKey();
+    final GlobalKey deleteButtonKey = GlobalKey();
+
+    await tester.pumpWidget(
+      _chipWithOptionalDeleteButton(
+        labelKey: labelKey,
+        deleteButtonKey: deleteButtonKey,
+        deletable: true,
+      ),
+    );
+
+    Focus.of(deleteButtonKey.currentContext!).requestFocus();
+    await tester.pump();
+
+    // They shouldn't have the same focus node.
+    expect(Focus.of(deleteButtonKey.currentContext!), isNot(equals(Focus.of(labelKey.currentContext!))));
+    expect(Focus.of(deleteButtonKey.currentContext!).hasFocus, isTrue);
+    expect(Focus.of(deleteButtonKey.currentContext!).hasPrimaryFocus, isTrue);
+    // Delete button is a child widget of the Chip, so the Chip should have focus if
+    // the delete button does.
+    expect(Focus.of(labelKey.currentContext!).hasFocus, isTrue);
+    expect(Focus.of(labelKey.currentContext!).hasPrimaryFocus, isFalse);
+
+    Focus.of(labelKey.currentContext!).requestFocus();
+    await tester.pump();
+
+    expect(Focus.of(deleteButtonKey.currentContext!).hasFocus, isFalse);
+    expect(Focus.of(deleteButtonKey.currentContext!).hasPrimaryFocus, isFalse);
+    expect(Focus.of(labelKey.currentContext!).hasFocus, isTrue);
+    expect(Focus.of(labelKey.currentContext!).hasPrimaryFocus, isTrue);
+  });
+
   testWidgets('Delete button creates non-centered, unique ripple when tapped', (WidgetTester tester) async {
-    // Creates a chip with a delete button.
     final UniqueKey labelKey = UniqueKey();
     final UniqueKey deleteButtonKey = UniqueKey();
 
@@ -1100,13 +1183,63 @@ void main() {
     await tester.pump(const Duration(milliseconds: 100));
     await tester.pump(const Duration(milliseconds: 100));
 
-    // There should be exactly one ink-creating widget.
-    expect(find.byType(InkWell), findsOneWidget);
-    expect(find.byType(InkResponse), findsNothing);
+    // There should be one unique ink ripple.
+    expect(box, ripplePattern(const Offset(3.0, 3.0), 1.44));
+    expect(box, uniqueRipplePattern(const Offset(3.0, 3.0), 1.44));
+
+    // There should be no tooltip.
+    expect(findTooltipContainer('Delete'), findsNothing);
+
+    // Waits for 200 ms again.
+    await tester.pump(const Duration(milliseconds: 100));
+    await tester.pump(const Duration(milliseconds: 100));
+
+    // The ripple should grow, but the center should move,
+    // Towards the center of the delete icon.
+    expect(box, ripplePattern(const Offset(5.0, 5.0), 4.32));
+    expect(box, uniqueRipplePattern(const Offset(5.0, 5.0), 4.32));
+
+    // There should be no tooltip.
+    expect(findTooltipContainer('Delete'), findsNothing);
+
+    // Waits for a very long time.
+    // This is pressing and holding the delete button.
+    await tester.pumpAndSettle();
+
+    // There should be a tooltip.
+    expect(findTooltipContainer('Delete'), findsOneWidget);
+
+    await gesture.up();
+  });
+
+  testWidgets('Delete button in a chip with null onPressed creates ripple when tapped', (WidgetTester tester) async {
+    final UniqueKey labelKey = UniqueKey();
+    final UniqueKey deleteButtonKey = UniqueKey();
+
+    await tester.pumpWidget(
+      _chipWithOptionalDeleteButton(
+        labelKey: labelKey,
+        onPressed: null,
+        deleteButtonKey: deleteButtonKey,
+        deletable: true,
+      ),
+    );
+
+    final RenderBox box = getMaterialBox(tester);
+
+    // Taps at a location close to the center of the delete icon.
+    final Offset centerOfDeleteButton = tester.getCenter(find.byKey(deleteButtonKey));
+    final Offset tapLocationOfDeleteButton = centerOfDeleteButton + const Offset(-10, -10);
+    final TestGesture gesture = await tester.startGesture(tapLocationOfDeleteButton);
+    await tester.pump();
+
+    // Waits for 200 ms.
+    await tester.pump(const Duration(milliseconds: 100));
+    await tester.pump(const Duration(milliseconds: 100));
 
     // There should be one unique ink ripple.
-    expect(box, ripplePattern(const Offset(3.0, 3.0), 3.5));
-    expect(box, uniqueRipplePattern(const Offset(3.0, 3.0), 3.5));
+    expect(box, ripplePattern(const Offset(3.0, 3.0), 1.44));
+    expect(box, uniqueRipplePattern(const Offset(3.0, 3.0), 1.44));
 
     // There should be no tooltip.
     expect(findTooltipContainer('Delete'), findsNothing);
@@ -1117,8 +1250,8 @@ void main() {
 
     // The ripple should grow, but the center should move,
     // Towards the center of the delete icon.
-    expect(box, ripplePattern(const Offset(5.0, 5.0), 10.5));
-    expect(box, uniqueRipplePattern(const Offset(5.0, 5.0), 10.5));
+    expect(box, ripplePattern(const Offset(5.0, 5.0), 4.32));
+    expect(box, uniqueRipplePattern(const Offset(5.0, 5.0), 4.32));
 
     // There should be no tooltip.
     expect(findTooltipContainer('Delete'), findsNothing);
@@ -1149,7 +1282,7 @@ void main() {
 
     // Taps at a location close to the center of the delete icon,
     // Which is on the left side of the chip.
-    final Offset topLeftOfInkWell = tester.getTopLeft(find.byType(InkWell));
+    final Offset topLeftOfInkWell = tester.getTopLeft(find.byType(InkWell).first);
     final Offset tapLocation = topLeftOfInkWell + const Offset(8, 8);
     final TestGesture gesture = await tester.startGesture(tapLocation);
     await tester.pump();
@@ -1236,7 +1369,7 @@ void main() {
                       }
                     : null,
                   selected: selected,
-                  label: Text('Chip', key: labelKey),
+                  label: Text('Long Chip Label', key: labelKey),
                   shape: const StadiumBorder(),
                   showCheckmark: true,
                   tapEnabled: true,
@@ -1254,7 +1387,7 @@ void main() {
     await pushChip(
       avatar: SizedBox(width: 40.0, height: 40.0, key: avatarKey),
     );
-    expect(tester.getSize(find.byType(RawChip)), equals(const Size(104.0, 48.0)));
+    expect(tester.getSize(find.byType(RawChip)), equals(const Size(258.0, 48.0)));
 
     // Turn on selection.
     await pushChip(
@@ -1318,7 +1451,7 @@ void main() {
                       }
                     : null,
                   selected: selected,
-                  label: Text('Chip', key: labelKey),
+                  label: Text('Long Chip Label', key: labelKey),
                   shape: const StadiumBorder(),
                   showCheckmark: true,
                   tapEnabled: true,
@@ -1333,7 +1466,7 @@ void main() {
 
     // Without avatar, but not selectable.
     await pushChip();
-    expect(tester.getSize(find.byType(RawChip)), equals(const Size(80.0, 48.0)));
+    expect(tester.getSize(find.byType(RawChip)), equals(const Size(234.0, 48.0)));
 
     // Turn on selection.
     await pushChip(selectable: true);
@@ -1395,7 +1528,7 @@ void main() {
                       }
                     : null,
                   selected: selected,
-                  label: Text('Chip', key: labelKey),
+                  label: Text('Long Chip Label', key: labelKey),
                   shape: const StadiumBorder(),
                   showCheckmark: false,
                   tapEnabled: true,
@@ -1737,6 +1870,7 @@ void main() {
                                 textDirection: TextDirection.ltr,
                                 flags: <SemanticsFlag>[
                                   SemanticsFlag.isButton,
+                                  SemanticsFlag.isFocusable,
                                 ],
                               ),
                             ],
@@ -3196,7 +3330,7 @@ void main() {
 
     // Tap at the delete icon of the chip, which is at the right
     // side of the chip
-    final Offset topRightOfInkwell = tester.getTopLeft(find.byType(InkWell));
+    final Offset topRightOfInkwell = tester.getTopLeft(find.byType(InkWell).first);
     final Offset tapLocationOfDeleteButton = topRightOfInkwell + const Offset(8, 8);
     final TestGesture tapGesture = await tester.startGesture(tapLocationOfDeleteButton);
 
@@ -3212,7 +3346,7 @@ void main() {
   });
 
   testWidgets('intrinsicHeight implementation meets constraints', (WidgetTester tester) async {
-    // Regression text for https://github.com/flutter/flutter/issues/49478.
+    // Regression test for https://github.com/flutter/flutter/issues/49478.
     await tester.pumpWidget(_wrapForChip(
       child: const Chip(
         label: Text('text'),

packages/flutter/test/rendering/mock_canvas.dart

@@ -1399,8 +1399,7 @@ class _SomethingPaintPredicate extends _PaintPredicate {
       currentCall = call.current;
       if (!currentCall.invocation.isMethod)
         throw 'It called $currentCall, which was not a method, when the paint pattern expected a method call';
-      call.moveNext();
-    } while (!_runPredicate(currentCall.invocation.memberName, currentCall.invocation.positionalArguments));
+    } while (call.moveNext() && !_runPredicate(currentCall.invocation.memberName, currentCall.invocation.positionalArguments));
   }
 
   bool _runPredicate(Symbol methodName, List<dynamic> arguments) {