added optional labelText and semanticLabel to Checkbox #116551
Conversation
flutter-dashboard
bot
added
f: material design
| /// The label text that will be rendered side by side with the [Checkbox]. | ||
| /// Serves as default semantics label for the [Checkbox] if provided. |
There was a problem hiding this comment.
| /// The label text that will be rendered side by side with the [Checkbox]. | |
| /// Serves as default semantics label for the [Checkbox] if provided. | |
| /// The label text that will be rendered next to the [Checkbox]. | |
| /// | |
| /// If provided, serves as the default for [semanticLabel]. |
| // TODO(harperl-lgtm): Actually render the labelText in build method. | ||
| final String? labelText; | ||
|
|
||
| /// The semantics label that will be announced by a screen reader. |
There was a problem hiding this comment.
| /// The semantics label that will be announced by a screen reader. | |
| /// The semantic label that will be announced by screen readers. |
|
|
||
| /// The semantics label that will be announced by a screen reader. | ||
| /// | ||
| /// Overrides labelText in terms of semantic labeling of the [Checkbox] |
There was a problem hiding this comment.
| /// Overrides labelText in terms of semantic labeling of the [Checkbox] | |
| /// Semantic label for the checkbox. | |
| /// | |
| /// Announced in accessibility modes (e.g TalkBack/VoiceOver). | |
| /// This label does not show in the UI. | |
| /// | |
| /// Defaults to [labelText], if specified. |
| this.shape, | ||
| this.side, | ||
| this.isError = false, | ||
| this.labelText, |
There was a problem hiding this comment.
Let's avoid adding an unused property and save that for a future PR
|
@darrenaustin Was curious about your thoughts on the proposed TODO for this. |
darrenaustin
left a comment
darrenaustin
left a comment
There was a problem hiding this comment.
LGTM, with a couple of comments below. If an app already has a label that they have added themselves and wrapped the check box and label in a MergeSemantics, what happens with this new semanticLabel?
It sounds reasonable to me, but perhaps there was a reason we chose not to implement it with a label. @HansMuller do you have any opinion/context on this? |
Long ago we decided not to make intrinsic labels a feature of components like Checkbox. Over time that gap has been filled in - for better or worse - by the FooListTile components. If the FooListTile component is inconvenient for a common case, then we should make it convenient, perhaps with a named constructor. |
|
Thank you so much Hans for your insights! :D I agree it makes sense to pass a semantic label to a named constructor. Added in new commit! |
|
This looks good. Are you going to add a |
|
I thought you'd just add a single semanticLabel constructor parameter to CheckboxListTile, I don't think the named constructor is needed (unless I'm missing something). I realize that I brought up FooListTile named constructors in #116551 (comment), but that suggestion assumed that an (unspecifed) common case was inconvenient. In this case, it seems like just passing the semanticLabel parameter through, from the CheckboxListTile to its Checkbox, would be sufficient and convenient. |
…manticLabel to exsiting constructor
I see. Thanks for clarifying Hans!! Constructor updated. |
|
Looping in @Hangyujin who is going to take over some of my work. And thank you Hangyu for taking care of my PRs :)! |
|
Closing this PR and reopen it int #124555 because the author is no longer working on this. |
Re-open from #116551 This PR added optional labelText (which will be rendered side by side with Checkbox in the future, and thus is also announced by default by screen readers), and semanticLabel(which will be announced by screen reader if provided, overrides labelText, in order to do that we might want to wrap the Text widget inside ExcludeSemantics in the future). Issues fixed: [b/239564167]
Re-open from flutter#116551 This PR added optional labelText (which will be rendered side by side with Checkbox in the future, and thus is also announced by default by screen readers), and semanticLabel(which will be announced by screen reader if provided, overrides labelText, in order to do that we might want to wrap the Text widget inside ExcludeSemantics in the future). Issues fixed: [b/239564167]
5 participants
This PR added optional labelText (which will be rendered side by side with Checkbox in the future, and thus is also announced by default by screen readers), and semanticLabel(which will be announced by screen reader if provided, overrides labelText, in order to do that we might want to wrap the Text widget inside ExcludeSemantics in the future).
Issues fixed:
[b/239564167]
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.