CupertinoTextField vertical alignment #34723

justinmc · 2019-06-19T16:16:20Z

Description

This PR adds the textAlignVertical parameter to CupertinoTextField. It works the same as TextField with the slight differences that 1. including any prefix or suffix causes alignment to default to the center, and 2. the text centers itself within the vertical space of the prefix/suffix. Both of these are existing behaviors that have been preserved by this PR.

@@ -898,7 +914,26 @@ class _CupertinoTextFieldState extends State<CupertinoTextField> with AutomaticK
        ignoring: !enabled,
        child: Container(
          decoration: effectiveDecoration,
-          child: _addTextDependentAttachments(paddedEditable, textStyle, placeholderStyle),
+          child: TextSelectionGestureDetector(


The Align widget is what does the vertical alignment. I had to move the TextSelectionGestureDetector here in order to get the entire input to be interactive in all cases like it should be.

justinmc · 2019-06-26T22:46:19Z

packages/flutter/lib/src/cupertino/text_field.dart

+    // Because TextSelectionGestureDetector listens to taps that happen on
+    // widgets in front of it, tapping the clear button will also trigger this
+    // handler here. If this is the case, return.
+    if (_clearing) {


This is hacky. Is there any other way to prevent taps on the clear button from triggering the TextSelectionGestureDetector? I'm back to the confusing gesture arena problem of multiple widgets getting the same event.

Ideally we want to eliminate gestures that start (with a down event) in either the text field's prefix or the clear button suffix. The TextSelectionGestureDetector would have to ignore gestures that start within the corresponding render boxes, per hitTest.

This probably isn't academic. One could use a Slider as prefix for example.

justinmc · 2019-06-26T22:51:05Z

packages/flutter/test/cupertino/text_field_test.dart

+        // Tapping anywhere inside focuses it.
+        expect(focusNode.hasFocus, false);
+        await tester.tapAt(tester.getTopLeft(find.byType(CupertinoTextField)));
+        await tester.pumpAndSettle(const Duration(milliseconds: 400)); // timer error if no duration


If I didn't add this Duration, then I got failures due to "A Timer is still pending even after the widget tree was disposed." Any other way to fix that? Am I doing something weird here?

This might be the 300ms doubleTap timer from packages/flutter/lib/src/widgets/text_selection.dart. Pump and settle only waits for 100ms, so the timer may survive to the end of the test. Assuming that's correct, you could explicitly add a second 300 ms pump (after a no-args pumpAndSettle) to ensure that the double tap timer has expired.

That seems to be it, thanks for the tip.

…for interactivity

… key instead

justinmc · 2019-07-01T17:36:20Z

~~Now the clear button and any prefix or suffix will be hit tested by tap events, and if hit, the underlying CupertinoTextField will ignore the events.~~

Changed so that only the clear button is opaque to tap events. See comments below.

HansMuller

LGTM with some feedback about the details.

HansMuller · 2019-07-01T18:00:52Z

packages/flutter/lib/src/cupertino/text_field.dart

  void _handleSingleTapUp(TapUpDetails details) {
+    // Because TextSelectionGestureDetector listens to taps that happen on
+    // widgets in front of it, tapping the prefix or suffix will also trigger
+    // this handler here. If this is the case, ignore the tap.


this handler here => this handler.
If this is the case ... => If the the prefix or suffix widget recognizes the up event, then prevent the text field from seeing it.

HansMuller · 2019-07-01T18:02:00Z

packages/flutter/lib/src/cupertino/text_field.dart

+    if (widget.textAlignVertical != null) {
+      return widget.textAlignVertical;
+    }
+    return _hasDecoration ? TextAlignVertical.center : TextAlignVertical.top;


A comment about backwards compatibility might be helpful here

HansMuller · 2019-07-01T18:02:32Z

packages/flutter/lib/src/cupertino/text_field.dart

@@ -742,7 +784,10 @@ class _CupertinoTextFieldState extends State<CupertinoTextField> with AutomaticK
        // Insert a prefix at the front if the prefix visibility mode matches
        // the current text state.
        if (_showPrefixWidget(text)) {
-          rowChildren.add(widget.prefix);
+          rowChildren.add(Container(


KeyedSubtree instead of Container? Here and below.

We talked and decided to remove this behavior on the prefix and suffix, so for now only the clear button will be opaque to tap events, and I'll get rid of these containers.

HansMuller · 2019-07-01T18:04:18Z

packages/flutter/lib/src/painting/alignment.dart

@@ -633,3 +633,44 @@ class _MixedAlignment extends AlignmentGeometry {
    return null;
  }
 }
+
+/// The vertical alignment of text within an input.


input => input field or input box? Here and below.

HansMuller · 2019-07-01T18:05:57Z

packages/flutter/lib/src/painting/alignment.dart

+/// The vertical alignment of text within an input.
+///
+/// A single [y] value that can range from -1.0 to 1.0. -1.0 aligns to the top
+/// of the input so that the top of the first line of text fits within the input


If you used "input box" then:
of the input box, so that the top of the first line of text fits within the box

etc

HansMuller · 2019-07-01T18:07:00Z

packages/flutter/lib/src/painting/alignment.dart

+       assert(y >= -1.0 && y <= 1.0);
+
+  /// A value ranging from -1.0 to 1.0 that defines the topmost and bottommost
+  /// locations of the top and bottom of the input text box.


"the input text box", NICE. Maybe: a text input box (here and elsewhere).

HansMuller · 2019-07-01T18:07:18Z

packages/flutter/lib/src/painting/alignment.dart

+  /// locations of the top and bottom of the input text box.
+  final double y;
+
+  /// Aligns a TextField's input Text with the topmost location within the


within the => within a

TextField => TextField's input box.

HansMuller · 2019-07-01T18:09:26Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -650,6 +651,12 @@ void main() {
            + tester.getSize(find.byIcon(CupertinoIcons.add)).width
            + 6.0,
      );
+
+      // Tapping on the prefix isn't received by the CupertinoTextField.


Tapping on the prefix doesn't cause the CupertinoTextField to take the keyboard focus.

Removing this test after the functionality was removed.

HansMuller · 2019-07-01T20:55:54Z

packages/flutter/test/cupertino/text_field_test.dart

@@ -3087,6 +3102,9 @@ void main() {
          ),
        ),
      );
+
+      tester.binding.window.physicalSizeTestValue = null;


Were tests failing because we hadn't reset these values?

Yes! It took me awhile to track it down to this.

HansMuller · 2019-07-01T21:03:40Z

packages/flutter/test/cupertino/text_field_test.dart

+        // Tapping anywhere inside focuses it.
+        expect(focusNode.hasFocus, false);
+        await tester.tapAt(tester.getTopLeft(find.byType(CupertinoTextField)));
+        await tester.pumpAndSettle(const Duration(milliseconds: 400)); // timer error if no duration


This might be the 300ms doubleTap timer from packages/flutter/lib/src/widgets/text_selection.dart. Pump and settle only waits for 100ms, so the timer may survive to the end of the test. Assuming that's correct, you could explicitly add a second 300 ms pump (after a no-args pumpAndSettle) to ensure that the double tap timer has expired.

CupertinoTextField now supports vertical alignment via the textAlignVertical parameter.

* commit 'a0c47e2216a2b50b70c478001cf5652f31a783af': (187 commits) Do not use ideographic baseline for RenderPargraph baseline (flutter#35493) Add type to StreamChannel in generated test code. (flutter#35367) Fix RenderFittedBox when child.size.isEmpty (flutter#35487) add APK build time benchmarks (flutter#35481) fix Selection handles position is off (flutter#34665) Move usage flutter create tests into memory filesystem. (flutter#35160) Include tags in SemanticsNode debug properties (flutter#35491) Re-apply 'Add currentSystemFrameTimeStamp to SchedulerBinding' (flutter#35492) CupertinoTextField vertical alignment (flutter#34723) Mark update-packages as non-experimental (flutter#35467) fix default artifacts to exclude ios and android (flutter#35303) Roll engine ffba2f6..7d3e722 (59 commits) (flutter#35489) Update macrobenchmarks README and app name (flutter#35477) mark windows and macos chrome dev mode as flaky (flutter#35495) Use the new service protocol message names (flutter#35482) Manual roll of engine 45b66b7...ffba2f6 (flutter#35464) more ui-as-code (flutter#35393) update reassemble doc (flutter#35164) New parameter for RawGestureDetector to customize semantics mapping (flutter#33936) Add --target support for Windows and Linux (flutter#34660) ...

CupertinoTextField now supports vertical alignment via the textAlignVertical parameter.

googlebot added the cla: yes label Jun 19, 2019

justinmc self-assigned this Jun 19, 2019

justinmc added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Jun 19, 2019

Align CupertinoTextField with Align widget

d21f322

justinmc force-pushed the text-field-vertical-align-cupertino branch from 1b0211e to d21f322 Compare June 20, 2019 17:02

justinmc added 3 commits June 20, 2019 10:23

Provide default behavior when TextVerticalAlign isnt provided that ma…

4898a70

…tches original behavior

Move TextAlignVertical definition to place accessible by material and…

d85a12b

… cupertino

Field is interactive for its full height despite alignment

6318cf3

justinmc force-pushed the text-field-vertical-align-cupertino branch 2 times, most recently from 24826bc to bc17df9 Compare June 20, 2019 19:43

Decoration follows alignment

376bdd6

justinmc force-pushed the text-field-vertical-align-cupertino branch from bc17df9 to 376bdd6 Compare June 20, 2019 19:44

Put TSGD back where it was, add expands logic in RenderText, add w/h for

a5f79b4

Align

justinmc force-pushed the text-field-vertical-align-cupertino branch from 599c480 to a5f79b4 Compare June 25, 2019 22:50

Merge branch 'master' into text-field-vertical-align-cupertino

07d05b9

justinmc commented Jun 26, 2019

View reviewed changes

justinmc force-pushed the text-field-vertical-align-cupertino branch 2 times, most recently from 6a17d5f to 94c8ddd Compare June 26, 2019 22:49

justinmc commented Jun 26, 2019

View reviewed changes

justinmc force-pushed the text-field-vertical-align-cupertino branch from 94c8ddd to 47c87a4 Compare June 26, 2019 22:52

Clear button problem fixed with hacky _clearing flag, and many tests …

b8c6750

…for interactivity

justinmc force-pushed the text-field-vertical-align-cupertino branch from 47c87a4 to b8c6750 Compare June 26, 2019 22:53

justinmc removed the work in progress; do not review label Jun 26, 2019

justinmc changed the title ~~WIP CupertinoTextField vertical alignment~~ CupertinoTextField vertical alignment Jun 26, 2019

justinmc requested a review from HansMuller June 26, 2019 22:54

Work around _clearing variable by hit testing clear button via global…

4bb48ba

… key instead

justinmc added 4 commits June 28, 2019 10:18

Hit test directly on renderbox rather than creating a Rect.

fd7c9ae

Also don't receive taps on prefix or suffix

bef95ed

Test that CupertinoTextField ignores taps on prefix/suffix

1613002

Make code more DRY and clarify test comments

082bd29

justinmc force-pushed the text-field-vertical-align-cupertino branch from 1af3afa to 082bd29 Compare July 1, 2019 17:34

HansMuller approved these changes Jul 1, 2019

View reviewed changes

CR fixes, and remove event ignoring code for prefix/suffix

c91d139

justinmc merged commit 2d2bb6b into flutter:master Jul 3, 2019

justinmc deleted the text-field-vertical-align-cupertino branch July 3, 2019 15:22

darrenaustin pushed a commit to darrenaustin/flutter that referenced this pull request Jul 3, 2019

CupertinoTextField vertical alignment (flutter#34723)

9065d49

CupertinoTextField now supports vertical alignment via the textAlignVertical parameter.

justinmc mentioned this pull request Jul 25, 2019

keyboard is opening when pressing on a suffix icon inside an Input decoration of a TextField #36948

Closed

johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019

CupertinoTextField vertical alignment (flutter#34723)

e0db2f7

CupertinoTextField now supports vertical alignment via the textAlignVertical parameter.

justinmc mentioned this pull request Aug 6, 2019

Clearing a TextField by tapping on its suffixIcon causes its ripple to break, and the field stays gray. #36324

Closed

This was referenced Aug 23, 2019

TextFormField TextAlignVertical support #39123

Closed

Add the textAlignVertical param to TextFormField #39144

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021

-              ///  * [TextField.textAlignVertical], which is passed on to the [InputDecorator].
-              ///  * [InputDecorator.textAlignVertical], which defines the alignment of
-              ///    prefix, input, and suffix, within the [InputDecorator].
-              class TextAlignVertical {

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CupertinoTextField vertical alignment #34723

CupertinoTextField vertical alignment #34723

justinmc commented Jun 19, 2019 •

edited

justinmc Jun 26, 2019

justinmc Jun 26, 2019

justinmc Jun 26, 2019

HansMuller Jun 27, 2019

justinmc Jun 26, 2019 •

edited

HansMuller Jul 1, 2019

justinmc Jul 1, 2019

justinmc commented Jul 1, 2019 •

edited

HansMuller left a comment

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

justinmc Jul 1, 2019

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

HansMuller Jul 1, 2019

justinmc Jul 1, 2019

HansMuller Jul 1, 2019

justinmc Jul 1, 2019

HansMuller Jul 1, 2019

CupertinoTextField vertical alignment #34723

CupertinoTextField vertical alignment #34723

Conversation

justinmc commented Jun 19, 2019 • edited

Description

Related

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc Jun 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc commented Jul 1, 2019 • edited

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc commented Jun 19, 2019 •

edited

justinmc Jun 26, 2019 •

edited

justinmc commented Jul 1, 2019 •

edited