Adds radio group widget r2 #168161
Adds radio group widget r2 #168161
Conversation
github-actions
bot
added
framework
chunhtai
force-pushed
the
radio-group-r2
branch
4 times, most recently
from
4b70dfd to
69f766b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new RadioGroup widget that wraps and manages Radio widgets across material and cupertino examples, tests, and demos.
- Replaces direct Radio widget usage with RadioGroup in tests and examples.
- Updates several test cases and demo code to validate the new widget behavior.
- Adds a new example file for the RadioGroup widget while adjusting integration test expectations.
Reviewed Changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examples/api/test/material/radio/radio.toggleable.0_test.dart | Updates widget predicate and expectation from Radio to RadioGroup. |
| examples/api/test/material/radio/radio.0_test.dart | Changes widget extraction to use RadioGroup instead of Radio. |
| examples/api/test/cupertino/radio/* | Adjusts tests from CupertinoRadio to RadioGroup widget usage. |
| examples/api/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme | Enables GPU validation mode in the Xcode scheme. |
| examples/api/lib/widgets/radio_group/radio_group.0.dart | Introduces the new RadioGroup widget and its usage examples. |
| examples/api/lib/material/radio_list_tile/* | Wraps RadioListTile examples in a RadioGroup and updates parameter handling. |
| examples/api/lib/material/radio/* | Replaces separate Radio widgets with a RadioGroup wrapper. |
| examples/api/lib/cupertino/radio/* | Updates Cupertino radio demos to use RadioGroup for group value management. |
| dev/integration_tests/* | Updates demos and tests references from the old to the new RadioGroup widget. |
Comments suppressed due to low confidence (2)
examples/api/lib/material/radio_list_tile/custom_labeled_radio.1.dart:31
- Consider adding a doc comment for the 'onInkWellTap' parameter to explain its role in replacing the previous onChanged callback.
final VoidCallback onInkWellTap,
examples/api/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme:51
- Ensure that enabling GPU validation mode is intentional for the intended build configurations, as it may impact performance if left enabled in non-debug builds.
enableGPUValidationMode = "1"
| widget.onChanged != null || | ||
| widget.groupRegistry != null || | ||
| RadioGroup.maybeOf<T>(context) != null, | ||
| 'Radio is enabled but has no CupertinoRadio.onChange or registry above', |
There was a problem hiding this comment.
| 'Radio is enabled but has no CupertinoRadio.onChange or registry above', | |
| 'Radio is enabled but has no CupertinoRadio.onChange or RadioGroupRegistry. ', |
We might want to expand this error message by making this an assert block so we can specify that either the registry provided by the user did not work, or an ancestor one was not found using the BuildContext.
There was a problem hiding this comment.
I am not sure I follow, as long as one of them is provided, they will pass the check.
| /// Whether this radio button is checked. | ||
| /// | ||
| /// To control this value, set [value] and [groupValue] appropriately. | ||
| bool get checked => value == groupValue; |
There was a problem hiding this comment.
I wonder if this might cause a little breakage. Probably something to mention in the migration guide for this change explaining some of the motivations here re: accessibility
There was a problem hiding this comment.
will include this in migration guide
| _radios.add(radio); | ||
| assert( | ||
| _debugCheckOnlySingleSelection(), | ||
| "RadioGroupPolicy can't be used for a radio group that allows multiple selection", |
There was a problem hiding this comment.
Oh wait, this makes me wonder, if it is a multiple selection radio, does that mean users should use the deprecated APIs?
There was a problem hiding this comment.
If they want to use multi selection radio, they will have make each radio a radio group, it may be slightly more verbose, but I don't think it is a common use case. Radios are meant to be mutually exclusive.
I will make sure I include this example in migration guide.
There was a problem hiding this comment.
This one is a pretty good example of RadioGroup now 👍
There was a problem hiding this comment.
Seeing this example makes me think that there should be a way to build LabeledRadio using RawRadio. The need for onInkWellTap seems like a red flag. I feel like LabeledRadio should be able to toggle the value when it gets tapped in the same way that Radio does.
Is RawRadio meant to allow something like this? Is it a weird case because it contains a radio button itself? I guess it might need access to the registry in order to do this.
I feel like this is a common real-ish example of when someone might want to build their own radio button using RawRadio. Am I off or what do you think?
There was a problem hiding this comment.
Even If using RawRadio, we still needs to handle inkWell tap if it is wrapped under RawRadio. In gesture arena, InkWell will win the gesture.
If we somehow let the RawRadio to steal the gesture from InkWell, the ink splash won't be trigger.
Also for this specific use case, they can use RadioListTile. which handles all the ugliness.
There was a problem hiding this comment.
I made some changes to get rid of the onInkWellTap. I still have to handle the tap in inkwell, but at least it becomes an internal implementation.
| /// in the sub tree with the same type T as a group. Radios with different types | ||
| /// are not included in the group. |
There was a problem hiding this comment.
Is it definitely a mistake to put a Radio into a RadioGroup of a different type? I wonder if we can trigger an assert in that case somehow? (or maybe you already did somewhere)
There was a problem hiding this comment.
you can nest them if you like
RadioGroup<int>(
child: Column(
children: [
Radio<int>(value:0),
Radio<int>(value:1),
RadioGroup<String>(child: Radio<String>(value:'a'))
]
),
)we could force them to not allow nesting, but I think that reduce some flexibility.
| /// same value type. | ||
| /// | ||
| /// Using this widget also provides keyboard navigation and semantics for the | ||
| /// radio buttons that matches [APG](https://www.w3.org/WAI/ARIA/apg/patterns/radio/) |
| /// * Tab and Shift+Tab: moves focus into and out of radio group. When focus | ||
| /// moves into a radio group and a radio button is select, focus is set on | ||
| /// selected button. Otherwise, it focus the first radio button in reading | ||
| /// order |
There was a problem hiding this comment.
Missing a period at the end.
| /// The `onChanged` is called when the selection has changed in the subtree | ||
| /// radios. | ||
| const RadioGroup({super.key, this.groupValue, required this.onChanged, required this.child}) | ||
| : super(); |
There was a problem hiding this comment.
Is the super call necessary?
| /// The value can be null if when unselect the [RawRadio] with | ||
| /// [RawRadio.toggleable] set to true. |
There was a problem hiding this comment.
I think: "if when unselect" => "when unselecting"
|
|
||
| /// Registers a radio client. | ||
| /// | ||
| /// the subclass provides additional features, such as keyboard navigation |
| /// radio button's [groupValue]. | ||
| /// {@macro flutter.widget.RawRadio.groupValue} | ||
| /// | ||
| /// If [enabled] is false, the radio will not be interactable. |
There was a problem hiding this comment.
interactable => interactive
| /// To indicate returning to an indeterminate state, [onChanged] will be | ||
| /// called with null. | ||
| /// | ||
| /// If true, [onChanged] is called with [value] when selected while | ||
| /// [groupValue] != [value], and with null when selected again while | ||
| /// [groupValue] == [value]. | ||
| /// | ||
| /// If false, [onChanged] will be called with [value] when it is selected | ||
| /// while [groupValue] != [value], and only by selecting another radio button | ||
| /// in the group (i.e. changing the value of [groupValue]) can this radio | ||
| /// button be unselected. | ||
| /// To indicate returning to an indeterminate state. |
There was a problem hiding this comment.
Did you mean to remove this block? The remaining sentence doesn't make sense by itself.
chunhtai
force-pushed
the
radio-group-r2
branch
2 times, most recently
from
84f1213 to
bdf82c8
Compare
|
all comment addressed, migration guide added in pr description, this is ready for another look |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new RadioGroup widget to centralize groupValue/onChanged logic for radio controls and refactors existing examples and tests to use it.
- Adds a standalone
RadioGroupexample and wraps allRadio,RadioListTile, andCupertinoRadiosamples in it. - Updates API example code to remove per-widget
groupValue/onChanged, delegating those toRadioGroup. - Adjusts tests to query
RadioGroup.groupValueinstead of inspecting individual radio widgets.
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| examples/api/lib/widgets/radio_group/radio_group.0.dart | New RadioGroup example widget |
| examples/api/lib/material/radio_list_tile/radio_list_tile.toggleable.0.dart | Wrapped toggleable list tile demo in RadioGroup |
| examples/api/lib/material/radio_list_tile/radio_list_tile.1.dart | Wrapped grocery list tile demo in RadioGroup |
| examples/api/lib/material/radio_list_tile/radio_list_tile.0.dart | Wrapped basic list tile demo in RadioGroup |
| examples/api/lib/material/radio_list_tile/custom_labeled_radio.1.dart | Refactored LabeledRadio demo for RadioGroup |
| examples/api/lib/material/radio_list_tile/custom_labeled_radio.0.dart | Refactored LinkedLabelRadio demo for RadioGroup |
| examples/api/lib/material/radio/radio.toggleable.0.dart | Wrapped toggleable radio demo in RadioGroup |
| examples/api/lib/material/radio/radio.0.dart | Wrapped basic radio demo in RadioGroup |
| examples/api/lib/cupertino/radio/cupertino_radio.toggleable.0.dart | Wrapped Cupertino toggleable demo in RadioGroup |
| examples/api/lib/cupertino/radio/cupertino_radio.0.dart | Wrapped Cupertino radio demo in RadioGroup |
| examples/api/test/material/radio_list_tile/custom_labeled_radio.0_test.dart | Test now asserts on RadioGroup |
| examples/api/test/material/radio/radio.toggleable.0_test.dart | Test now asserts on RadioGroup |
| examples/api/test/material/radio/radio.0_test.dart | Test now reads groupValue from RadioGroup |
| examples/api/test/cupertino/radio/cupertino_radio.toggleable.0_test.dart | Test now asserts on RadioGroup |
| examples/api/test/cupertino/radio/cupertino_radio.0_test.dart | Test now reads groupValue from RadioGroup |
| dev/integration_tests/new_gallery/lib/demos/material/selection_controls_demo.dart | Removed redundant onChanged: null |
| dev/integration_tests/flutter_gallery/test/demo/material/expansion_panels_demo_test.dart | Updated radio finder to RadioListTile |
| dev/integration_tests/flutter_gallery/lib/gallery/example_code.dart | Removed redundant onChanged: null |
| dev/integration_tests/flutter_gallery/lib/demo/material/selection_controls_demo.dart | Removed redundant onChanged: null |
| dev/benchmarks/macrobenchmarks/lib/src/web/material3.dart | Removed redundant onChanged: null |
Comments suppressed due to low confidence (1)
dev/integration_tests/flutter_gallery/test/demo/material/expansion_panels_demo_test.dart:41
- [nitpick] The getter name
_radiosimplies it returns rawRadiowidgets, but it actually returnsRadioListTile<Location>instances. Consider renaming it to clarify that it yieldsRadioListTileelements and avoid confusion with the existing_radioListTilesgetter.
List<RadioListTile<Location>> get _radios => List<RadioListTile<Location>>.from(
| onChanged(newValue!); | ||
| }, | ||
| ), | ||
| Radio<bool>(value: value), |
There was a problem hiding this comment.
The LinkedLabelRadio widget no longer wraps its children in an InkWell or handle taps on the label, so tapping the label won't toggle the radio. Consider reintroducing an InkWell (similar to LabeledRadio) and invoking RadioGroup.maybeOf(context)?.onChanged(value) on tap.
Copilot uses AI. Check for mistakes.
_Description of what this PR is changing or adding, and why:_ migration guide for flutter/flutter#168161 _Issues fixed by this PR (if any):_ flutter/flutter#113562 _PRs or commits this PR depends on (if any):_ flutter/flutter#168161 ## Presubmit checklist - [ ] This PR is marked as draft with an explanation if not meant to land until a future stable release. - [ ] This PR doesn’t contain automatically generated corrections (Grammarly or similar). - [ ] This PR follows the [Google Developer Documentation Style Guidelines](https://developers.google.com/style) — for example, it doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person). - [ ] This PR uses [semantic line breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks) of 80 characters or fewer. --------- Co-authored-by: Shams Zakhour (ignore Sfshaza) <44418985+sfshaza2@users.noreply.github.com>
previous #167363
I have to factor out abstract class for RadioGroupRegistry and RadioClient which are subclassed by RadioGroupState and RawRadio respectively.
I have to do this because the RadioListTile that has 2 focusnode one for listTile and one for the radio it builds. The issue is that RadioGroup's keyboard shortcut has to tightly couples with the focus node of each radio, but the radioListtile has to mute the radio's focusnode so it can act as one control under keyboard shortcut
flutter/packages/flutter/lib/src/material/radio_list_tile.dart
Line 484 in d582b35
Therefore i abstract the out the logic of RadioGroup so that another widget can participate by implementing the interface.
fixes #113562
migration guide: https://github.com/flutter/website/pull/12080/files
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.