TextStyle level leadingDistribution #24025

LongCatIsLooong · 2021-01-28T19:55:22Z

The framework failure seems to be caused by some toString changes.
I'll check with Christian and see if this produces the correct graphics.

Text is not vertically centered with customized height flutter#72521

Screencast

There're 2 different TextSpans rendered side by side: "firstTextRunljx" and "secondTextRunljx".
Their styles are controlled by the 2nd and 3rd groups of controls. The sliders control the height multipliers, ranging from 0.0x - 20.0x.
The blue box rendered behind the text is the space the TextSpans occupied as seen by the framework

example.mp4

TODO (after this PR)

Skia & canvaskit
CSS implementation (add AD Scaling?)
Framework & documentation

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-01-28T19:55:25Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

LongCatIsLooong · 2021-01-28T20:12:04Z

third_party/txt/src/txt/paragraph_txt.cc

+                                   : metrics_font_height + metrics.fLeading;
+
+    // Scale the ascent and descent such that the sum of ascent and
+    // descent is `style.height * style.font_size`.


Diff 1/2: fontsize * style.height * style.font_size -> style.height * style.font_size

LongCatIsLooong · 2021-01-28T20:12:42Z

third_party/txt/src/txt/paragraph_txt.cc

+    // https://glyphsapp.com/tutorials/vertical-metrics
+    //
+    // Doing this ascent/descent normalization to the EM Square allows
+    // a sane, consistent, and reasonable "blob_height" to be specified,


Diff 2/2: "text height" -> "blob_height"

LongCatIsLooong · 2021-01-28T20:41:05Z

third_party/txt/src/txt/paragraph_txt.cc

              : (paragraph_style_.strut_leading *
                 paragraph_style_.strut_font_size);
+
+      const double available_height = paragraph_style_.strut_text_height_behavior & TextHeightBehavior::kHalfLeading


Does not default to paragraphStyle.textHeightBehavior (or should it?)

LongCatIsLooong · 2021-01-28T20:59:50Z

third_party/txt/src/txt/paragraph_txt.cc

+    const double font_height =
+        shouldNormalizeFont ? style.font_size : metrics_font_height;
+
+    const size_t text_height_behavior =


Defaults to ParagraphStyle.textHeightBehavior if TextStyle.textHeightBehavior is not specified.

In the framework TextPainter does not (directly) have a height multiplier field, but currently has a textHeightBehavior field. With this implementation TextPainter.textHeightBehavior has the lowest precedence in the textHeightBehavior defaulting rules.

https://github.com/flutter/flutter/blob/b926c7b696eb1a5f601ef74cd191b3b6b2b22729/packages/flutter/lib/src/painting/text_painter.dart#L411-L437

Hixie · 2021-01-28T23:13:56Z

lib/ui/text.dart


+/// {@macro dart.ui.leadingDistribution}
+enum LeadingDistribution {
+  /// Distributes the ["leading"](https://www.w3.org/TR/CSS2/visudet.html#leading)


let's link to https://en.wikipedia.org/wiki/Leading rather than the CSS spec though

Would it be better to avoid using the term altogether then? The Wikipedia page says "the exact definition varies", and "In electronic typesetting, "leading" often instead means the distance from one baseline to the next. ".

Hixie · 2021-01-28T23:14:41Z

lib/ui/text.dart

+  /// font's ascent/discent ratio.
+  ///
+  /// {@template dart.ui.leading}
+  /// The leading space of a text run is defined as


"leading" is a noun, not an adjective. (It's pronounced like the metal Pb, as opposed to like the opposite of "following")

(which is to say, it's "the leading", not "the leading space".)

Hixie · 2021-01-28T23:16:06Z

lib/ui/text.dart

             'letterSpacing: ${      _encoded[0] & 0x00800 == 0x00800  ? "${_letterSpacing}x"                    : "unspecified"}, '
             'wordSpacing: ${        _encoded[0] & 0x01000 == 0x01000  ? "${_wordSpacing}x"                      : "unspecified"}, '
             'height: ${             _encoded[0] & 0x02000 == 0x02000  ? "${_height}x"                           : "unspecified"}, '
+             'textHeightBehavior: ${ _encoded[0] & 0x0100 == 0x0100    ? "${TextHeightBehavior.fromEncoded(_encoded[8])}x"  : "unspecified"}, '


i suspect the "x" here is a copy/paste typo?

Hixie · 2021-01-28T23:17:55Z

third_party/txt/src/txt/text_style.h

+// Multiple behaviors can be applied at once with a bitwise | operator. For
+// example, disabling first ascent and last descent can achieved with:
+//
+//   (kDisableFirstAscent | kDisableLastDescent).


we should update the docs to mention half-leading too

Hixie · 2021-01-28T23:18:51Z

glancing through this it seems reasonable. (didn't do a detailed review)

…height-behavior

johnsonmh

Looks great!

johnsonmh · 2021-02-03T23:37:54Z

lib/web_ui/lib/src/engine/text/paragraph.dart

    List<String>? fontFamilyFallback,
    double? fontSize,
    double? height,
+    //TODO(LongCatIsLooong): implement textHeightBehavir.


Nit: textHeightBehavior, or maybe this comment should be removed entirely?

johnsonmh · 2021-02-04T00:14:12Z

third_party/txt/src/txt/paragraph_txt.cc

+    const double leading =
+        text_height_behavior & TextHeightBehavior::kHalfLeading
+            ? blob_height - font_height
+        : style.has_height_override ? 0.0


I can't quite figure out this part of the ternary. This 0.0 will be hit when: "if not half leading, and style has a height override, then leading == 0"? Is that because we don't consider there to be any "leading" with AD scaling, instead of adding leading we just stretch the ascent and descent?

Side note and totally personal preference so feel free to ignore: it takes me a few extra seconds to parse the nested ternary, I personally would find something like this easier to read:

if (halfLeading) { leading = blob_height - font_height; else { leading = style.has_height_override ? 0.0 : metrics.fLeading; }

johnsonmh · 2021-02-04T00:18:51Z

third_party/txt/src/txt/paragraph_txt.cc

+        half_leading;
+    double ascent = disableAscent ? -metrics.fAscent : modifiedAscent;
+    double descent = disableDescent ? metrics.fDescent
+                                    //: blob_height - leading - modifiedAscent;


This comment should probably be removed.

johnsonmh · 2021-02-04T00:20:24Z

third_party/txt/src/txt/paragraph_txt.cc

+    double ascent = disableAscent ? -metrics.fAscent : modifiedAscent;
+    double descent = disableDescent ? metrics.fDescent
+                                    //: blob_height - leading - modifiedAscent;
+                                    : metrics.fDescent / metrics_font_height *


For consistency, consider putting this in a double called modifiedDescent.

johnsonmh · 2021-02-04T00:27:00Z

third_party/txt/src/txt/paragraph_txt.cc

  code_unit_runs_.clear();
  inline_placeholder_code_unit_runs_.clear();
-  max_right_ = FLT_MIN;
+  max_right_ = -FLT_MAX;


I'm not familiar with what this does, is this change related/required by the current change? Will it have any side effects, or is FLT_MIN always the - of FLT_MAX?

According to the documentation FLT_MIN is the smallest positive double (that's not 0). I had to change this for some strut related stuff so descent can go negative. Some of other occurrences may not be directly related to this change but they all seem to make more sense with -FLT_MAX.

Yes - I think all of these initializers should have been -FLT_MAX. They are all used in std::max(current_value, new_value) expressions, and the goal is to ensure that any new_value is greater than the initialized current_value.

Another option would be using std::numeric_limits<double>::lowest()

Switched to std::numeric_limits as they all seem to be doubles.

johnsonmh · 2021-02-04T00:28:48Z

third_party/txt/src/txt/text_style.cc

+  if (has_text_height_behavior_override !=
+      other.has_text_height_behavior_override)
+    return false;
+  if (has_text_height_behavior_override &&


why not just text_height_behavior != other.text_height_behavior?

chinmaygarde · 2021-02-04T22:36:08Z

@Hixie Are you still planning on the detailed review?

chinmaygarde · 2021-02-04T22:37:17Z

Looks like the presubmit failure is real. The test needs to be updated.

LongCatIsLooong · 2021-02-19T02:34:05Z

Updated the PR to:

add LeadingDistribution to TextHeightBehavior, TextStyle and StrutStyle (instead of adding it to TextHeightBehavior and adding TextHeightBehavior to TextStyle and StrutStyle). How it works largely remains the same.
move some ui.TextStyle.toString tests from framework to text_test.dart

LongCatIsLooong · 2021-02-19T20:23:17Z

Ready for review.
@johnsonmh @Hixie @jason-simmons there's an API change since you last reviewed the PR. Could you take a look?

GaryQian · 2021-02-24T01:41:18Z

lib/ui/text.dart

 }

+/// {@macro dart.ui.leadingDistribution}
+enum LeadingDistribution {


Optional, but it seems that most text related enums/classes are prefixed with Text.

I don't have a particularly strong conviction either way, but It would also be reasonable to call this TextLeadingDistribution for consistency. That being said, as far as I know, the concept of leading is unique to text layout, so this naming should be sufficient (currently) to identify the feature uniquely.

GaryQian · 2021-02-24T01:44:49Z

lib/ui/text.dart

  ///   height after layout to be taller than specified here. The `fontSize` must
  ///   be provided for this property to take effect.
  ///
  /// * `leading`: The minimum amount of leading between lines as a multiple of


Probably want to improve documentation on this property as well to better distinguish it, since there are now two similarly named properties right next to each other.

GaryQian · 2021-02-24T01:48:08Z

third_party/txt/src/txt/paragraph_style.h

  kDisableFirstAscent = 0x1,
  kDisableLastDescent = 0x2,
  kDisableAll = 0x1 | 0x2,
+  kHalfLeading = 0x1 << 2,


I would prefer consistent naming in the engine and the framework. This was called even in framework. Two names for the same property makes this much more confusing to read and maintain.

johnsonmh

LGTM

johnsonmh · 2021-02-25T01:25:21Z

lib/web_ui/lib/src/engine/text/paragraph.dart

    List<String>? fontFamilyFallback,
    double? fontSize,
    double? height,
+    //TODO(LongCatIsLooong): implement leadingDistribution.


is this TODO meant to be here? It looks like this is implemented?

by that I meant currently setting leadingDistribution doesn't do anything on web and it needs to be implemented.

* 4f660da Roll Dart SDK from 63c6585d8499 to 6b7054122356 (1 revision) (flutter/engine#24625) * 25887bc Roll Skia from e79bb32365ea to 4ac9aadd306b (4 revisions) (flutter/engine#24630) * 1bdf5d3 TextStyle level leadingDistribution (flutter/engine#24025) * c4678d6 Fix input flow event logic (flutter/engine#24526)

This reverts commit 1bdf5d3.

… in flutter#24665 This reverts commit 6191270.

(#24668)

…#24665)

… in flutter#24665 (flutter#24668)

…#24665)

… in flutter#24665 (flutter#24668)

LongCatIsLooong added 2 commits January 25, 2021 14:04

wip

db75633

wip

401aca1

google-cla bot added the cla: yes label Jan 28, 2021

googlebot added the cla: yes label Jan 28, 2021

LongCatIsLooong commented Jan 28, 2021

View reviewed changes

Hixie reviewed Jan 28, 2021

View reviewed changes

docs

26a16e8

LongCatIsLooong force-pushed the block-level-text-height-behavior branch 3 times, most recently from b2c3923 to 547fc9e Compare February 2, 2021 06:05

fix tests

63f5fbf

LongCatIsLooong force-pushed the block-level-text-height-behavior branch from 547fc9e to 63f5fbf Compare February 2, 2021 06:26

LongCatIsLooong added 3 commits February 2, 2021 17:19

more tests

45ce33f

fix tests

c0b1bef

Merge remote-tracking branch 'upstream/master' into block-level-text-…

b703222

…height-behavior

LongCatIsLooong force-pushed the block-level-text-height-behavior branch from 94606b1 to b703222 Compare February 3, 2021 05:38

more tests

d6c9f49

LongCatIsLooong force-pushed the block-level-text-height-behavior branch from 426ba83 to d6c9f49 Compare February 3, 2021 19:36

LongCatIsLooong changed the title ~~[WIP] TextStyle level textHeightBehavior~~ TextStyle level textHeightBehavior Feb 3, 2021

LongCatIsLooong requested review from GaryQian and johnsonmh February 3, 2021 23:29

johnsonmh approved these changes Feb 4, 2021

View reviewed changes

fix tests

c312968

LongCatIsLooong force-pushed the block-level-text-height-behavior branch from b5fba22 to 45964a1 Compare February 19, 2021 01:58

LongCatIsLooong changed the title ~~TextStyle level textHeightBehavior~~ TextStyle level leadingDistribution Feb 22, 2021

LongCatIsLooong requested review from Hixie, jason-simmons and johnsonmh February 22, 2021 23:35

GaryQian reviewed Feb 24, 2021

View reviewed changes

review

f3e74f4

LongCatIsLooong force-pushed the block-level-text-height-behavior branch from 1843fce to f3e74f4 Compare February 24, 2021 21:06

johnsonmh approved these changes Feb 25, 2021

View reviewed changes

LongCatIsLooong added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 25, 2021

fluttergithubbot merged commit 1bdf5d3 into flutter:master Feb 25, 2021

LongCatIsLooong deleted the block-level-text-height-behavior branch February 25, 2021 19:54

engine-flutter-autoroll mentioned this pull request Feb 25, 2021

Roll Engine from a252ec09b74c to c4678d65a62b (4 revisions) flutter/flutter#76814

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 25, 2021

1bdf5d3 TextStyle level leadingDistribution (flutter/engine#24025)

c4d5876

LongCatIsLooong added a commit that referenced this pull request Feb 26, 2021

Revert "TextStyle level leadingDistribution (#24025)"

f9a73cc

This reverts commit 1bdf5d3.

LongCatIsLooong mentioned this pull request Feb 26, 2021

Revert "TextStyle level leadingDistribution" #24665

Merged

fluttergithubbot pushed a commit that referenced this pull request Feb 26, 2021

Revert "TextStyle level leadingDistribution (#24025)" (#24665)

6191270

LongCatIsLooong added a commit to LongCatIsLooong/engine that referenced this pull request Feb 26, 2021

Reland "TextStyle level leadingDistribution (flutter#24025)" reverted…

eee4f73

… in flutter#24665 This reverts commit 6191270.

fluttergithubbot pushed a commit that referenced this pull request Mar 1, 2021

Reland "TextStyle level leadingDistribution (#24025)" reverted in #24665

ef8353b

(#24668)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

TextStyle level leadingDistribution (flutter#24025)

8cca273

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Revert "TextStyle level leadingDistribution (flutter#24025)" (flutter…

c13c91c

…#24665)

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Reland "TextStyle level leadingDistribution (flutter#24025)" reverted…

b341546

… in flutter#24665 (flutter#24668)

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

TextStyle level leadingDistribution (flutter#24025)

4cf6d7b

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

Revert "TextStyle level leadingDistribution (flutter#24025)" (flutter…

1fa3c59

…#24665)

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

Reland "TextStyle level leadingDistribution (flutter#24025)" reverted…

17608ac

… in flutter#24665 (flutter#24668)

TextStyle level leadingDistribution #24025

TextStyle level leadingDistribution #24025

Uh oh!

Conversation

LongCatIsLooong commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screencast

TODO (after this PR)

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 28, 2021

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!

LongCatIsLooong Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Hixie commented Jan 28, 2021

Uh oh!

johnsonmh 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!

chinmaygarde commented Feb 4, 2021

Uh oh!

chinmaygarde commented Feb 4, 2021

Uh oh!

LongCatIsLooong commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LongCatIsLooong commented Feb 19, 2021

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!

johnsonmh 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

LongCatIsLooong commented Jan 28, 2021 •

edited

Loading

LongCatIsLooong Jan 28, 2021 •

edited

Loading

LongCatIsLooong commented Feb 19, 2021 •

edited

Loading