Fix chip ripple bug — No longer two ripples #42779

pennzht · 2019-10-15T19:03:39Z

Description

This PR fixes the "Chip Ripple" bug:

The existing behavior: When you create a filter chip with a delete icon, you see two ripples when you tap and hold the delete icon: one for the delete icon, and another for the entire chip.
New behavior: There is only a ripple for the delete icon when you tap and hold the delete icon, and only a ripple for the entire chip if you tap anywhere else on the chip.

Related Issues

b/140930220

Tests

I added the following tests:

Chip creates centered, unique ripple when label is tapped.
Delete button creates non-centered, unique ripple when tapped.
- Tapping the delete button creates a different ripple.
- Tapping the delete button does NOT create two different ripples.
RTL delete button responds to tap on the left of the chip.
- Chip correctly supports right-to-left text direction.
Chip without delete button creates correct ripple.
- Chip without a delete button will create the same ripple throughout the chip.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
- This is a bugfix. Documentation is not edited.
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

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

packages/flutter/test/material/chip_test.dart

rami-a · 2019-10-15T19:35:00Z

packages/flutter/test/material/chip_test.dart

+  TextDirection textDirection = TextDirection.ltr,
+}){
+  return _wrapForChip(
+    child: Wrap(


What is the need for the Wrap widget? Can you remove it?

@rami-a @HansMuller @gspencergoog

I used a Wrap because some existing tests also used it, even when it has only one child.
It seems that in other tests of chip_test.dart, the Chip widget is always wrapped in some other widget, such as Wrap, Column, Row, or Center.
Does anyone know the reason behind this practice?

The ripple behaves differently when not wrapped in a Wrap widget (but it always makes exactly one ripple).

One use case for the Chip is to be inside of a Wrap field that also allows entering text. Think of the "To:" field in GMail: you type addresses, and they convert to wrapped chips.

That's why I used initially Wrap in tests so that we can be sure that the Chip behaves properly in that context.

rami-a

██╗      ██████╗ ████████╗███╗   ███╗
██║     ██╔════╝ ╚══██╔══╝████╗ ████║
██║     ██║  ███╗   ██║   ██╔████╔██║
██║     ██║   ██║   ██║   ██║╚██╔╝██║
███████╗╚██████╔╝   ██║   ██║ ╚═╝ ██║
╚══════╝ ╚═════╝    ╚═╝   ╚═╝     ╚═╝

packages/flutter/test/material/chip_test.dart

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

packages/flutter/test/material/chip_test.dart

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

packages/flutter/test/material/chip_test.dart

johnsonmh

██╗      ██████╗ ████████╗███╗   ███╗
██║     ██╔════╝ ╚══██╔══╝████╗ ████║
██║     ██║  ███╗   ██║   ██╔████╔██║
██║     ██║   ██║   ██║   ██║╚██╔╝██║
███████╗╚██████╔╝   ██║   ██║ ╚═╝ ██║
╚══════╝ ╚═════╝    ╚═╝   ╚═╝     ╚═╝

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

HansMuller

LGTM

gspencergoog

* Fixed chip ripple. * Fixed chip ripple. All tests passed. * Fix one minor detail. * Fixed reference box. * Playing around 2. * Added tests for chip ripple. * Reverting print-debugging statements * Remove extra blank line. * Fixed chip ripple. * Remove commented code. * Reconciles with upstream/master. * Remove print-debugging statement. * Remove empty line. * Edit comments. * Edit style and comments. * Edit style. * Fix style and capitalization. * Return bool. * Edit style. * Use getMaterialBox instead of Material.of(...). * Experiment 3. * Revert. * Using tester.pump instead of pumpFrames * Delete pumpFrames. * Edit comments.

pennzht added 15 commits October 8, 2019 16:29

Fixed chip ripple.

3f5b586

Fixed chip ripple. All tests passed.

a9c7116

Fix one minor detail.

e0b0baf

Fixed reference box.

7696e49

Playing around 2.

a9f68a0

Added tests for chip ripple.

7bcce4b

Reverting print-debugging statements

b5072b2

Remove extra blank line.

2198b80

Fixed chip ripple.

973b689

Remove commented code.

fe8989f

Reconciles with upstream/master.

6bcbaa0

Reconcile merge conflicts.

d5489e1

Remove print-debugging statement.

2e46da6

Remove empty line.

efdb8a5

Edit comments.

a58e2ac

pennzht requested review from HansMuller, gspencergoog and johnsonmh October 15, 2019 19:03

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 15, 2019

googlebot added the cla: yes label Oct 15, 2019

pennzht changed the title ~~Fix chip ripple reconcile master~~ Fix chip ripple bug — No longer two ripples Oct 15, 2019

pennzht requested a review from rami-a October 15, 2019 19:13

rami-a reviewed Oct 15, 2019

View reviewed changes

Edit style and comments.

7ee0353

rami-a approved these changes Oct 15, 2019

View reviewed changes

clocksmith reviewed Oct 15, 2019

View reviewed changes

packages/flutter/test/material/chip_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/chip_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/chip_test.dart Outdated Show resolved Hide resolved

gspencergoog reviewed Oct 15, 2019

View reviewed changes

Edit style.

3e670ff

pennzht requested a review from clocksmith October 16, 2019 16:43

pennzht requested a review from gspencergoog October 16, 2019 16:44

clocksmith approved these changes Oct 18, 2019

View reviewed changes

johnsonmh reviewed Oct 18, 2019

View reviewed changes

pennzht added 4 commits October 18, 2019 11:44

Fix style and capitalization.

194cf53

Return bool.

15d4f0a

Edit style.

a0deaa5

Use getMaterialBox instead of Material.of(...).

f6bb21b

pennzht requested a review from johnsonmh October 18, 2019 16:20

johnsonmh approved these changes Oct 18, 2019

View reviewed changes

pennzht added 4 commits October 18, 2019 17:01

Experiment 3.

38e7501

Revert.

96511ad

Using tester.pump instead of pumpFrames

6fca65e

Delete pumpFrames.

19ebd10

HansMuller reviewed Oct 21, 2019

View reviewed changes

packages/flutter/lib/src/material/chip.dart Show resolved Hide resolved

HansMuller approved these changes Oct 21, 2019

View reviewed changes

Edit comments.

5155c55

gspencergoog approved these changes Oct 23, 2019

View reviewed changes

pennzht merged commit 320a933 into flutter:master Oct 23, 2019

perclasson mentioned this pull request Oct 24, 2019

[Chip] Make sure InkResponse is in the foreground on delete for chips with background color #41463

Merged

9 tasks

pennzht mentioned this pull request Oct 25, 2019

[Chip] Chip with onDeleted but without onPressed has no ripple when tapping delete button. #43534

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Fix chip ripple bug — No longer two ripples #42779

Fix chip ripple bug — No longer two ripples #42779

Uh oh!

Conversation

pennzht commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rami-a Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

pennzht Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

gspencergoog Oct 15, 2019

Choose a reason for hiding this comment

Uh oh!

rami-a left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnsonmh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pennzht commented Oct 15, 2019 •

edited

Loading