Large Dropdown Menu Fix #22594
Large Dropdown Menu Fix #22594
Conversation
Test scroll position when dropdown placed not at the top of screen
showButtonMenu with offset
…/master merged
| @@ -335,19 +335,33 @@ class _DropdownRoute<T> extends PopupRoute<_DropdownRouteResult<T>> { | |||
| final double buttonTop = buttonRect.top; | |||
| final double selectedItemOffset = selectedIndex * _kMenuItemHeight + kMaterialListPadding.top; | |||
| double menuTop = (buttonTop - selectedItemOffset) - (_kMenuItemHeight - buttonRect.height) / 2.0; | |||
| final double originalMenuTop = menuTop; | |||
| const double topPreferredLimit = _kMenuItemHeight; | |||
| if (menuTop < topPreferredLimit) | |||
| menuTop = math.min(buttonTop, topPreferredLimit); | |||
There was a problem hiding this comment.
how about this case, does it affect things?
There was a problem hiding this comment.
It doesn't because the menuTop variable is just being changed to a slightly larger value that will never be bigger than buttonTop. (So [buttonTop - menuTop] will always be >= 0).
| int value = 0; | ||
| final List<DropdownMenuItem<int>> items = <DropdownMenuItem<int>>[]; | ||
| for (int i = 0; i < 222; ++i) | ||
| items.add(DropdownMenuItem<int>(value: i, child: Text('$i'))); |
There was a problem hiding this comment.
there's a constructor that will autogenerate a list like this
There was a problem hiding this comment.
Which looks like? :)
|
|
||
| await tester.tap(find.text('0')); | ||
| await tester.pump(); | ||
| await tester.pump(const Duration(seconds: 1)); |
There was a problem hiding this comment.
what should the scroll offset be at the start of the animation? should it be 0.0 then also, or does the menu start scrolled so that the items line up, and then fling back?
|
cc @mklim |
|
cc @HansMuller |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
I think the existing code is somewhat counter intuitive (using math.min to adjust the top of the menu and math.max to adjust the bottom for example) because Despite the fact that the menu's limits are potentially larger, maxMenuHeight should still be In other words we're OK for the top or bottom of the menu to be offscreen if the top or bottom of the dropdown button is overlaps the edge of the screen. I think the scrollOffset should always be math.max(0.0, selectedItemOffset - (buttonTop - menuTop)). That accounts for the fact that the selected item can't always be scrolled so that it lines up with the dropdown button. |
| // If there are too many elements in the menu, we need to shrink it down | ||
| // so it is at most the maxMenuHeight. | ||
| final double menuHeight = math.min(maxMenuHeight, preferredMenuHeight); | ||
|
|
||
| double bottom = menuTop + menuHeight; |
There was a problem hiding this comment.
For the sake of consistency, you might as well call bottom menuBottom.
| if (menuTop < topLimit) | ||
| menuTop = math.min(buttonTop, topLimit); | ||
| if (bottom > bottomLimit) { | ||
| bottom = math.max(buttonTop + _kMenuItemHeight, bottomLimit); |
There was a problem hiding this comment.
buttonTop + _kMenuItemHeight should be buttonBottom
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
Fixed #20333