Skip to content
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

Jump to bottom

fix DropdownMenu keyboard navigation when entries are filtered empty #155252

Merged
merged 13 commits into from
Oct 2, 2024
Merged

fix DropdownMenu keyboard navigation when entries are filtered empty #155252

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b5a4018
update page_transition_theme
PurplePolyhedron Sep 12, 2024
ece136c
Update dropdown_menu.dart
PurplePolyhedron Sep 16, 2024
3ac3d59
Update dropdown_menu_test
PurplePolyhedron Sep 16, 2024
db95943
merge master
PurplePolyhedron Sep 16, 2024
408e76a
Merge branch 'flutter:master' into dropdown_menu_filtered_entries
PurplePolyhedron Oct 1, 2024
05b4839
Update dropdown_menu.dart
PurplePolyhedron Oct 1, 2024
66f82e6
Update dropdown_menu_test.dart
PurplePolyhedron Oct 1, 2024
26834cf
fix dropdown_menu_test.dart typo
PurplePolyhedron Oct 1, 2024
af74eeb
Update dropdown_menu_test.dart
PurplePolyhedron Oct 1, 2024
ac0bb91
Update dropdown_menu_test.dart
PurplePolyhedron Oct 1, 2024
71aaeb6
Merge branch 'flutter:master' into dropdown_menu_filtered_entries
PurplePolyhedron Oct 2, 2024
fd2f2d7
Update dropdown_menu_test.dart
PurplePolyhedron Oct 2, 2024
298912a
Update dropdown_menu_test.dart
PurplePolyhedron Oct 2, 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
1 change: 1 addition & 0 deletions packages/flutter/lib/src/material/dropdown_menu.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,7 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
} else {
filteredEntries = widget.dropdownMenuEntries;
}
_menuHasEnabledItem = filteredEntries.any((DropdownMenuEntry<T> entry) => entry.enabled);

if (_enableSearch) {
if (widget.searchCallback != null) {
Expand Down
22 changes: 22 additions & 0 deletions packages/flutter/test/material/dropdown_menu_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,28 @@ void main() {
expect(tester.takeException(), isNull);
});

// Regression test for https://github.com/flutter/flutter/issues/154532.
testWidgets('Keyboard navigation does not throw when no entries match the filter',
(WidgetTester tester) async {
await tester.pumpWidget(MaterialApp(
home: Scaffold(
body: DropdownMenu<TestMenu>(
enableFilter: true,
dropdownMenuEntries: menuChildren,
),
),
));
await tester.tap(find.byType(DropdownMenu<TestMenu>));
await tester.pump();
await tester.enterText(find.byType(TextField).first, 'No match');
await tester.pump();
await tester.sendKeyEvent(LogicalKeyboardKey.arrowDown);
await tester.pump();
await tester.enterText(find.byType(TextField).first, 'No match 2');
await tester.pump();
expect(tester.takeException(), isNull);
}, variant: TargetPlatformVariant.desktop());
Copy link
Contributor

Choose a reason for hiding this comment

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

Keyboard navigation is also supported on mobile (Android for sure, iOS probably).
I tried changing TargetPlatformVariant.desktop() to TargetPlatformVariant.all() and the test still passed.

Copy link
Contributor Author

@PurplePolyhedron PurplePolyhedron Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The different is requestFocusOnTap default to false on mobile, The widget won't gain focus and won't receive key event. (I think this might be why other keyboard navigation tests used TargetPlatformVariant.desktop() )

I can see setting requestFocusOnTap to true being overall the better than setting TargetPlatformVariant

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. 🙏
For the moment, it makes sense to do the same as the other existing tests.
We can revisit this on a separate "test only" PR (feel free to ping me if you file one, I like PRs that aim at improving existing tests 🔥 ), setting requestFocusOnTap to true seems to be a nice option (a comment might be interesting to remind why).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops sorry, I was reading the wrong version. I see now that you applies this change, it is ok to me. Feel free to add a comment if you think it worths it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've changed it back to TargetPlatformVariant.desktop() for now.

Copy link
Contributor Author

@PurplePolyhedron PurplePolyhedron Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, it appears to be some delays and I didn't see the new message in time, I will try to file a PR about overall requestFocusOnTap in keyboard navigation in a few days.

nate-thegrate marked this conversation as resolved.
Show resolved Hide resolved

// Regression test for https://github.com/flutter/flutter/issues/147253.
testWidgets('Default search prioritises the current highlight on desktop platforms',
(WidgetTester tester) async {
Expand Down