Skip to content

Commit

Permalink
Add a sentinel value for TextStyle.height (#149049)
Browse files Browse the repository at this point in the history
Fixes: flutter/flutter#58765

The rationale for the choice of the sentinel value: flutter/engine#52940
The exact value of `kTextHeightNone` should be kept as an implementation detail. It's unfortunate that the current value `0` is dangerously close to `TextStyle.height`'s valid domain. If we ever allow `TextStyle.height == 0` (which totally makes sense) then it shouldn't be difficult to change the const.
  • Loading branch information
LongCatIsLooong authored May 29, 2024
1 parent 90937b0 commit 557fca4
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 10 deletions.
2 changes: 1 addition & 1 deletion packages/flutter/lib/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
/// painting boxes.
library painting;

export 'dart:ui' show PlaceholderAlignment, Shadow, TextHeightBehavior, TextLeadingDistribution;
export 'dart:ui' show PlaceholderAlignment, Shadow, TextHeightBehavior, TextLeadingDistribution, kTextHeightNone;

export 'src/painting/alignment.dart';
export 'src/painting/basic_types.dart';
Expand Down
19 changes: 10 additions & 9 deletions packages/flutter/lib/src/painting/text_style.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import 'dart:ui' as ui show
Shadow,
StrutStyle,
TextStyle,
kTextHeightNone,
lerpDouble;

import 'package:flutter/foundation.dart';
Expand Down Expand Up @@ -640,14 +641,13 @@ class TextStyle with Diagnosticable {

/// The height of this text span, as a multiple of the font size.
///
/// When [height] is null or omitted, the line height will be determined
/// by the font's metrics directly, which may differ from the fontSize.
/// When [height] is non-null, the line height of the span of text will be a
/// multiple of [fontSize] and be exactly `fontSize * height` logical pixels
/// tall.
/// When [height] is [kTextHeightNone], the line height will be determined by
/// the font's metrics directly, which may differ from the fontSize. Otherwise
/// the line height of the span of text will be a multiple of [fontSize],
/// and be exactly `fontSize * height` logical pixels tall.
///
/// For most fonts, setting [height] to 1.0 is not the same as omitting or
/// setting height to null because the [fontSize] sets the height of the EM-square,
/// For most fonts, setting [height] to 1.0 is not the same as setting height
/// to [kTextHeightNone] because the [fontSize] sets the height of the EM-square,
/// which is different than the font provided metrics for line height. The
/// following diagram illustrates the difference between the font-metrics
/// defined line height and the line height produced with `height: 1.0`
Expand Down Expand Up @@ -954,7 +954,8 @@ class TextStyle with Diagnosticable {
/// [TextStyle] with a [FontWeight.w300].
///
/// If the underlying values are null, then the corresponding factors and/or
/// deltas must not be specified.
/// deltas must not be specified. Additionally, if [height] is [kTextHeightNone]
/// it will not be modified by this method.
///
/// If [foreground] is specified on this object, then applying [color] here
/// will have no effect and if [background] is specified on this object, then
Expand Down Expand Up @@ -1014,7 +1015,7 @@ class TextStyle with Diagnosticable {
letterSpacing: letterSpacing == null ? null : letterSpacing! * letterSpacingFactor + letterSpacingDelta,
wordSpacing: wordSpacing == null ? null : wordSpacing! * wordSpacingFactor + wordSpacingDelta,
textBaseline: textBaseline ?? this.textBaseline,
height: height == null ? null : height! * heightFactor + heightDelta,
height: (height == null || height == ui.kTextHeightNone) ? height : height! * heightFactor + heightDelta,
leadingDistribution: leadingDistribution ?? this.leadingDistribution,
locale: locale ?? this.locale,
foreground: foreground,
Expand Down
11 changes: 11 additions & 0 deletions packages/flutter/test/painting/text_painter_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1696,6 +1696,17 @@ void main() {
);
});

test('kTextHeightNone unsets the text height multiplier', () {
final TextPainter painter = TextPainter(
textDirection: TextDirection.ltr,
text: const TextSpan(
style: TextStyle(fontSize: 10, height: 1000),
children: <TextSpan>[TextSpan(text: 'A', style: TextStyle(height: kTextHeightNone))],
),
)..layout();
expect(painter.height, 10);
});

test('TextPainter dispatches memory events', () async {
await expectLater(
await memoryEvents(() => TextPainter().dispose(), TextPainter),
Expand Down
5 changes: 5 additions & 0 deletions packages/flutter/test/painting/text_style_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,11 @@ void main() {
style.apply(leadingDistribution: TextLeadingDistribution.proportional).leadingDistribution,
TextLeadingDistribution.proportional,
);

expect(
const TextStyle(height: kTextHeightNone).apply(heightFactor: 1000, heightDelta: 1000).height,
kTextHeightNone,
);
});

test('TextStyle fontFamily and package', () {
Expand Down

0 comments on commit 557fca4

Please sign in to comment.