Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Nov 4, 2025

Description

This PR fixes escape key not closing the menu when DropdownMenu.requestFocusOnTap is false.

Related Issue

Fixes DropdownMenu menu panel does not close when pressing ESC and requestFocusOnTap is false
Part of #123797

Tests

  • Adds 3 tests.
  • Updates 2 non-related tests where I spotted some nits.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Nov 4, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request provides a solid fix for an issue where the DropdownMenu could not be closed with the escape key when requestFocusOnTap is false. The implementation correctly uses Actions and Shortcuts to handle the DismissIntent. The accompanying tests are thorough, covering both the fix and potential regressions. The test refactoring, including the adoption of tester.sendKeyEvent and improved naming, enhances the overall quality of the test suite. The changes are well-executed and ready to merge.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly fixes an issue where the DropdownMenu could not be closed with the Escape key when requestFocusOnTap is false. The fix is implemented by properly adding a DismissIntent and a corresponding shortcut for the Escape key. The change is well-supported by new regression tests that cover both requestFocusOnTap states, as well as a more complex interaction scenario. Additionally, many existing tests were cleaned up for consistency and to use modern testing APIs, which improves the overall quality of the test suite. The changes are solid and address the problem effectively.

@bleroux bleroux force-pushed the fix_DropdownMenu_escape_key_does_not_close_the_menu branch from 62386c0 to f030950 Compare November 4, 2025 19:05
@bleroux bleroux requested a review from justinmc November 4, 2025 21:44
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

await tester.pump();
expect(findMenuPanel(), findsNothing);
},
variant: TargetPlatformVariant.all(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think you shouldn't need TargetPlatformVariant.all for any of these tests, unless there's specifically platform-dependent stuff happening here. This will end up getting tested on all platforms by CI anyway.

Copy link
Contributor Author

@bleroux bleroux Nov 5, 2025

Choose a reason for hiding this comment

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

I used TargetPlatformVariant.all() because those tests depends on DropdownMenu.requestFocusOnTap value which is platform dependent.

But you're right that this is not needed for the two first tests I added because in these tests DropdownMenu.requestFocusOnTap is explicitly set. I pushed a commit to remove TargetPlatformVariant.all() usage for these tests.

@bleroux bleroux force-pushed the fix_DropdownMenu_escape_key_does_not_close_the_menu branch from f030950 to 47f39c7 Compare November 5, 2025 09:33
@bleroux bleroux force-pushed the fix_DropdownMenu_escape_key_does_not_close_the_menu branch from 47f39c7 to a5a524e Compare November 5, 2025 21:41
@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 5, 2025
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 6, 2025
@auto-submit
Copy link
Contributor

auto-submit bot commented Nov 6, 2025

autosubmit label was removed for flutter/flutter/178002, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@bleroux bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 6, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Nov 6, 2025
Merged via the queue into flutter:master with commit c093967 Nov 6, 2025
78 checks passed
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 6, 2025
@bleroux bleroux deleted the fix_DropdownMenu_escape_key_does_not_close_the_menu branch November 6, 2025 11:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 6, 2025
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 6, 2025
flutter/flutter@e5d5c01...c5e809a

2025-11-06 [email protected] Remove WindowingOwner.hasTopLevelWindows (flutter/flutter#178033)
2025-11-06 [email protected] Fix DropdownMenu escape key does not close the menu (flutter/flutter#178002)
2025-11-06 [email protected] Update documentation tool reference in image.dart (flutter/flutter#177782)
2025-11-06 [email protected] Roll Skia from 4eb2383d38f2 to 5c4e1352128f (5 revisions) (flutter/flutter#178094)
2025-11-06 [email protected] Revert "Refactor OverlayPortal semantics (#173005)" (flutter/flutter#178007)
2025-11-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix verified input test failure in CI (attempt 4) (#178018)" (flutter/flutter#178089)
2025-11-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[web] Unify Surface code between Skwasm and CanvasKit (#177138)" (flutter/flutter#178085)
2025-11-05 [email protected] Fix verified input test failure in CI (attempt 4) (flutter/flutter#178018)
2025-11-05 [email protected] fix: inconsistent horizontal spacing between hours and mins in time picker for non-english language (flutter/flutter#173706)
2025-11-05 [email protected] Roll Fuchsia Linux SDK from mpsxF1gd-jbKNvmpm... to cm88aTLui5yorSGYQ... (flutter/flutter#178074)
2025-11-05 [email protected] Fix(ios): Remove arm64 exclusion to support Xcode 26 simulators (flutter/flutter#177065)
2025-11-05 [email protected] Roll Skia from 2ff897e9b440 to 4eb2383d38f2 (18 revisions) (flutter/flutter#178070)
2025-11-05 [email protected] [web] Unify Surface code between Skwasm and CanvasKit (flutter/flutter#177138)
2025-11-05 [email protected] Update more missing ninja deps (flutter/flutter#178079)
2025-11-05 [email protected] Add ninja / cmake deps to failing tests (flutter/flutter#178054)
2025-11-05 [email protected] [web] Don't add webparagraph suite to CI (flutter/flutter#177681)
2025-11-05 [email protected] Fixing broken link in engine readme (flutter/flutter#177987)
2025-11-05 [email protected] Print reason for adb command failure in verified input test (attempt 3) (flutter/flutter#178005)
2025-11-05 [email protected] Fix `ReorderableList` items jumping when drag direction reverses mid-animation (flutter/flutter#173241)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `overflow_bar.0.dart‎` example (flutter/flutter#177813)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `data_table.1.dart‎` example (flutter/flutter#177812)
2025-11-05 [email protected] Replace deprecated `withOpacity` in `switch.1.dart` example (flutter/flutter#177811)
2025-11-04 [email protected] Roll Skia from c89b6118266b to 2ff897e9b440 (6 revisions) (flutter/flutter#177999)
2025-11-04 [email protected] Fix verified input test in CI (attempt 2) (flutter/flutter#177961)
2025-11-04 [email protected] Replace rendering for solid color circles (both filled and stroked) to use SDFs (flutter/flutter#177482)
2025-11-04 [email protected] Roll Fuchsia Linux SDK from vxK5obzfr1X9P2kSh... to mpsxF1gd-jbKNvmpm... (flutter/flutter#177996)
2025-11-04 [email protected] Validate that platforms specified in .ci.yaml target names match the platforms defined in the platform_properties section (flutter/flutter#177523)
2025-11-04 [email protected] Roll Packages from 1a7075b to 3d926aa (6 revisions) (flutter/flutter#177998)
2025-11-04 [email protected] Remove dead code from snippet_generator (flutter/flutter#174440)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
IvoneDjaja pushed a commit to IvoneDjaja/flutter that referenced this pull request Nov 22, 2025
## Description

This PR fixes escape key not closing the menu when
`DropdownMenu.requestFocusOnTap` is false.

## Related Issue

Fixes [DropdownMenu menu panel does not close when pressing ESC and
requestFocusOnTap is
false](flutter#177993)
Part of flutter#123797
 
## Tests

- Adds 3 tests.
- Updates 2 non-related tests where I spotted some nits.
reidbaker pushed a commit to AbdeMohlbi/flutter that referenced this pull request Dec 10, 2025
## Description

This PR fixes escape key not closing the menu when
`DropdownMenu.requestFocusOnTap` is false.

## Related Issue

Fixes [DropdownMenu menu panel does not close when pressing ESC and
requestFocusOnTap is
false](flutter#177993)
Part of flutter#123797
 
## Tests

- Adds 3 tests.
- Updates 2 non-related tests where I spotted some nits.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 menu panel does not close when pressing ESC and requestFocusOnTap is false

2 participants