Unverified Commit f92637d7 authored by Taha Tesser's avatar Taha Tesser Committed by GitHub

Fix chip delete button tap target spilling into the label. (#130896)

fixes [Chip's delete button tap target is too big](https://github.com/flutter/flutter/issues/129986)

### Description 

This PR fixes the issue where the chip delete button is tappable within the label. 

### Code sample

<details> 
<summary>expand to view the code sample</summary> 

```dart
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      theme: ThemeData(useMaterial3: true),
      home: const Example(),
    );
  }
}

class Example extends StatelessWidget {
  const Example({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Sample'),
      ),
      body: Center(
        child: Chip(
          label: const Text('Really Long Label'),
          onDeleted: () {},
        ),
      ),
    );
  }
}
``` 
	
</details>

### Before

https://github.com/flutter/flutter/assets/48603081/14b369c5-c740-4dfc-a512-779bd3a1a46b

### After

https://github.com/flutter/flutter/assets/48603081/08c6e232-0237-4ab2-9829-66ee8e5cead2
parent 0aba94f4
...@@ -1763,6 +1763,7 @@ class _RenderChip extends RenderBox with SlottedContainerRenderObjectMixin<_Chip ...@@ -1763,6 +1763,7 @@ class _RenderChip extends RenderBox with SlottedContainerRenderObjectMixin<_Chip
} }
final bool hitIsOnDeleteIcon = deleteIcon != null && _hitIsOnDeleteIcon( final bool hitIsOnDeleteIcon = deleteIcon != null && _hitIsOnDeleteIcon(
padding: theme.padding, padding: theme.padding,
labelPadding: theme.labelPadding,
tapPosition: position, tapPosition: position,
chipSize: size, chipSize: size,
deleteButtonSize: deleteIcon!.size, deleteButtonSize: deleteIcon!.size,
...@@ -2186,6 +2187,7 @@ class _UnconstrainedInkSplashFactory extends InteractiveInkFeatureFactory { ...@@ -2186,6 +2187,7 @@ class _UnconstrainedInkSplashFactory extends InteractiveInkFeatureFactory {
bool _hitIsOnDeleteIcon({ bool _hitIsOnDeleteIcon({
required EdgeInsetsGeometry padding, required EdgeInsetsGeometry padding,
required EdgeInsetsGeometry labelPadding,
required Offset tapPosition, required Offset tapPosition,
required Size chipSize, required Size chipSize,
required Size deleteButtonSize, required Size deleteButtonSize,
...@@ -2197,10 +2199,10 @@ bool _hitIsOnDeleteIcon({ ...@@ -2197,10 +2199,10 @@ bool _hitIsOnDeleteIcon({
final Size deflatedSize = resolvedPadding.deflateSize(chipSize); final Size deflatedSize = resolvedPadding.deflateSize(chipSize);
final Offset adjustedPosition = tapPosition - Offset(resolvedPadding.left, resolvedPadding.top); final Offset adjustedPosition = tapPosition - Offset(resolvedPadding.left, resolvedPadding.top);
// The delete button hit area should be at least the width of the delete // 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 // button and right label padding, but, if there's room, up to 24 pixels
// icon (corresponding to part of a 48x48 square that Material would prefer // from the center of the delete icon (corresponding to part of a 48x48 square
// for touch targets), but no more than approximately half of the overall size // that Material would prefer for touch targets), but no more than approximately
// of the chip when the chip is small. // 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 // 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, // width of the tappable region within the chip, not outside of the chip,
...@@ -2211,7 +2213,7 @@ bool _hitIsOnDeleteIcon({ ...@@ -2211,7 +2213,7 @@ bool _hitIsOnDeleteIcon({
// chip will still hit the chip, not the delete button. // chip will still hit the chip, not the delete button.
final double accessibleDeleteButtonWidth = math.min( final double accessibleDeleteButtonWidth = math.min(
deflatedSize.width * 0.499, deflatedSize.width * 0.499,
math.max(deleteButtonSize.width, 24.0 + deleteButtonSize.width / 2.0), math.min(labelPadding.resolve(textDirection).right + deleteButtonSize.width, 24.0 + deleteButtonSize.width / 2.0),
); );
switch (textDirection) { switch (textDirection) {
case TextDirection.ltr: case TextDirection.ltr:
......
...@@ -639,7 +639,7 @@ void main() { ...@@ -639,7 +639,7 @@ void main() {
}, },
); );
testWidgets('delete button tap target is the right proportion of the chip', (WidgetTester tester) async { testWidgets('Delete button tap target is the right proportion of the chip', (WidgetTester tester) async {
final UniqueKey deleteKey = UniqueKey(); final UniqueKey deleteKey = UniqueKey();
bool calledDelete = false; bool calledDelete = false;
await tester.pumpWidget( await tester.pumpWidget(
...@@ -657,12 +657,15 @@ void main() { ...@@ -657,12 +657,15 @@ void main() {
), ),
), ),
); );
await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(24.0, 0.0));
// Test correct tap target size.
await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(18.0, 0.0)); // Half the width of the delete button + right label padding.
await tester.pump(); await tester.pump();
expect(calledDelete, isTrue); expect(calledDelete, isTrue);
calledDelete = false; calledDelete = false;
await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(25.0, 0.0)); // Test incorrect tap target size.
await tester.tapAt(tester.getCenter(find.byKey(deleteKey)) - const Offset(19.0, 0.0));
await tester.pump(); await tester.pump();
expect(calledDelete, isFalse); expect(calledDelete, isFalse);
calledDelete = false; calledDelete = false;
...@@ -3459,6 +3462,48 @@ void main() { ...@@ -3459,6 +3462,48 @@ void main() {
expect(getMaterialBox(tester), paints..rrect(color: selectedColor)); expect(getMaterialBox(tester), paints..rrect(color: selectedColor));
}); });
testWidgets('Delete button tap target area does not include label', (WidgetTester tester) async {
bool calledDelete = false;
await tester.pumpWidget(
wrapForChip(
child: Column(
children: <Widget>[
Chip(
label: const Text('Chip'),
onDeleted: () {
calledDelete = true;
},
),
],
),
),
);
// Tap on the delete button.
await tester.tapAt(tester.getCenter(find.byType(Icon)));
await tester.pump();
expect(calledDelete, isTrue);
calledDelete = false;
final Offset labelCenter = tester.getCenter(find.text('Chip'));
// Tap on the label.
await tester.tapAt(labelCenter);
await tester.pump();
expect(calledDelete, isFalse);
// Tap before end of the label.
final Size labelSize = tester.getSize(find.text('Chip'));
await tester.tapAt(Offset(labelCenter.dx + (labelSize.width / 2) - 1, labelCenter.dy));
await tester.pump();
expect(calledDelete, isFalse);
// Tap after end of the label.
await tester.tapAt(Offset(labelCenter.dx + (labelSize.width / 2), labelCenter.dy));
await tester.pump();
expect(calledDelete, isTrue);
});
group('Material 2', () { group('Material 2', () {
// These tests are only relevant for Material 2. Once Material 2 // These tests are only relevant for Material 2. Once Material 2
// support is deprecated and the APIs are removed, these tests // support is deprecated and the APIs are removed, these tests
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment