The initial/selected item on popup menu should always be visible #143118

QuncCccccc · 2024-02-07T21:34:48Z

With the change of #143121, this PR is to add auto scroll to PopupMenuButton so when we open the menu, it will automatically scroll to the selected item.

auto_scroll_fix.mov

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.

HansMuller

LGTM

HansMuller · 2024-02-08T17:11:29Z

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

I'm not sure what the distinction between this and SchedulerBinding.instance.addPostFrameCallback is. Most of our code appears to use the SchedulerBinding version.

Fixes #142896 The original code below is to always place the selected item above(overlap) the popup button so that the selected item can always be visible: https://github.com/flutter/flutter/blob/f8a77225f360556322934a9a831ffc9c9f3125da/packages/flutter/lib/src/material/popup_menu.dart#L723-L732 But when menu height is constrained and the menu itself is super long, the selected item still assumes there is enough space to push up all the items whose index is smaller than the selected index. As a result, every time when the menu is open, the calculation provides a different result to be the offset for the selected index, and then with a constrained height, the menu looks jumping all over the place based on the different selected index. https://github.com/flutter/flutter/assets/36861262/ad761f95-0ff5-4311-a81d-dac56df879c5 Even though the original calculation is to make the selected item visible when open the menu, the menu doesn't auto scroll and only expands itself as much as possible to show the selected one. In this case, if the screen it too small to show the selected item, we still cannot see it. This can be fixed by using `Scrollable.ensureVisible()`(#143118). So we remove the calculation in this PR and the menu will always show up based on the top left of the anchor(button). https://github.com/flutter/flutter/assets/36861262/03272f26-9440-4ac4-a701-9a0b41776ff9

tvolkert

Does this keep the behavior that the selected item will appear over the button when a PopupMenuButton is used?

tvolkert · 2024-02-12T21:46:24Z

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


  final RelativeRect position;
  final List<PopupMenuEntry<T>> items;
+  final List<GlobalKey>? itemKeys;


Are there any other users of _PopupMenuRoute? If not, this doesn't need to be nullable.

You are right. Thanks for catching this! Updating.

tvolkert · 2024-02-12T21:47:10Z

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

      semanticLabel ??= MaterialLocalizations.of(context).popupMenuLabel;
  }

+  final List<GlobalKey> menuItemKeys = itemKeys ?? List<GlobalKey>.generate(items.length, (int index) => GlobalKey());


If we create global keys here, don't we need to dispose them?

Weird - I thought you needed to dispose GlobalKeys, but it looks like I'm wrong - please disregard 🙂

tvolkert · 2024-02-12T21:48:14Z

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

  required BuildContext context,
  required RelativeRect position,
  required List<PopupMenuEntry<T>> items,
+  List<GlobalKey>? itemKeys,


What's the use case for exposing these in the API?

If we do expose them, please cover them in the dart docs above.

Oh sorry, I think we don't have to expose this. I used it to do some experiments but forgot to clean it up! Updating.

auto-submit · 2024-02-12T21:58:46Z

auto label is removed for flutter/flutter/143118, due to This PR has not met approval requirements for merging. Changes were requested by {tvolkert}, please make the needed changes and resubmit this PR.
The PR author is a member of flutter-hackers and needs 0 more review(s) in order to merge this PR.

Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

QuncCccccc · 2024-02-12T23:26:54Z

Does this keep the behavior that the selected item will appear over the button when a PopupMenuButton is used?

Yes, the selected item will still appear over the button but the menu position would be different. With this PR's change, the selected item will always on the top of the menu and the menu is aligned with the button. The difference is the top of the menu is always the same as the button top unless there is not enough space for menu, then the menu would try to show as much as contents like the original way.

I used the same demo👇 as the one shown in previous PR comment

video demo

Screen.Recording.2024-02-07.at.3.55.58.PM.mov

tvolkert · 2024-02-13T00:05:49Z

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

  final _PopupMenuRoute<T> route;
  final String? semanticLabel;
  final BoxConstraints? constraints;
+  final List<GlobalKey>? itemKeys;


Looks like this can be non-nullable now too?

Yes, just updated. Thanks!!

tvolkert

LGTM - thanks!

…ble (flutter/flutter#143118)

Roll Flutter from a628814 to d7867ca (66 revisions) flutter/flutter@a628814...d7867ca 2024-02-16 [email protected] Roll Packages from ef349be to c56c12d (5 revisions) (flutter/flutter#143581) 2024-02-16 [email protected] Update `MaterialStatesController` docs for calling `setState` in a listener (flutter/flutter#143453) 2024-02-16 [email protected] Update `DataTable` docs for disabled `DataRow` ink well (flutter/flutter#143450) 2024-02-16 [email protected] Roll Flutter Engine from b7103bc8b374 to dd530f1556df (17 revisions) (flutter/flutter#143565) 2024-02-16 [email protected] Manual roll Flutter Engine from b7103bc8b374 to 7de84271eb65 (flutter/flutter#143564) 2024-02-16 [email protected] Manual roll Flutter Engine from d3c71d78f8ef to df0dc1fc06ca (flutter/flutter#143563) 2024-02-16 [email protected] Manual roll Flutter Engine from bc4dd534a0fa to d3c71d78f8ef (flutter/flutter#143561) 2024-02-16 [email protected] Manual roll Flutter Engine from edb2745e9834 to bc4dd534a0fa (flutter/flutter#143559) 2024-02-16 [email protected] Manual roll Flutter Engine from 15a358bbaf71 to edb2745e9834 (flutter/flutter#143555) 2024-02-16 [email protected] [devicelab] retain prior events for flutter gallery. (flutter/flutter#143554) 2024-02-16 [email protected] Reland "Disentangle and align flutter build web --wasm flags (#143517)" (flutter/flutter#143549) 2024-02-16 [email protected] Roll Flutter Engine from 3af336bfb2df to 15a358bbaf71 (1 revision) (flutter/flutter#143428) 2024-02-15 [email protected] Android Gradle file templates: make it easier to convert them to Kotlin DSL in the future (flutter/flutter#142146) 2024-02-15 [email protected] Remove bringup from win arm64 builds. (flutter/flutter#143548) 2024-02-15 [email protected] Fix minor spelling error (flutter/flutter#143541) 2024-02-15 [email protected] [devicelab] migrate new gallery benchmarks to local copy. (flutter/flutter#143545) 2024-02-15 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.24.1 to 3.24.3 (flutter/flutter#143546) 2024-02-15 [email protected] [flutter_tool] [dap] Forward Flutter progress events to DAP client (flutter/flutter#142524) 2024-02-15 [email protected] Swap the tasks that have been running fine for a while. (flutter/flutter#143544) 2024-02-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Disentangle and align `flutter build web --wasm` flags (#143517)" (flutter/flutter#143547) 2024-02-15 [email protected] Reland simulatedAccessibilityTraversal fix (flutter/flutter#143527) 2024-02-15 [email protected] Disentangle and align `flutter build web --wasm` flags (flutter/flutter#143517) 2024-02-15 [email protected] [devicelab] introduce new old gallery. (flutter/flutter#143486) 2024-02-15 [email protected] Remove certs dependency. (flutter/flutter#143495) 2024-02-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix and test SemanticsController.simulatedAccessibilityTraversal (#143386)" (flutter/flutter#143523) 2024-02-15 [email protected] Roll Packages from a864254 to ef349be (9 revisions) (flutter/flutter#143521) 2024-02-15 [email protected] Modify `plugin_ffi` and `package_ffi` template (flutter/flutter#143376) 2024-02-15 [email protected] Remove certs installation from win_arm builds. (flutter/flutter#143487) 2024-02-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[a11y] Add isEnabled semantics flag to text field (#143334)" (flutter/flutter#143494) 2024-02-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[a11y] Fix date picker cannot focus on the edit field (#143117)" (flutter/flutter#143493) 2024-02-14 [email protected] [a11y] Fix date picker cannot focus on the edit field (flutter/flutter#143117) 2024-02-14 [email protected] cleanup now-irrelevant ignores for `deprecated_member_use` (flutter/flutter#143403) 2024-02-14 [email protected] Fix and test SemanticsController.simulatedAccessibilityTraversal (flutter/flutter#143386) 2024-02-14 [email protected] Disable deprecation warnings for mega_gallery (flutter/flutter#143466) 2024-02-14 [email protected] The initial/selected item on popup menu should always be visible (flutter/flutter#143118) 2024-02-14 [email protected] Roll native_assets_builder to 0.5.0 (flutter/flutter#143472) 2024-02-14 [email protected] InputDecorator M3 test migration step2 (flutter/flutter#143369) 2024-02-14 [email protected] Add more documentation for TextEditingController default constructor (flutter/flutter#143452) 2024-02-14 [email protected] Format all kotlin according to ktlint (flutter/flutter#143390) 2024-02-14 [email protected] Marks Linux_pixel_7pro integration_ui_keyboard_resize to be unflaky (flutter/flutter#143440) 2024-02-14 [email protected] Roll Packages from 9385bbb to a864254 (6 revisions) (flutter/flutter#143454) 2024-02-14 [email protected] [tools] Add column header for emulators information (flutter/flutter#142853) 2024-02-14 [email protected] Use `dart compile wasm` for wasm compilations (flutter/flutter#143298) 2024-02-14 [email protected] [devicelab] retain first frame data in certain integration tests. (flutter/flutter#143419) 2024-02-14 [email protected] Roll Flutter Engine from 0849250a1419 to 3af336bfb2df (2 revisions) (flutter/flutter#143427) 2024-02-14 [email protected] Roll Flutter Engine from 215d55f4f82d to 0849250a1419 (2 revisions) (flutter/flutter#143425) ...

…ble (flutter/flutter#143118)

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

QuncCccccc changed the title ~~The selected item on popup menu is always visible even though the menu height is constrained~~ The initial/selected item on popup menu should always be visible Feb 7, 2024

QuncCccccc mentioned this pull request Feb 7, 2024

Correct menu position when menu is constrained #143121

Merged

8 tasks

QuncCccccc marked this pull request as ready for review February 7, 2024 23:42

QuncCccccc requested a review from HansMuller February 8, 2024 00:03

HansMuller approved these changes Feb 8, 2024

View reviewed changes

QuncCccccc force-pushed the auto_scroll_popup branch from de0270b to 6c11f84 Compare February 8, 2024 20:47

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

QuncCccccc added 9 commits February 12, 2024 10:27

Make sure the selected item is always visible

af5e971

Update

68464c7

Revert unintended change

08f7140

Update unit test message

3628184

Revert unintended change

babaff4

Update unit test

87edc6f

Address comment

c18bb51

Accidentally delete a test while fixing conflict

f48f61a

Trigger google testing

d49217a

QuncCccccc force-pushed the auto_scroll_popup branch from 931db17 to d49217a Compare February 12, 2024 18:28

tvolkert suggested changes Feb 12, 2024

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 12, 2024

Address comments

7d2f59b

tvolkert reviewed Feb 13, 2024

View reviewed changes

Address PR comment

1521e28

tvolkert approved these changes Feb 13, 2024

View reviewed changes

tvolkert added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 13, 2024

auto-submit bot merged commit 9bc8393 into flutter:master Feb 14, 2024

engine-flutter-autoroll mentioned this pull request Feb 15, 2024

Roll Flutter from a628814ebd25 to 97ab92ee353e (40 revisions) flutter/packages#6133

Closed

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

9bc8393 The initial/selected item on popup menu should always be visi…

a06b64d

…ble (flutter/flutter#143118)

engine-flutter-autoroll mentioned this pull request Feb 15, 2024

Roll Flutter from a628814ebd25 to c0778318f9c9 (41 revisions) flutter/packages#6135

Closed

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

9bc8393 The initial/selected item on popup menu should always be visi…

01a3402

…ble (flutter/flutter#143118)

engine-flutter-autoroll mentioned this pull request Feb 15, 2024

Roll Flutter from a628814ebd25 to d00fe8faae66 (42 revisions) flutter/packages#6136

Closed

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

9bc8393 The initial/selected item on popup menu should always be visi…

02333f9

…ble (flutter/flutter#143118)

engine-flutter-autoroll mentioned this pull request Feb 16, 2024

Roll Flutter from a628814ebd25 to a603a178757f (65 revisions) flutter/packages#6144

Closed

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

9bc8393 The initial/selected item on popup menu should always be visi…

62f0a41

…ble (flutter/flutter#143118)

engine-flutter-autoroll mentioned this pull request Feb 16, 2024

Roll Flutter from a628814ebd25 to d7867ca7d635 (66 revisions) flutter/packages#6146

Merged

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

9bc8393 The initial/selected item on popup menu should always be visi…

056c135

…ble (flutter/flutter#143118)

HansMuller mentioned this pull request Feb 16, 2024

Flutter Framework Team: Notable Commits #121415

Closed

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

9bc8393 The initial/selected item on popup menu should always be visi…

32ed473

…ble (flutter/flutter#143118)

The initial/selected item on popup menu should always be visible #143118

The initial/selected item on popup menu should always be visible #143118

Uh oh!

Conversation

QuncCccccc commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Feb 12, 2024

Uh oh!

QuncCccccc commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QuncCccccc commented Feb 7, 2024 •

edited

Loading

QuncCccccc commented Feb 12, 2024 •

edited

Loading