From 804a7b285f6d5ce2f4f3b70ea1ee81ff2744cd7a Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Fri, 8 Sep 2023 09:52:57 -0700 Subject: [PATCH] Make `CupertinoTextField` at least as tall as its first line of placeholder (#134198) Fixes https://github.com/flutter/flutter/issues/133241 and some CupertinoTextField cleanup. --- .../lib/src/cupertino/search_field.dart | 2 +- .../flutter/lib/src/cupertino/text_field.dart | 173 ++++++++++-------- .../src/cupertino/text_form_field_row.dart | 2 +- packages/flutter/lib/src/rendering/stack.dart | 14 +- .../test/cupertino/text_field_test.dart | 66 ++++++- 5 files changed, 161 insertions(+), 96 deletions(-) diff --git a/packages/flutter/lib/src/cupertino/search_field.dart b/packages/flutter/lib/src/cupertino/search_field.dart index 0748feb13ff..a5ffc2c5c49 100644 --- a/packages/flutter/lib/src/cupertino/search_field.dart +++ b/packages/flutter/lib/src/cupertino/search_field.dart @@ -444,7 +444,7 @@ class _CupertinoSearchTextFieldState extends State suffix: suffix, keyboardType: widget.keyboardType, onTap: widget.onTap, - enabled: widget.enabled, + enabled: widget.enabled ?? true, suffixMode: widget.suffixMode, placeholder: placeholder, placeholderStyle: placeholderStyle, diff --git a/packages/flutter/lib/src/cupertino/text_field.dart b/packages/flutter/lib/src/cupertino/text_field.dart index 4c4b940fcef..bda1ad6aecc 100644 --- a/packages/flutter/lib/src/cupertino/text_field.dart +++ b/packages/flutter/lib/src/cupertino/text_field.dart @@ -261,7 +261,7 @@ class CupertinoTextField extends StatefulWidget { this.onSubmitted, this.onTapOutside, this.inputFormatters, - this.enabled, + this.enabled = true, this.cursorWidth = 2.0, this.cursorHeight, this.cursorRadius = const Radius.circular(2.0), @@ -393,7 +393,7 @@ class CupertinoTextField extends StatefulWidget { this.onSubmitted, this.onTapOutside, this.inputFormatters, - this.enabled, + this.enabled = true, this.cursorWidth = 2.0, this.cursorHeight, this.cursorRadius = const Radius.circular(2.0), @@ -653,7 +653,9 @@ class CupertinoTextField extends StatefulWidget { /// Text fields in disabled states have a light grey background and don't /// respond to touch events including the [prefix], [suffix] and the clear /// button. - final bool? enabled; + /// + /// Defaults to true. + final bool enabled; /// {@macro flutter.widgets.editableText.cursorWidth} final double cursorWidth; @@ -946,7 +948,7 @@ class _CupertinoTextFieldState extends State with Restoratio if (widget.controller == null) { _createLocalController(); } - _effectiveFocusNode.canRequestFocus = widget.enabled ?? true; + _effectiveFocusNode.canRequestFocus = widget.enabled; _effectiveFocusNode.addListener(_handleFocusChanged); } @@ -965,7 +967,7 @@ class _CupertinoTextFieldState extends State with Restoratio (oldWidget.focusNode ?? _focusNode)?.removeListener(_handleFocusChanged); (widget.focusNode ?? _focusNode)?.addListener(_handleFocusChanged); } - _effectiveFocusNode.canRequestFocus = widget.enabled ?? true; + _effectiveFocusNode.canRequestFocus = widget.enabled; } @override @@ -1079,41 +1081,16 @@ class _CupertinoTextFieldState extends State with Restoratio @override bool get wantKeepAlive => _controller?.value.text.isNotEmpty ?? false; - bool _shouldShowAttachment({ + static bool _shouldShowAttachment({ required OverlayVisibilityMode attachment, required bool hasText, }) { - switch (attachment) { - case OverlayVisibilityMode.never: - return false; - case OverlayVisibilityMode.always: - return true; - case OverlayVisibilityMode.editing: - return hasText; - case OverlayVisibilityMode.notEditing: - return !hasText; - } - } - - bool _showPrefixWidget(TextEditingValue text) { - return widget.prefix != null && _shouldShowAttachment( - attachment: widget.prefixMode, - hasText: text.text.isNotEmpty, - ); - } - - bool _showSuffixWidget(TextEditingValue text) { - return widget.suffix != null && _shouldShowAttachment( - attachment: widget.suffixMode, - hasText: text.text.isNotEmpty, - ); - } - - bool _showClearButton(TextEditingValue text) { - return _shouldShowAttachment( - attachment: widget.clearButtonMode, - hasText: text.text.isNotEmpty, - ); + return switch (attachment) { + OverlayVisibilityMode.never => false, + OverlayVisibilityMode.always => true, + OverlayVisibilityMode.editing => hasText, + OverlayVisibilityMode.notEditing => !hasText, + }; } // True if any surrounding decoration widgets will be shown. @@ -1134,6 +1111,32 @@ class _CupertinoTextFieldState extends State with Restoratio return _hasDecoration ? TextAlignVertical.center : TextAlignVertical.top; } + void _onClearButtonTapped() { + final bool hadText = _effectiveController.text.isNotEmpty; + _effectiveController.clear(); + if (hadText) { + // Tapping the clear button is also considered a "user initiated" change + // (instead of a programmatical one), so call `onChanged` if the text + // changed as a result. + widget.onChanged?.call(_effectiveController.text); + } + } + + Widget _buildClearButton() { + return GestureDetector( + key: _clearGlobalKey, + onTap: widget.enabled ? _onClearButtonTapped : null, + child: Padding( + padding: const EdgeInsets.symmetric(horizontal: 6.0), + child: Icon( + CupertinoIcons.clear_thick_circled, + size: 18.0, + color: CupertinoDynamicColor.resolve(_kClearButtonColor, context), + ), + ), + ); + } + Widget _addTextDependentAttachments(Widget editableText, TextStyle textStyle, TextStyle placeholderStyle) { // If there are no surrounding widgets, just return the core editable text // part. @@ -1145,59 +1148,69 @@ class _CupertinoTextFieldState extends State with Restoratio return ValueListenableBuilder( valueListenable: _effectiveController, child: editableText, - builder: (BuildContext context, TextEditingValue? text, Widget? child) { + builder: (BuildContext context, TextEditingValue text, Widget? child) { + final bool hasText = text.text.isNotEmpty; + final String? placeholderText = widget.placeholder; + final Widget? placeholder = placeholderText == null + ? null + // Make the placeholder invisible when hasText is true. + : Visibility( + maintainAnimation: true, + maintainSize: true, + maintainState: true, + visible: !hasText, + child: SizedBox( + width: double.infinity, + child: Padding( + padding: widget.padding, + child: Text( + placeholderText, + // This is to make sure the text field is always tall enough + // to accommodate the first line of the placeholder, so the + // text does not shrink vertically as you type (however in + // rare circumstances, the height may still change when + // there's no placeholder text). + maxLines: hasText ? 1 : widget.maxLines, + overflow: placeholderStyle.overflow, + style: placeholderStyle, + textAlign: widget.textAlign, + ), + ), + ), + ); + + final Widget? prefixWidget = _shouldShowAttachment(attachment: widget.prefixMode, hasText: hasText) ? widget.prefix : null; + + // Show user specified suffix if applicable and fall back to clear button. + final bool showUserSuffix = _shouldShowAttachment(attachment: widget.suffixMode, hasText: hasText); + final bool showClearButton = _shouldShowAttachment(attachment: widget.clearButtonMode, hasText: hasText); + final Widget? suffixWidget = switch ((showUserSuffix, showClearButton)) { + (false, false) => null, + (true, false) => widget.suffix, + (true, true) => widget.suffix ?? _buildClearButton(), + (false, true) => _buildClearButton(), + }; return Row(children: [ // Insert a prefix at the front if the prefix visibility mode matches // the current text state. - if (_showPrefixWidget(text!)) widget.prefix!, + if (prefixWidget != null) prefixWidget, // In the middle part, stack the placeholder on top of the main EditableText // if needed. Expanded( child: Stack( + // Ideally this should be baseline aligned. However that comes at + // the cost of the ability to compute the intrinsic dimensions of + // this widget. + // See also https://github.com/flutter/flutter/issues/13715. + alignment: AlignmentDirectional.center, + textDirection: widget.textDirection, children: [ - if (widget.placeholder != null && text.text.isEmpty) - SizedBox( - width: double.infinity, - child: Padding( - padding: widget.padding, - child: Text( - widget.placeholder!, - maxLines: widget.maxLines, - overflow: placeholderStyle.overflow ?? TextOverflow.ellipsis, - style: placeholderStyle, - textAlign: widget.textAlign, - ), - ), - ), - child!, + if (placeholder != null) placeholder, + editableText, ], ), ), - // First add the explicit suffix if the suffix visibility mode matches. - if (_showSuffixWidget(text)) - widget.suffix! - // Otherwise, try to show a clear button if its visibility mode matches. - else if (_showClearButton(text)) - GestureDetector( - key: _clearGlobalKey, - onTap: widget.enabled ?? true ? () { - // Special handle onChanged for ClearButton - // Also call onChanged when the clear button is tapped. - final bool textChanged = _effectiveController.text.isNotEmpty; - _effectiveController.clear(); - if (widget.onChanged != null && textChanged) { - widget.onChanged!(_effectiveController.text); - } - } : null, - child: Padding( - padding: const EdgeInsets.symmetric(horizontal: 6.0), - child: Icon( - CupertinoIcons.clear_thick_circled, - size: 18.0, - color: CupertinoDynamicColor.resolve(_kClearButtonColor, context), - ), - ), - ), + if (suffixWidget != null) suffixWidget ]); }, ); @@ -1251,7 +1264,7 @@ class _CupertinoTextFieldState extends State with Restoratio }; } - final bool enabled = widget.enabled ?? true; + final bool enabled = widget.enabled; final Offset cursorOffset = Offset(_iOSHorizontalCursorOffsetPixels / MediaQuery.devicePixelRatioOf(context), 0); final List formatters = [ ...?widget.inputFormatters, diff --git a/packages/flutter/lib/src/cupertino/text_form_field_row.dart b/packages/flutter/lib/src/cupertino/text_form_field_row.dart index 268d8c06125..ed20b583a5f 100644 --- a/packages/flutter/lib/src/cupertino/text_form_field_row.dart +++ b/packages/flutter/lib/src/cupertino/text_form_field_row.dart @@ -219,7 +219,7 @@ class CupertinoTextFormFieldRow extends FormField { onEditingComplete: onEditingComplete, onSubmitted: onFieldSubmitted, inputFormatters: inputFormatters, - enabled: enabled, + enabled: enabled ?? true, cursorWidth: cursorWidth, cursorHeight: cursorHeight, cursorColor: cursorColor, diff --git a/packages/flutter/lib/src/rendering/stack.dart b/packages/flutter/lib/src/rendering/stack.dart index fb8aa3f97ee..83ebbaaf447 100644 --- a/packages/flutter/lib/src/rendering/stack.dart +++ b/packages/flutter/lib/src/rendering/stack.dart @@ -569,15 +569,11 @@ class RenderStack extends RenderBox double width = constraints.minWidth; double height = constraints.minHeight; - final BoxConstraints nonPositionedConstraints; - switch (fit) { - case StackFit.loose: - nonPositionedConstraints = constraints.loosen(); - case StackFit.expand: - nonPositionedConstraints = BoxConstraints.tight(constraints.biggest); - case StackFit.passthrough: - nonPositionedConstraints = constraints; - } + final BoxConstraints nonPositionedConstraints = switch (fit) { + StackFit.loose => constraints.loosen(), + StackFit.expand => BoxConstraints.tight(constraints.biggest), + StackFit.passthrough => constraints, + }; RenderBox? child = firstChild; while (child != null) { diff --git a/packages/flutter/test/cupertino/text_field_test.dart b/packages/flutter/test/cupertino/text_field_test.dart index 46c6262b1f6..bd5721aba3c 100644 --- a/packages/flutter/test/cupertino/text_field_test.dart +++ b/packages/flutter/test/cupertino/text_field_test.dart @@ -1071,7 +1071,8 @@ void main() { await tester.enterText(find.byType(CupertinoTextField), 'input'); await tester.pump(); - expect(find.text('placeholder'), findsNothing); + final Element element = tester.element(find.text('placeholder')); + expect(Visibility.of(element), false); }, ); @@ -1964,7 +1965,9 @@ void main() { expect(find.text('field 1'), findsOneWidget); expect(find.text("j'aime la poutine"), findsOneWidget); - expect(find.text('field 2'), findsNothing); + + final Element placeholder2Element = tester.element(find.text('field 2')); + expect(Visibility.of(placeholder2Element), false); }, skip: isContextMenuProvidedByPlatform); // [intended] only applies to platforms where we supply the context menu. testWidgets( @@ -8096,9 +8099,7 @@ void main() { await tester.pumpWidget( const CupertinoApp( home: Center( - child: CupertinoTextField( - enabled: true, - ), + child: CupertinoTextField(), ), ), ); @@ -9867,4 +9868,59 @@ void main() { skip: isContextMenuProvidedByPlatform, // [intended] only applies to platforms where we supply the context menu. variant: TargetPlatformVariant.all(excluding: { TargetPlatform.iOS }), ); + + testWidgets('Does not shrink in height when enters text when there is large single-line placeholder', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/133241. + final TextEditingController controller = TextEditingController(); + await tester.pumpWidget( + CupertinoApp( + home: Align( + alignment: Alignment.topCenter, + child: CupertinoTextField( + placeholderStyle: const TextStyle(fontSize: 100), + placeholder: 'p', + controller: controller, + ), + ), + ), + ); + + final Rect rectWithPlaceholder = tester.getRect(find.byType(CupertinoTextField)); + controller.value = const TextEditingValue(text: 'input'); + await tester.pump(); + + final Rect rectWithText = tester.getRect(find.byType(CupertinoTextField)); + expect(rectWithPlaceholder, rectWithText); + }); + + testWidgets('Does not match the height of a multiline placeholder', (WidgetTester tester) async { + final TextEditingController controller = TextEditingController(); + await tester.pumpWidget( + CupertinoApp( + home: Align( + alignment: Alignment.topCenter, + child: CupertinoTextField( + placeholderStyle: const TextStyle(fontSize: 100), + placeholder: 'p' * 50, + maxLines: null, + controller: controller, + ), + ), + ), + ); + + final Rect rectWithPlaceholder = tester.getRect(find.byType(CupertinoTextField)); + controller.value = const TextEditingValue(text: 'input'); + await tester.pump(); + + final Rect rectWithText = tester.getRect(find.byType(CupertinoTextField)); + // The text field is still top aligned. + expect(rectWithPlaceholder.top, rectWithText.top); + // But after entering text the text field should shrink since the + // placeholder text is huge and multiline. + expect(rectWithPlaceholder.height, greaterThan(rectWithText.height)); + // But still should be taller than or the same height of the first line of + // placeholder. + expect(rectWithText.height, greaterThan(100)); + }); }