Skip to content

Conversation

@PurplePolyhedron
Copy link
Contributor

fixes #154532

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Sep 16, 2024
@Piinks Piinks changed the title fix DropdownMenu keyboard navigation when entires are filtered empty fix DropdownMenu keyboard navigation when entries are filtered empty Sep 25, 2024
@Piinks Piinks requested a review from QuncCccccc September 25, 2024 18:10
Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

Many thanks for this fix 🙏 and sorry for the delay.
Some minor comments.

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

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

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.

Copy link
Contributor

@bleroux bleroux left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking care of this fix

@bleroux bleroux requested a review from nate-thegrate October 1, 2024 18:46
Copy link
Contributor

@nate-thegrate nate-thegrate left a comment

Choose a reason for hiding this comment

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

LGTM! Great to see a 1-line fix 🙂👍

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 2, 2024
auto-submit bot pushed a commit that referenced this pull request Oct 2, 2024
I accidentally submitted the wrong test in the last commit in #155252
Re-uploading the correct one.
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 3, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 4, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 5, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 6, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 8, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 8, 2024
Manual roll Flutter from 0975e61 to ec2e12b (54 revisions)

Manual roll requested by [email protected]

flutter/flutter@0975e61...ec2e12b

2024-10-03 [email protected] Roll Flutter Engine from bd44b58e3204 to 247bc68c578e (7 revisions) (flutter/flutter#156144)
2024-10-03 [email protected] Roll Flutter Engine from 70232fa124d0 to bd44b58e3204 (1 revision) (flutter/flutter#156124)
2024-10-03 [email protected] Roll Flutter Engine from 33ac1b30ab0a to 70232fa124d0 (2 revisions) (flutter/flutter#156122)
2024-10-02 [email protected] Update `ThemeData.dialogTheme` type to accept `DialogThemeData` (flutter/flutter#155129)
2024-10-02 [email protected] Roll Flutter Engine from 751ab9b3c5eb to 33ac1b30ab0a (4 revisions) (flutter/flutter#156118)
2024-10-02 [email protected] Add back main() methods to benchmark benches. (flutter/flutter#156083)
2024-10-02 [email protected] Roll pub packages (flutter/flutter#156117)
2024-10-02 [email protected] Add `mouseCursor` property to `CupertinoCheckbox` (flutter/flutter#151788)
2024-10-02 [email protected] Roll Flutter Engine from 3bdc1c0a30b6 to 751ab9b3c5eb (3 revisions) (flutter/flutter#156115)
2024-10-02 [email protected] Roll pub packages (flutter/flutter#156114)
2024-10-02 [email protected] [ Cocoon ] Wait for task results to be received by the task runner before shutting down the task process (flutter/flutter#156002)
2024-10-02 [email protected] Allow mixing route transitions in one app. (flutter/flutter#150031)
2024-10-02 [email protected] Roll Flutter Engine from f20681241753 to 3bdc1c0a30b6 (5 revisions) (flutter/flutter#156107)
2024-10-02 [email protected] Update Upgrading-Engine's-Android-API-version.md to reflect code move (flutter/flutter#156108)
2024-10-02 [email protected] Roll Packages from ebcc4f0 to 7c97c88 (5 revisions) (flutter/flutter#156106)
2024-10-02 [email protected] Roll pub packages (flutter/flutter#156105)
2024-10-02 [email protected] fix wrong test in "fixing `DropdownMenu` keyboard navigation"  (flutter/flutter#156084)
2024-10-02 [email protected] fix ReorderableList not passing in item extent builder (flutter/flutter#155994)
2024-10-02 98614782+auto-submit[bot]@users.noreply.github.com Reverts "integration_test: migrate to build.gradle.kts (#154125)" (flutter/flutter#156087)
2024-10-02 [email protected] Add deprecation warning for "flutter create --ios-language" (flutter/flutter#155867)
2024-10-02 [email protected] update flutter create generated projects to use package:flutter_lints 5.0.0 (flutter/flutter#156011)
2024-10-02 [email protected] Roll Flutter Engine from d48c35d16814 to f20681241753 (1 revision) (flutter/flutter#156080)
2024-10-02 [email protected] [Docs] `CupertinoListTile` API Example (flutter/flutter#154548)
2024-10-02 [email protected] integration_test: migrate to build.gradle.kts (flutter/flutter#154125)
2024-10-02 [email protected] Marks Windows_mokey native_assets_android to be flaky (flutter/flutter#156064)
2024-10-02 [email protected] Fix DropdownMenu does not rematch initialSelection when entries have changed (flutter/flutter#155757)
2024-10-02 [email protected] Roll Flutter Engine from 9b224bd2f895 to d48c35d16814 (1 revision) (flutter/flutter#156074)
2024-10-02 [email protected] Roll Flutter Engine from 8774940b9ddc to 9b224bd2f895 (1 revision) (flutter/flutter#156065)
2024-10-02 [email protected] Roll Flutter Engine from 21ad04948457 to 8774940b9ddc (1 revision) (flutter/flutter#156055)
2024-10-02 [email protected] Roll Flutter Engine from 767bdc38cf51 to 21ad04948457 (1 revision) (flutter/flutter#156049)
2024-10-02 [email protected] Roll Flutter Engine from 055969512dc5 to 767bdc38cf51 (1 revision) (flutter/flutter#156043)
2024-10-02 [email protected] Roll Flutter Engine from e0f049d69240 to 055969512dc5 (2 revisions) (flutter/flutter#156042)
2024-10-02 [email protected] fix `DropdownMenu` keyboard navigation when entries are filtered empty (flutter/flutter#155252)
2024-10-02 [email protected] Roll Flutter Engine from a5bc2e2708c7 to e0f049d69240 (1 revision) (flutter/flutter#156039)
2024-10-02 [email protected] mark {Linux,Windows} tool_integration_tests_* non-bringup (flutter/flutter#155773)
2024-10-02 [email protected] Roll Flutter Engine from a7abf7a8163e to a5bc2e2708c7 (2 revisions) (flutter/flutter#156038)
2024-10-02 [email protected] Roll Flutter Engine from df1982dd4482 to a7abf7a8163e (1 revision) (flutter/flutter#156032)
2024-10-02 [email protected] Add `enableSplash` parameter to `CarouselView` (flutter/flutter#155214)
2024-10-02 [email protected] Roll pub packages (flutter/flutter#156030)
2024-10-01 [email protected] Roll Flutter Engine from ab48d6d8c167 to df1982dd4482 (1 revision) (flutter/flutter#156029)
2024-10-01 [email protected] Marks Mac_arm64_ios hot_mode_dev_cycle_ios__benchmark to be unflaky (flutter/flutter#147289)
2024-10-01 [email protected] Add WidgetStateMouseCursor example and tests for it. (flutter/flutter#155552)
2024-10-01 49699333+dependabot[bot]@users.noreply.github.com Bump codecov/codecov-action from 4.5.0 to 4.6.0 (flutter/flutter#156024)
2024-10-01 [email protected] Roll Flutter Engine from e7b3ce717006 to ab48d6d8c167 (1 revision) (flutter/flutter#156023)
...
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DropdownMenu throws RangeError when no matching results are found in DropdownMenuEntries

3 participants