Skip to content

Conversation

gspencergoog
Copy link
Contributor

Description

This converts the MenuAnchor class to use OverlayPortal instead of directly using the overlay.

Related Issues

Tests

  • No tests yet (hence it is a draft)

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. f: focus Focus traversal, gaining or losing focus labels Jul 13, 2023
@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch from 093f7f5 to b698928 Compare July 13, 2023 23:13
assert(object.attached);
final RenderAbstractViewport viewport = RenderAbstractViewport.of(object);
final RenderAbstractViewport? viewport = RenderAbstractViewport.maybeOf(object);
// If no viewport is found, return.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LongCatIsLooong @Piinks

Is ignoring missing RenderAbstractViewports here a bad idea? I needed to make this change because it used to assert when Scrollable.ensureVisible finds a Scrollable in the widget tree, but it doesn't have an associated RenderAbstractViewport in the render tree, because the OverlayPortal is moving (I think?) the render objects into the overlay.

The menu items scroll just fine, so I think it is calling ensureVisible on the Scrollable in the menu (which is in the overlay), but then when it keeps going up the widget tree to make sure that nested scrollables are visible, it finds a Scrollable that isn't an ancestor anymore in the render tree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh if I write position.ensureVisible using a RenderObject that's not even in the subtree, with the new implementation it would fail silently?

My understanding is ensureVisible only adjusts the scroll offets on scrollable containers to make something visible. So using Scrollable.ensureVisbile for revealing a focused widget may not work well in some cases. If a widget creates an Overlay, to make one of its children visible, it can also change the positioning of these children to reveal the child? If the child is behind some other children then no matter how you scroll the scrollable containers it will still be obstructed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh if I write position.ensureVisible using a RenderObject that's not even in the subtree, with the new implementation it would fail silently?

Yes, it would ignore requests to make a render object visible that isn't a child of a viewport.

So using Scrollable.ensureVisible for revealing a focused widget may not work well in some cases.

Yes, you're right that ensureVisible only affects scrollable regions: it doesn't do anything to move things out of the way or otherwise move the widgets around. It would be pretty challenging to get that right, given all the ways that the widgets could be laid out. Scrolling things doesn't work in all cases, but it does in the most common case where you've hit tab while at the end of a list, and you want to scroll the list to show the newly focused thing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its ok... my first thought was a ScrollPosition should always have a viewport, since it is a subclass of ViewportOffset, and if we have made it here and there is not viewport something may have gone terribly wrong...
But reading through the comments here I think it makes sense in this case when the render objects are in transit to the overlay.

@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch 3 times, most recently from 6948da7 to 49fa7cf Compare July 18, 2023 21:28
@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch 2 times, most recently from d7af424 to 7e99b31 Compare August 21, 2023 17:41
@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch 2 times, most recently from b9e0cc4 to ec6cb37 Compare August 31, 2023 19:30
@github-actions github-actions bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos labels Sep 1, 2023
@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch 3 times, most recently from 4734463 to 38f49c0 Compare September 6, 2023 00:41
@gspencergoog gspencergoog force-pushed the menu_overlay_portal branch 3 times, most recently from b4252a6 to fbcd044 Compare October 16, 2023 21:18
@gspencergoog gspencergoog marked this pull request as ready for review October 16, 2023 21:26
@@ -462,17 +505,17 @@ class _MenuAnchorState extends State<MenuAnchor> {

_MenuAnchorState get _topLevel {
_MenuAnchorState handle = this;
while (handle._parent!._isTopLevel) {
while (handle._parent != null && handle._parent!._isTopLevel) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this part. From the code being _isTopLevel means the _MenuAnchorState's parent is the root node. So I'm assuming _isTopLevel == true is unique in every path (from the leaf _MenuAnchorState to the root) . If that's true then handle._parent!._isTopLevel can only be true for one single node in each path, then I think either the while should just be an if, or the condition actually should be negated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top level node is the child of the root node that is the ancestor of (or equal to) the handle node. This code walks up the focus path until it finds the _MenuAnchorState ancestor for which _isTopLevel returns true. Yes, only one node in the path will match _isTopLevel == true, but it's more efficient to walk up the path than to check to see if one of the top level nodes has a descendant that is equal to handle.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the condition be handle._parent != null && !handle._parent!._isTopLevel so the while actually loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG. You're right, it should be negated.

_overlayEntry?.dispose();
_overlayEntry = null;
// Don't hide if we're in the middle of a build.
if (SchedulerBinding.instance.schedulerPhase != SchedulerPhase.persistentCallbacks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #136769.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2023
@auto-submit auto-submit bot merged commit 13a0d47 into flutter:master Oct 18, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 18, 2023
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Oct 19, 2023
Manual roll Flutter from 6eea6e2 to 189196d (44 revisions)

Manual roll requested by [email protected]

flutter/flutter@6eea6e2...189196d

2023-10-18 [email protected] Roll Flutter Engine from ab86c53c19cd to 6caee3236d37 (2 revisions) (flutter/flutter#136834)
2023-10-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Reland] Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136839)
2023-10-18 [email protected] Convert menus to use OverlayPortal (flutter/flutter#130534)
2023-10-18 [email protected] [Reland] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136751)
2023-10-18 [email protected] Roll Flutter Engine from 0eff5d191856 to ab86c53c19cd (1 revision) (flutter/flutter#136832)
2023-10-18 [email protected] Fixes ability to call nextFocus() on a node to focus its descendant (flutter/flutter#136773)
2023-10-18 [email protected] Roll Flutter Engine from b778a07f8ae9 to 0eff5d191856 (6 revisions) (flutter/flutter#136829)
2023-10-18 [email protected] Implement GApplication:shutdown so a Flutter developer knows where to put code that should occur on application shutdown. (flutter/flutter#136780)
2023-10-18 [email protected] Roll Flutter Engine from 7f37c9b181af to b778a07f8ae9 (1 revision) (flutter/flutter#136818)
2023-10-18 [email protected] Fix `Slider` `onChanged` callback order & never calls `onChangeStart` on  `SliderInteraction.slideOnly` allowed interaction (flutter/flutter#136720)
2023-10-18 [email protected] Roll Flutter Engine from 78026b4003fe to 7f37c9b181af (1 revision) (flutter/flutter#136817)
2023-10-18 [email protected] [Feat] Stroke color for Slider value indicator (flutter/flutter#135986)
2023-10-18 [email protected] Fixed : Empty Rows shown at last page in Paginated data table (flutter/flutter#132646)
2023-10-18 [email protected] Roll Flutter Engine from 46923fd39032 to 78026b4003fe (1 revision) (flutter/flutter#136814)
2023-10-18 [email protected] Roll Packages from d439062 to 14aa69e (1 revision) (flutter/flutter#136813)
2023-10-18 [email protected] Don't build native assets in `flutter build bundle` (flutter/flutter#136641)
2023-10-18 [email protected] Roll Flutter Engine from c9c9684e03a3 to 46923fd39032 (1 revision) (flutter/flutter#136795)
2023-10-18 [email protected] Roll Flutter Engine from 0c1c29271e8b to c9c9684e03a3 (1 revision) (flutter/flutter#136792)
2023-10-18 [email protected] Roll Flutter Engine from 1de09d13e708 to 0c1c29271e8b (3 revisions) (flutter/flutter#136789)
2023-10-18 [email protected] Roll Flutter Engine from 6fc36e61a99a to 1de09d13e708 (1 revision) (flutter/flutter#136785)
2023-10-18 [email protected] Roll Flutter Engine from 3f818efff3c5 to 6fc36e61a99a (1 revision) (flutter/flutter#136781)
2023-10-18 [email protected] Roll Flutter Engine from 2eef9b43cfb4 to 3f818efff3c5 (3 revisions) (flutter/flutter#136777)
2023-10-18 [email protected] Roll Flutter Engine from 5df7af34a718 to 2eef9b43cfb4 (4 revisions) (flutter/flutter#136774)
2023-10-18 [email protected] Flutter preview device (flutter/flutter#135639)
2023-10-18 [email protected] Add findChildIndexCallback examples (flutter/flutter#133469)
2023-10-18 [email protected] Adds API for performing semantics actions in tests (flutter/flutter#132598)
2023-10-18 [email protected] Roll Flutter Engine from e57b5bac4244 to 5df7af34a718 (2 revisions) (flutter/flutter#136772)
2023-10-17 [email protected] Roll Flutter Engine from f9f937e51080 to e57b5bac4244 (3 revisions) (flutter/flutter#136768)
2023-10-17 [email protected] clean up `--dart-define-from-file` option tests (flutter/flutter#135980)
2023-10-17 [email protected] GoldenFileComparators should dispose created Image objects.  (flutter/flutter#136716)
2023-10-17 [email protected] make integration_test_driver_extended.dart support writeResponseData--(done) (flutter/flutter#128382)
2023-10-17 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.6.0 to 4.1.1 (flutter/flutter#136762)
2023-10-17 [email protected] Roll Flutter Engine from 289f29b1ad00 to f9f937e51080 (1 revision) (flutter/flutter#136755)
2023-10-17 [email protected] Roll Flutter Engine from 659e68a097b5 to 289f29b1ad00 (1 revision) (flutter/flutter#136752)
2023-10-17 [email protected] Support --web-header option for flutter run (flutter/flutter#136297)
2023-10-17 [email protected] Add Android 14 physical devices to firebase tests (flutter/flutter#136736)
2023-10-17 [email protected] Roll Flutter Engine from 62a90a91cee3 to 659e68a097b5 (3 revisions) (flutter/flutter#136746)
2023-10-17 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Skip injecting Bonjour settings when port publication is disabled" (flutter/flutter#136750)
2023-10-17 [email protected] Revert "[SingleChildScrollView] Correct the offset pixels if it is out of range during layout" (flutter/flutter#136744)
2023-10-17 [email protected] Skip injecting Bonjour settings when port publication is disabled (flutter/flutter#136562)
2023-10-17 [email protected] Roll Flutter Engine from 3ecbe924a598 to 62a90a91cee3 (2 revisions) (flutter/flutter#136739)
2023-10-17 [email protected] Roll Flutter Engine from 0a4d8b99a95b to 3ecbe924a598 (3 revisions) (flutter/flutter#136732)
2023-10-17 [email protected] Reenable NexusLowRes API 29 (flutter/flutter#136686)
2023-10-17 [email protected] Reenable the nexus 6p tests (flutter/flutter#136689)
...
@Hixie
Copy link
Contributor

Hixie commented Apr 22, 2024

Per comment in #146764, this may have regressed correctness in DropdownMenus.

@Hixie
Copy link
Contributor

Hixie commented Apr 22, 2024

...for which there is a fix in #147057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: focus Focus traversal, gaining or losing focus f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MenuAnchor menus should use OverlayPortal
4 participants