Make `chip.dart` use `WidgetStatesController` by ValentinVignal · Pull Request #161487 · flutter/flutter

ValentinVignal · 2025-01-12T14:15:39Z

Fixes #128289

Follow up of #128507 and #159422

Makes RawChip use WidgetStatesController instead of MaterialStateMixin
Remove references of MaterialState to replace them with WidgetState

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

ValentinVignal

I had to make the behavior of chips consistent with InkWell to not hit the issue:

The widget on which setState() or markNeedsBuild() was called was:
  RawChip
The widget which was currently being built when the offending call was made was:
  InkWell

ValentinVignal · 2025-01-12T14:18:15Z

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

+    statesController.update(WidgetState.disabled, !canTap);
+    statesController.update(WidgetState.selected, widget.selected);


I need to use canTap instead of widget.isEnabled as InkWell does its logic on the provided callbacks

ValentinVignal · 2025-01-12T14:18:28Z

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

+    if (_canRawChipTap(oldWidget) != _canRawChipTap(widget)) {
      setState(() {
-        setMaterialState(MaterialState.disabled, !widget.isEnabled);
+        statesController.update(WidgetState.disabled, !canTap);


ValentinVignal · 2025-01-12T14:19:25Z

packages/flutter/test/material/chip_test.dart

+        child: RawChip(
+          label: const Text('text'),
+          backgroundColor: backgroundColor,
+          shape: shape,
+          onPressed: () {},
+        ),


This also introduces this breaking change. A callback needs to be provided in order for the chip to be truly enabled

ValentinVignal · 2025-01-12T14:20:43Z

@bleroux @nate-thegrate @victorsanni , here is the follow up of what we discussed #159422.

I put comments where there are some potential issues that could make this PR not mergeable. Tell me what you think about it

bleroux · 2025-01-13T09:07:04Z

Your are right, that it is probably to much of a breaking change.

If I understand well, the problem is that InkWell will change the WidgetStatesController state to disabled when there is no callback and this collides with RawChip definition of the disabled state.
Can't we pass a non-empty callbalk directly to the InkWell created on line 1402 based on RawChip.isEnabled (that way both will be enabled or disabled at the same time, or maybe this is not enough because InkWell can be enabled due to other callbacks being defined?

Another way would to not share the WidgetStatesController between the RawChip and the InkWell, but in that case not sure it is worth using WidgetStatesController instead of MaterialStateMixin?

nate-thegrate · 2025-01-13T19:48:52Z

As far as MaterialStateMixin, I believe there are two ways forward:

Deprecate & rename to WidgetStateMixin
Use WidgetStatesController instead
(the mixin could either be deprecated or moved out once we decide where the other legacy Material 2 things are going)

I agree with the points @bleroux made. I'm leaning toward not sharing the WidgetStatesController between the RawChip and the InkWell: it's true that this would reduce the benefit of the states controller over the mixin, but given that the mixin is only used by 2 classes in the framework, it might make the most sense to stop using it, so future maintainers would have one less thing to learn.

ValentinVignal · 2025-01-25T14:44:16Z

Thank you for the feedbacks @bleroux and @nate-thegrate . I changed the code to not share the states controller with InkWell.

Also, I believe this PR can be exempted from tests?

nate-thegrate

LGTM, thanks for following up here!

nate-thegrate · 2025-01-27T23:41:17Z

I believe this PR can be exempted from tests?

Yeah, I think you're right, since the goal here is to factor out MaterialStateMixin without any changes to functionality. Feel free to reach out in hackers-💭 for an exemption :)

ValentinVignal · 2025-02-07T08:32:10Z

Any feedback on this @Renzo-Olivares @QuncCccccc ?

QuncCccccc · 2025-02-12T18:50:55Z

Sorry for the late response! I will take a look at this PR as soon as possible!

QuncCccccc

Thanks for your patience. Could you help fix the failed tests?

Just a question, do we want to use WidgetStatesController to replace MaterialStateMixin? I'm actually not super familiar with the context, maybe @MitchellGoodwin have any thoughts?

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

MitchellGoodwin · 2025-03-14T19:32:56Z

Just a question, do we want to use WidgetStatesController to replace MaterialStateMixin?

Yes, we prefer WidgetStatesController now. MaterialStateMixin has been semi-deprecated for a while.

…r#161487)

Fixes flutter#128289 Follow up of flutter#128507 and flutter#159422 - Makes `RawChip` use `WidgetStatesController` instead of `MaterialStateMixin` - Remove references of `MaterialState` to replace them with `WidgetState` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Qun Cheng <[email protected]>

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 12, 2025

ValentinVignal requested review from TahaTesser, bleroux, nate-thegrate and victorsanni January 12, 2025 14:16

ValentinVignal commented Jan 12, 2025

View reviewed changes

TahaTesser removed their request for review January 13, 2025 10:27

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch from 174a7a3 to 7208482 Compare January 25, 2025 14:42

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch from 2de0a76 to 85704ba Compare January 26, 2025 05:36

nate-thegrate reviewed Jan 27, 2025

View reviewed changes

nate-thegrate added the refactor Improving readability/efficiency without behavioral changes label Jan 27, 2025

nate-thegrate approved these changes Jan 27, 2025

View reviewed changes

Piinks requested review from QuncCccccc and Renzo-Olivares January 29, 2025 19:07

victorsanni removed their request for review January 30, 2025 17:47

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch from 558d1a7 to b92cb74 Compare February 7, 2025 08:31

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch 2 times, most recently from 1f59d27 to f385523 Compare February 26, 2025 08:51

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch from f385523 to d6b1ffd Compare March 10, 2025 10:57

QuncCccccc reviewed Mar 14, 2025

View reviewed changes

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

Make Chips use the MaterialStatesControllers

5366429

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

2a616cf

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

424afb4

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

7093e09

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

10acbea

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 27, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

be0ac3b

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

1245dad

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

3b460a4

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

3b9833f

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 28, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

3b6587e

…r#161487)

Piinks mentioned this pull request Mar 28, 2025

Flutter Notable Commits - 2025 #161460

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

523d873

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 29, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

fc29219

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

2d81822

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 30, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

70a4168

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

286f226

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

df1e0d8

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

fa9845f

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

f2d7f14

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

94ebd8f

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

55f4a09

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 31, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

55a3e31

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

ff7ae42

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

1e74e21

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

c5ff64e

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 1, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

fd2c6d6

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

ffc80a6

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

24471f0

…r#161487)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

096c4ac Make chip.dart use WidgetStatesController (flutter/flutte…

4340ba1

…r#161487)

		statesController.update(WidgetState.disabled, !canTap);
		statesController.update(WidgetState.selected, widget.selected);

Conversation

ValentinVignal commented Jan 12, 2025

Pre-launch Checklist

Uh oh!

ValentinVignal left a comment

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Jan 12, 2025

Uh oh!

bleroux commented Jan 13, 2025

Uh oh!

nate-thegrate commented Jan 13, 2025

Uh oh!

ValentinVignal commented Jan 25, 2025

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

nate-thegrate commented Jan 27, 2025

Uh oh!

ValentinVignal commented Feb 7, 2025

Uh oh!

QuncCccccc commented Feb 12, 2025

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MitchellGoodwin commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants