Improve `CupertinoCheckbox`'s fidelity and customizability #148804

victorsanni · 2024-05-21T21:33:18Z

This PR improves on the look and feel of CupertinoCheckbox to more closely match native iOS/macOS checkboxes. It also adds functionality to resolve the checkbox's fill color in different WidgetStates, configure a mouse cursor and a semantic label, all improving customizability.

This PR also adds examples and tests to CupertinoCheckbox.

Before	After

Native macOS	Flutter Before	Flutter After

Light mode	Dark mode

Part of #148719.

Related PR exploring these changes: #147892

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.

MitchellGoodwin

LGTM. With one question about shadows.

MitchellGoodwin · 2024-05-22T17:01:50Z

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

+        ],
+      );
+      final Paint gradientPaint = Paint()
+        ..shader = topEdgeGradient.createShader(shadowRect);


Should we add a way to customize the shadow in someway? Either turn it off completely with a bool or let them provide their own shadow? It doesn't look like this shadow could be removed if wanted.

I am exploring passing a LinearGradient as a parameter to CupertinoCheckbox to be used in place of topEdgeGradient. Is that a good idea, considering shadowRect is independent of topEdgeGradient (and used alongside it in drawing the shadow)? Is there a better way to do this (callbacks, etc) or should I just keep it simple and use a bool?

I've been thinking about this more. I think we don't want to expose too complicated of a parameter, because it gets messy when if we think about the Material check box. It raises the question of whether we should add the equivalent to Material. But we don't provide shadow properties a lot because we generally encourage designing through composability. So we give them an easy way to remove the shadow, and if they want a different one, they wrap the widget with their own shadow. A bool could work, but maybe it would be more elegant to provide a shadowColor? And they could hide the shadow by making it transparent?

Yeah, I have seen shadowColor in the material library. I think it is a pretty common pattern there 👍

On further inspection I've found that the shadow in native macOS checkboxes are even smaller and less-layered than previously thought. Considering its relatively minimal visual (and functional) effect, and the complexity of an additional parameter (without a Material equivalent, to boot), it might be worth it to consider sacrificing native fidelity for an easier to use, Material-compatible API in this instance. @MitchellGoodwin @Piinks

I'm fine without a shadow. They can wrap them with a shadow if they want one.

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

justinmc

One architecture idea: What if there was a RawCheckbox in the widgets library that accepted the painter as a parameter? Then Checkbox and CupertinoCheckbox just wrap that. That may also eliminate a bunch of duplicate code and tests.

One big adaptivity question: Should we be trying to make Cupertino widgets customizable in the same way that Material widgets are? Is this exposing us to more maintenance burden?

Like we talked about with @Piinks, I'd be interested to see a feature comparison between this approach and your original PR.

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

justinmc · 2024-05-23T17:57:22Z

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

    required this.onChanged,
+    this.mouseCursor,
    this.activeColor,
    this.inactiveColor,


Checkbox has no inactiveColor. Does it do that with MaterialStateProperty? Anything we can do to align them?

Yes, Checkbox does that with MaterialStateProperty. My guess is Checkbox and CupertinoCheckbox's API's were designed before MaterialStateProperty could configure fillColor in both active and inactive states, and so activeColor and inactiveColor aren't needed anymore in both Checkbox and CupertinoCheckbox. Both parameters could (and probably should) be removed, but wouldn't that break users?

It could be breaking, but we typically will deprecate and maintain backwards compatibility for a while so folks can take the time to migrate to the new API. Since CupertinoSwitch uses the same active/inactive parameters, I think this is OK. @MitchellGoodwin had some really good thoughts on how to evaluate when we do or don't use WSP here: #149465 (comment)

Currently inactiveColor only changes the border color when the checkbox is unselected. Which the changes to shape in this PR make that a little redundant. I think inactiveColor should be deprecated, because now it just doesn't do much. There's an argument to deprecate activeColor as well, but I think it's fine to keep it around for the simple cases.

Filed tracking issue #151252

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

packages/flutter/test/cupertino/checkbox_test.dart

examples/api/lib/cupertino/checkbox/cupertino_checkbox.0.dart

examples/api/test/cupertino/checkbox/cupertino_checkbox.0_test.dart

justinmc · 2024-05-24T18:21:13Z

To expand on what I mean about a "feature comparison" as I mentioned above in my review, I'm talking about answering questions like these:

Direct differences in the parameters list and capabilities.
How does Checkbox use Theme? Does CupertinoCheckbox use CupertinoTheme?
Checkbox makes use of the Material widget and CupertinoCheckbox does not. What is CupertinoCheckbox missing out on there?
If I've developed an Android app in Material first, and I've themed my Checkbox, what is the experience of building a similarly-themed iOS checkbox with this PR vs. with the approach in Make Checkbox adaptive without delegating to CupertinoCheckbox #147892? I imagine it might be easier to get an iOS checkbox with the same styles as the Material one if you're using 147892, but I wonder about the details.
Any other possible differences I'm not thinking of.

QuncCccccc · 2024-05-30T19:36:29Z

I can try to answer this question:)

I imagine it might be easier to get an iOS checkbox with the same styles as the Material one if you're using 147892, but I wonder about the details.

This Switch example shows how we can theme iOS Switch from Material Switch:

Create a subclass of Adaptation<T> with SwitchThemeData type, like SwitchThemeAdaptation.
Implement the adapt method in this class and provide SwitchThemeData based on different platforms.
if we want a similar configuration for both iOS and Material, then probably just to provide the same or similar SwitchThemeData like:

SwitchThemeData adapt(ThemeData theme, SwitchThemeData defaultValue) {
  switch (theme.platform) {
    case TargetPlatform.fuchsia:
    case TargetPlatform.linux:
    case TargetPlatform.windows:
    case TargetPlatform.macOS:
      return defaultValue;
    case TargetPlatform.android:
    case TargetPlatform.iOS:
      return SwitchThemeData(
        // some customization that we want.
      );
  }
}

the last step would be adding this SwitchThemeAdaptation to ThemeData.adaptations which takes a list of adaptation, so we can add as many adaptive widget theme as we want:)

To get the adaptive widget theme:

Use Theme.of(context).getAdaptation<T>, like the switchAdaptation
Get widget theme by calling Adaptation<T>.adapt() method, like here.

victorsanni · 2024-05-30T20:59:50Z

Direct differences in the parameters list and capabilities.

CupertinoCheckbox as in this PR only adds new extra parameters without modifying any previous parameters. The new parameters added are: mouseCursor, fillColor, isError, and semanticLabel, with the same capabilities as their material counterparts, and shadowColor which is specific to CupertinoCheckbox.

How does Checkbox use Theme? Does CupertinoCheckbox use CupertinoTheme?

As far as I see, Checkbox only uses Theme to:

Check the target platform
Check if Material 3 should be used.

However, Checkbox gets theme properties from CheckboxThemeData. When configuring the material checkbox’s properties, the order of precedence is:

      widget.property > CheckboxThemeData.property > defaults

CupertinoThemeData does not have component themes, so this order of precedence with CupertinoCheckbox is just

       widget.property > defaults

CupertinoCheckbox doesn't use CupertinoTheme, but CupertinoSwitch does, for example to set the track color to theme.primaryColor. CupertinoThemeData is very limited though.

Checkbox makes use of the Material widget and >CupertinoCheckbox does not. What is CupertinoCheckbox missing out on there?

I might be misunderstanding this question, but I think it more that Checkbox requires one of its ancestors to be a Material widget, not that it itself uses the Material widget. When I removed the line assert(debugCheckHasMaterial(context));
and ran a cupertino app with Checkbox having no material ancestor, I didn’t run into any errors/change in behavior.

If I've developed an Android app in Material first, and I've themed my Checkbox, what is the experience of building a similarly-themed iOS checkbox with this PR vs. with the approach in Make Checkbox adaptive without delegating to CupertinoCheckbox #147892? I imagine it might be easier to get an iOS checkbox with the same styles as the Material one if you're using 147892, but I wonder about the details.

Does theming the checkbox here mean:
a) creating an app-wide CheckboxTheme by setting CheckboxThemeData values, or
b) Individually configuring some checkbox by setting parameters?

For a), CupertinoCheckbox as in this PR doesn’t provide app-wide theming, but using Checkbox.adaptive as in #147892 does as Qun explained above.

For b), both CupertinoCheckbox as in this PR and Checkbox.adaptive as in #147892 have all the non-material-design-specific parameters as Checkbox, so the developer would only need to remove the non-material-design-specific parameters (hoverColor, overlayColor, splashRadius, materialTapTargetSize, and visualDensity).

Piinks

This LGTM to me as well modulo a doc nit :)

Some of the outstanding questions were around theming.. we talked offline about this a bit on Monday. Since CupertinoCheckbox does not currently use a theme, we won't be adding it here right now. It's ok, and probably best, for us to make these changes incrementally. This PR brings the checkbox up to date with full fidelity and some better alignment with material. After this PR, we could see if we want to incorporate CupertinoTheme in some way.

As far as cross library theming goes, we aren't diving too deeply into that yet. Having high fidelity native looking iOS widgets is our first priority, and in the meantime we are gathering more information from users to help inform how we can improve the adaptive angle. 👍

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

Piinks · 2024-06-05T19:45:49Z

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

+        ],
+      );
+      final Paint gradientPaint = Paint()
+        ..shader = topEdgeGradient.createShader(shadowRect);


Yeah, I have seen shadowColor in the material library. I think it is a pretty common pattern there 👍

Piinks · 2024-06-05T19:48:47Z

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

    required this.onChanged,
+    this.mouseCursor,
    this.activeColor,
    this.inactiveColor,


It could be breaking, but we typically will deprecate and maintain backwards compatibility for a while so folks can take the time to migrate to the new API. Since CupertinoSwitch uses the same active/inactive parameters, I think this is OK. @MitchellGoodwin had some really good thoughts on how to evaluate when we do or don't use WSP here: #149465 (comment)

Co-authored-by: Kate Lovett <[email protected]>

flutter-dashboard · 2024-07-08T23:28:33Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

victorsanni · 2024-07-09T00:00:10Z

Closing due to a bad merge leading to pollution with unrelated commits. New PR is #151441.

Brings over the changes from `Switch.adaptive` into `CupertinoSwitch`. This change adds the following `Switch` parameters to `CupertinoSwitch`: `inactiveThumbColor,` `activeThumbImage`, `onActiveThumbImageError`, `inactiveThumbImage`, `onInactiveThumbImageError`, `trackOutlineColor`, `trackOutlineWidth`, `thumbIcon`, `mouseCursor`. The following `Switch` parameters are ignored: * `activeTrackColor`: because `activeColor` has the same function. * `inactiveTrackColor`: because `trackColor` has the same function. * `materialTapTargetSize`: because it is only applicable in `Material`. * `hoverColor`, `overlayColor`, `splashRadius`: because these parameters configure the radial reaction overlay in `Material`, so are not applicable here. The following `CupertinoSwitch` parameters which are absent from `Switch.adaptive` are retained: * `onLabelColor`, * `offLabelColor`, * `applyTheme` `trackColor` and `thumbColor` are of type `WidgetStateProperty` in Material `Switch`, but are currently of type `Color` in `CupertinoSwitch`. For backwards compatibility, both parameters are kept as `Color`s, but can be resolved in different `WidgetState`s using `WidgetStateColor.resolve()`. This PR does not apply any fidelity updates to `CupertinoSwitch`. Part of #149275 Related PRs: #130425 #148804

Improve CupertinoCheckbox's fidelity and customizability

6b7326e

victorsanni marked this pull request as ready for review May 21, 2024 21:34

Minor docs and formatting changes

93bb81d

victorsanni requested review from MitchellGoodwin and Piinks May 21, 2024 23:03

MitchellGoodwin reviewed May 22, 2024

View reviewed changes

MitchellGoodwin mentioned this pull request May 22, 2024

WidgetState mapping #146043

Merged

Make side non-nullable

0ba4831

victorsanni requested a review from justinmc May 22, 2024 20:14

victorsanni added 3 commits May 22, 2024 17:27

Make dropshadow optional

5b924ff

Minor fixes

3d0a806

Configure shadow color

6860f32

justinmc reviewed May 23, 2024

View reviewed changes

Apply Justin's suggested changes

ce6d639

victorsanni mentioned this pull request May 29, 2024

Improve CupertinoSwitch's fidelity and customizability #149277

Closed

9 tasks

Add version number to documentation

18d3128

This was referenced Jun 1, 2024

Roll Switch.adaptive into CupertinoSwitch #149464

Closed

Roll Switch.adaptive changes into CupertinoSwitch #149465

Merged

Piinks approved these changes Jun 5, 2024

View reviewed changes

victorsanni and others added 3 commits June 5, 2024 22:16

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

e50c4d2

Co-authored-by: Kate Lovett <[email protected]>

Add test for mouse cursor in different widget states

b099dd0

Merge branch 'master' into improve-cupertino-checkbox-fidelity

8c17b90

victorsanni removed the request for review from christopherfujino July 8, 2024 22:52

victorsanni added 3 commits July 8, 2024 15:53

Merge branch 'master' into improve-cupertino-checkbox-fidelity

e11e08e

(take 2) Revert checkbox.adaptive

e986f1d

Revert checkbox.adaptive tests

63da870

github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jul 8, 2024

victorsanni marked this pull request as draft July 8, 2024 23:26

Add hasFocusAction: true for semantics

3a2b26d

victorsanni mentioned this pull request Jul 8, 2024

Improve CupertinoCheckbox fidelity #151441

Merged

9 tasks

victorsanni closed this Jul 9, 2024

victorsanni deleted the improve-cupertino-checkbox-fidelity branch July 13, 2024 00:15

Improve CupertinoCheckbox's fidelity and customizability #148804

Improve CupertinoCheckbox's fidelity and customizability #148804

Uh oh!

Conversation

victorsanni commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

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!

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc commented May 24, 2024

Uh oh!

QuncCccccc commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

victorsanni commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Jul 8, 2024

Uh oh!

victorsanni commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

Improve `CupertinoCheckbox`'s fidelity and customizability #148804

Improve `CupertinoCheckbox`'s fidelity and customizability #148804

victorsanni commented May 21, 2024 •

edited

Loading

QuncCccccc commented May 30, 2024 •

edited

Loading

victorsanni commented May 30, 2024 •

edited

Loading

victorsanni commented Jul 9, 2024 •

edited

Loading