From f04a5afb58b758b3e87485d12eebef1eb2ef6d48 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 18 Apr 2023 12:50:12 -0700 Subject: [PATCH] Limit the number of Material spell check suggestions to 3 (#124899) Fixes a bug where the spell check menu could overflow. --- .../spell_check_suggestions_toolbar.dart | 15 ++-- .../spell_check_suggestions_toolbar.dart | 68 ++++++++----------- .../spell_check_suggestions_toolbar_test.dart | 62 ++++++++++++++++- .../spell_check_suggestions_toolbar_test.dart | 57 +++++++++++++++- 4 files changed, 147 insertions(+), 55 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart b/packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart index 5a90459a347..436dd2b39de 100644 --- a/packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart +++ b/packages/flutter/lib/src/cupertino/spell_check_suggestions_toolbar.dart @@ -12,7 +12,7 @@ import 'text_selection_toolbar.dart'; import 'text_selection_toolbar_button.dart'; /// iOS only shows 3 spell check suggestions in the toolbar. -const int _maxSuggestions = 3; +const int _kMaxSuggestions = 3; /// The default spell check suggestions toolbar for iOS. /// @@ -20,11 +20,13 @@ const int _maxSuggestions = 3; /// readjusts to fit above bottom view insets. class CupertinoSpellCheckSuggestionsToolbar extends StatelessWidget { /// Constructs a [CupertinoSpellCheckSuggestionsToolbar]. + /// + /// [buttonItems] must not contain more than three items. const CupertinoSpellCheckSuggestionsToolbar({ super.key, required this.anchors, required this.buttonItems, - }); + }) : assert(buttonItems.length <= _kMaxSuggestions); /// The location on which to anchor the menu. final TextSelectionToolbarAnchors anchors; @@ -32,6 +34,8 @@ class CupertinoSpellCheckSuggestionsToolbar extends StatelessWidget { /// The [ContextMenuButtonItem]s that will be turned into the correct button /// widgets and displayed in the spell check suggestions toolbar. /// + /// Must not contain more than three items. + /// /// See also: /// /// * [AdaptiveTextSelectionToolbar.buttonItems], the list of @@ -71,11 +75,7 @@ class CupertinoSpellCheckSuggestionsToolbar extends StatelessWidget { final List buttonItems = []; // Build suggestion buttons. - int suggestionCount = 0; - for (final String suggestion in spanAtCursorIndex.suggestions) { - if (suggestionCount >= _maxSuggestions) { - break; - } + for (final String suggestion in spanAtCursorIndex.suggestions.take(_kMaxSuggestions)) { buttonItems.add(ContextMenuButtonItem( onPressed: () { if (!editableTextState.mounted) { @@ -89,7 +89,6 @@ class CupertinoSpellCheckSuggestionsToolbar extends StatelessWidget { }, label: suggestion, )); - suggestionCount += 1; } return buttonItems; } diff --git a/packages/flutter/lib/src/material/spell_check_suggestions_toolbar.dart b/packages/flutter/lib/src/material/spell_check_suggestions_toolbar.dart index 74fddcb522c..6ace839c990 100644 --- a/packages/flutter/lib/src/material/spell_check_suggestions_toolbar.dart +++ b/packages/flutter/lib/src/material/spell_check_suggestions_toolbar.dart @@ -18,17 +18,23 @@ import 'text_selection_toolbar_text_button.dart'; // Size eyeballed on Pixel 4 emulator running Android API 31. const double _kDefaultToolbarHeight = 193.0; +/// The maximum number of suggestions in the toolbar is 3, plus a delete button. +const int _kMaxSuggestions = 3; + /// The default spell check suggestions toolbar for Android. /// /// Tries to position itself below the [anchor], but if it doesn't fit, then it /// readjusts to fit above bottom view insets. class SpellCheckSuggestionsToolbar extends StatelessWidget { /// Constructs a [SpellCheckSuggestionsToolbar]. + /// + /// [buttonItems] must not contain more than four items, generally three + /// suggestions and one delete button. const SpellCheckSuggestionsToolbar({ super.key, required this.anchor, required this.buttonItems, - }); + }) : assert(buttonItems.length <= _kMaxSuggestions + 1); /// {@template flutter.material.SpellCheckSuggestionsToolbar.anchor} /// The focal point below which the toolbar attempts to position itself. @@ -38,6 +44,9 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget { /// The [ContextMenuButtonItem]s that will be turned into the correct button /// widgets and displayed in the spell check suggestions toolbar. /// + /// Must not contain more than four items, typically three suggestions and a + /// delete button. + /// /// See also: /// /// * [AdaptiveTextSelectionToolbar.buttonItems], the list of @@ -52,13 +61,6 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget { /// running Android API 31. static const double kToolbarContentDistanceBelow = TextSelectionToolbar.kHandleSize - 3.0; - /// Builds the default Android Material spell check suggestions toolbar. - static Widget _spellCheckSuggestionsToolbarBuilder(BuildContext context, Widget child) { - return _SpellCheckSuggestionsToolbarContainer( - child: child, - ); - } - /// Builds the button items for the toolbar based on the available /// spell check suggestions. static List? buildButtonItems( @@ -77,7 +79,7 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget { final List buttonItems = []; // Build suggestion buttons. - for (final String suggestion in spanAtCursorIndex.suggestions) { + for (final String suggestion in spanAtCursorIndex.suggestions.take(_kMaxSuggestions)) { buttonItems.add(ContextMenuButtonItem( onPressed: () { if (!editableTextState.mounted) { @@ -190,10 +192,10 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget { // This duration was eyeballed on a Pixel 2 emulator running Android // API 28 for the Material TextSelectionToolbar. duration: const Duration(milliseconds: 140), - child: _spellCheckSuggestionsToolbarBuilder(context, _SpellCheckSuggestsionsToolbarItemsLayout( + child: _SpellCheckSuggestionsToolbarContainer( height: spellCheckSuggestionsToolbarHeight, children: [..._buildToolbarButtons(context)], - )), + ), ), ), ); @@ -204,10 +206,12 @@ class SpellCheckSuggestionsToolbar extends StatelessWidget { /// toolbar. class _SpellCheckSuggestionsToolbarContainer extends StatelessWidget { const _SpellCheckSuggestionsToolbarContainer({ - required this.child, + required this.height, + required this.children, }); - final Widget child; + final double height; + final List children; @override Widget build(BuildContext context) { @@ -216,34 +220,16 @@ class _SpellCheckSuggestionsToolbarContainer extends StatelessWidget { // API 31 for the SpellCheckSuggestionsToolbar. elevation: 2.0, type: MaterialType.card, - child: child, - ); - } -} - -/// Renders the spell check suggestions toolbar items in the correct positions -/// in the menu. -class _SpellCheckSuggestsionsToolbarItemsLayout extends StatelessWidget { - const _SpellCheckSuggestsionsToolbarItemsLayout({ - required this.height, - required this.children, - }); - - final double height; - - final List children; - - @override - Widget build(BuildContext context) { - return SizedBox( - // This width was eyeballed on a Pixel 4 emulator running Android - // API 31 for the SpellCheckSuggestionsToolbar. - width: 165, - height: height, - child: Column( - mainAxisSize: MainAxisSize.min, - crossAxisAlignment: CrossAxisAlignment.stretch, - children: children, + child: SizedBox( + // This width was eyeballed on a Pixel 4 emulator running Android + // API 31 for the SpellCheckSuggestionsToolbar. + width: 165.0, + height: height, + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: children, + ), ), ); } diff --git a/packages/flutter/test/cupertino/spell_check_suggestions_toolbar_test.dart b/packages/flutter/test/cupertino/spell_check_suggestions_toolbar_test.dart index 55f569330ce..0ea80dae23a 100644 --- a/packages/flutter/test/cupertino/spell_check_suggestions_toolbar_test.dart +++ b/packages/flutter/test/cupertino/spell_check_suggestions_toolbar_test.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:flutter/cupertino.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -10,6 +11,56 @@ import 'package:flutter_test/flutter_test.dart'; void main() { TestWidgetsFlutterBinding.ensureInitialized(); + testWidgets('more than three suggestions throws an error', (WidgetTester tester) async { + Future pumpToolbar(List suggestions) async { + await tester.pumpWidget( + CupertinoApp( + home: Center( + child: CupertinoSpellCheckSuggestionsToolbar( + anchors: const TextSelectionToolbarAnchors( + primaryAnchor: Offset.zero, + ), + buttonItems: suggestions.map((String string) { + return ContextMenuButtonItem( + onPressed: () {}, + label: string, + ); + }).toList(), + ), + ), + ), + ); + } + await pumpToolbar(['hello', 'yellow', 'yell']); + expect(() async { + await pumpToolbar(['hello', 'yellow', 'yell', 'yeller']); + }, throwsAssertionError); + }, + skip: kIsWeb, // [intended] + ); + + test('buildSuggestionButtons only considers the first three suggestions', () { + final _FakeEditableTextState editableTextState = _FakeEditableTextState( + suggestions: [ + 'hello', + 'yellow', + 'yell', + 'yeller', + ], + ); + final List? buttonItems = + CupertinoSpellCheckSuggestionsToolbar.buildButtonItems(editableTextState); + expect(buttonItems, isNotNull); + final Iterable labels = buttonItems!.map((ContextMenuButtonItem buttonItem) { + return buttonItem.label; + }); + expect(labels, hasLength(3)); + expect(labels, contains('hello')); + expect(labels, contains('yellow')); + expect(labels, contains('yell')); + expect(labels, isNot(contains('yeller'))); + }); + testWidgets('buildButtonItems builds a "No Replacements Found" button when no suggestions', (WidgetTester tester) async { await tester.pumpWidget( CupertinoApp( @@ -41,17 +92,22 @@ class _FakeEditableText extends EditableText { } class _FakeEditableTextState extends EditableTextState { + _FakeEditableTextState({ + this.suggestions, + }); + + final List? suggestions; @override TextEditingValue get currentTextEditingValue => TextEditingValue.empty; @override SuggestionSpan? findSuggestionSpanAtCursorIndex(int cursorIndex) { - return const SuggestionSpan( - TextRange( + return SuggestionSpan( + const TextRange( start: 0, end: 0, ), - [], + suggestions ?? [], ); } } diff --git a/packages/flutter/test/material/spell_check_suggestions_toolbar_test.dart b/packages/flutter/test/material/spell_check_suggestions_toolbar_test.dart index 78d1569baeb..b4517f9192a 100644 --- a/packages/flutter/test/material/spell_check_suggestions_toolbar_test.dart +++ b/packages/flutter/test/material/spell_check_suggestions_toolbar_test.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:flutter/cupertino.dart' show CupertinoTextSelectionToolbar; +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -87,6 +88,50 @@ void main() { expect(toolbarY, equals(expectedToolbarY)); }); + testWidgets('more than three suggestions throws an error', (WidgetTester tester) async { + Future pumpToolbar(List suggestions) async { + await tester.pumpWidget( + MaterialApp( + home: Scaffold( + body: SpellCheckSuggestionsToolbar( + anchor: const Offset(0.0, _kAnchor - _kTestToolbarOverlap), + buttonItems: buildSuggestionButtons(suggestions), + ), + ), + ), + ); + } + await pumpToolbar(['hello', 'yellow', 'yell']); + expect(() async { + await pumpToolbar(['hello', 'yellow', 'yell', 'yeller']); + }, throwsAssertionError); + }, + skip: kIsWeb, // [intended] + ); + + test('buildSuggestionButtons only considers the first three suggestions', () { + final _FakeEditableTextState editableTextState = _FakeEditableTextState( + suggestions: [ + 'hello', + 'yellow', + 'yell', + 'yeller', + ], + ); + final List? buttonItems = + SpellCheckSuggestionsToolbar.buildButtonItems(editableTextState); + expect(buttonItems, isNotNull); + final Iterable labels = buttonItems!.map((ContextMenuButtonItem buttonItem) { + return buttonItem.label; + }); + expect(labels, hasLength(4)); + expect(labels, contains('hello')); + expect(labels, contains('yellow')); + expect(labels, contains('yell')); + expect(labels, contains(null)); // For the delete button. + expect(labels, isNot(contains('yeller'))); + }); + test('buildButtonItems builds only a delete button when no suggestions', () { final _FakeEditableTextState editableTextState = _FakeEditableTextState(); final List? buttonItems = @@ -98,17 +143,23 @@ void main() { } class _FakeEditableTextState extends EditableTextState { + _FakeEditableTextState({ + this.suggestions, + }); + + final List? suggestions; + @override TextEditingValue get currentTextEditingValue => TextEditingValue.empty; @override SuggestionSpan? findSuggestionSpanAtCursorIndex(int cursorIndex) { - return const SuggestionSpan( - TextRange( + return SuggestionSpan( + const TextRange( start: 0, end: 0, ), - [], + suggestions ?? [], ); } }