Skip to content

Reland: Request focus if accessibility focus is given to a Focus widget (#142942) #149840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 2 commits into from
Jun 12, 2024
Merged

Reland: Request focus if accessibility focus is given to a Focus widget (#142942) #149840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05d0f06
Make semantics matchers agnostic about SemanticsAction.focus
gspencergoog Jun 6, 2024
8397ff9
Add issue URL
gspencergoog Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions dev/integration_tests/flutter_gallery/test/demo/material/chip_demo_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ void main() {
isEnabled: true,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
label: 'Update border shape',
));

Expand All @@ -29,6 +30,7 @@ void main() {
isEnabled: true,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
label: 'Reset chips',
));

Expand Down
9 changes: 9 additions & 0 deletions packages/flutter/lib/src/widgets/focus_scope.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,15 @@ class _FocusState extends State<Focus> {
Widget child = widget.child;
if (widget.includeSemantics) {
child = Semantics(
// Automatically request the focus for a focusable widget when it
// receives an input focus action from the semantics. Nothing is needed
// for losing the focus because if focus is lost, that means another
// node will gain focus and take focus from this widget.
// TODO(gspencergoog): Allow this to be set on iOS once the issue is
// addressed: https://github.com/flutter/flutter/issues/150030
onFocus: defaultTargetPlatform != TargetPlatform.iOS && _couldRequestFocus
Copy link
Copy Markdown
Member

@jmagman jmagman Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this issue reproduce on Android? I'm not sure if it's iOS-only, or if there's only a test on iOS so that's where it was caught.

Copy link
Copy Markdown
Contributor Author

@gspencergoog gspencergoog Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, no, but as you say, there are no tests on Android that test this. The failure mode seems iOS specific (I doubt Android would implement their text field identically to iOS to the point where it has a similar effect on the bounding box of the native text field), but I suppose something different could happen on Android.

I could also exclude Android explicitly, however, if you think that it's likely enough to worry about.

Copy link
Copy Markdown
Member

@jmagman jmagman Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♀️ Not sure if this is anything to worry about. If it's anywhere else it may actually be on macOS, which this web/desktop feature would be targeting. I was trying to replicate this test on macOS, but I was hitting some wacky accessibility problems that tricked the XCUITests.

? focusNode.requestFocus
: null,
focusable: _couldRequestFocus,
focused: _hadPrimaryFocus,
child: widget.child,
Expand Down
10 changes: 7 additions & 3 deletions packages/flutter/test/cupertino/checkbox_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
));

Expand All @@ -54,6 +55,7 @@ void main() {
isChecked: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
));

Expand All @@ -73,6 +75,7 @@ void main() {
hasEnabledState: true,
// isFocusable is delayed by 1 frame.
isFocusable: true,
hasFocusAction: true,
));

await tester.pump();
Expand Down Expand Up @@ -178,6 +181,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
));
handle.dispose();
Expand Down Expand Up @@ -247,7 +251,7 @@ void main() {
SemanticsFlag.isFocusable,
SemanticsFlag.isCheckStateMixed,
],
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
), hasLength(1));

await tester.pumpWidget(
Expand All @@ -268,7 +272,7 @@ void main() {
SemanticsFlag.isChecked,
SemanticsFlag.isFocusable,
],
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
), hasLength(1));

await tester.pumpWidget(
Expand All @@ -288,7 +292,7 @@ void main() {
SemanticsFlag.isEnabled,
SemanticsFlag.isFocusable,
],
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
), hasLength(1));

semantics.dispose();
Expand Down
4 changes: 4 additions & 0 deletions packages/flutter/test/cupertino/radio_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ void main() {
],
actions: <SemanticsAction>[
SemanticsAction.tap,
SemanticsAction.focus,
],
),
);
Expand All @@ -172,6 +173,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
isInMutuallyExclusiveGroup: true,
));
Expand All @@ -191,6 +193,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
isInMutuallyExclusiveGroup: true,
isChecked: true,
Expand All @@ -211,6 +214,7 @@ void main() {
hasEnabledState: true,
isFocusable: true,
isInMutuallyExclusiveGroup: true,
hasFocusAction: true,
));

await tester.pump();
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/test/cupertino/route_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1945,7 +1945,7 @@ void main() {
await tester.pumpAndSettle();

expect(semantics, isNot(includesNodeWith(
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
label: 'Dismiss',
)));
debugDefaultTargetPlatformOverride = null;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/test/cupertino/text_field_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2621,7 +2621,7 @@ void main() {
),
);

expect(semantics, isNot(includesNodeWith(actions: <SemanticsAction>[SemanticsAction.tap])));
expect(semantics, isNot(includesNodeWith(actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus])));

semantics.dispose();
});
Expand Down
2 changes: 2 additions & 0 deletions packages/flutter/test/material/back_button_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
));
handle.dispose();
Expand Down Expand Up @@ -258,6 +259,7 @@ void main() {
hasEnabledState: true,
isEnabled: true,
hasTapAction: true,
hasFocusAction: true,
isFocusable: true,
));
handle.dispose();
Expand Down
14 changes: 12 additions & 2 deletions packages/flutter/test/material/bottom_navigation_bar_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2111,6 +2111,7 @@ void main() {
isFocusable: true,
isSelected: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2120,6 +2121,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2129,6 +2131,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
});
Expand Down Expand Up @@ -2165,6 +2168,7 @@ void main() {
isFocusable: true,
isSelected: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2174,6 +2178,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2183,6 +2188,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
});
Expand Down Expand Up @@ -2515,6 +2521,7 @@ void main() {
isFocusable: true,
isSelected: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2524,6 +2531,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
});
Expand Down Expand Up @@ -2558,6 +2566,7 @@ void main() {
isFocusable: true,
isSelected: true,
hasTapAction: true,
hasFocusAction: true,
),
);
expect(
Expand All @@ -2567,6 +2576,7 @@ void main() {
textDirection: TextDirection.ltr,
isFocusable: true,
hasTapAction: true,
hasFocusAction: true,
),
);
});
Expand Down Expand Up @@ -2747,13 +2757,13 @@ void main() {
SemanticsFlag.isSelected,
SemanticsFlag.isFocusable,
],
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
label: 'A\nTab 1 of 2',
textDirection: TextDirection.ltr,
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
actions: <SemanticsAction>[SemanticsAction.tap, SemanticsAction.focus],
label: 'B\nTab 2 of 2',
textDirection: TextDirection.ltr,
),
Expand Down
Loading