Skip to content

Conversation

justinmc
Copy link
Contributor

@justinmc justinmc commented Jun 12, 2019

Description

TextFields currently vertically center their text when they have an outline border. The difference between this and top alignment is almost nothing in most cases except when the input is very tall (see issue). We need to decide if we need that vertically centering functionality at all, and if so, when exactly.

Currently, this PR is just an exploration of how we could force top alignment, since according to #29927 (comment), we may not need this vertical centering at all.

Related Issues

Closes #29927
Reopened from #31079

@justinmc justinmc added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. and removed cla: yes labels Jun 12, 2019
@justinmc justinmc force-pushed the text-field-vertical-align branch from a457e20 to 01ebf49 Compare June 13, 2019 16:59
@justinmc
Copy link
Contributor Author

justinmc commented Jun 13, 2019

TODO:

  1. Ensure compatibility with labels and hints.
  2. Ensure compatibility with alignLabelWithHint (labelText of multiline TextFormField should be top aligned #18081 (comment)).
  3. Ensure that the default cases of the tests added in this PR pass on master.
  4. Delete outlineBaseline code.

@justinmc justinmc force-pushed the text-field-vertical-align branch 2 times, most recently from 9493d4c to b1be182 Compare June 17, 2019 17:40
@justinmc
Copy link
Contributor Author

justinmc commented Jun 17, 2019

This PR adds a parameter textAlignVertical that determines text's vertical position within the field.

No border

TextField(
  textAlignVertical: TextAlignVertical.top, // default
)

TextField(
  textAlignVertical: TextAlignVertical.center,
)

TextField(
  textAlignVertical: TextAlignVertical.bottom,
)

TextField(
  textAlignVertical: TextAlignVertical(y: -0.5),
)

Border

Previously, when the TextField had a border, the text would be centered. This behavior is preserved by default, but it's also possible to position the text within a bordered TextField using textAlignVertical:

TextField(
  textAlignVertical: TextAlignVertical.top,
  decoration: InputDecoration(
    border: OutlineInputBorder(),
  ),
)

alignLabelWithHint

The behavior of alignLabelWithHint in existing cases shouldn't change with this PR. This was previously used to place the label at the top of the input when it would otherwise be centered.

However, textAlignVeritcal does affect the position of the hint, and so now alignLabelWithHint can be used to place the label in places not possible before:

TextField(
  textAlignVertical: TextAlignVertical.bottom,
  decoration: InputDecoration(
    alignLabelWithHint: true,
    hintText: 'hint',
    labelText: 'label',
  ),
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outlineBaseline is no longer needed because centering is done by textAlignVertical now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some of this comment needs to be retained above?

@justinmc justinmc requested a review from HansMuller June 17, 2019 21:47
@justinmc
Copy link
Contributor Author

I confirmed that this address the problem mentioned in #18081 (comment).

@justinmc justinmc force-pushed the text-field-vertical-align branch from 1759ff3 to b9e01fc Compare June 17, 2019 22:49
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good; just small potatoes.

if (_textAlignVertical == value)
return;
_textAlignVertical = value;
markNeedsLayout();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe only do this if the "effective" value of _textAlignVertical changed. For example if it was null and _isOutlineAligned is true, then only markNeedsLayout if the old value wasn't TextAlignVerticalCenter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart.

// input and align the remaining part of the text and prefix/suffix.
final double overflow = math.max(0, contentHeight - maxContainerHeight);
final double baselineAdjustment = fixAboveInput - overflow;
final double textAlignVerticalFactor = (textAlignVertical.y + 1.0) / 2.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious (to me) what the + 1.0 and the 1 - adjustments are for here and on the next line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some of this comment needs to be retained above?

start += centerLayout(prefixIcon, start);
}
if (label != null)
if (label != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE


/// The vertical alignment of text within an input.
///
/// This consists simply of a double [y] that can range from -1.0 to 1.0. -1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This consists simply of a double [y] => A single [y] value

}) : assert(y != null),
assert(y >= -1.0 && y <= 1.0);

/// A value ranging from -1.0 to 1.0 that corresponds to the topmost to the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that corresponds to the topmost to the => bottommost possible text position => that defines the topmost and bottommost locations of the top and bottom of the input text box.

/// bottommost possible text position.
final double y;

/// A TextAlignVertical that aligns to the top of the input.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these values might be easier to understand if they were defined in terms of their effect on a TextField's input text. Like: Aligns a TextField's input Text with the topmost location within the TextField.


@override
String toString() {
return 'TextAlignVertical(y: $y)';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'd want $runtimeType(y: $y) because subclassing

/// {@template flutter.widgets.inputDecorator.textAlignVertical}
/// How the text should be aligned vertically.
///
/// Cannot be null.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

final TextAlign textAlign;

/// {@template flutter.widgets.inputDecorator.textAlignVertical}
/// How the text should be aligned vertically.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to explain this a little more carefully, since what we're talking about depends on how the textfield is decorated.

@justinmc justinmc self-assigned this Jun 18, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/// How the text should be aligned vertically.
///
/// Determines the alignment of the baseline within the available space of
/// the input. For example, TextAlignVertical.top will place the baseline
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space after the "."

Maybe say: input (typically a TextField).

/// the input. For example, TextAlignVertical.top will place the baseline
/// such that the text, and any attached decoration like prefix and suffix,
/// is as close to the top of the input as possible without overflowing.
/// Likewise, prefix and suffix height are included in the calculation at
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The heights of the prefix and suffix are similarly included for other alignment values.

///
/// See also:
///
/// * [TextField.textAlignVertical]
Copy link
Contributor

@HansMuller HansMuller Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Links like this are usually followed by a brief explanation. Like:

[TextField.textAlignVertical], which defines the alignment of a text field's prefix, input, and suffix, within the text field's [InputDecorator].

@justinmc justinmc merged commit b798b27 into flutter:master Jun 19, 2019
@justinmc justinmc deleted the text-field-vertical-align branch June 19, 2019 15:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Textfield text is centered vertically when expands = true and using an OutlineInputBorder
3 participants