Add applyFocusChangesIfNeeded, have menus restore focus before activating #130536

gspencergoog · 2023-07-13T23:11:41Z

Description

This modifies the MenuAnchor onPressed activation to delay until after the current frame is built, and resolve any focus changes before it invokes the onPressed, so that actions that operate on the primaryFocus can have a chance of working on the focused item they were meant to work on.

Related Issues

Fixes Menu items steal focus, so implementing copy/paste is very hard. #118731

Tests

No tests yet (hence draft still)

anlumo · 2023-07-20T14:39:58Z

Isn't that only one part of the menu handling? When opening menus, menu items have to be disabled if there's no Action to handle them. I'm checking this by Actions.maybeFind on the current focus node, but if I do that in the SubmenuButton's onOpen, the focus has already been stolen.

EDIT: that aspect might be fixed as a side effect of #125000.

gspencergoog · 2023-07-20T17:19:26Z

@anlumo Yes, this PR only addresses the restoring of focus before executing onPressed. We probably should invoke onOpen before requesting focus, but I don't think that will help you if the submenu button is itself in a submenu, since the parent submenu button will already have focus.

All of this is a side effect of wanting to allow any widget in the menus: regular widgets need to be able to accept focus to know when to be activated by shortcuts (e.g. the enter button), so we can't avoid assigning focus to them temporarily. Otherwise, we would just show focus highlights on hover, etc, but not actually use the focus system to do it. I've been trying to come up with interesting modifications to how focus can be grouped or stacked, but so far none of them have been viable for one reason or another.

…ing.

This PR was generated by `flutter update-packages --force-upgrade`.

anlumo · 2023-07-20T19:56:00Z

@gspencergoog I've tried using a FocusScope around the non-menu widget tree in the hopes of getting a separate focus tree (so the menu item checks would check FocusScopeNode.focusedChild instead of FocusManager.instance.primaryFocus, getting the item that would be in focus when the menu weren't open). According to the documentation, this should work, but in practice after opening the menu this just returns an unknown FocusNode I can't locate in the widget tree, making it useless.

gspencergoog · 2023-07-20T20:13:29Z

Right, there's only one focus tree (and only one primaryFocus), so it doesn't matter what the structure of the tree is.

The best idea I have so far is to change the focus system so that it can have a stack of focus states, and push the stack when any menu opens, pop it when the menu closes, and provide a way to interrogate the previous focus states, but that has a lot of implementation problems (for instance, how to keep the previous states up to date with a changing widget tree while they're not active), and complicates the logic for finding an action, or anything, based on focus.

anlumo · 2023-07-20T20:21:59Z

@gspencergoog so the documentation on FocusScope/FocusScopeNode is wrong?

FocusScopeNodes remember the last FocusNode that was focused within their descendants

and on FocusScopeNode:

A subclass of FocusNode that acts as a scope for its descendants, maintaining information about which descendant is currently or was last focused.

and

focusedChild → FocusNode?
Returns the child of this node that should receive focus if this scope node receives focus.

This sounds to me that when the focus moves from a descendant of the FocusScopeNode to a non-descendant of it, it remembers the old FocusNode that had the focus within its tree.

Apparently it does something else? Otherwise, this would be good enough for a menu system.

gspencergoog · 2023-07-20T20:43:00Z

The documentation is correct, but the key missing point there is that while the FocusScope remembers previously focused children, and even keeps track of which one was last focused (focusedChild), this is mainly just keeping the history: if the scope itself doesn't have focus, then those nodes don't either. But if the focus goes to another node, and then back to the scope, then the scope remembers which of its children should get the focus and redirects it there. This is mainly used as a mechanism for when a dialog or other UI pops up, and then goes away again, so that the previously focused item receives the focus when the dialog goes away. Which sounds like it would be good for menus, but while the dialog (or menu) is up, the dialog (or menu) has the focus, so looking for something based on focus when the menu is up will be looking in the wrong place.

I should probably clarify that in the documentation, thanks for pointing it out.

anlumo · 2023-07-20T21:10:15Z

@gspencergoog but the actions run by menu items don't have to use the FocusManager.instance.primaryFocus.

What I'm doing right now is:

My content (the body of my Scaffold, while the main menu is in the appBar) has a FocusNode called contentFocus.

The top level menus of my menu bar are declared like this:

SubmenuButton(
  onOpen: () {
    final primaryFocus = FocusManager.instance.primaryFocus;
    if (primaryFocus != null) {
      var parent = primaryFocus.parent;
      while (parent != null) {
        if (parent == contentFocus) {
          actionTarget = primaryFocus;
          break;
        }
        parent = parent.parent;
      }
    }

    updateNotifier.onOpen(actionTarget);
  },
}

This check for the parent is necessary, because as you said onOpen is also triggered when moving between menus, where the old menu item has focus. I'm just keeping my old focusNode if this is the case.

The updateNotifier notifies all submenu items which then checks for the intent:

    final context = actionTarget?.context;
    if (context != null) {
      final active = Actions.maybeFind(context, intent: intent) != null;
      _actionTarget = actionTarget; // remember in instance variable
      // set active state on menu item
    }

and then in onPressed:

if (_actionTarget?.context != null) {
  Actions.invoke(_actionTarget!.context!, intent);
}

Of course, this is way abbreviated, but that's the general idea. This actually works for me now, but it's way more complex than it should be for a common user interface element like a menu.

QuncCccccc

LGTM!

gspencergoog · 2023-07-20T22:11:04Z

This actually works for me now, but it's way more complex than it should be for a common user interface element like a menu.

I agree, it is too complex.

anlumo · 2023-07-20T22:52:54Z

@gspencergoog I've created a separate ticket for this: #131020

… activating (flutter/flutter#130536)

Roll Flutter from d07e8ae to 9cfbf6b (58 revisions) flutter/flutter@d07e8ae...9cfbf6b 2023-07-21 [email protected] Roll Packages from 674179f to 2266a76 (6 revisions) (flutter/flutter#131058) 2023-07-21 [email protected] Add tests for navigation_drawer_theme_test.dart (flutter/flutter#130465) 2023-07-21 [email protected] Roll Flutter Engine from f812cf373b6b to f5c1650c7acc (1 revision) (flutter/flutter#131037) 2023-07-21 [email protected] Roll Flutter Engine from 264685f0aecb to f812cf373b6b (1 revision) (flutter/flutter#131032) 2023-07-21 [email protected] Roll Flutter Engine from 8ff10f5a7667 to 264685f0aecb (1 revision) (flutter/flutter#131031) 2023-07-21 [email protected] Roll Flutter Engine from b5a6b1c9cba5 to 8ff10f5a7667 (5 revisions) (flutter/flutter#131029) 2023-07-21 [email protected] Roll Flutter Engine from 218b71cd7a45 to b5a6b1c9cba5 (1 revision) (flutter/flutter#131025) 2023-07-21 [email protected] Roll Flutter Engine from ab7d424d4467 to 218b71cd7a45 (2 revisions) (flutter/flutter#131024) 2023-07-21 [email protected] Use downgraded analyze for flutter/packages (flutter/flutter#130878) 2023-07-21 [email protected] Roll pub packages (flutter/flutter#131022) 2023-07-20 [email protected] Roll Flutter Engine from 9b2ebf2afe00 to ab7d424d4467 (4 revisions) (flutter/flutter#131015) 2023-07-20 [email protected] Add applyFocusChangeIfNeeded, have menus restore focus before activating (flutter/flutter#130536) 2023-07-20 [email protected] Migrate more integration tests to process result matcher (flutter/flutter#130994) 2023-07-20 [email protected] Roll Flutter Engine from 062079ba30b6 to 9b2ebf2afe00 (2 revisions) (flutter/flutter#131013) 2023-07-20 [email protected] Trivial grammar and wrapping fix for docs (flutter/flutter#130955) 2023-07-20 [email protected] Updated the ThemeData API example (flutter/flutter#130954) 2023-07-20 [email protected] Roll Flutter Engine from a3fc18514cd6 to 062079ba30b6 (3 revisions) (flutter/flutter#131010) 2023-07-20 [email protected] Roll Flutter Engine from 6d7842d25f81 to a3fc18514cd6 (2 revisions) (flutter/flutter#131007) 2023-07-20 [email protected] Update `TextSelectionTheme`, `ThemeData`, `TimePicker`, and `TimePickerTheme` tests for M2/M3 (flutter/flutter#130547) 2023-07-20 [email protected] Roll Flutter Engine from c645eb6da8a9 to 6d7842d25f81 (1 revision) (flutter/flutter#130992) 2023-07-20 [email protected] Update AutoComplete test for M3 migration (flutter/flutter#130883) 2023-07-20 [email protected] Update about tests for M3 (flutter/flutter#130970) 2023-07-20 [email protected] Roll Packages from 209db21 to 674179f (4 revisions) (flutter/flutter#130989) 2023-07-20 [email protected] Roll Flutter Engine from e40995da7869 to c645eb6da8a9 (1 revision) (flutter/flutter#130988) 2023-07-20 [email protected] Upgrade leak_tracker. (flutter/flutter#130951) 2023-07-20 [email protected] Roll Flutter Engine from 2df3b9c4b2a4 to e40995da7869 (2 revisions) (flutter/flutter#130985) 2023-07-20 [email protected] Roll Flutter Engine from b494143fb0bc to 2df3b9c4b2a4 (3 revisions) (flutter/flutter#130973) 2023-07-20 [email protected] Fix chip delete button tap target spilling into the label. (flutter/flutter#130896) 2023-07-20 [email protected] Fix IconButton leaks its internal MaterialStatesController (flutter/flutter#130720) 2023-07-20 [email protected] Update banner_theme_test.dart for M3 (flutter/flutter#130884) 2023-07-20 [email protected] Roll Flutter Engine from 204625490ca1 to b494143fb0bc (1 revision) (flutter/flutter#130966) 2023-07-20 [email protected] Make PollingDeviceDiscovery start the initial poll faster. (flutter/flutter#130755) 2023-07-20 [email protected] Roll Flutter Engine from c902fec1e3ce to 204625490ca1 (1 revision) (flutter/flutter#130962) 2023-07-20 [email protected] More documentation for MediaQuery and friends (flutter/flutter#130509) 2023-07-20 [email protected] Roll Flutter Engine from 56e88e8b0eef to c902fec1e3ce (1 revision) (flutter/flutter#130960) 2023-07-20 [email protected] Automatically create the layer when setting hints in PaintingContext (flutter/flutter#130364) 2023-07-20 [email protected] Roll Flutter Engine from e2a36be2f084 to 56e88e8b0eef (1 revision) (flutter/flutter#130959) 2023-07-20 [email protected] Further clarify Stack documentation on overflowing (flutter/flutter#130776) 2023-07-20 [email protected] Roll Flutter Engine from eff70f7287f9 to e2a36be2f084 (2 revisions) (flutter/flutter#130956) 2023-07-20 [email protected] Can traverse if current focused node skips traversal (flutter/flutter#130812) 2023-07-20 [email protected] Document that you can't change initialRoute usefully (flutter/flutter#130450) 2023-07-20 [email protected] Roll Flutter Engine from 7671e2f2a9fc to eff70f7287f9 (3 revisions) (flutter/flutter#130953) 2023-07-19 [email protected] Roll Flutter Engine from 938140a974b0 to 7671e2f2a9fc (3 revisions) (flutter/flutter#130948) 2023-07-19 [email protected] Roll Flutter Engine from 0af285219809 to 938140a974b0 (1 revision) (flutter/flutter#130943) 2023-07-19 [email protected] Add docs to Route.maintainState (flutter/flutter#130638) 2023-07-19 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.20.4 to 2.21.0 (flutter/flutter#130941) ...

…ing (flutter#130536) ## Description This modifies the `MenuAnchor` `onPressed` activation to delay until after the current frame is built, and resolve any focus changes before it invokes the `onPressed`, so that actions that operate on the `primaryFocus` can have a chance of working on the focused item they were meant to work on. ## Related Issues - Fixes flutter#118731 ## Tests - No tests yet (hence draft still)

Zulip is an open-source team chat app, with a new Flutter-based mobile client now in beta. This is that client's test suite. I believe these will be the only tests currently in this registry for an app, rather than a library. That should naturally give them a different mix of use cases and types of tests. Concretely, we've seen a handful of breaking changes over the past year that weren't caught by any of Flutter's existing test suites. These were in text hit-testing: zulip/zulip-flutter@ba7a2bf flutter/flutter#140621 and Material menu behavior: zulip/zulip-flutter@38ed6c8 flutter/flutter#130536 and SlottedContainerRenderObjectMixin gaining a type parameter: zulip/zulip-flutter@2f0f469 flutter/flutter#126108 I'm not complaining, to be clear, and none of these were particularly onerous for us to adapt to. By registering these tests, I'm hoping to provide feedback on future such breakages at a point where it's actionable. Omitted here are several tests that re-generate generated files and check they match what's in the tree. Those are pretty slow, and I think they're pretty insensitive to changes in the Flutter tree anyway; rather they depend on pigeon, json_serializable, build_runner, and drift_dev.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: focus Focus traversal, gaining or losing focus labels Jul 13, 2023

gspencergoog force-pushed the apply_focus_changes branch 2 times, most recently from dc4c0a7 to a091feb Compare July 17, 2023 18:24

github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Jul 17, 2023

gspencergoog force-pushed the apply_focus_changes branch 4 times, most recently from 6d001a0 to 2639277 Compare July 19, 2023 20:28

gspencergoog changed the title ~~Add applyFocusChangeIfNeeded, have menus restore focus before activat…~~ Add applyFocusChangeIfNeeded, have menus restore focus before activating Jul 20, 2023

gspencergoog and others added 5 commits July 20, 2023 10:34

Add applyFocusChangeIfNeeded, have menus restore focus before activat…

ec7d0de

…ing.

Revert remaining overlay portal changes

f32c0e4

Fix layout tests

f7500c0

Roll pub packages (flutter#130608)

1804e23

This PR was generated by `flutter update-packages --force-upgrade`.

Add test

5fcd643

gspencergoog force-pushed the apply_focus_changes branch from 2639277 to 5fcd643 Compare July 20, 2023 17:53

gspencergoog marked this pull request as ready for review July 20, 2023 17:53

gspencergoog requested a review from QuncCccccc July 20, 2023 19:35

QuncCccccc approved these changes Jul 20, 2023

View reviewed changes

gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 20, 2023

auto-submit bot merged commit e4a39fa into flutter:master Jul 20, 2023

anlumo mentioned this pull request Jul 20, 2023

Simplify Menu Bar Handling #131020

Open

engine-flutter-autoroll mentioned this pull request Jul 21, 2023

Roll Flutter from d07e8aece184 to aafdbc6d12cd (57 revisions) flutter/packages#4542

Closed

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

e4a39fa Add applyFocusChangeIfNeeded, have menus restore focus before…

1f743b6

… activating (flutter/flutter#130536)

engine-flutter-autoroll mentioned this pull request Jul 21, 2023

Roll Flutter from d07e8aece184 to 9cfbf6b9fd40 (58 revisions) flutter/packages#4544

Merged

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

e4a39fa Add applyFocusChangeIfNeeded, have menus restore focus before…

2d619bb

… activating (flutter/flutter#130536)

HansMuller mentioned this pull request Jul 21, 2023

Flutter Framework Team: Notable Commits #121415

Closed

gnprice mentioned this pull request Jul 24, 2023

Register our tests in Flutter's "customer tests" suite zulip/zulip-flutter#239

Closed

gspencergoog changed the title ~~Add applyFocusChangeIfNeeded, have menus restore focus before activating~~ Add applyFocusChangesIfNeeded, have menus restore focus before activating Nov 14, 2023

gnprice mentioned this pull request May 22, 2024

Add zulip.test flutter/tests#368

Merged

ahmedrasar mentioned this pull request Jun 11, 2025

Make DropdownMenu TextField reactive to label changes #162062

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add applyFocusChangesIfNeeded, have menus restore focus before activating #130536

Add applyFocusChangesIfNeeded, have menus restore focus before activating #130536

Uh oh!

gspencergoog commented Jul 13, 2023 •

edited

Loading

Uh oh!

anlumo commented Jul 20, 2023 •

edited

Loading

Uh oh!

gspencergoog commented Jul 20, 2023 •

edited

Loading

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023 •

edited

Loading

Uh oh!

QuncCccccc left a comment

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add applyFocusChangesIfNeeded, have menus restore focus before activating #130536

Add applyFocusChangesIfNeeded, have menus restore focus before activating #130536

Uh oh!

Conversation

gspencergoog commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Uh oh!

anlumo commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Jul 20, 2023

Uh oh!

anlumo commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gspencergoog commented Jul 13, 2023 •

edited

Loading

anlumo commented Jul 20, 2023 •

edited

Loading

gspencergoog commented Jul 20, 2023 •

edited

Loading

anlumo commented Jul 20, 2023 •

edited

Loading