TextStyle.height property as a multiple of font size instead of multiple of ascent+descent+leading. (flutter#9041)

Author: GaryQian

lib/stub_ui/lib/src/ui/text.dart

@@ -298,7 +298,8 @@ class TextStyle {
   /// * `letterSpacing`: The amount of space (in logical pixels) to add between each letter.
   /// * `wordSpacing`: The amount of space (in logical pixels) to add at each sequence of white-space (i.e. between each word).
   /// * `textBaseline`: The common baseline that should be aligned between this text span and its parent text span, or, for the root text spans, with the line box.
-  /// * `height`: The height of this text span, as a multiple of the font size.
+  /// * `height`: The height of this text span, as a multiplier of the font size. Omitting `height` will allow the line height
+  ///   to take the height as defined by the font, which may not be exactly the height of the fontSize.
   /// * `locale`: The locale used to select region-specific glyphs.
   /// * `background`: The paint drawn as a background for the text.
   /// * `foreground`: The paint used to draw the text. If this is specified, `color` must be null.
@@ -623,13 +624,11 @@ class ParagraphStyle {
   /// * `fontSize`: The size of glyphs (in logical pixels) to use when painting
   ///   the text.
   ///
-  /// * `height`: The minimum height of the line boxes, as a multiple of the
-  ///   font size. The lines of the paragraph will be at least
-  ///   `(height + leading) * fontSize` tall when fontSize
-  ///   is not null. When fontSize is null, there is no minimum line height. Tall
-  ///   glyphs due to baseline alignment or large [TextStyle.fontSize] may cause
-  ///   the actual line height after layout to be taller than specified here.
-  ///   [fontSize] must be provided for this property to take effect.
+  /// * `height`: The fallback height of the spans as a multiplier of the font
+  ///   size. The fallback height is used when no height is provided through
+  ///   [TextStyle.height]. Omitting `height` here and in [TextStyle] will allow
+  ///   the line height to take the height as defined by the font, which may not
+  ///   be exactly the height of the `fontSize`.
   ///
   /// * `fontWeight`: The typeface thickness to use when painting the text
   ///   (e.g., bold).
@@ -764,22 +763,24 @@ class StrutStyle {
   /// * `fontFamily`: The name of the font to use when painting the text (e.g.,
   ///   Roboto).
   ///
-  /// * `fontFamilyFallback`: An ordered list of font family names that will be searched for when
-  ///    the font in `fontFamily` cannot be found.
+  /// * `fontFamilyFallback`: An ordered list of font family names that will be
+  ///    searched for when the font in `fontFamily` cannot be found.
   ///
   /// * `fontSize`: The size of glyphs (in logical pixels) to use when painting
   ///   the text.
   ///
-  /// * `lineHeight`: The minimum height of the line boxes, as a multiple of the
+  /// * `height`: The minimum height of the line boxes, as a multiplier of the
   ///   font size. The lines of the paragraph will be at least
-  ///   `(lineHeight + leading) * fontSize` tall when fontSize
-  ///   is not null. When fontSize is null, there is no minimum line height. Tall
-  ///   glyphs due to baseline alignment or large [TextStyle.fontSize] may cause
-  ///   the actual line height after layout to be taller than specified here.
-  ///   [fontSize] must be provided for this property to take effect.
+  ///   `(height + leading) * fontSize` tall when `fontSize` is not null. Omitting
+  ///   `height` will allow the minimum line height to take the height as defined
+  ///   by the font, which may not be exactly the height of the `fontSize`. When
+  ///   `fontSize` is null, there is no minimum line height. Tall glyphs due to
+  ///   baseline alignment or large [TextStyle.fontSize] may cause the actual line
+  ///   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
-  ///   the font size. [fontSize] must be provided for this property to take effect.
+  ///   the font size. `fontSize` must be provided for this property to take effect.
   ///
   /// * `fontWeight`: The typeface thickness to use when painting the text
   ///   (e.g., bold).
@@ -788,11 +789,11 @@ class StrutStyle {
   ///   italics).
   ///
   /// * `forceStrutHeight`: When true, the paragraph will force all lines to be exactly
-  ///   `(lineHeight + leading) * fontSize` tall from baseline to baseline.
+  ///   `(height + leading) * fontSize` tall from baseline to baseline.
   ///   [TextStyle] is no longer able to influence the line height, and any tall
-  ///   glyphs may overlap with lines above. If a [fontFamily] is specified, the
+  ///   glyphs may overlap with lines above. If a `fontFamily` is specified, the
   ///   total ascent of the first line will be the min of the `Ascent + half-leading`
-  ///   of the [fontFamily] and `(lineHeight + leading) * fontSize`. Otherwise, it
+  ///   of the `fontFamily` and `(height + leading) * fontSize`. Otherwise, it
   ///   will be determined by the Ascent + half-leading of the first text.
   StrutStyle({
     String fontFamily,

lib/ui/text.dart

@@ -527,7 +527,8 @@ class TextStyle {
   /// * `letterSpacing`: The amount of space (in logical pixels) to add between each letter.
   /// * `wordSpacing`: The amount of space (in logical pixels) to add at each sequence of white-space (i.e. between each word).
   /// * `textBaseline`: The common baseline that should be aligned between this text span and its parent text span, or, for the root text spans, with the line box.
-  /// * `height`: The height of this text span, as a multiplier of the font size.
+  /// * `height`: The height of this text span, as a multiplier of the font size. Omitting `height` will allow the line height
+  ///   to take the height as defined by the font, which may not be exactly the height of the fontSize.
   /// * `locale`: The locale used to select region-specific glyphs.
   /// * `background`: The paint drawn as a background for the text.
   /// * `foreground`: The paint used to draw the text. If this is specified, `color` must be null.
@@ -776,9 +777,11 @@ class ParagraphStyle {
   /// * `fontSize`: The fallback size of glyphs (in logical pixels) to
   ///   use when painting the text. This is used when there is no [TextStyle].
   ///
-  /// * `height`: The height of the spans as a multiplier of the font size. The
-  ///   fallback height to use when no height is provided in through
-  ///   [TextStyle.height].
+  /// * `height`: The fallback height of the spans as a multiplier of the font
+  ///   size. The fallback height is used when no height is provided through
+  ///   [TextStyle.height]. Omitting `height` here and in [TextStyle] will allow
+  ///   the line height to take the height as defined by the font, which may not
+  ///   be exactly the height of the `fontSize`.
   ///
   /// * `fontWeight`: The typeface thickness to use when painting the text
   ///   (e.g., bold).
@@ -962,22 +965,24 @@ class StrutStyle {
   /// * `fontFamily`: The name of the font to use when painting the text (e.g.,
   ///   Roboto).
   ///
-  /// * `fontFamilyFallback`: An ordered list of font family names that will be searched for when
-  ///    the font in `fontFamily` cannot be found.
+  /// * `fontFamilyFallback`: An ordered list of font family names that will be
+  ///    searched for when the font in `fontFamily` cannot be found.
   ///
   /// * `fontSize`: The size of glyphs (in logical pixels) to use when painting
   ///   the text.
   ///
   /// * `height`: The minimum height of the line boxes, as a multiplier of the
-  ///   font size. The lines of the paragraph will be at least `(height + leading)
-  ///   * fontSize` tall when fontSize is not null. When fontSize is null, there
-  ///   is no minimum line height. Tall glyphs due to baseline alignment or large
-  ///   [TextStyle.fontSize] may cause the actual line height after layout to be
-  ///   taller than specified here. [fontSize] must be provided for this property
-  ///   to take effect.
+  ///   font size. The lines of the paragraph will be at least
+  ///   `(height + leading) * fontSize` tall when `fontSize` is not null. Omitting
+  ///   `height` will allow the minimum line height to take the height as defined
+  ///   by the font, which may not be exactly the height of the `fontSize`. When
+  ///   `fontSize` is null, there is no minimum line height. Tall glyphs due to
+  ///   baseline alignment or large [TextStyle.fontSize] may cause the actual line
+  ///   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
-  ///   the font size. [fontSize] must be provided for this property to take effect.
+  ///   the font size. `fontSize` must be provided for this property to take effect.
   ///
   /// * `fontWeight`: The typeface thickness to use when painting the text
   ///   (e.g., bold).
@@ -988,9 +993,9 @@ class StrutStyle {
   /// * `forceStrutHeight`: When true, the paragraph will force all lines to be exactly
   ///   `(height + leading) * fontSize` tall from baseline to baseline.
   ///   [TextStyle] is no longer able to influence the line height, and any tall
-  ///   glyphs may overlap with lines above. If a [fontFamily] is specified, the
+  ///   glyphs may overlap with lines above. If a `fontFamily` is specified, the
   ///   total ascent of the first line will be the min of the `Ascent + half-leading`
-  ///   of the [fontFamily] and `(height + leading) * fontSize`. Otherwise, it
+  ///   of the `fontFamily` and `(height + leading) * fontSize`. Otherwise, it
   ///   will be determined by the Ascent + half-leading of the first text.
   StrutStyle({
     String fontFamily,

lib/ui/text/paragraph_builder.cc

@@ -204,6 +204,7 @@ void decodeStrut(Dart_Handle strut_data,
   }
   if (mask & sHeightMask) {
     paragraph_style.strut_height = float_data[float_count++];
+    paragraph_style.strut_has_height_override = true;
   }
   if (mask & sLeadingMask) {
     paragraph_style.strut_leading = float_data[float_count++];
@@ -261,6 +262,7 @@ ParagraphBuilder::ParagraphBuilder(
 
   if (mask & psHeightMask) {
     style.height = height;
+    style.has_height_override = true;
   }
 
   if (mask & psStrutStyleMask) {
@@ -401,6 +403,7 @@ void ParagraphBuilder::pushStyle(tonic::Int32List& encoded,
 
   if (mask & tsHeightMask) {
     style.height = height;
+    style.has_height_override = true;
   }
 
   if (mask & tsLocaleMask) {

third_party/txt/src/txt/paragraph.cc

@@ -538,14 +538,30 @@ void Paragraph::ComputeStrut(StrutMetrics* strut, SkFont& font) {
     SkFontMetrics strut_metrics;
     font.getMetrics(&strut_metrics);
 
-    strut->ascent = paragraph_style_.strut_height * -strut_metrics.fAscent;
-    strut->descent = paragraph_style_.strut_height * strut_metrics.fDescent;
-    strut->leading =
-        // Use font's leading if there is no user specified strut leading.
-        paragraph_style_.strut_leading < 0
-            ? strut_metrics.fLeading
-            : (paragraph_style_.strut_leading *
-               (strut_metrics.fDescent - strut_metrics.fAscent));
+    if (paragraph_style_.strut_has_height_override) {
+      double metrics_height = -strut_metrics.fAscent + strut_metrics.fDescent;
+      strut->ascent = (-strut_metrics.fAscent / metrics_height) *
+                      paragraph_style_.strut_height *
+                      paragraph_style_.strut_font_size;
+      strut->descent = (strut_metrics.fDescent / metrics_height) *
+                       paragraph_style_.strut_height *
+                       paragraph_style_.strut_font_size;
+      strut->leading =
+          // Zero leading if there is no user specified strut leading.
+          paragraph_style_.strut_leading < 0
+              ? 0
+              : (paragraph_style_.strut_leading *
+                 paragraph_style_.strut_font_size);
+    } else {
+      strut->ascent = -strut_metrics.fAscent;
+      strut->descent = strut_metrics.fDescent;
+      strut->leading =
+          // Use font's leading if there is no user specified strut leading.
+          paragraph_style_.strut_leading < 0
+              ? strut_metrics.fLeading
+              : (paragraph_style_.strut_leading *
+                 paragraph_style_.strut_font_size);
+    }
     strut->half_leading = strut->leading / 2;
     strut->line_height = strut->ascent + strut->descent + strut->leading;
   }
@@ -1073,11 +1089,21 @@ void Paragraph::Layout(double width, bool force) {
                                    const TextStyle& style,
                                    PlaceholderRun* placeholder_run) {
       if (!strut_.force_strut) {
-        double ascent =
-            (-metrics.fAscent + metrics.fLeading / 2) * style.height;
-        double descent =
-            (metrics.fDescent + metrics.fLeading / 2) * style.height;
-
+        double ascent;
+        double descent;
+        if (style.has_height_override) {
+          // Scale the ascent and descent such that the sum of ascent and
+          // descent is `fontsize * style.height * style.font_size`.
+          double metrics_height = -metrics.fAscent + metrics.fDescent;
+          ascent = (-metrics.fAscent / metrics_height) * style.height *
+                   style.font_size;
+          descent = (metrics.fDescent / metrics_height) * style.height *
+                    style.font_size;
+        } else {
+          // Use the font-provided ascent, descent, and leading directly.
+          ascent = (-metrics.fAscent + metrics.fLeading / 2);
+          descent = (metrics.fDescent + metrics.fLeading / 2);
+        }
         ComputePlaceholder(placeholder_run, ascent, descent);
 
         max_ascent = std::max(ascent, max_ascent);

third_party/txt/src/txt/paragraph_style.cc

@@ -14,10 +14,10 @@
  * limitations under the License.
  */
 
-#include <vector>
-
 #include "paragraph_style.h"
 
+#include <vector>
+
 namespace txt {
 
 TextStyle ParagraphStyle::GetTextStyle() const {
@@ -30,6 +30,7 @@ TextStyle ParagraphStyle::GetTextStyle() const {
   }
   result.locale = locale;
   result.height = height;
+  result.has_height_override = has_height_override;
   return result;
 }
 

third_party/txt/src/txt/paragraph_style.h

@@ -50,6 +50,7 @@ class ParagraphStyle {
   std::string font_family = "";
   double font_size = 14;
   double height = 1;
+  bool has_height_override = false;
 
   // Strut properties. strut_enabled must be set to true for the rest of the
   // properties to take effect.
@@ -60,6 +61,7 @@ class ParagraphStyle {
   std::vector<std::string> strut_font_families;
   double strut_font_size = 14;
   double strut_height = 1;
+  bool strut_has_height_override = false;
   double strut_leading = -1;  // Negative to use font's default leading. [0,inf)
                               // to use custom leading as a ratio of font size.
   bool force_strut_height = false;

third_party/txt/src/txt/text_style.cc

@@ -15,6 +15,7 @@
  */
 
 #include "text_style.h"
+
 #include "font_style.h"
 #include "font_weight.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -46,6 +47,8 @@ bool TextStyle::equals(const TextStyle& other) const {
     return false;
   if (height != other.height)
     return false;
+  if (has_height_override != other.has_height_override)
+    return false;
   if (locale != other.locale)
     return false;
   if (foreground != other.foreground)

third_party/txt/src/txt/text_style.h

@@ -51,6 +51,7 @@ class TextStyle {
   double letter_spacing = 0.0;
   double word_spacing = 0.0;
   double height = 1.0;
+  bool has_height_override = false;
   std::string locale;
   bool has_background = false;
   SkPaint background;