Make `chip.dart` use `WidgetStatesController` #161487

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.

(cherry picked from commit 27dfa14192e045ff62c1f93e5d7e9f605341d4d2)

Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>

ValentinVignal · 2025-03-20T09:29:13Z

@QuncCccccc It looks like a rebase did the trick :)

QuncCccccc

LGTM! Thanks for helping the update:)

MitchellGoodwin · 2025-03-21T20:33:28Z

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

@@ -1011,6 +1009,9 @@ class _RawChipState extends State<RawChip>
  late CurvedAnimation enableAnimation;
  late CurvedAnimation selectionFade;

+  late final WidgetStatesController statesController =
+      WidgetStatesController()..addListener(() => setState(() {}));


Should the listener being added be moved after the .update calls in initState?

Sure, I can do that !

@MitchellGoodwin I made the changes in Add listener after updates

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 bleroux, nate-thegrate, TahaTesser 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 Renzo-Olivares and QuncCccccc 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

ValentinVignal and others added 5 commits March 19, 2025 16:24

Update the tests

5cba4ef

Don't share states controller with ink well

2ef6eb0

Don't modify generated code

b5f96ac

(cherry picked from commit 27dfa14192e045ff62c1f93e5d7e9f605341d4d2)

order imports

3f6f957

Remove empty line

722e032

Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>

ValentinVignal force-pushed the flutter/make-chips-use-material-states-controller branch from 9f1058f to 722e032 Compare March 19, 2025 15:24

dkwingsmt requested a review from QuncCccccc March 19, 2025 18:41

QuncCccccc approved these changes Mar 21, 2025

View reviewed changes

ValentinVignal added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 21, 2025

MitchellGoodwin reviewed Mar 21, 2025

View reviewed changes

ValentinVignal removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2025

Add listener after updates

4eed830

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `chip.dart` use `WidgetStatesController` #161487

Make `chip.dart` use `WidgetStatesController` #161487

ValentinVignal commented Jan 12, 2025

ValentinVignal left a comment

ValentinVignal Jan 12, 2025

ValentinVignal Jan 12, 2025

ValentinVignal Jan 12, 2025

ValentinVignal commented Jan 12, 2025

bleroux commented Jan 13, 2025

nate-thegrate commented Jan 13, 2025

ValentinVignal commented Jan 25, 2025

nate-thegrate left a comment

nate-thegrate commented Jan 27, 2025

ValentinVignal commented Feb 7, 2025

QuncCccccc commented Feb 12, 2025

QuncCccccc left a comment

MitchellGoodwin commented Mar 14, 2025

ValentinVignal commented Mar 20, 2025

QuncCccccc left a comment

MitchellGoodwin Mar 21, 2025 •

edited

Loading

ValentinVignal Mar 22, 2025

ValentinVignal Mar 22, 2025

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

Make chip.dart use WidgetStatesController #161487

Are you sure you want to change the base?

Make chip.dart use WidgetStatesController #161487

Conversation

ValentinVignal commented Jan 12, 2025

Pre-launch Checklist

ValentinVignal left a comment

Choose a reason for hiding this comment

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

ValentinVignal Jan 12, 2025

Choose a reason for hiding this comment

ValentinVignal commented Jan 12, 2025

bleroux commented Jan 13, 2025

nate-thegrate commented Jan 13, 2025

ValentinVignal commented Jan 25, 2025

nate-thegrate left a comment

Choose a reason for hiding this comment

nate-thegrate commented Jan 27, 2025

ValentinVignal commented Feb 7, 2025

QuncCccccc commented Feb 12, 2025

QuncCccccc left a comment

Choose a reason for hiding this comment

MitchellGoodwin commented Mar 14, 2025

ValentinVignal commented Mar 20, 2025

QuncCccccc left a comment

Choose a reason for hiding this comment

MitchellGoodwin Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

ValentinVignal Mar 22, 2025

Choose a reason for hiding this comment

ValentinVignal Mar 22, 2025

Choose a reason for hiding this comment

Make `chip.dart` use `WidgetStatesController` #161487

Make `chip.dart` use `WidgetStatesController` #161487

MitchellGoodwin Mar 21, 2025 •

edited

Loading