Force line height in TextFields with strut by GaryQian · Pull Request #27612 · flutter/flutter

GaryQian · 2019-02-06T20:35:23Z

Fixes #20183

This uses strut to force line heights to stay consistent even when taller glyphs are typed. This guarantees the TextField height is large enough to contain the text as well as prevents clipping of the text.

When not explicitly provided, the strut is derived from the TextStyle of the textfield. Strut can be completely disabled by passing StrutStyle.disabled(), which will have the effect of no strut at all.

Since CJK fonts are frequently taller than alphanumeric fonts, Textfields with the font set to alphanumeric fonts tend to experience less line spacing between CJK text than usual. The opposite is also true, where alphanumeric text will see more line spacing than normal when a CJK font is set.

git log --oneline --no-merges 33bb91c..21c5919
21c5919 [re-land] Use all font managers to discover fonts for strut. (#7803)
5809ade Make the layout of dynamic patch bundle similar to APK. (#7925)
e2a449a Move canvas clear after preroll (#7923)
8529dbc Remove unused FML file export.h (#7926)
f80928a Roll src/third_party/skia fdd15284affe..ee1c8a733e5b (15 commits) (#7924)
93f339f fix Memory leak when using PlatformView [IOS] #24714 (#7919)
2d33e77 Roll src/third_party/skia 969659dbb313..fdd15284affe (2 commits) (#7922)
6d4edf2 Roll src/third_party/skia 9431161ca973..969659dbb313 (3 commits) (#7921)

GaryQian · 2019-02-07T23:42:30Z

Requires engine roll with flutter/engine#7734 to pass tests.

HansMuller

This looks good. Some of the test are a little hard to judge because the relationship between the vertical dimensions they're checking and the text style etc isn't obvious.

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

packages/flutter/lib/src/rendering/editable.dart

packages/flutter/test/material/input_decorator_test.dart

packages/flutter/test/material/text_field_test.dart

GaryQian · 2019-02-08T18:43:31Z

This change depends on flutter/engine#7734 and will include an engine roll.

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

HansMuller

LGTM

HansMuller · 2019-02-12T00:39:20Z

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

+  /// Equivalent to having no strut at all. All lines will be laid out according to
+  /// the properties defined in [TextStyle].
+  ///
+  /// Default strut is not the same as [StrutStyle.disabled].


I believe that you mean StrutStyle.disabled != StrutStyle().

This line is a little confusing; maybe it doesn't bear saying at all?

Yeah, the distinction is between omitting it and having it inherit vs passing [StrutStyle.disabled]. I guess if they dont disable it, it isnt particularly jarring as it doesnt have any direct impact that would be visible.

But yeah, it would be inherently true that there was a difference or else it wouldnt exist (in a well designed API at least). I'll remove it.

HansMuller · 2019-02-12T00:41:55Z

packages/flutter/lib/src/widgets/editable_text.dart

+  /// to lock all lines to the height of the base [TextStyle], provided by
+  /// [style]. This ensures the typed text fits within the alotted space.
+  ///
+  /// If null, the strut used will is inherit values from the [style] and will


If null, a [StrutStyle] based on [style], with [StrutStyle.forceStrutHeight] set to true, will be used. If [style] is null (the default) then the theme's [TextStyle] will be used instead.

However: we don't seem to be defaulting style to the theme's style?

This behavior is actually inherited from the TextStyle taking the theme's style, which the StrutStyle then in turn inherits.

HansMuller · 2019-02-12T00:42:38Z

packages/flutter/lib/src/widgets/editable_text.dart

+  /// instead.
+  ///
+  /// To disable strut-based vertical alignment and allow dynamic vertical
+  /// layout based on the glyphs typed, passing [StrutStyle.disabled] will


passing [StrutStyle.disabled] will make strut take no effect => use [StrutStyle]

HansMuller · 2019-02-12T00:49:16Z

packages/flutter/test/cupertino/text_field_test.dart

+
+      expect(
+        tester.getSize(find.byType(CupertinoTextField)),
+        const Size(200, 63), // 29 is the height of the default font * 3 (17 * 3) + decoration (12).


NICE. Thanks for these explanations!

matthew-carroll

More comments to come...our Cupertino review meeting ran out of time.

matthew-carroll · 2019-02-19T22:20:03Z

packages/flutter/lib/src/widgets/editable_text.dart

+  ///
+  /// To disable strut-based vertical alignment and allow dynamic vertical
+  /// layout based on the glyphs typed, use [StrutStyle.disabled].
+  /// {@endtemplate}


Would you mind adding a URL reference to something that explains the concept of a strut in general? It's an odd name for those that are not already familiar with it.

I can put a link to https://www.w3.org/TR/CSS2/visudet.html#line-height in the docs for StrutStyle. Sticking the link everywhere strut is used might cause a bit too much clutter.

I was just recommending that it be added to this template. The other places where this template appears is likely to be the first and only interaction most developers have with this concept, so it seemed like a good place for an external reference.

https://en.wikipedia.org/wiki/Strut_(typesetting) may also be a good more generic albeit brief reference.

Ahh my bad, missed that this was the template! That's a good idea.

matthew-carroll · 2019-02-19T22:21:27Z

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

    }
    final CupertinoThemeData themeData = CupertinoTheme.of(context);
-    final TextStyle textStyle = widget.style ?? themeData.textTheme.textStyle;
+    final TextStyle textStyle = themeData.textTheme.textStyle.merge(widget.style);


This is technically a breaking change. Can you elaborate on why this was changed? Also, do you plan to announce this breaking change when merged?

I think I should talk to someone more familiar with Cupertino about the exact desired behavior here. This is the behavior in material, and the way strut inherits the textStyle's properties is dependent on the textstyle being complete.

Talked with xster and we have agreed the change here is reasonable. I have sent a breaking change announce out and added the API break tag. The change here should be minor and can be bypassed with the inherit: false parameter in TextStyle

matthew-carroll · 2019-02-19T22:32:07Z

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

  }

+  @override
+  String toStringShort() => '$runtimeType';


Is there a reason that this is the desired behavior?

This was based around the same override in text_style.dart:935 where presumably, the full toString that includes all of the properties, is often times too long and makes output unreadable. A similar argument can be applied to StrutStyle, which also contains a large number of potentially cluttering properties that are not necessarily relevant in the short version of the toString.

… version

GaryQian added 6 commits January 30, 2019 15:30

Begin to pass strutStyle properties

5d9ac93

Generate strutStyle from TextStyle

758684a

Merge

a7a43cd

Additional work

0362e9a

Force line height in TextFields with strut

cddf37e

Merge branch 'master' of github.com:flutter/flutter into textfield

3030464

googlebot added the cla: yes label Feb 6, 2019

GaryQian mentioned this pull request Feb 6, 2019

chinese font display not well on nexus 5x #18665

Closed

GaryQian added 7 commits February 6, 2019 14:42

remove extra code

ccaa7b2

Refactor getter and improve comments

1a8a75c

Remove whitespace, more docs

96837a7

Handle _package properly

1ccd970

Begin to fix tests, add strut versions of tests

5324175

More test fixes

9712630

Merge branch 'master' of github.com:flutter/flutter into textfield

1202898

GaryQian added 2 commits February 7, 2019 15:04

Update tests for engine changes

a31cec6

Merge branch 'master' of github.com:flutter/flutter into textfield

de95375

GaryQian added a commit to flutter/goldens that referenced this pull request Feb 7, 2019

Add goldens for flutter/flutter#27612

62c8fdd

GaryQian added 2 commits February 7, 2019 15:17

Fix golden tests

380692f

Add more textfield strut tests

43ade35

GaryQian mentioned this pull request Feb 7, 2019

Use all font managers to discover fonts for strut. flutter/engine#7734

Merged

GaryQian requested review from HansMuller, Hixie and xster February 7, 2019 23:43

HansMuller reviewed Feb 8, 2019

View reviewed changes

GaryQian added 4 commits February 8, 2019 12:49

Minor refactors, add more tests and comments, revise docs.

a4667d0

Edit docs, add back in commented assert

84cef7b

Additional comments explaining tests

117ed13

Make analzer happy

53d9a7c

xster reviewed Feb 8, 2019

View reviewed changes

packages/flutter/lib/src/cupertino/text_field.dart Outdated Show resolved Hide resolved

Comments, and redundant conditional

dec691c

GaryQian mentioned this pull request Feb 8, 2019

LibTxt: Implement Strut and provide an API to it. Fix text clipping in textboxes. #26085

Closed

6 tasks

HansMuller approved these changes Feb 12, 2019

View reviewed changes

GaryQian added 4 commits February 11, 2019 17:09

Fix comments and docs

a071f5f

Fix confusing comment syntax

d41e54a

Roll engine

743f9c9

Merge branch 'master' into textfield

1af059e

GaryQian mentioned this pull request Feb 12, 2019

Fix goldens to use latest versions #27815

Closed

matthew-carroll reviewed Feb 19, 2019

View reviewed changes

GaryQian added 3 commits February 19, 2019 16:45

Add links for reference information on strut

d1e5278

Merge branch 'textfield' of github.com:garyqian/flutter into textfield

4beefd2

Add strut reference links to strut template

f4ea009

zoechi mentioned this pull request Feb 20, 2019

TextField cursor's problem with emoji #28192

Closed

GaryQian added the c: API break Backwards-incompatible API changes label Feb 20, 2019

GaryQian added 6 commits February 20, 2019 14:53

Inherit strutstyle from textstyle always and merge with user provided…

1130f63

… version

Merge with upstream/master

fbf9e97

Merge branch 'master' into textfield

8b9b856

Merge branch 'master' into textfield

de69899

Remove whitespace at end

4db6ddf

Merge branch 'master' into textfield

0642aa7

GaryQian merged commit 7b5a769 into flutter:master Feb 23, 2019

xster mentioned this pull request Mar 7, 2019

Make the text field cursor configurable #15571

Closed

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

Conversation

GaryQian commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GaryQian commented Feb 7, 2019

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GaryQian commented Feb 8, 2019

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthew-carroll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

GaryQian commented Feb 6, 2019 •

edited

Loading