Allow unsetting TextStyle.height (#52940)

Introduces a sentinel value `kTextHeightNone` for `ui.TextStyle.height` which can be used to "unset" the current `TextStyle.height` (and for consistency, it applies to `StructStyle.height` and `ParagraphStyle.height` too). Documentation of `TextStyle.height` can be found [here](https://main-api.flutter.dev/flutter/painting/TextStyle/height.html) (the one from `painting` library not `dart:ui`).
 
part of flutter/flutter#58765: currently `TextStyle.height` uses `null` as the sentinel value for "no height multiplier specified, use the font height", which has conflicting semantics: it means the height multiplier is not set (so the span height is determined by font metrics) but in reality it also means the height should inherit from its parent span (or in `copyWith` context, it means do not override the height).

The new sentinel value `kTextHeightNone` is currently set to `0.0`. This is because skparagraph internally uses 0 for "no height multiplier", so using 0 should minimize the behavior change: 
https://github.com/google/skia/blob/62f369c759947272dfdd2d8f060afadbcc361e79/modules/skparagraph/src/Run.cpp#L65-L67

This MAY still change the current behavior: for consistency setting `StructStyle.height` / `ParagraphStyle.height` to the sentinel value also unsets the height multiplier which may not be the current behavior.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Author: LongCatIsLooong

lib/ui/text.dart

@@ -3,6 +3,12 @@
 // found in the LICENSE file.
 part of dart.ui;
 
+/// A [TextStyle.height] value that indicates the text span should take
+/// the height defined by the font, which may not be exactly the height of
+/// [TextStyle.fontSize].
+// To change the sentinel value, search for "kTextHeightNone" in the source code.
+const double kTextHeightNone = 0.0;
+
 /// Whether to use the italic type variation of glyphs in the font.
 ///
 /// Some modern fonts allow this to be selected in a more fine-grained manner.
@@ -1687,10 +1693,10 @@ 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. Omitting `height` will allow the line height
+  /// * `height`: The height of this text span, as a multiplier of the font size. Setting the `height` to `kTextHeightNone` will allow the line height
   ///   to take the height as defined by the font, which may not be exactly the height of the fontSize.
-  /// * `leadingDistribution`: When `height` is specified, how the extra vertical space should be distributed over and under the text. Defaults
-  ///   to the paragraph's [TextHeightBehavior] if left unspecified.
+  /// * `leadingDistribution`: When `height` is set to a non-null that is not `kTextHeightNone`, how the extra vertical space should be distributed over and under the text.
+  ///   Defaults to the paragraph's [TextHeightBehavior] if left unspecified.
   /// * `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.
@@ -1825,6 +1831,7 @@ class TextStyle {
   @override
   String toString() {
     final List<String>? fontFamilyFallback = _fontFamilyFallback;
+    final String heightText = _encoded[0] & 0x02000 == 0x02000  ? (_height == kTextHeightNone ? 'kTextHeightNone' : '${_height}x') : 'unspecified';
     return 'TextStyle('
              'color: ${              _encoded[0] & 0x00002 == 0x00002  ? Color(_encoded[1])                           : "unspecified"}, '
              'decoration: ${         _encoded[0] & 0x00004 == 0x00004  ? TextDecoration._(_encoded[2])                : "unspecified"}, '
@@ -1843,7 +1850,7 @@ class TextStyle {
              'fontSize: ${           _encoded[0] & 0x00400 == 0x00400  ? _fontSize                                    : "unspecified"}, '
              'letterSpacing: ${      _encoded[0] & 0x00800 == 0x00800  ? "${_letterSpacing}x"                         : "unspecified"}, '
              'wordSpacing: ${        _encoded[0] & 0x01000 == 0x01000  ? "${_wordSpacing}x"                           : "unspecified"}, '
-             'height: ${             _encoded[0] & 0x02000 == 0x02000  ? "${_height}x"                                : "unspecified"}, '
+             'height: $heightText, '
              'leadingDistribution: ${_leadingDistribution ?? "unspecified"}, '
              'locale: ${             _encoded[0] & 0x04000 == 0x04000  ? _locale                                      : "unspecified"}, '
              'background: ${         _encoded[0] & 0x08000 == 0x08000  ? _background                                  : "unspecified"}, '
@@ -1923,7 +1930,9 @@ Int32List _encodeParagraphStyle(
     result[0] |= 1 << 8;
     // Passed separately to native.
   }
-  if (height != null) {
+  // Paragraph styles are unique in a paragraph, there is no inheriting so
+  // height == null and height == kTextHeightNone are semantically equivalent.
+  if (height != null && height != kTextHeightNone) {
     result[0] |= 1 << 9;
     // Passed separately to native.
   }
@@ -1973,9 +1982,10 @@ class ParagraphStyle {
   ///
   /// * `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`.
+  ///   [TextStyle.height]. Omitting `height` here (or setting it to
+  ///   [kTextHeightNone]) 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`.
   ///
   /// * `textHeightBehavior`: Specifies how the `height` multiplier is
   ///   applied to ascent of the first line and the descent of the last line.
@@ -2110,9 +2120,12 @@ ByteData _encodeStrut(
   FontWeight? fontWeight,
   FontStyle? fontStyle,
   bool? forceStrutHeight) {
+  // Strut styles are unique in a paragraph, there is no inheriting so
+  // height == null and height == kTextHeightNone are semantically equivalent.
+  final bool hasHeightOverride = height != null && height != kTextHeightNone;
   if (fontFamily == null &&
     fontSize == null &&
-    height == null &&
+    !hasHeightOverride &&
     leadingDistribution == null &&
     leading == null &&
     fontWeight == null &&
@@ -2146,7 +2159,7 @@ ByteData _encodeStrut(
     data.setFloat32(byteCount, fontSize, _kFakeHostEndian);
     byteCount += 4;
   }
-  if (height != null) {
+  if (hasHeightOverride) {
     bitmask |= 1 << 5;
     data.setFloat32(byteCount, height, _kFakeHostEndian);
     byteCount += 4;
@@ -2186,12 +2199,13 @@ class StrutStyle {
   /// * `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. 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.
+  ///   `height` (or setting it to [kTextHeightNone]) 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

lib/ui/text/paragraph_builder.cc

@@ -30,6 +30,8 @@
 namespace flutter {
 namespace {
 
+const double kTextHeightNone = 0.0;
+
 // TextStyle
 
 const int kTSLeadingDistributionIndex = 0;
@@ -445,7 +447,7 @@ void ParagraphBuilder::pushStyle(const tonic::Int32List& encoded,
 
   if (mask & kTSHeightMask) {
     style.height = height;
-    style.has_height_override = true;
+    style.has_height_override = style.height != kTextHeightNone;
   }
 
   if (mask & kTSLocaleMask) {

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

@@ -39,7 +39,7 @@ class CkParagraphStyle implements ui.ParagraphStyle {
           maxLines,
           _computeEffectiveFontFamily(fontFamily),
           fontSize,
-          height,
+          height == ui.kTextHeightNone ? null : height,
           textHeightBehavior,
           fontWeight,
           fontStyle,
@@ -55,7 +55,7 @@ class CkParagraphStyle implements ui.ParagraphStyle {
         _originalFontFamily = fontFamily,
         _effectiveFontFamily = _computeEffectiveFontFamily(fontFamily),
         _fontSize = fontSize,
-        _height = height,
+        _height = height == ui.kTextHeightNone ? null : height,
         _textHeightBehavior = textHeightBehavior,
         _strutStyle = strutStyle,
         _ellipsis = ellipsis,
@@ -413,6 +413,9 @@ class CkTextStyle implements ui.TextStyle {
   /// The values in this text style are used unless [other] specifically
   /// overrides it.
   CkTextStyle mergeWith(CkTextStyle other) {
+    final double? textHeight = other.height == ui.kTextHeightNone
+      ? null
+      : (other.height ?? height);
     return CkTextStyle._(
       color: other.color ?? color,
       decoration: other.decoration ?? decoration,
@@ -429,7 +432,7 @@ class CkTextStyle implements ui.TextStyle {
       fontSize: other.fontSize ?? fontSize,
       letterSpacing: other.letterSpacing ?? letterSpacing,
       wordSpacing: other.wordSpacing ?? wordSpacing,
-      height: other.height ?? height,
+      height: textHeight,
       leadingDistribution: other.leadingDistribution ?? leadingDistribution,
       locale: other.locale ?? locale,
       background: other.background ?? background,
@@ -699,7 +702,7 @@ class CkStrutStyle implements ui.StrutStyle {
   })  : _fontFamily = _computeEffectiveFontFamily(fontFamily),
         _fontFamilyFallback = ui_web.debugEmulateFlutterTesterEnvironment ? null : fontFamilyFallback,
         _fontSize = fontSize,
-        _height = height,
+        _height = height == ui.kTextHeightNone ? null : height,
         _leadingDistribution = leadingDistribution,
         _leading = leading,
         _fontWeight = fontWeight,

lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart

@@ -594,7 +594,7 @@ final class SkwasmStrutStyle extends SkwasmObjectWrapper<RawStrutStyle> implemen
     if (fontSize != null) {
       strutStyleSetFontSize(handle, fontSize);
     }
-    if (height != null) {
+    if (height != null && height != ui.kTextHeightNone) {
       strutStyleSetHeight(handle, height);
     }
     if (leadingDistribution != null) {
@@ -734,7 +734,7 @@ class SkwasmParagraphStyle extends SkwasmObjectWrapper<RawParagraphStyle> implem
     if (maxLines != null) {
       paragraphStyleSetMaxLines(handle, maxLines);
     }
-    if (height != null) {
+    if (height != null && height != ui.kTextHeightNone) {
       paragraphStyleSetHeight(handle, height);
     }
     if (textHeightBehavior != null) {

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

@@ -401,33 +401,29 @@ abstract class StyleNode {
   /// The resolved text style is equivalent to the entire ascendent chain of
   /// parent style nodes.
   EngineTextStyle resolveStyle() {
-    final EngineTextStyle? style = _cachedStyle;
-    if (style == null) {
-      return _cachedStyle ??= EngineTextStyle(
-        color: _color,
-        decoration: _decoration,
-        decorationColor: _decorationColor,
-        decorationStyle: _decorationStyle,
-        decorationThickness: _decorationThickness,
-        fontWeight: _fontWeight,
-        fontStyle: _fontStyle,
-        textBaseline: _textBaseline,
-        fontFamily: _fontFamily,
-        fontFamilyFallback: _fontFamilyFallback,
-        fontFeatures: _fontFeatures,
-        fontVariations: _fontVariations,
-        fontSize: _fontSize,
-        letterSpacing: _letterSpacing,
-        wordSpacing: _wordSpacing,
-        height: _height,
-        leadingDistribution: _leadingDistribution,
-        locale: _locale,
-        background: _background,
-        foreground: _foreground,
-        shadows: _shadows,
-      );
-    }
-    return style;
+    return _cachedStyle ??= EngineTextStyle(
+      color: _color,
+      decoration: _decoration,
+      decorationColor: _decorationColor,
+      decorationStyle: _decorationStyle,
+      decorationThickness: _decorationThickness,
+      fontWeight: _fontWeight,
+      fontStyle: _fontStyle,
+      textBaseline: _textBaseline,
+      fontFamily: _fontFamily,
+      fontFamilyFallback: _fontFamilyFallback,
+      fontFeatures: _fontFeatures,
+      fontVariations: _fontVariations,
+      fontSize: _fontSize,
+      letterSpacing: _letterSpacing,
+      wordSpacing: _wordSpacing,
+      height: _height,
+      leadingDistribution: _leadingDistribution,
+      locale: _locale,
+      background: _background,
+      foreground: _foreground,
+      shadows: _shadows,
+    );
   }
 
   ui.Color? get _color;
@@ -510,7 +506,9 @@ class ChildStyleNode extends StyleNode {
   double? get _wordSpacing => style.wordSpacing ?? parent._wordSpacing;
 
   @override
-  double? get _height => style.height ?? parent._height;
+  double? get _height {
+    return style.height == ui.kTextHeightNone ? null : (style.height ?? parent._height);
+  }
 
   @override
   ui.TextLeadingDistribution? get _leadingDistribution => style.leadingDistribution ?? parent._leadingDistribution;

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

@@ -435,7 +435,7 @@ class EngineParagraphStyle implements ui.ParagraphStyle {
     this.maxLines,
     this.fontFamily,
     this.fontSize,
-    this.height,
+    double? height,
     ui.TextHeightBehavior? textHeightBehavior,
     this.fontWeight,
     this.fontStyle,
@@ -444,7 +444,8 @@ class EngineParagraphStyle implements ui.ParagraphStyle {
     this.locale,
   })  : _textHeightBehavior = textHeightBehavior,
         // TODO(mdebbar): add support for strut style., b/128317744
-        _strutStyle = strutStyle as EngineStrutStyle?;
+        _strutStyle = strutStyle as EngineStrutStyle?,
+        height = height == ui.kTextHeightNone ? null : height;
 
   final ui.TextAlign? textAlign;
   final ui.TextDirection? textDirection;
@@ -808,7 +809,7 @@ class EngineStrutStyle implements ui.StrutStyle {
   })  : _fontFamily = fontFamily,
         _fontFamilyFallback = fontFamilyFallback,
         _fontSize = fontSize,
-        _height = height,
+        _height = height == ui.kTextHeightNone ? null : height,
         _leadingDistribution = leadingDistribution,
         _leading = leading,
         _fontWeight = fontWeight,

lib/web_ui/lib/text.dart

@@ -4,6 +4,11 @@
 
 part of ui;
 
+// This constant must be consistent with `kTextHeightNone` defined in
+// flutter/lib/ui/text.dart.
+// To change the sentinel value, search for "kTextHeightNone" in the source code.
+const double kTextHeightNone = 0.0;
+
 enum FontStyle {
   normal,
   italic,

lib/web_ui/skwasm/text/text_style.cpp

@@ -6,6 +6,8 @@
 #include "../wrappers.h"
 #include "third_party/skia/modules/skparagraph/include/Paragraph.h"
 
+const double kTextHeightNone = 0.0;
+
 using namespace skia::textlayout;
 using namespace Skwasm;
 
@@ -96,7 +98,7 @@ SKWASM_EXPORT void textStyle_setWordSpacing(TextStyle* style,
 
 SKWASM_EXPORT void textStyle_setHeight(TextStyle* style, SkScalar height) {
   style->setHeight(height);
-  style->setHeightOverride(true);
+  style->setHeightOverride(height != kTextHeightNone);
 }
 
 SKWASM_EXPORT void textStyle_setHalfLeading(TextStyle* style,

lib/web_ui/test/ui/paragraph_builder_test.dart

@@ -7,6 +7,7 @@ import 'package:test/test.dart';
 import 'package:ui/ui.dart';
 
 import '../common/test_initialization.dart';
+import 'utils.dart';
 
 void main() {
   internalBootstrapBrowserTest(() => testMain);
@@ -81,4 +82,46 @@ Future<void> testMain() async {
       returnsNormally,
     );
   });
+
+  test('kTextHeightNone unsets the height multiplier', () {
+    const double fontSize = 10;
+    const String text = 'A';
+    final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle(fontSize: fontSize, height: 10));
+    builder.pushStyle(TextStyle(height: kTextHeightNone));
+    builder.addText(text);
+    final Paragraph paragraph = builder.build()
+      ..layout(const ParagraphConstraints(width: 1000));
+    // The height should be much smaller than fontSize * 10.
+    expect(paragraph.height, lessThan(2 * fontSize));
+  });
+
+  test('kTextHeightNone ParagraphStyle', () {
+    const double fontSize = 10;
+    final ParagraphBuilder builder = ParagraphBuilder(
+      ParagraphStyle(fontSize: fontSize, height: kTextHeightNone, fontFamily: 'FlutterTest'),
+    );
+    builder.addText('A');
+    final Paragraph paragraph = builder.build()
+      ..layout(const ParagraphConstraints(width: 1000));
+    // The height should be much smaller than fontSize * 10.
+    expect(paragraph.height, lessThan(2 * fontSize));
+  });
+
+  test('kTextHeightNone StrutStyle', () {
+    const double fontSize = 10;
+    final ParagraphBuilder builder = ParagraphBuilder(
+      ParagraphStyle(
+        fontSize: 100,
+        fontFamily: 'FlutterTest',
+        strutStyle: StrutStyle(forceStrutHeight: true, height: kTextHeightNone, fontSize: fontSize),
+      ),
+    );
+    builder.addText('A');
+    final Paragraph paragraph = builder.build()
+      ..layout(const ParagraphConstraints(width: 1000));
+    // The height should be much smaller than fontSize * 10.
+    expect(paragraph.height, lessThan(2 * fontSize));
+  },
+    skip: isHtml, // The HTML renderer does not support struts.
+  );
 }