Navigator.pop before PopupMenuItem onTap call #127446

mhbdev · 2023-05-23T23:41:17Z

The order of calling Navigator.pop and PopupMenuItem.onTap has been changed so before calling PopupMenuItem onTap method, PopupMenuBotton onSelect method is going to be called.

close #127443

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

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

packages/flutter/test/material/popup_menu_test.dart

packages/flutter/lib/src/material/popup_menu.dart

chunhtai

LGTM

chunhtai · 2023-06-06T15:54:42Z

cc @QuncCccccc as secondary reviewer

QuncCccccc

LGTM. Nice improvement:) Thanks!

QuncCccccc · 2023-06-08T19:21:23Z

The Google testing failed but seems it is not related to this PR change. @mhbdev Could you maybe rebase master to trigger the test again?

auto-submit · 2023-06-08T20:02:38Z

auto label is removed for flutter/flutter, pr: 127446, due to - The status or check suite Mac build_tests_2_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-06-08T20:07:23Z

auto label is removed for flutter/flutter, pr: 127446, due to - The status or check suite Mac build_tests_2_4 has failed. Please fix the issues identified (or deflake) before re-applying this label.

Added a descriptive comment

Changed the test for popup_menu to make sure the test is related to the issue

mhbdev · 2023-06-08T21:56:24Z

The Google testing failed but seems it is not related to this PR change. @mhbdev Could you maybe rebase master to trigger the test again?

Hello there! I tried to rebase master but still the same issue happening. I have no clue why this is getting stuck. All checks have been passed before but now ...

QuncCccccc · 2023-06-08T22:01:21Z

The Google testing failed but seems it is not related to this PR change. @mhbdev Could you maybe rebase master to trigger the test again?

Hello there! I tried to rebase master but still the same issue happening. I have no clue why this is getting stuck. All checks have been passed before but now ...

The luci-flutter test is just showing the tree status and will be fixed later. Just checked the Google testing, looks like all tests have passed, so it might just need some time to change the pending status:)

Thanks a lot for the help. The autosubmit label should be able to merge this change once the tree is green.

mhbdev · 2023-06-08T22:34:19Z

The Google testing failed but seems it is not related to this PR change. @mhbdev Could you maybe rebase master to trigger the test again?

Hello there! I tried to rebase master but still the same issue happening. I have no clue why this is getting stuck. All checks have been passed before but now ...

The luci-flutter test is just showing the tree status and will be fixed later. Just checked the Google testing, looks like all tests have passed, so it might just need some time to change the pending status:)

Thanks a lot for the help. The autosubmit label should be able to merge this change once the tree is green.

Thank you for the explanation ☺️

…r#127446)

flutter/flutter@6e254a3...da127f1 2023-06-09 hans.muller@gmail.com Updated material button theme tests for Material3 (flutter/flutter#128543) 2023-06-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from cb93477008d6 to 93afba901b3b (2 revisions) (flutter/flutter#128573) 2023-06-09 6655696+guidezpl@users.noreply.github.com Improve defaults generation with logging, stats, and token validation (flutter/flutter#128244) 2023-06-09 whesse@google.com [testing] Make the FLUTTER_STORAGE_BASE_URL warning non-fatal (flutter/flutter#128335) 2023-06-09 danny@tuppeny.com [flutter_tools] [DAP] Don't try to restart/reload if app hasn't started yet (flutter/flutter#128267) 2023-06-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8f9e608d39ab to cb93477008d6 (3 revisions) (flutter/flutter#128568) 2023-06-09 tessertaha@gmail.com Replace `MaterialButton` from test classes (flutter/flutter#128466) 2023-06-09 tessertaha@gmail.com Fix `showBottomSheet` doesn't remove scrim when draggable sheet is dismissed (flutter/flutter#128455) 2023-06-09 engine-flutter-autoroll@skia.org Manual roll Flutter Engine from a5f7d5d75ff2 to 8f9e608d39ab (31 revisions) (flutter/flutter#128554) 2023-06-09 ychris@google.com Revert "test owners: cyanglaz -> vashworth" (flutter/flutter#128462) 2023-06-09 43054281+camsim99@users.noreply.github.com [Android] Bump integration tests using `compileSdkVersion` 31 to 33 (flutter/flutter#128072) 2023-06-09 dkwingsmt@users.noreply.github.com Remove single view assumption from MouseTracker, and unify its hit testing code flow (flutter/flutter#127060) 2023-06-09 christopherfujino@gmail.com [flutter_tools] Precache after channel switch (flutter/flutter#118129) 2023-06-08 leigha.jarett@gmail.com Adding migration guide for Material 3 colors (flutter/flutter#128429) 2023-06-08 gspencergoog@users.noreply.github.com Add `AppLifecycleListener`, with support for application exit handling (flutter/flutter#123274) 2023-06-08 thkim1011@users.noreply.github.com Sliver Main Axis Group (flutter/flutter#126596) 2023-06-08 31859944+LongCatIsLooong@users.noreply.github.com Reduce `_DoubleClampVisitor` false positives (flutter/flutter#128539) 2023-06-08 leigha.jarett@gmail.com Advise developers to use OverflowBar instead of ButtonBar (flutter/flutter#128437) 2023-06-08 jacksongardner@google.com Reland "Migrate benchmarks to package:web" (flutter/flutter#128266) 2023-06-08 53684884+mhbdev@users.noreply.github.com Navigator.pop before PopupMenuItem onTap call (flutter/flutter#127446) 2023-06-08 leroux_bruno@yahoo.fr Fix navigation rail with long labels misplaced highlights (flutter/flutter#128324) 2023-06-08 tessertaha@gmail.com Update `chip.dart` to use set of `MaterialState` (flutter/flutter#128507) 2023-06-08 jcollins@google.com Update flutter to dartdoc 6.3.0 and hide Icons implementation from doc pages (flutter/flutter#128442) 2023-06-08 31859944+LongCatIsLooong@users.noreply.github.com Disable blinking cursor when `EditableText.showCursor` is false (flutter/flutter#127562) 2023-06-08 41930132+hellohuanlin@users.noreply.github.com [floating_cursor_selection]add more comments on the tricky part (flutter/flutter#127227) 2023-06-08 goderbauer@google.com Move RenderObjectElement.updateChildren to Element (flutter/flutter#128458) 2023-06-08 goderbauer@google.com Fix PointerEventConverter doc (flutter/flutter#128452) 2023-06-08 engine-flutter-autoroll@skia.org Roll Packages from a84b2c2 to e13b8c4 (9 revisions) (flutter/flutter#128508) 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 rmistry@google.com,stuartmorgan@google.com 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#127446)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 23, 2023

github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 23, 2023

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jun 1, 2023

HansMuller requested a review from chunhtai June 2, 2023 21:38

chunhtai reviewed Jun 5, 2023

View reviewed changes

packages/flutter/test/material/popup_menu_test.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/material/popup_menu.dart Show resolved Hide resolved

mhbdev requested a review from chunhtai June 6, 2023 11:54

chunhtai approved these changes Jun 6, 2023

View reviewed changes

chunhtai requested a review from QuncCccccc June 6, 2023 15:54

QuncCccccc approved these changes Jun 8, 2023

View reviewed changes

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

mhbdev force-pushed the fix-popupmenubutton-handletap branch from 3bb3e7e to 6e97a42 Compare June 8, 2023 20:14

mhbdev added 4 commits June 9, 2023 00:32

Navigator.pop before PopupMenuItem onTap call

e445f27

Update popup_menu.dart

2114778

Added a descriptive comment

Update popup_menu_test.dart

7249a28

Changed the test for popup_menu to make sure the test is related to the issue

Update popup_menu.dart

6d619b4

mhbdev force-pushed the fix-popupmenubutton-handletap branch from 6e97a42 to 6d619b4 Compare June 8, 2023 21:02

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 8, 2023

auto-submit bot merged commit 4e9869b into flutter:master Jun 8, 2023

engine-flutter-autoroll mentioned this pull request Jun 9, 2023

Roll Flutter from 6e254a3f9fb7 to da127f15ad54 (28 revisions) flutter/packages#4170

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 9, 2023

4e9869b Navigator.pop before PopupMenuItem onTap call (flutter/flutte…

c572f73

…r#127446)

HansMuller mentioned this pull request Jun 9, 2023

Flutter Framework Team: Notable Commits #121415

Closed

narumi147 mentioned this pull request Jul 4, 2023

[PopupMenu] allow onTap called after Navigator.pop #87766

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

4e9869b Navigator.pop before PopupMenuItem onTap call (flutter/flutte…

09e5d22

…r#127446)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4e9869b Navigator.pop before PopupMenuItem onTap call (flutter/flutte…

9234641

…r#127446)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4e9869b Navigator.pop before PopupMenuItem onTap call (flutter/flutte…

91270be

…r#127446)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

4e9869b Navigator.pop before PopupMenuItem onTap call (flutter/flutte…

3c84ec0

…r#127446)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Navigator.pop before PopupMenuItem onTap call #127446

Navigator.pop before PopupMenuItem onTap call #127446

Uh oh!

mhbdev commented May 23, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

chunhtai left a comment

Uh oh!

chunhtai commented Jun 6, 2023

Uh oh!

QuncCccccc left a comment

Uh oh!

QuncCccccc commented Jun 8, 2023 •

edited

Loading

Uh oh!

auto-submit bot commented Jun 8, 2023

Uh oh!

auto-submit bot commented Jun 8, 2023

Uh oh!

mhbdev commented Jun 8, 2023

Uh oh!

QuncCccccc commented Jun 8, 2023

Uh oh!

mhbdev commented Jun 8, 2023

Uh oh!

Uh oh!

Navigator.pop before PopupMenuItem onTap call #127446

Navigator.pop before PopupMenuItem onTap call #127446

Uh oh!

Conversation

mhbdev commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Jun 6, 2023

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

auto-submit bot commented Jun 8, 2023

Uh oh!

auto-submit bot commented Jun 8, 2023

Uh oh!

mhbdev commented Jun 8, 2023

Uh oh!

QuncCccccc commented Jun 8, 2023

Uh oh!

mhbdev commented Jun 8, 2023

Uh oh!

Uh oh!

mhbdev commented May 23, 2023 •

edited

Loading

QuncCccccc commented Jun 8, 2023 •

edited

Loading