fix DropdownMenu menu vertical position #157496
Conversation
4fa5fd5 to
f41296c
Compare
| child: menuAnchor, | ||
| ); | ||
| // Put the menu anchor into a Column with CrossAxisAlignment.stretch | ||
| // to loose just the height constraints. Doing so, the menu anchor |
| ); | ||
| // Put the menu anchor into a Column with CrossAxisAlignment.stretch | ||
| // to loose just the height constraints. Doing so, the menu anchor | ||
| // adjusts to the inner text field intrinsic heigth even if it received |
There was a problem hiding this comment.
| // adjusts to the inner text field intrinsic heigth even if it received | |
| // adjusts to the inner text field intrinsic height even if it received |
|
It looks like there are several legit Google test failures. I was expecting maybe some visual diff tests where the menu items have moved or changed size, but I don't see any like that. Instead I see stuff like a tap happening on a dropdown item, then a pumpAndSettle, but now the wrong item is selected or the selection didn't change. Any thoughts on that?? Sorry for the awkward second hand description here. |
|
Taking a look. |
| // The first one is the real button item in the menu. | ||
| // The second one is not visible, it is part of _DropdownMenuBody | ||
| // which is used to compute the dropdown width. | ||
| return find.widgetWithText(MenuItemButton, label).first; |
There was a problem hiding this comment.
We should not use .first to get the menu item because we layout the items for the first time to get each items' size but we don't paint them. The .last should be the "real" buttons that show in the menu.
Oh sorry, I didn't see the comment above. Why do we change the order? I'm a little confused. Which part of change make the order get changed?
There was a problem hiding this comment.
The order change is a result of the new tree structure.
Before this PR _DropdownMenuBody was inside the MenuAnchor, with this PR MenuAnchor is inside _DropdownMenuBody.
Having two occurrences of each widget is somewhat confusing, especially for the testing story, whether the real widget is first or second, I don’t know if there is a way to avoid this.
Do you see any reason (other than not breaking existing tests) why the real item should be the second one and not the first one? In both cases, it seems arbitrary to me?
Nonetheless, If it breaks to many existing tests and it is difficult to update them, we can try to tweak the order, not sure if it is possible.
Before going further, I will investigate if the issue can be fix at MenuAnchor level according to #157496 (comment)
|
Should we fix this problem in import 'package:flutter/material.dart';
void main() => runApp(const MyApp());
class MyApp extends StatelessWidget {
const MyApp({super.key});
@override
Widget build(BuildContext context) {
return MaterialApp(
title: 'Flutter Demo',
home: Container(
alignment: Alignment.center,
color: Colors.amber,
child: Scaffold(
body: SizedBox(
height: 500,
width: 500,
child: ColoredBox(
color: Colors.amber,
child: MenuAnchor(
builder: (context, controller, child) {
return TextField(
decoration: InputDecoration(
border: OutlineInputBorder(),
),
onTap: () {
if (controller.isOpen) {
controller.close();
} else {
controller.open();
}
}
);
},
menuChildren: [
MenuItemButton(
child: Text('This is a very long label',),
)
],
),
// child: DropdownMenu(
// // expandedInsets: EdgeInsets.zero,
// dropdownMenuEntries: [
// DropdownMenuEntry(
// value: '',
// label: 'This is a very long label',
// ),
// ],
// ),
),
),
),
),
);
}
}
|
71549db to
0e9cac3
Compare
Very good point, I don’t know if this can be fixed at |
## Description
This PR introduces some utility functions to simplify some DropdownMenu tests.
The main purpose is to centralize and document how tests should find menu items, because it is tricky as there are two occurrences of each widgets and using '.last' is mandatory:
```dart
Finder findMenuItemButton(String label) {
// For each menu items there are two MenuItemButton widgets.
// The last one is the real button item in the menu.
// The first one is not visible, it is part of _DropdownMenuBody
// which is used to compute the dropdown width.
return find.widgetWithText(MenuItemButton, label).last;
}
```
## Related Issue
This is extracted from #157496.
## Tests
Refactors many existing tests.
|
Closing as I will try a simpler approach. |
Description
This PR fixes
DropdownMenumenu vertical position when tight constraints are given to theDropdownMenu.Before:
After:
Related Issue
Fixes the minHeight part of DropdownMenu does not correctly handle incoming maxWidth and minHeight constraints.
(The maxWidth part was already fixed in #147233).
Tests
Adds 4 tests.
Refactors many existing tests.