Prevent dropdown menu's scroll offset from going negative#22235
Prevent dropdown menu's scroll offset from going negative#22235mklim merged 2 commits intoflutter:masterfrom mklim:dropdown_scroll
Conversation
|
@HansMuller I dug through the history a bit but didn't see the reasoning behind |
|
@mklim - the overall idea is to have the menu appear with the selected item lined up with (itself) the dropdown menu button's value. Forgetting scrolling for the moment: if the menu contains 4 items [A, B, C, D] the selected item is C, the dropdown looks like "Hello: C v", and when the menu pops up it looks like: As you can see, the top of the menu is negative relative to the top of the dropdown button. Things get more complicated when it's also necessary to scroll the selected item into view of course. LIkewise if the dropdown menu is near enough to an edge (edges) to prevent positioning it exactly where we want it. |
|
Thanks, makes sense. I'll add a test for that and rework this to make sure it still handles that case. |
|
Updated. |
HansMuller
left a comment
There was a problem hiding this comment.
This all looks good. I'd just like to get to the bottom of the attached assertions. Maybe we're trying to determine if tester.renderObject is turning up scrollable child that's no longer visible? Seems like that should be checked with expect, not assert(). And not by checking the attached property, but by checking that it's within the scrollable's viewport.
There was a problem hiding this comment.
This is OK, but it seems like an odd way to say:
math.max(0.0, selectedItemOffset - (buttonTop - menuTop))
There was a problem hiding this comment.
We typically only use => for one-liners that return a value.
Why not just put the buildFrame() call in-line?
There was a problem hiding this comment.
tester.renderObject<RenderBox>(...)
There was a problem hiding this comment.
await tester.pumpAndSettle()
There was a problem hiding this comment.
As before. Typically we don't add a level of indirection like this.
There was a problem hiding this comment.
Why are we asserting attached here and elsewhere? I don't believe WidgetTester.renderObject will succeed for an unattached renderer.
... I see, these assertions appear through this file, but not anywhere else (in fact I appear to responsible for a few of them).
There was a problem hiding this comment.
await tester.pumpAndSettle()
In long lists this resulted in the dropdown scrolling to the very last item in its list. Now clamping the value at `0.0`. Added a test to verify that the selected item aligns with the button to test the offset. Fixes #15346
|
Thanks for the review! This patch looks much cleaner now.
They definitely weren't necessary for the test cases I added, I was just following the pattern I saw in other cases in this file. I assumed that it was common or best practice to assert that elements exist before acting on them since I saw it in other test cases. For my cases the last |
HansMuller
left a comment
There was a problem hiding this comment.
LGTM
Eventually we should get to the bottom of the attached assertions in the rest of dropdown_test.dart. This PR looks good without them :-).
|
@fkorotkov, why is there still one GitHub check status that says it hasn't completed, when Cirrus says it has? It looks like GitHub is listing |
|
@gspencergoog there are two of them for the same task! One passed and one reported as still queued Looking into it. |
|
@fkorotkov Thanks! I just recently tried restarting the task, if that makes a difference. The new run from that is https://cirrus-ci.com/task/5629948521873408. |
| if (preferredMenuHeight > maxMenuHeight) | ||
| scrollOffset = selectedItemOffset - (buttonTop - menuTop); | ||
| final double scrollOffset = (preferredMenuHeight > maxMenuHeight) ? | ||
| math.max(0.0, selectedItemOffset - (buttonTop - menuTop)) : 0.0; |
There was a problem hiding this comment.
what i find weird about this approach is that it's asymmetrical. Why do we clamp at zero but not at some equivalent large offset?
There was a problem hiding this comment.
I never saw a situation where selectedItemOffset - (buttonTop - menuTop) produced values that were incorrectly large, so I didn't see a need for it.
In my testing it looked like this did correctly calculate an offset for all items after the first one. It perfectly aligned them with the dropdown button itself if there was room to scroll to them. But it was consistently setting a negative value for when either nothing or the first item in a list was selected. In short lists that doesn't matter since there's no room to scroll, but as soon as there was a scrollbar then it was wrapping around to the bottom of the list to match the negative scrollOffset.
@jslavitz's fix in #22594 looks more robust to me. If the problem is with the variables in this calculation, updating that makes more sense than clamping off known incorrect bounds.
|
Hey! I've got a fix here: #22594 that I think makes sense. |
Before the offset was adjusted by
- (buttonTop - menuTop), which meantthat with no item selected the offset would actually be a negative
number. In long lists that would result in the the dropdown scrolling to
the very last item in the list. Changed to be the
selectedItemOffsetdirectly instead, since the extra margin seemed unnecessary.
Fixes #15346