Fix menu anchor state handling #157612

YeungKC · 2024-10-25T16:51:54Z

This commit refactors the _MenuAnchorState class in menu_anchor.dart to include a check for the mounted state and the scheduler phase before calling setState(). This ensures that UI updates are only performed when the widget is still mounted and not during the persistent callbacks phase. Additionally, a new test case is added in menu_anchor_test.dart to verify that the isOpen state of the MenuAnchor widget is updated correctly when the button is pressed.

Fix: #157606

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.

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.
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.

This commit refactors the `_MenuAnchorState` class in `menu_anchor.dart` to include a check for the mounted state and the scheduler phase before calling `setState()`. This ensures that UI updates are only performed when the widget is still mounted and not during the persistent callbacks phase. Additionally, a new test case is added in `menu_anchor_test.dart` to verify that the `isOpen` state of the `MenuAnchor` widget is updated correctly when the button is pressed. Fix: flutter#157606

nate-thegrate

Thanks for the fix, this looks awesome!

nate-thegrate · 2024-10-25T17:00:15Z

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

+    if (mounted && SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks) {
+      setState(() {
+        // Mark dirty to ensure UI updates
+      });
+    }


I had no idea that the SchedulerPhase could be used this way, thanks!

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

nate-thegrate · 2024-10-25T17:45:54Z

Actually, quick question before we land this one—there looks to be a _MenuAnchorScope class set up to notify when the value of isOpen changes.

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

Lines 2533 to 2551 in add7e81

    
           class _MenuAnchorScope extends InheritedWidget { 
        
             const _MenuAnchorScope({ 
        
               required super.child, 
        
               required this.anchorKey, 
        
               required this.anchor, 
        
               required this.isOpen, 
        
             }); 
        
             final GlobalKey anchorKey; 
        
             final _MenuAnchorState anchor; 
        
             final bool isOpen; 
        
             @override 
        
             bool updateShouldNotify(_MenuAnchorScope oldWidget) { 
        
               return anchorKey != oldWidget.anchorKey 
        
                   || anchor != oldWidget.anchor 
        
                   || isOpen != oldWidget.isOpen; 
        
             } 
        
           }

Currently it doesn't look like the inherited widget rebuilds when the menu is opened; maybe we should remove the isOpen field to prevent confusion.

YeungKC · 2024-10-25T18:35:03Z

The _MenuAnchorScope is indeed designed to notify its dependents when the isOpen state changes, but this notification doesn't directly trigger a rebuild of the builder. Instead, it triggers the didChangeDependencies method in the dependent widgets.

nate-thegrate · 2024-10-25T18:52:34Z

The _MenuAnchorScope is indeed designed to notify its dependents when the isOpen state changes, but this notification doesn't directly trigger a rebuild of the builder. Instead, it triggers the didChangeDependencies method in the dependent widgets.

True that!

I think what I'm trying to get to is: this bug indicates that _MenuAnchorState does not rebuild when the menu is opened.

Perhaps it'd be good to remove the _MenuAnchorScope.isOpen flag, since currently it isn't set up to report accurate values. Alternatively, we could rework _MenuAnchorScope so it's rebuilt when the controller opens/closes.

YeungKC · 2024-10-25T18:59:38Z

Consider a scenario where we're not using the builder pattern, but instead using a simple child widget. After this PR is merged, such a child widget would still be able to detect updates through didChangeDependencies.

nate-thegrate · 2024-10-25T19:13:23Z

Consider a scenario where we're not using the builder pattern, but instead using a simple child widget. After this PR is merged, such a child widget would still be able to detect updates through didChangeDependencies.

Perhaps so, though we'd need some way to let the child depend on _MenuAnchorScope through a public API.

My concern is:

context.dependOnInheritedWidgetOfExactType<_MenuAnchorScope>()!.isOpen;

In order for this value to be accurate, the inherited widget needs to be rebuilt when the controller opens/closes. And since rebuilding the _MenuAnchorState would also involve re-evaluating the builder, I believe this bug is evidence that _MenuAnchorScope.isOpen is sometimes inaccurate.

Since it's a private class and isOpen isn't accessed anywhere, perhaps we should remove the field.

YeungKC · 2024-10-25T19:34:19Z

I don't believe that _MenuAnchorScope.isOpen would be inaccurate. Could you provide a test case demonstrating this?

While I understand that _MenuAnchorScope.isOpen can't be accessed directly, triggering didChangeDependencies seems more "correct" to me. If I were to use didChangeDependencies as a trigger and then check the controller's isOpen state, that would also be a valid approach.

I think we may be straying too far from the original intent of this PR. Perhaps it would be better to open a separate issue to discuss this particular concern in more depth.

nate-thegrate · 2024-10-25T19:37:38Z

I think we may be straying too far from the original intent of this PR.

Fair point, especially if we end up removing the _MenuAnchorScope.isOpen field.

If we decide to keep it, however, updating _MenuAnchorState to rebuild when its value changes would mean that the changes in this PR are unnecessary.

YeungKC · 2024-10-25T19:43:33Z

I see your point. However, I think there's an important aspect to consider here. Without this PR, the _MenuAnchorScope wouldn't update its isOpen state, and consequently, updateShouldNotify wouldn't return true when the menu opens or closes.

nate-thegrate · 2024-10-25T19:54:58Z

I see your point. However, I think there's an important aspect to consider here. Without this PR, the _MenuAnchorScope wouldn't update its isOpen state, and consequently, updateShouldNotify wouldn't return true when the menu opens or closes.

100% agree. In my mind I was thinking "if _MenuAnchorScope is updated to rebuild when the controller opens/closes, then we could just add a dependency to the builder"

What didn't connect for me till just now: since this change calls _MenuAnchorState.setState, the inherited widget is now guaranteed to be up-to-date, so that's pretty awesome 👍

bleroux

LGTM! Thanks for the contribution.

flutter/flutter@42132e8...fe71cad 2024-10-30 [email protected] Update CHANGELOG.md to correct ios vs macos issue (flutter/flutter#157822) 2024-10-30 [email protected] Add ability to customize the default `Slider` padding (flutter/flutter#156143) 2024-10-30 [email protected] Fix menu anchor state handling (flutter/flutter#157612) 2024-10-30 [email protected] Add test for `interactive_viewer.0.dart` (flutter/flutter#157773) 2024-10-30 [email protected] Add test for `scroll_metrics_notification.0.dart` (flutter/flutter#157768) 2024-10-30 [email protected] Add boundary feature to the drag gesture. (flutter/flutter#147521) 2024-10-30 [email protected] Fix `ResizeImage` documentation (flutter/flutter#157619) 2024-10-29 [email protected] Roll Flutter Engine from 795b5492f1b9 to 999797a2f690 (1 revision) (flutter/flutter#157825) 2024-10-29 [email protected] Avoid labeling all PRs as 'text-input' (flutter/flutter#157805) 2024-10-29 [email protected] Roll Packages from e0c4f55 to 028027e (8 revisions) (flutter/flutter#157813) 2024-10-29 [email protected] Roll Flutter Engine from 725c8e4bc379 to 795b5492f1b9 (5 revisions) (flutter/flutter#157820) 2024-10-29 [email protected] Fix and remove a few `no-shuffle` tags in `flutter_tools`. (flutter/flutter#157656) 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],[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

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 25, 2024

YeungKC requested a review from nate-thegrate October 25, 2024 16:52

doc

4fdfa4f

nate-thegrate approved these changes Oct 25, 2024

View reviewed changes

format

159d877

YeungKC marked this pull request as ready for review October 25, 2024 17:12

YeungKC requested a review from dkwingsmt October 25, 2024 17:14

YeungKC changed the title ~~Fix menu anchor state handling and add test case~~ Fix menu anchor state handling Oct 25, 2024

YeungKC added 2 commits October 26, 2024 02:16

format

b72d5fc

Merge branch 'master' into fix/MenuAnchor

9bd57ff

YeungKC requested review from TahaTesser, bleroux, gspencergoog and justinmc October 25, 2024 18:07

bleroux approved these changes Oct 30, 2024

View reviewed changes

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2024

auto-submit bot merged commit ec50578 into flutter:master Oct 30, 2024
73 checks passed

YeungKC mentioned this pull request Oct 30, 2024

[menu_anchor.dart] MenuAnchor crashes when used as context menu and opened during didUpdateWidget. #150346

Closed

engine-flutter-autoroll mentioned this pull request Oct 30, 2024

Roll Flutter from 42132e879b42 to fe71cadf1959 (12 revisions) flutter/packages#7961

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 30, 2024

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

159215a

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

c3e2387

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

996050d

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

3a264a9

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

b6d02ab

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

ec50578 Fix menu anchor state handling (flutter/flutter#157612)

81d0e0b

Fix menu anchor state handling #157612

Fix menu anchor state handling #157612

Uh oh!

Conversation

YeungKC commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nate-thegrate commented Oct 25, 2024

Uh oh!

YeungKC commented Oct 25, 2024

Uh oh!

nate-thegrate commented Oct 25, 2024

Uh oh!

YeungKC commented Oct 25, 2024

Uh oh!

nate-thegrate commented Oct 25, 2024

Uh oh!

YeungKC commented Oct 25, 2024

Uh oh!

nate-thegrate commented Oct 25, 2024

Uh oh!

YeungKC commented Oct 25, 2024

Uh oh!

nate-thegrate commented Oct 25, 2024

Uh oh!

bleroux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YeungKC commented Oct 25, 2024 •

edited

Loading