Unverified Commit 8ec4c59e authored by Greg Spencer's avatar Greg Spencer Committed by GitHub

Fix `DropdownMenu` throwing `TextEditingController` disposed error on select (#139385)

## Description

When a DropdownMenu exists within another menu and a MenuItem is selected, the `TextEditingController` of the DropdownMenu is used after it has been disposed.

This PR manages the local `TextController` instance better, making sure that the one used when building is the current one, and handling the case where the `controller` is set on the widget after initial creation.

Also, places where we were setting the text and selection separately were converted to use `TextEditingValue` and set the value atomically.

## Related Issues
 - Fixes https://github.com/flutter/flutter/issues/139266

## Tests
 - Added tests (Created by @josh-burton  in https://github.com/flutter/flutter/pull/139268 - Thanks Josh!)
parent 79d6781b
...@@ -348,18 +348,20 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -348,18 +348,20 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
final GlobalKey _leadingKey = GlobalKey(); final GlobalKey _leadingKey = GlobalKey();
late List<GlobalKey> buttonItemKeys; late List<GlobalKey> buttonItemKeys;
final MenuController _controller = MenuController(); final MenuController _controller = MenuController();
late final TextEditingController _textEditingController;
late bool _enableFilter; late bool _enableFilter;
late List<DropdownMenuEntry<T>> filteredEntries; late List<DropdownMenuEntry<T>> filteredEntries;
List<Widget>? _initialMenu; List<Widget>? _initialMenu;
int? currentHighlight; int? currentHighlight;
double? leadingPadding; double? leadingPadding;
bool _menuHasEnabledItem = false; bool _menuHasEnabledItem = false;
TextEditingController? _localTextEditingController;
TextEditingController get _textEditingController {
return widget.controller ?? (_localTextEditingController ??= TextEditingController());
}
@override @override
void initState() { void initState() {
super.initState(); super.initState();
_textEditingController = widget.controller ?? TextEditingController();
_enableFilter = widget.enableFilter; _enableFilter = widget.enableFilter;
filteredEntries = widget.dropdownMenuEntries; filteredEntries = widget.dropdownMenuEntries;
buttonItemKeys = List<GlobalKey>.generate(filteredEntries.length, (int index) => GlobalKey()); buttonItemKeys = List<GlobalKey>.generate(filteredEntries.length, (int index) => GlobalKey());
...@@ -367,16 +369,33 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -367,16 +369,33 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
final int index = filteredEntries.indexWhere((DropdownMenuEntry<T> entry) => entry.value == widget.initialSelection); final int index = filteredEntries.indexWhere((DropdownMenuEntry<T> entry) => entry.value == widget.initialSelection);
if (index != -1) { if (index != -1) {
_textEditingController.text = filteredEntries[index].label; _textEditingController.value = TextEditingValue(
_textEditingController.selection = text: filteredEntries[index].label,
TextSelection.collapsed(offset: _textEditingController.text.length); selection: TextSelection.collapsed(offset: filteredEntries[index].label.length),
);
} }
refreshLeadingPadding(); refreshLeadingPadding();
} }
@override
void dispose() {
if (_localTextEditingController != null) {
debugPrint('Disposing of $_textEditingController');
}
_localTextEditingController?.dispose();
_localTextEditingController = null;
super.dispose();
}
@override @override
void didUpdateWidget(DropdownMenu<T> oldWidget) { void didUpdateWidget(DropdownMenu<T> oldWidget) {
super.didUpdateWidget(oldWidget); super.didUpdateWidget(oldWidget);
if (oldWidget.controller != widget.controller) {
if (widget.controller != null) {
_localTextEditingController?.dispose();
_localTextEditingController = null;
}
}
if (oldWidget.enableSearch != widget.enableSearch) { if (oldWidget.enableSearch != widget.enableSearch) {
if (!widget.enableSearch) { if (!widget.enableSearch) {
currentHighlight = null; currentHighlight = null;
...@@ -394,9 +413,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -394,9 +413,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
if (oldWidget.initialSelection != widget.initialSelection) { if (oldWidget.initialSelection != widget.initialSelection) {
final int index = filteredEntries.indexWhere((DropdownMenuEntry<T> entry) => entry.value == widget.initialSelection); final int index = filteredEntries.indexWhere((DropdownMenuEntry<T> entry) => entry.value == widget.initialSelection);
if (index != -1) { if (index != -1) {
_textEditingController.text = filteredEntries[index].label; _textEditingController.value = TextEditingValue(
_textEditingController.selection = text: filteredEntries[index].label,
TextSelection.collapsed(offset: _textEditingController.text.length); selection: TextSelection.collapsed(offset: filteredEntries[index].label.length),
);
} }
} }
} }
...@@ -463,7 +483,6 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -463,7 +483,6 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
List<Widget> _buildButtons( List<Widget> _buildButtons(
List<DropdownMenuEntry<T>> filteredEntries, List<DropdownMenuEntry<T>> filteredEntries,
TextEditingController textEditingController,
TextDirection textDirection, TextDirection textDirection,
{ int? focusedIndex, bool enableScrollToHighlight = true} { int? focusedIndex, bool enableScrollToHighlight = true}
) { ) {
...@@ -519,9 +538,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -519,9 +538,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
trailingIcon: entry.trailingIcon, trailingIcon: entry.trailingIcon,
onPressed: entry.enabled onPressed: entry.enabled
? () { ? () {
textEditingController.text = entry.label; _textEditingController.value = TextEditingValue(
textEditingController.selection = text: entry.label,
TextSelection.collapsed(offset: textEditingController.text.length); selection: TextSelection.collapsed(offset: entry.label.length),
);
currentHighlight = widget.enableSearch ? i : null; currentHighlight = widget.enableSearch ? i : null;
widget.onSelected?.call(entry.value); widget.onSelected?.call(entry.value);
} }
...@@ -535,37 +555,43 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -535,37 +555,43 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
return result; return result;
} }
void handleUpKeyInvoke(_) => setState(() { void handleUpKeyInvoke(_) {
if (!_menuHasEnabledItem || !_controller.isOpen) { setState(() {
return; if (!_menuHasEnabledItem || !_controller.isOpen) {
} return;
_enableFilter = false; }
currentHighlight ??= 0; _enableFilter = false;
currentHighlight = (currentHighlight! - 1) % filteredEntries.length; currentHighlight ??= 0;
while (!filteredEntries[currentHighlight!].enabled) {
currentHighlight = (currentHighlight! - 1) % filteredEntries.length; currentHighlight = (currentHighlight! - 1) % filteredEntries.length;
} while (!filteredEntries[currentHighlight!].enabled) {
final String currentLabel = filteredEntries[currentHighlight!].label; currentHighlight = (currentHighlight! - 1) % filteredEntries.length;
_textEditingController.text = currentLabel; }
_textEditingController.selection = final String currentLabel = filteredEntries[currentHighlight!].label;
TextSelection.collapsed(offset: _textEditingController.text.length); _textEditingController.value = TextEditingValue(
}); text: currentLabel,
selection: TextSelection.collapsed(offset: currentLabel.length),
);
});
}
void handleDownKeyInvoke(_) => setState(() { void handleDownKeyInvoke(_) {
if (!_menuHasEnabledItem || !_controller.isOpen) { setState(() {
return; if (!_menuHasEnabledItem || !_controller.isOpen) {
} return;
_enableFilter = false; }
currentHighlight ??= -1; _enableFilter = false;
currentHighlight = (currentHighlight! + 1) % filteredEntries.length; currentHighlight ??= -1;
while (!filteredEntries[currentHighlight!].enabled) {
currentHighlight = (currentHighlight! + 1) % filteredEntries.length; currentHighlight = (currentHighlight! + 1) % filteredEntries.length;
} while (!filteredEntries[currentHighlight!].enabled) {
final String currentLabel = filteredEntries[currentHighlight!].label; currentHighlight = (currentHighlight! + 1) % filteredEntries.length;
_textEditingController.text = currentLabel; }
_textEditingController.selection = final String currentLabel = filteredEntries[currentHighlight!].label;
TextSelection.collapsed(offset: _textEditingController.text.length); _textEditingController.value = TextEditingValue(
}); text: currentLabel,
selection: TextSelection.collapsed(offset: currentLabel.length),
);
});
}
void handlePressed(MenuController controller) { void handlePressed(MenuController controller) {
if (controller.isOpen) { if (controller.isOpen) {
...@@ -580,18 +606,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -580,18 +606,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
setState(() {}); setState(() {});
} }
@override
void dispose() {
if (widget.controller == null) {
_textEditingController.dispose();
}
super.dispose();
}
@override @override
Widget build(BuildContext context) { Widget build(BuildContext context) {
final TextDirection textDirection = Directionality.of(context); final TextDirection textDirection = Directionality.of(context);
_initialMenu ??= _buildButtons(widget.dropdownMenuEntries, _textEditingController, textDirection, enableScrollToHighlight: false); _initialMenu ??= _buildButtons(widget.dropdownMenuEntries, textDirection, enableScrollToHighlight: false);
final DropdownMenuThemeData theme = DropdownMenuTheme.of(context); final DropdownMenuThemeData theme = DropdownMenuTheme.of(context);
final DropdownMenuThemeData defaults = _DropdownMenuDefaultsM3(context); final DropdownMenuThemeData defaults = _DropdownMenuDefaultsM3(context);
...@@ -610,7 +628,7 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -610,7 +628,7 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
} }
} }
final List<Widget> menu = _buildButtons(filteredEntries, _textEditingController, textDirection, focusedIndex: currentHighlight); final List<Widget> menu = _buildButtons(filteredEntries, textDirection, focusedIndex: currentHighlight);
final TextStyle? effectiveTextStyle = widget.textStyle ?? theme.textStyle ?? defaults.textStyle; final TextStyle? effectiveTextStyle = widget.textStyle ?? theme.textStyle ?? defaults.textStyle;
...@@ -670,9 +688,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> { ...@@ -670,9 +688,10 @@ class _DropdownMenuState<T> extends State<DropdownMenu<T>> {
if (currentHighlight != null) { if (currentHighlight != null) {
final DropdownMenuEntry<T> entry = filteredEntries[currentHighlight!]; final DropdownMenuEntry<T> entry = filteredEntries[currentHighlight!];
if (entry.enabled) { if (entry.enabled) {
_textEditingController.text = entry.label; _textEditingController.value = TextEditingValue(
_textEditingController.selection = text: entry.label,
TextSelection.collapsed(offset: _textEditingController.text.length); selection: TextSelection.collapsed(offset: entry.label.length),
);
widget.onSelected?.call(entry.value); widget.onSelected?.call(entry.value);
} }
} else { } else {
......
...@@ -1805,6 +1805,109 @@ void main() { ...@@ -1805,6 +1805,109 @@ void main() {
await tester.pump(); await tester.pump();
checkExpectedHighlight(searchResult: 'Read', otherItems: <String>['All', 'Unread']); checkExpectedHighlight(searchResult: 'Read', otherItems: <String>['All', 'Unread']);
}); });
testWidgetsWithLeakTracking('onSelected gets called when a selection is made in a nested menu',
(WidgetTester tester) async {
int selectionCount = 0;
final ThemeData themeData = ThemeData();
final List<DropdownMenuEntry<TestMenu>> menuWithDisabledItems = <DropdownMenuEntry<TestMenu>>[
const DropdownMenuEntry<TestMenu>(value: TestMenu.mainMenu0, label: 'Item 0'),
];
await tester.pumpWidget(MaterialApp(
theme: themeData,
home: StatefulBuilder(builder: (BuildContext context, StateSetter setState) {
return Scaffold(
body: MenuAnchor(
menuChildren: <Widget>[
DropdownMenu<TestMenu>(
dropdownMenuEntries: menuWithDisabledItems,
onSelected: (_) {
setState(() {
selectionCount++;
});
},
),
],
builder: (BuildContext context, MenuController controller, Widget? widget) {
return IconButton(
icon: const Icon(Icons.smartphone_rounded),
onPressed: () {
controller.open();
},
);
},
),
);
}),
));
// Open the first menu
await tester.tap(find.byType(IconButton));
await tester.pump();
// Open the dropdown menu
await tester.tap(find.byType(DropdownMenu<TestMenu>));
await tester.pump();
final Finder item1 = find.widgetWithText(MenuItemButton, 'Item 0').last;
await tester.tap(item1);
await tester.pumpAndSettle();
expect(selectionCount, 1);
});
testWidgetsWithLeakTracking('When onSelected is called and menu is closed, no textEditingController exception is thrown',
(WidgetTester tester) async {
int selectionCount = 0;
final ThemeData themeData = ThemeData();
final List<DropdownMenuEntry<TestMenu>> menuWithDisabledItems = <DropdownMenuEntry<TestMenu>>[
const DropdownMenuEntry<TestMenu>(value: TestMenu.mainMenu0, label: 'Item 0'),
];
await tester.pumpWidget(MaterialApp(
theme: themeData,
home: StatefulBuilder(builder: (BuildContext context, StateSetter setState) {
return Scaffold(
body: MenuAnchor(
menuChildren: <Widget>[
DropdownMenu<TestMenu>(
dropdownMenuEntries: menuWithDisabledItems,
onSelected: (_) {
setState(() {
selectionCount++;
});
},
),
],
builder: (BuildContext context, MenuController controller, Widget? widget) {
return IconButton(
icon: const Icon(Icons.smartphone_rounded),
onPressed: () {
controller.open();
},
);
},
),
);
}),
));
// Open the first menu
await tester.tap(find.byType(IconButton));
await tester.pump();
// Open the dropdown menu
await tester.tap(find.byType(DropdownMenu<TestMenu>));
await tester.pump();
final Finder item1 = find.widgetWithText(MenuItemButton, 'Item 0').last;
await tester.tap(item1);
await tester.pumpAndSettle();
expect(selectionCount, 1);
expect(tester.takeException(), isNull);
});
} }
enum TestMenu { enum TestMenu {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment