Reland #151599 (Add button semantics in list tile ) with a flag to control behavior. (#152526)

https://github.com/flutter/flutter/pull/151599 was reverted because it was a breaking change to g3

Will reland 151599 in 5 steps 
1. Make changes with an additional parameter ( bool internalAddSemanticForOnTap = false)  
2. Send regular CLs in google3 to pass internalAddSemanticForOnTap: true, and update the tests / text goldens accordingly.
3. Send a PR to flip the default value to true.
4. Send CLs internally to remove internalAddSemanticForOnTap: true.
5. Send another PR to remove the now-redundant internalAddSemanticForOnTap flag.

 (<----This PR is step 1)
This commit is contained in:
hangyu 2024-07-30 13:38:06 -07:00 committed by GitHub
parent 5f460e10fc
commit dc6a15995e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 72 additions and 0 deletions

View File

@ -205,6 +205,7 @@ class CheckboxListTile extends StatelessWidget {
this.onFocusChange, this.onFocusChange,
this.enableFeedback, this.enableFeedback,
this.checkboxSemanticLabel, this.checkboxSemanticLabel,
this.internalAddSemanticForOnTap = false,
}) : _checkboxType = _CheckboxType.material, }) : _checkboxType = _CheckboxType.material,
assert(tristate || value != null), assert(tristate || value != null),
assert(!isThreeLine || subtitle != null); assert(!isThreeLine || subtitle != null);
@ -249,6 +250,7 @@ class CheckboxListTile extends StatelessWidget {
this.onFocusChange, this.onFocusChange,
this.enableFeedback, this.enableFeedback,
this.checkboxSemanticLabel, this.checkboxSemanticLabel,
this.internalAddSemanticForOnTap = false,
}) : _checkboxType = _CheckboxType.adaptive, }) : _checkboxType = _CheckboxType.adaptive,
assert(tristate || value != null), assert(tristate || value != null),
assert(!isThreeLine || subtitle != null); assert(!isThreeLine || subtitle != null);
@ -464,6 +466,13 @@ class CheckboxListTile extends StatelessWidget {
/// inoperative. /// inoperative.
final bool? enabled; final bool? enabled;
/// Whether to add button:true to the semantics if onTap is provided.
/// This is a temporary flag to help changing the behavior of ListTile onTap semantics.
///
// TODO(hangyujin): Remove this flag after fixing related g3 tests and flipping
// the default value to true.
final bool internalAddSemanticForOnTap;
/// {@macro flutter.material.checkbox.semanticLabel} /// {@macro flutter.material.checkbox.semanticLabel}
final String? checkboxSemanticLabel; final String? checkboxSemanticLabel;
@ -567,6 +576,7 @@ class CheckboxListTile extends StatelessWidget {
focusNode: focusNode, focusNode: focusNode,
onFocusChange: onFocusChange, onFocusChange: onFocusChange,
enableFeedback: enableFeedback, enableFeedback: enableFeedback,
internalAddSemanticForOnTap: internalAddSemanticForOnTap,
), ),
); );
} }

View File

@ -263,6 +263,7 @@ class ExpansionTile extends StatefulWidget {
this.enableFeedback = true, this.enableFeedback = true,
this.enabled = true, this.enabled = true,
this.expansionAnimationStyle, this.expansionAnimationStyle,
this.internalAddSemanticForOnTap = false,
}) : assert( }) : assert(
expandedCrossAxisAlignment != CrossAxisAlignment.baseline, expandedCrossAxisAlignment != CrossAxisAlignment.baseline,
'CrossAxisAlignment.baseline is not supported since the expanded children ' 'CrossAxisAlignment.baseline is not supported since the expanded children '
@ -562,6 +563,13 @@ class ExpansionTile extends StatefulWidget {
/// {@end-tool} /// {@end-tool}
final AnimationStyle? expansionAnimationStyle; final AnimationStyle? expansionAnimationStyle;
/// Whether to add button:true to the semantics if onTap is provided.
/// This is a temporary flag to help changing the behavior of ListTile onTap semantics.
///
// TODO(hangyujin): Remove this flag after fixing related g3 tests and flipping
// the default value to true.
final bool internalAddSemanticForOnTap;
@override @override
State<ExpansionTile> createState() => _ExpansionTileState(); State<ExpansionTile> createState() => _ExpansionTileState();
} }
@ -753,6 +761,7 @@ class _ExpansionTileState extends State<ExpansionTile> with SingleTickerProvider
subtitle: widget.subtitle, subtitle: widget.subtitle,
trailing: widget.showTrailingIcon ? widget.trailing ?? _buildTrailingIcon(context) : null, trailing: widget.showTrailingIcon ? widget.trailing ?? _buildTrailingIcon(context) : null,
minTileHeight: widget.minTileHeight, minTileHeight: widget.minTileHeight,
internalAddSemanticForOnTap: widget.internalAddSemanticForOnTap,
), ),
), ),
), ),

View File

@ -410,6 +410,7 @@ class ListTile extends StatelessWidget {
this.minLeadingWidth, this.minLeadingWidth,
this.minTileHeight, this.minTileHeight,
this.titleAlignment, this.titleAlignment,
this.internalAddSemanticForOnTap = false,
}) : assert(!isThreeLine || subtitle != null); }) : assert(!isThreeLine || subtitle != null);
/// A widget to display before the title. /// A widget to display before the title.
@ -738,6 +739,13 @@ class ListTile extends StatelessWidget {
/// [ListTileThemeData]. /// [ListTileThemeData].
final ListTileTitleAlignment? titleAlignment; final ListTileTitleAlignment? titleAlignment;
/// Whether to add button:true to the semantics if onTap is provided.
/// This is a temporary flag to help changing the behavior of ListTile onTap semantics.
///
// TODO(hangyujin): Remove this flag after fixing related g3 tests and flipping
// the default value to true.
final bool internalAddSemanticForOnTap;
/// Add a one pixel border in between each tile. If color isn't specified the /// Add a one pixel border in between each tile. If color isn't specified the
/// [ThemeData.dividerColor] of the context's [Theme] is used. /// [ThemeData.dividerColor] of the context's [Theme] is used.
/// ///
@ -910,6 +918,7 @@ class ListTile extends StatelessWidget {
autofocus: autofocus, autofocus: autofocus,
enableFeedback: enableFeedback ?? tileTheme.enableFeedback ?? true, enableFeedback: enableFeedback ?? tileTheme.enableFeedback ?? true,
child: Semantics( child: Semantics(
button: internalAddSemanticForOnTap && (onTap != null || onLongPress != null),
selected: selected, selected: selected,
enabled: enabled, enabled: enabled,
child: Ink( child: Ink(

View File

@ -200,6 +200,7 @@ class RadioListTile<T> extends StatelessWidget {
this.focusNode, this.focusNode,
this.onFocusChange, this.onFocusChange,
this.enableFeedback, this.enableFeedback,
this.internalAddSemanticForOnTap = false,
}) : _radioType = _RadioType.material, }) : _radioType = _RadioType.material,
useCupertinoCheckmarkStyle = false, useCupertinoCheckmarkStyle = false,
assert(!isThreeLine || subtitle != null); assert(!isThreeLine || subtitle != null);
@ -240,6 +241,7 @@ class RadioListTile<T> extends StatelessWidget {
this.onFocusChange, this.onFocusChange,
this.enableFeedback, this.enableFeedback,
this.useCupertinoCheckmarkStyle = false, this.useCupertinoCheckmarkStyle = false,
this.internalAddSemanticForOnTap = false,
}) : _radioType = _RadioType.adaptive, }) : _radioType = _RadioType.adaptive,
assert(!isThreeLine || subtitle != null); assert(!isThreeLine || subtitle != null);
@ -449,6 +451,13 @@ class RadioListTile<T> extends StatelessWidget {
final _RadioType _radioType; final _RadioType _radioType;
/// Whether to add button:true to the semantics if onTap is provided.
/// This is a temporary flag to help changing the behavior of ListTile onTap semantics.
///
// TODO(hangyujin): Remove this flag after fixing related g3 tests and flipping
// the default value to true.
final bool internalAddSemanticForOnTap;
/// Whether to use the checkbox style for the [CupertinoRadio] control. /// Whether to use the checkbox style for the [CupertinoRadio] control.
/// ///
/// Only usable under the [RadioListTile.adaptive] constructor. If set to /// Only usable under the [RadioListTile.adaptive] constructor. If set to
@ -546,6 +555,7 @@ class RadioListTile<T> extends StatelessWidget {
focusNode: focusNode, focusNode: focusNode,
onFocusChange: onFocusChange, onFocusChange: onFocusChange,
enableFeedback: enableFeedback, enableFeedback: enableFeedback,
internalAddSemanticForOnTap: internalAddSemanticForOnTap,
), ),
); );
} }

View File

@ -209,6 +209,7 @@ class SwitchListTile extends StatelessWidget {
this.visualDensity, this.visualDensity,
this.enableFeedback, this.enableFeedback,
this.hoverColor, this.hoverColor,
this.internalAddSemanticForOnTap = false,
}) : _switchListTileType = _SwitchListTileType.material, }) : _switchListTileType = _SwitchListTileType.material,
applyCupertinoTheme = false, applyCupertinoTheme = false,
assert(activeThumbImage != null || onActiveThumbImageError == null), assert(activeThumbImage != null || onActiveThumbImageError == null),
@ -266,6 +267,7 @@ class SwitchListTile extends StatelessWidget {
this.visualDensity, this.visualDensity,
this.enableFeedback, this.enableFeedback,
this.hoverColor, this.hoverColor,
this.internalAddSemanticForOnTap = false,
}) : _switchListTileType = _SwitchListTileType.adaptive, }) : _switchListTileType = _SwitchListTileType.adaptive,
assert(!isThreeLine || subtitle != null), assert(!isThreeLine || subtitle != null),
assert(activeThumbImage != null || onActiveThumbImageError == null), assert(activeThumbImage != null || onActiveThumbImageError == null),
@ -517,6 +519,12 @@ class SwitchListTile extends StatelessWidget {
/// {@macro flutter.cupertino.CupertinoSwitch.applyTheme} /// {@macro flutter.cupertino.CupertinoSwitch.applyTheme}
final bool? applyCupertinoTheme; final bool? applyCupertinoTheme;
/// Whether to add button:true to the semantics if onTap is provided.
/// This is a temporary flag to help changing the behavior of ListTile onTap semantics.
///
// TODO(hangyujin): Remove this flag after fixing related g3 tests and flipping
// the default value to true.
final bool internalAddSemanticForOnTap;
@override @override
Widget build(BuildContext context) { Widget build(BuildContext context) {
final Widget control; final Widget control;
@ -616,6 +624,7 @@ class SwitchListTile extends StatelessWidget {
onFocusChange: onFocusChange, onFocusChange: onFocusChange,
enableFeedback: enableFeedback, enableFeedback: enableFeedback,
hoverColor: hoverColor, hoverColor: hoverColor,
internalAddSemanticForOnTap: internalAddSemanticForOnTap,
), ),
); );
} }

View File

@ -1180,10 +1180,12 @@ void main() {
onChanged: (bool? value) { log.add(value); }, onChanged: (bool? value) { log.add(value); },
title: const Text('Hello'), title: const Text('Hello'),
checkboxSemanticLabel: 'there', checkboxSemanticLabel: 'there',
internalAddSemanticForOnTap: true,
), ),
)); ));
expect(tester.getSemantics(find.byType(CheckboxListTile)), matchesSemantics( expect(tester.getSemantics(find.byType(CheckboxListTile)), matchesSemantics(
isButton: true,
hasCheckedState: true, hasCheckedState: true,
isChecked: true, isChecked: true,
hasEnabledState: true, hasEnabledState: true,

View File

@ -707,10 +707,12 @@ void main() {
children: <Widget>[ children: <Widget>[
ExpansionTile( ExpansionTile(
title: Text('First Expansion Tile'), title: Text('First Expansion Tile'),
internalAddSemanticForOnTap: true,
), ),
ExpansionTile( ExpansionTile(
initiallyExpanded: true, initiallyExpanded: true,
title: Text('Second Expansion Tile'), title: Text('Second Expansion Tile'),
internalAddSemanticForOnTap: true,
), ),
], ],
), ),
@ -727,6 +729,7 @@ void main() {
expect( expect(
tester.getSemantics(find.byType(ListTile).first), tester.getSemantics(find.byType(ListTile).first),
matchesSemantics( matchesSemantics(
isButton: true,
hasTapAction: true, hasTapAction: true,
hasFocusAction: true, hasFocusAction: true,
hasEnabledState: true, hasEnabledState: true,
@ -742,6 +745,7 @@ void main() {
expect( expect(
tester.getSemantics(find.byType(ListTile).last), tester.getSemantics(find.byType(ListTile).last),
matchesSemantics( matchesSemantics(
isButton: true,
hasTapAction: true, hasTapAction: true,
hasFocusAction: true, hasFocusAction: true,
hasEnabledState: true, hasEnabledState: true,

View File

@ -270,18 +270,22 @@ void main() {
children: <Widget>[ children: <Widget>[
const ListTile( const ListTile(
title: Text('one'), title: Text('one'),
internalAddSemanticForOnTap: true,
), ),
ListTile( ListTile(
title: const Text('two'), title: const Text('two'),
onTap: () {}, onTap: () {},
internalAddSemanticForOnTap: true,
), ),
const ListTile( const ListTile(
title: Text('three'), title: Text('three'),
selected: true, selected: true,
internalAddSemanticForOnTap: true,
), ),
const ListTile( const ListTile(
title: Text('four'), title: Text('four'),
enabled: false, enabled: false,
internalAddSemanticForOnTap: true,
), ),
], ],
), ),
@ -304,6 +308,7 @@ void main() {
), ),
TestSemantics.rootChild( TestSemantics.rootChild(
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
SemanticsFlag.isEnabled, SemanticsFlag.isEnabled,
SemanticsFlag.isFocusable, SemanticsFlag.isFocusable,

View File

@ -393,6 +393,7 @@ void main() {
groupValue: 2, groupValue: 2,
onChanged: (int? i) {}, onChanged: (int? i) {},
title: const Text('Title'), title: const Text('Title'),
internalAddSemanticForOnTap: true,
), ),
), ),
); );
@ -405,6 +406,7 @@ void main() {
TestSemantics( TestSemantics(
id: 1, id: 1,
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasCheckedState, SemanticsFlag.hasCheckedState,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
SemanticsFlag.isEnabled, SemanticsFlag.isEnabled,
@ -429,6 +431,7 @@ void main() {
groupValue: 2, groupValue: 2,
onChanged: (int? i) {}, onChanged: (int? i) {},
title: const Text('Title'), title: const Text('Title'),
internalAddSemanticForOnTap: true,
), ),
), ),
); );
@ -441,6 +444,7 @@ void main() {
TestSemantics( TestSemantics(
id: 1, id: 1,
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasCheckedState, SemanticsFlag.hasCheckedState,
SemanticsFlag.isChecked, SemanticsFlag.isChecked,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
@ -466,6 +470,7 @@ void main() {
groupValue: 2, groupValue: 2,
onChanged: null, onChanged: null,
title: Text('Title'), title: Text('Title'),
internalAddSemanticForOnTap: true,
), ),
), ),
); );
@ -502,6 +507,7 @@ void main() {
groupValue: 2, groupValue: 2,
onChanged: null, onChanged: null,
title: Text('Title'), title: Text('Title'),
internalAddSemanticForOnTap: true,
), ),
), ),
); );

View File

@ -707,6 +707,7 @@ void main() {
title: const Text('Switch tile'), title: const Text('Switch tile'),
value: true, value: true,
onChanged: (bool? newValue) { }, onChanged: (bool? newValue) { },
internalAddSemanticForOnTap: true,
), ),
), ),
), ),
@ -744,6 +745,7 @@ void main() {
return false; return false;
}); });
expect(child, matchesSemantics( expect(child, matchesSemantics(
isButton: true,
hasToggledState: true, hasToggledState: true,
isToggled: true, isToggled: true,
isEnabled: true, isEnabled: true,

View File

@ -47,12 +47,14 @@ void main() {
onChanged: (bool value) { }, onChanged: (bool value) { },
title: const Text('AAA'), title: const Text('AAA'),
secondary: const Text('aaa'), secondary: const Text('aaa'),
internalAddSemanticForOnTap: true,
), ),
CheckboxListTile( CheckboxListTile(
value: true, value: true,
onChanged: (bool? value) { }, onChanged: (bool? value) { },
title: const Text('BBB'), title: const Text('BBB'),
secondary: const Text('bbb'), secondary: const Text('bbb'),
internalAddSemanticForOnTap: true,
), ),
RadioListTile<bool>( RadioListTile<bool>(
value: true, value: true,
@ -60,6 +62,7 @@ void main() {
onChanged: (bool? value) { }, onChanged: (bool? value) { },
title: const Text('CCC'), title: const Text('CCC'),
secondary: const Text('ccc'), secondary: const Text('ccc'),
internalAddSemanticForOnTap: true,
), ),
], ],
), ),
@ -72,6 +75,7 @@ void main() {
id: 1, id: 1,
rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0), rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0),
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
SemanticsFlag.hasToggledState, SemanticsFlag.hasToggledState,
SemanticsFlag.isEnabled, SemanticsFlag.isEnabled,
@ -86,6 +90,7 @@ void main() {
rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0), rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0),
transform: Matrix4.translationValues(0.0, 56.0, 0.0), transform: Matrix4.translationValues(0.0, 56.0, 0.0),
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasCheckedState, SemanticsFlag.hasCheckedState,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
SemanticsFlag.isChecked, SemanticsFlag.isChecked,
@ -100,6 +105,7 @@ void main() {
rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0), rect: const Rect.fromLTWH(0.0, 0.0, 800.0, 56.0),
transform: Matrix4.translationValues(0.0, 112.0, 0.0), transform: Matrix4.translationValues(0.0, 112.0, 0.0),
flags: <SemanticsFlag>[ flags: <SemanticsFlag>[
SemanticsFlag.isButton,
SemanticsFlag.hasCheckedState, SemanticsFlag.hasCheckedState,
SemanticsFlag.hasEnabledState, SemanticsFlag.hasEnabledState,
SemanticsFlag.isEnabled, SemanticsFlag.isEnabled,