[widgets/raw_menu_anchor.dart] Call onCloseRequested on closed menus#186376
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies RawMenuAnchor to trigger the close callback even when the menu is already hidden by removing the isOpen check in handleCloseRequest. Documentation and tests have been updated to reflect this new behavior. Feedback indicates that the documentation should be further updated for consistency regarding sibling menu behavior, and a re-entrancy guard should be considered to prevent potential infinite recursion when calling MenuController.close() within the close callback.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/flutter/lib/src/widgets/raw_menu_anchor.dart (316-317)
The updated documentation correctly reflects the new behavior. However, it creates an inconsistency with the subsequent lines (319-320), which still state that the callback is triggered when a sibling is opened "while this menu is open". Since the isOpen check has been removed from handleCloseRequest, this callback will now be triggered regardless of the menu's state. Please update line 320 to maintain consistency.
References
- Documentation should be useful and accurate. Ensure consistency across the documentation block. (link)
packages/flutter/lib/src/widgets/raw_menu_anchor.dart (800-802)
Removing the isOpen guard allows the callback to be triggered for closed menus, but it also removes a safety check that prevented infinite recursion if MenuController.close() was called from within the onCloseRequested callback. While this is an edge case, the previous implementation was more robust as it would break the loop once hideOverlay was called. Consider if a re-entrancy guard is needed to maintain robustness.
dkwingsmt
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the work! And sorry for late review.
|
I think it might make more sense to be a new doc, since the previous doc was "Changing RawMenuAnchor close order", a different topic. But since it didn't even break existing unit tests of the material+cupertino menu anchor, a migration guide isn't necessary (yet still welcome) if you prefer. :) |
0ea19c9 to
48dc6f0
Compare
|
Rebased to update the base commit so we can merge this. 🎊 |
…lutter#186376) Small breaking change to flutter#182357. In flutter#182357, I added a line to prevent `onCloseRequested` from being called on closed menus. I now remember the original design was actually intentional, because it makes synchronizing delays between sibling menu anchors trivial. Fixes flutter#186379 To demonstrate: ```dart RawMenuAnchor( onOpenRequested: (position, showOverlay) { final parentController = MenuController.maybeOf(context)!; // This fix makes it so that calling closeChildren on a parent menu controller immediately // calls onCloseRequested on all sibling RawMenuAnchors, allowing any pending // _openTimers on siblings to be canceled prior to those siblings calling showOverlay. parentController.closeChildren(); _openTimer = Timer(const Duration(milliseconds: 500), () { _openTimer = null; showOverlay(); }); }, onCloseRequested: (hideOverlay) { _openTimer?.cancel(); _openTimer = null; }, // ... ) ``` Considering this behavior was already tested, I modified the original test to reflect the changing behavior. *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* Since this is a breaking change to a breaking change that hasn't landed in stable, should I amend the original breaking change doc, or is a new doc required? @dkwingsmt @chunhtai ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] 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]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Small breaking change to #182357. In #182357, I added a line to prevent
onCloseRequestedfrom being called on closed menus. I now remember the original design was actually intentional, because it makes synchronizing delays between sibling menu anchors trivial.Fixes #186379
To demonstrate:
Considering this behavior was already tested, I modified the original test to reflect the changing behavior.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Since this is a breaking change to a breaking change that hasn't landed in stable, should I amend the original breaking change doc, or is a new doc required?
@dkwingsmt @chunhtai
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.