Refactor: Deprecate inactiveColor from cupertino checkbox #152981

rkishan516 · 2024-08-07T02:49:26Z

Refactor: Deprecate inactiveColor from cupertino checkbox
Fixes #151252

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.

flutter-dashboard · 2024-08-07T02:49:29Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

rkishan516 · 2024-08-07T02:52:17Z

@victorsanni does this need data driven fixes ?

victorsanni · 2024-08-07T17:17:50Z

@MitchellGoodwin I think this change should wait on the outcome of the WidgetStateProperty discussion?

Come to think of it, I wonder why inactiveColor can't just refer to the fill color of the unselected checkbox. It does seem like a simpler API.

victorsanni · 2024-08-07T17:19:06Z

@victorsanni does this need data driven fixes ?

I don't think so, since inactiveColor isn't currently used anyways.

MitchellGoodwin · 2024-08-07T17:47:17Z

I think this change should wait on the outcome of the WidgetStateProperty discussion?

I think it'd be fine to move forward on this. As a property, toggle colors react to interactive state out of the box more than most widgets, so I think pushing more strongly towards WidgetState is fine.

I don't think so, since inactiveColor isn't currently used anyways.

It probably should have been used as a default for the inactive color, if fillColor was empty. At least until it was deprecated. At a minimum we should update the docs to call out that it both does nothing and that the fillColor docs are incorrect in using it.

Come to think of it, I wonder why inactiveColor can't just refer to the fill color of the unselected checkbox. It does seem like a simpler API.

If you mean use inactiveColor as a Color, then it's because even when unselected it changes the fill color depending on if it's disabled or not as well. It's also reasonable to assume that somebody would want a different fill color for an unselected checkbox when hovered or focused as well. If you mean use inactiveColor as a WidgetStateProperty then it'd be weird that it'd technically also be checking for active states, as the type of WidgetStateProperty looks at all the WidgetState enum values. I don't think we have cases where we always ignore some of the states. It seems confusing from a documentation standpoint. Also there could be code duplication between fillColor and inactiveColor with what is the already verbose WidgetState.

does this need data driven fixes ?

@rkishan516 As mentioned inactiveColor doesn't currently do anything, and also it is difficult to convert a static value to a WidgetStateProperty resolver. I don't think there's a way to do it without guessing at what behavior they will want. Best not to make magical changes. This PR should just get a test exemption, in my opinion.

MitchellGoodwin · 2024-08-07T17:48:21Z

packages/flutter/lib/src/cupertino/checkbox.dart

  ///
  /// By default, [CupertinoColors.inactiveGray] is used.
+  @Deprecated(
+      'Use fillColor instead. '


Looks like this is indented too much

packages/flutter/lib/src/cupertino/checkbox.dart

victorsanni · 2024-08-07T17:53:46Z

At a minimum we should update the docs to call out that it both does nothing and that the fillColor docs are incorrect in using it.

Should this PR also remove every reference to inactiveColor in the documentation?

rkishan516 · 2024-08-07T18:38:33Z

@victorsanni I have updated the doc. Also removed instances of inactiveColor from doc. Can you please review ?

victorsanni · 2024-08-07T18:50:46Z

@victorsanni does this need data driven fixes ?

Following up from triage, this will need data driven fixes.

The suggestion was to apply fixes conditionally, so if the user already uses fillColor, do nothing. Otherwise, set fillColor to resolve to the user's inactiveColor in the unselected state.

MitchellGoodwin · 2024-08-07T19:00:42Z

Following up from triage, this will need data driven fixes.

The suggestion was to apply fixes conditionally, so if the user already uses fillColor, do nothing. Otherwise, set fillColor to resolve to the user's inactiveColor in the unselected state.

It should set fillColor to a copy of but use inactiveColor for the unselected state.

victorsanni · 2024-08-07T18:47:24Z

packages/flutter/lib/src/cupertino/checkbox.dart

  /// If [fillColor] resolves to null for the requested state, then the fill color
  /// falls back to [activeColor] if the state includes [WidgetState.selected],
-  /// or [inactiveColor] otherwise.
+  /// or [CupertinoColors.white] with 50% opacity otherwise.


I think the 50% opacity comes in when the checkbox is disabled and unselected? Otherwise its completely opaque?

MitchellGoodwin · 2024-08-07T21:50:51Z

packages/flutter/lib/src/cupertino/checkbox.dart

  final WidgetStateProperty<Color?>? fillColor;

-  /// The color used if the checkbox is inactive.
+  /// Note: [inactiveColor] is no longer being used.


We try not to use "Note" in our documentation. https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#avoid-empty-prose

I think the original first sentence was fine. It's a quick preview, and they'll see it's deprecated. It's probably best to explain in the paragraph below that it currently does nothing and to point people towards using fillColor to control the color when unselected.

rkishan516 · 2024-08-08T03:38:14Z

@victorsanni and @MitchellGoodwin I have made suggested changes.
I have added data driven fixes where inactiveColor get removed no matter fillColor is given by user or not.
if fillColor is given then I leave that as it is. And if it's not given then we add fillColor with WidgetStatePropertyAll(inactiveColor).
Certainly, we need to change the last part, Can you suggest what should we do here ?

MitchellGoodwin

Thank you for the updates! I left some comments on documentation and the default the data fix should use.

MitchellGoodwin · 2024-08-08T16:51:40Z

packages/flutter/lib/fix_data/fix_cupertino.yaml

+            name: 'fillColor'
+            style: optional_named
+            argumentValue:
+              expression: 'WidgetStatePropertyAll({% inactiveColor %})'


What we need to be the default is:

WidgetStateProperty.resolveWith<Color>((Set<WidgetState> states) { if (states.contains(WidgetState.disabled)) { return CupertinoColors.white.withOpacity(0.5); } if (states.contains(WidgetState.selected)) { return widget.activeColor ?? CupertinoDynamicColor.resolve(_kDefaultFillColor, context); } return {% inactiveColor %}; })

I have done the change, but since kDefaultFillColor is private, for now I have put the value of _ kDefaultFillColor as it is in here. But I don't think that's what we want. What do you think ?

Apologies, I should have mentioned that. I think it's fine. The risk would be if _kDefaultFillColor changes and somebody had waited until that point to use dart fix with an inactiveColor set but not activeColor and fillColor. I think all of those being true is pretty low, and even if they were at that point it would be an easy fix for them.

MitchellGoodwin · 2024-08-08T16:56:48Z

packages/flutter/lib/src/cupertino/checkbox.dart

-  /// state, [fillColor] will be used instead of [inactiveColor].
-  ///
-  /// By default, [CupertinoColors.inactiveGray] is used.
+  /// state, [fillColor] will be used instead of [CupertinoColors.white].


Maybe something like:

Currently [inactiveColor] is not used, and instead [fillColor] controls the color of the background of the checkbox at all states, including when unselected.

and the documentation for fillColor can be left to explain the default.

MitchellGoodwin · 2024-08-08T17:00:37Z

packages/flutter/lib/src/cupertino/checkbox.dart

+    @Deprecated(
+      'Use fillColor instead. '
+      'This feature was deprecated after v3.24.0-0.2.pre.'
+    )


Suggested change

@Deprecated(

'Use fillColor instead. '

'This feature was deprecated after v3.24.0-0.2.pre.'

)

@Deprecated(

'Use fillColor instead. '

`fillColor now manages the background color in all states. '

'This feature was deprecated after v3.24.0-0.2.pre.'

)

Prefer adding another line giving the reasoning.

victorsanni · 2024-08-09T17:36:10Z

packages/flutter/lib/src/cupertino/checkbox.dart

  /// If [fillColor] resolves to null for the requested state, then the fill color
  /// falls back to [activeColor] if the state includes [WidgetState.selected],
-  /// or [inactiveColor] otherwise.
+  /// or [CupertinoColors.white] with 50% if the state includes [WidgetState.disabled],


Suggested change

/// or [CupertinoColors.white] with 50% if the state includes [WidgetState.disabled],

/// [CupertinoColors.white] at 50% opacity if the checkbox is disabled,

victorsanni · 2024-08-09T17:36:50Z

packages/flutter/lib/src/cupertino/checkbox.dart

-  /// state, [fillColor] will be used instead of [inactiveColor].
-  ///
-  /// By default, [CupertinoColors.inactiveGray] is used.
+  /// Currently [inactiveColor] is not used, and instead [fillColor] controls the 


Suggested change

/// Currently [inactiveColor] is not used, and instead [fillColor] controls the

/// Currently [inactiveColor] is not used. Instead, [fillColor] controls the

packages/flutter/test_fixes/cupertino/cupertino.dart

rkishan516 · 2024-08-10T02:31:18Z

@victorsanni & @MitchellGoodwin I have pushed the mentioned changes.

victorsanni · 2024-08-10T14:36:18Z

/b/s/w/ir/x/w/flutter/packages/flutter/lib/src/cupertino/checkbox.dart:198: trailing U+0020 space character

rkishan516 · 2024-08-10T15:20:05Z

/b/s/w/ir/x/w/flutter/packages/flutter/lib/src/cupertino/checkbox.dart:198: trailing U+0020 space character

Have pushed the fix.

victorsanni · 2024-08-10T15:51:04Z

packages/flutter/lib/src/cupertino/checkbox.dart

-  /// state, [fillColor] will be used instead of [inactiveColor].
-  ///
-  /// By default, [CupertinoColors.inactiveGray] is used.
+  /// Currently [inactiveColor] is not used. instead, [fillColor] controls the


Suggested change

/// Currently [inactiveColor] is not used. instead, [fillColor] controls the

/// Currently [inactiveColor] is not used. Instead, [fillColor] controls the

victorsanni · 2024-08-10T15:51:27Z

packages/flutter/lib/src/cupertino/checkbox.dart

  /// falls back to [activeColor] if the state includes [WidgetState.selected],
-  /// or [inactiveColor] otherwise.
+  /// [CupertinoColors.white] at 50% opacity if checkbox is disabled,
+  /// or [CupertinoColors.white] otherwise.


Suggested change

/// or [CupertinoColors.white] otherwise.

/// and [CupertinoColors.white] otherwise.

rkishan516 · 2024-08-10T18:04:44Z

@victorsanni Pushed Suggested changes as per last conversation.

MitchellGoodwin

LGTM!

victorsanni

LGTM!

…utter/flutter#152981)

Manual roll requested by [email protected] flutter/flutter@814e49b...2afc452 2024-08-13 [email protected] Roll Flutter Engine from adb1fa9fdbcf to 9054eb43aa26 (2 revisions) (flutter/flutter#153338) 2024-08-13 [email protected] Roll Flutter Engine from af68e772d298 to adb1fa9fdbcf (2 revisions) (flutter/flutter#153336) 2024-08-13 [email protected] Roll Flutter Engine from 07ecd90b7755 to af68e772d298 (3 revisions) (flutter/flutter#153332) 2024-08-13 [email protected] Add fake dependency on flutter_gpu for the docs (flutter/flutter#153325) 2024-08-13 [email protected] Roll Flutter Engine from 5d8ee52e985b to 07ecd90b7755 (7 revisions) (flutter/flutter#153326) 2024-08-12 [email protected] Make CupertinoButton interactive by keyboard shortcuts (flutter/flutter#153126) 2024-08-12 [email protected] Added FlutterEngineGroups to engine architecture doc (flutter/flutter#153100) 2024-08-12 [email protected] Roll Flutter Engine from bcf2dcc09a13 to 5d8ee52e985b (4 revisions) (flutter/flutter#153313) 2024-08-12 [email protected] Disable DevTools when running the hot restart integration test in flutter_tools (flutter/flutter#153247) 2024-08-12 [email protected] Implemented CupertinoButton new styles/sizes (fixes #92525) (flutter/flutter#152845) 2024-08-12 [email protected] Roll pub packages (flutter/flutter#153297) 2024-08-12 [email protected] Refactor: Deprecate inactiveColor from cupertino checkbox (flutter/flutter#152981) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…2981) Refactor: Deprecate `inactiveColor` from cupertino checkbox Fixes flutter#151252

…utter/flutter#152981)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: cupertino flutter/packages/flutter/cupertino repository labels Aug 7, 2024

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch from 4a7de94 to cbb4f91 Compare August 7, 2024 04:19

MitchellGoodwin reviewed Aug 7, 2024

View reviewed changes

victorsanni reviewed Aug 7, 2024

View reviewed changes

packages/flutter/lib/src/cupertino/checkbox.dart Show resolved Hide resolved

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch 3 times, most recently from 407a58b to 8defb06 Compare August 7, 2024 18:28

victorsanni reviewed Aug 7, 2024

View reviewed changes

MitchellGoodwin reviewed Aug 7, 2024

View reviewed changes

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch 2 times, most recently from 4ff7cf0 to 041bfe7 Compare August 8, 2024 03:30

github-actions bot added the c: tech-debt Technical debt, code quality, testing, etc. label Aug 8, 2024

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch from 041bfe7 to f9f0e88 Compare August 8, 2024 03:31

MitchellGoodwin reviewed Aug 8, 2024

View reviewed changes

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch 2 times, most recently from fcc3619 to bed7109 Compare August 9, 2024 06:54

victorsanni reviewed Aug 9, 2024

View reviewed changes

MitchellGoodwin reviewed Aug 9, 2024

View reviewed changes

packages/flutter/test_fixes/cupertino/cupertino.dart Show resolved Hide resolved

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch from bed7109 to 8a988c1 Compare August 10, 2024 02:29

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch 2 times, most recently from 3620cfa to 2cf0c8c Compare August 10, 2024 15:19

victorsanni reviewed Aug 10, 2024

View reviewed changes

refactor: Deprecate inactiveColor from cupertino checkbox

d3bcb7e

rkishan516 force-pushed the remove-inactive-color-from-cupertino-checkbox branch from 2cf0c8c to d3bcb7e Compare August 10, 2024 18:03

MitchellGoodwin approved these changes Aug 12, 2024

View reviewed changes

victorsanni reviewed Aug 12, 2024

View reviewed changes

victorsanni approved these changes Aug 12, 2024

View reviewed changes

victorsanni added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 12, 2024

auto-submit bot merged commit 33c05da into flutter:master Aug 12, 2024

engine-flutter-autoroll mentioned this pull request Aug 13, 2024

Manual roll Flutter from 814e49be4922 to 2afc452d3a8e (12 revisions) flutter/packages#7399

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 13, 2024

33c05da Refactor: Deprecate inactiveColor from cupertino checkbox (fl…

1168033

…utter/flutter#152981)

Piinks mentioned this pull request Aug 16, 2024

Flutter Framework Team: Notable Commits #121415

Closed

DBowen33 pushed a commit to DBowen33/flutter that referenced this pull request Aug 16, 2024

Refactor: Deprecate inactiveColor from cupertino checkbox (flutter#15…

8086d15

…2981) Refactor: Deprecate `inactiveColor` from cupertino checkbox Fixes flutter#151252

Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024

Refactor: Deprecate inactiveColor from cupertino checkbox (flutter#15…

7a8ab0a

…2981) Refactor: Deprecate `inactiveColor` from cupertino checkbox Fixes flutter#151252

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024

33c05da Refactor: Deprecate inactiveColor from cupertino checkbox (fl…

60cbf58

…utter/flutter#152981)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024

33c05da Refactor: Deprecate inactiveColor from cupertino checkbox (fl…

28ab9ab

…utter/flutter#152981)

rkishan516 deleted the remove-inactive-color-from-cupertino-checkbox branch July 26, 2025 05:51

	/// or [CupertinoColors.white] with 50% if the state includes [WidgetState.disabled],
	/// [CupertinoColors.white] at 50% opacity if the checkbox is disabled,

	/// Currently [inactiveColor] is not used, and instead [fillColor] controls the
	/// Currently [inactiveColor] is not used. Instead, [fillColor] controls the

	/// or [CupertinoColors.white] otherwise.
	/// and [CupertinoColors.white] otherwise.

Refactor: Deprecate inactiveColor from cupertino checkbox #152981

Refactor: Deprecate inactiveColor from cupertino checkbox #152981

Uh oh!

Conversation

rkishan516 commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 7, 2024

Uh oh!

rkishan516 commented Aug 7, 2024

Uh oh!

victorsanni commented Aug 7, 2024

Uh oh!

victorsanni commented Aug 7, 2024

Uh oh!

MitchellGoodwin commented Aug 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

victorsanni commented Aug 7, 2024

Uh oh!

rkishan516 commented Aug 7, 2024

Uh oh!

victorsanni commented Aug 7, 2024

Uh oh!

MitchellGoodwin commented Aug 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented Aug 8, 2024

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rkishan516 commented Aug 10, 2024

Uh oh!

victorsanni commented Aug 10, 2024

Uh oh!

rkishan516 commented Aug 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkishan516 commented Aug 10, 2024

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

victorsanni left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rkishan516 commented Aug 7, 2024 •

edited

Loading