Unverified Commit 43de3365 authored by LongCatIsLooong's avatar LongCatIsLooong Committed by GitHub

Remove rounding from TextPainter (#127099)

To opt-in, run the tests with: `SKPARAGRAPH_REMOVE_ROUNDING_HACK=1 flutter test --dart-define=SKPARAGRAPH_REMOVE_ROUNDING_HACK=1 `

Migration plans:
1. Turn the flags on in CI, migrate customer tests if needed
1. Migrate internal customers
2. Remove the flag from skparagraph. Remove the framework flag with a manual engine roll.

Also fixes https://github.com/flutter/flutter/issues/52038
parent 07f7ffde
......@@ -271,9 +271,13 @@ class _TextLayout {
// object when it's no logner needed.
ui.Paragraph _paragraph;
/// Whether to enable the rounding in _applyFloatingPointHack and SkParagraph.
static const bool _shouldApplyFloatingPointHack = !bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK');
// TODO(LongCatIsLooong): https://github.com/flutter/flutter/issues/31707
// remove this hack as well as the flooring in `layout`.
static double _applyFloatingPointHack(double layoutValue) => layoutValue.ceilToDouble();
@pragma('vm:prefer-inline')
static double _applyFloatingPointHack(double layoutValue) => _shouldApplyFloatingPointHack ? layoutValue.ceilToDouble() : layoutValue;
/// Whether this layout has been invalidated and disposed.
///
......
......@@ -595,14 +595,19 @@ class TextStyle with Diagnosticable {
// in the [fontFamilyFallback] getter.
final String? _package;
/// The size of glyphs (in logical pixels) to use when painting the text.
/// The size of fonts (in logical pixels) to use when painting the text.
///
/// The value specified matches the dimension of the
/// [em square](https://fonts.google.com/knowledge/glossary/em) of the
/// underlying font, and more often then not isn't exactly the height or the
/// width of glyphs in the font.
///
/// During painting, the [fontSize] is multiplied by the current
/// `textScaleFactor` to let users make it easier to read text by increasing
/// its size.
///
/// [getParagraphStyle] will default to 14 logical pixels if the font size
/// isn't specified here.
/// The [getParagraphStyle] method defaults to 14 logical pixels if [fontSize]
/// is set to null.
final double? fontSize;
/// The typeface thickness to use when painting the text (e.g., bold).
......
......@@ -161,7 +161,8 @@ void main() {
// Also shows the previous page's title next to the back button.
expect(find.widgetWithText(CupertinoButton, 'An iPod'), findsOneWidget);
// 3 paddings + 1 test font character at font size 34.0.
expect(tester.getTopLeft(find.text('An iPod')).dx, 8.0 + 4.0 + 34.0 + 6.0);
// The epsilon is needed since the text theme has a negative letter spacing thus.
expect(tester.getTopLeft(find.text('An iPod')).dx, moreOrLessEquals(8.0 + 4.0 + 34.0 + 6.0, epsilon: 0.5));
});
testWidgets('Previous title is correct on first transition frame', (WidgetTester tester) async {
......@@ -267,7 +268,8 @@ void main() {
// from An iPod to Back (since An Internet communicator is too long to
// fit in the back button).
expect(find.widgetWithText(CupertinoButton, 'Back'), findsOneWidget);
expect(tester.getTopLeft(find.text('Back')).dx, 8.0 + 4.0 + 34.0 + 6.0);
// The epsilon is needed since the text theme has a negative letter spacing thus.
expect(tester.getTopLeft(find.text('Back')).dx, moreOrLessEquals(8.0 + 4.0 + 34.0 + 6.0, epsilon: 0.5));
});
testWidgets('Back swipe dismiss interrupted by route push', (WidgetTester tester) async {
......
......@@ -4726,7 +4726,7 @@ void main() {
child: CustomScrollView(
slivers: <Widget>[
const SliverAppBar.large(
title: Text(title),
title: Text(title, maxLines: 1),
),
SliverToBoxAdapter(
child: Container(
......@@ -4771,7 +4771,7 @@ void main() {
child: CustomScrollView(
slivers: <Widget>[
const SliverAppBar.medium(
title: Text(title),
title: Text(title, maxLines: 1),
),
SliverToBoxAdapter(
child: Container(
......@@ -4820,7 +4820,7 @@ void main() {
child: CustomScrollView(
slivers: <Widget>[
const SliverAppBar.large(
title: Text(title),
title: Text(title, maxLines: 1),
),
SliverToBoxAdapter(
child: Container(
......
......@@ -50,7 +50,10 @@ void main() {
expect(tester.getTopLeft(find.text('0')), const Offset(16, -4));
final RenderBox box = tester.renderObject(find.byType(Badge));
expect(box, paints..rrect(rrect: RRect.fromLTRBR(12, -4, 32, 12, const Radius.circular(8)), color: theme.colorScheme.error));
final RRect rrect = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? RRect.fromLTRBR(12, -4, 31.5, 12, const Radius.circular(8))
: RRect.fromLTRBR(12, -4, 32, 12, const Radius.circular(8));
expect(box, paints..rrect(rrect: rrect, color: theme.colorScheme.error));
});
testWidgets('Large Badge defaults with RTL', (WidgetTester tester) async {
......@@ -89,7 +92,10 @@ void main() {
expect(tester.getTopLeft(find.text('0')), const Offset(0, -4));
final RenderBox box = tester.renderObject(find.byType(Badge));
expect(box, paints..rrect(rrect: RRect.fromLTRBR(-4, -4, 16, 12, const Radius.circular(8)), color: theme.colorScheme.error));
final RRect rrect = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? RRect.fromLTRBR(-4, -4, 15.5, 12, const Radius.circular(8))
: RRect.fromLTRBR(-4, -4, 16, 12, const Radius.circular(8));
expect(box, paints..rrect(rrect: rrect, color: theme.colorScheme.error));
});
// Essentially the same as 'Large Badge defaults'
......@@ -143,7 +149,10 @@ void main() {
// T = alignment.top
// R = L + '0'.width + padding.width
// B = T + largeSize, R = largeSize/2
expect(box, paints..rrect(rrect: RRect.fromLTRBR(12, -4, 32, 12, const Radius.circular(8)), color: theme.colorScheme.error));
final RRect rrect = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? RRect.fromLTRBR(12, -4, 31.5, 12, const Radius.circular(8))
: RRect.fromLTRBR(12, -4, 32, 12, const Radius.circular(8));
expect(box, paints..rrect(rrect: rrect, color: theme.colorScheme.error));
await tester.pumpWidget(buildFrame(1000));
expect(find.text('999+'), findsOneWidget);
......
......@@ -465,18 +465,16 @@ void main() {
),
);
final double textWidth = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? width
: (width / 1.5).floorToDouble() * 1.5;
// The title is scaled and transformed to be 1.5 times bigger, when the
// FlexibleSpaceBar is fully expanded, thus we expect the width to be
// 1.5 times smaller than the full width. The height of the text is the same
// as the font size, with 10 dps bottom margin.
expect(
tester.getRect(find.byType(Text)),
Rect.fromLTRB(
0,
height - titleFontSize - 10,
(width / 1.5).floorToDouble() * 1.5,
height,
),
rectMoreOrLessEquals(Rect.fromLTRB(0, height - titleFontSize - 10, textWidth, height), epsilon: 0.0001),
);
});
......@@ -537,18 +535,16 @@ void main() {
// bottom edge.
const double bottomMargin = titleFontSize * (expandedTitleScale - 1);
final double textWidth = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? collapsedWidth
: (collapsedWidth / 3).floorToDouble() * 3;
// The title is scaled and transformed to be 3 times bigger, when the
// FlexibleSpaceBar is fully expanded, thus we expect the width to be
// 3 times smaller than the full width. The height of the text is the same
// as the font size, with 40 dps bottom margin to maintain its bottom position.
expect(
tester.getRect(title),
Rect.fromLTRB(
0,
height - titleFontSize - bottomMargin,
(collapsedWidth / 3).floorToDouble() * 3,
height,
),
rectMoreOrLessEquals(Rect.fromLTRB(0, height - titleFontSize - bottomMargin, textWidth, height), epsilon: 0.0001),
);
});
......
......@@ -5697,8 +5697,11 @@ void main() {
return false;
}
final Rect clipRect = arguments[0] as Rect;
// 133.3 is approximately 100 / 0.75 (_kFinalLabelScale)
expect(clipRect, rectMoreOrLessEquals(const Rect.fromLTWH(0, 0, 133.0, 16.0)));
// _kFinalLabelScale = 0.75
const double width = bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? 100 / 0.75
: 133.0;
expect(clipRect, rectMoreOrLessEquals(const Rect.fromLTWH(0, 0, width, 16.0), epsilon: 1e-5));
return true;
}),
);
......
......@@ -583,7 +583,7 @@ void main() {
// Padding at the top of the rail.
const double topPadding = 8.0;
// Width of a destination.
const double destinationWidth = 126.0;
const double destinationWidth = bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? 125.5 : 126.0;
// Height of a destination indicator with icon.
const double destinationHeight = 32.0;
// Space between the indicator and label.
......@@ -858,7 +858,7 @@ void main() {
// Padding at the top of the rail.
const double topPadding = 8.0;
// Width of a destination.
const double destinationWidth = 126.0;
const double destinationWidth = bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? 125.5 : 126.0;
// Height of a destination indicator with icon.
const double destinationHeight = 32.0;
// Space between the indicator and label.
......
......@@ -1045,7 +1045,7 @@ void main() {
child: Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(textScaleFactor: 1.3),
data: const MediaQueryData(textScaleFactor: 1.25),
child: Center(
child: OutlinedButton(
style: const ButtonStyle(
......@@ -1064,7 +1064,10 @@ void main() {
);
expect(tester.getSize(find.byType(OutlinedButton)), equals(const Size(88.0, 48.0)));
expect(tester.getSize(find.byType(Text)), const Size(55.0, 18.0));
expect(tester.getSize(find.byType(Text)), const Size(
bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? 52.5 : 53.0,
18.0,
));
// Set text scale large enough to expand text and button.
await tester.pumpWidget(
......
......@@ -607,8 +607,8 @@ void main() {
..line(
color: theme.colorScheme.primary,
strokeWidth: indicatorWeight,
p1: const Offset(65.5, indicatorY),
p2: const Offset(134.5, indicatorY),
p1: const Offset(bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? 65.75 : 65.5, indicatorY),
p2: const Offset(bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? 134.25 : 134.5, indicatorY),
),
);
});
......
......@@ -473,19 +473,31 @@ void main() {
const double indicatorWeight = 3.0;
final RRect rrect = const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? RRect.fromLTRBAndCorners(
64.75,
tabBarBox.size.height - indicatorWeight,
135.25,
tabBarBox.size.height,
topLeft: const Radius.circular(3.0),
topRight: const Radius.circular(3.0),
)
: RRect.fromLTRBAndCorners(
64.5,
tabBarBox.size.height - indicatorWeight,
135.5,
tabBarBox.size.height,
topLeft: const Radius.circular(3.0),
topRight: const Radius.circular(3.0),
);
expect(
tabBarBox,
paints
..rrect(
color: theme.colorScheme.primary,
rrect: RRect.fromLTRBAndCorners(
64.5,
tabBarBox.size.height - indicatorWeight,
135.5,
tabBarBox.size.height,
topLeft: const Radius.circular(3.0),
topRight: const Radius.circular(3.0),
),
rrect: rrect,
));
});
......
......@@ -608,7 +608,7 @@ void main() {
child: Directionality(
textDirection: TextDirection.ltr,
child: MediaQuery(
data: const MediaQueryData(textScaleFactor: 1.3),
data: const MediaQueryData(textScaleFactor: 1.25),
child: Center(
child: TextButton(
onPressed: () { },
......@@ -620,8 +620,14 @@ void main() {
),
);
expect(tester.getSize(find.byType(TextButton)), const Size(71.0, 48.0));
expect(tester.getSize(find.byType(Text)), const Size(55.0, 18.0));
const Size textButtonSize = bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? Size(68.5, 48.0)
: Size(69.0, 48.0);
const Size textSize = bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')
? Size(52.5, 18.0)
: Size(53.0, 18.0);
expect(tester.getSize(find.byType(TextButton)), textButtonSize);
expect(tester.getSize(find.byType(Text)), textSize);
// Set text scale large enough to expand text and button.
await tester.pumpWidget(
......
......@@ -744,7 +744,9 @@ void main() {
switch (materialType) {
case MaterialType.material2:
expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(154, 155)));
expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(281, 165)));
expect(tester.getBottomRight(find.text(selectTimeString)), equals(
const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? const Offset(280.5, 165) : const Offset(281, 165),
));
expect(tester.getBottomRight(find.text(okString)).dx, 644);
expect(tester.getBottomLeft(find.text(okString)).dx, 616);
expect(tester.getBottomRight(find.text(cancelString)).dx, 582);
......@@ -764,7 +766,9 @@ void main() {
switch (materialType) {
case MaterialType.material2:
expect(tester.getTopLeft(find.text(selectTimeString)), equals(const Offset(519, 155)));
expect(tester.getTopLeft(find.text(selectTimeString)), equals(
const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK') ? const Offset(519.5, 155) : const Offset(519, 155),
));
expect(tester.getBottomRight(find.text(selectTimeString)), equals(const Offset(646, 165)));
expect(tester.getBottomLeft(find.text(okString)).dx, 156);
expect(tester.getBottomRight(find.text(okString)).dx, 184);
......
......@@ -180,11 +180,11 @@ void main() {
);
// Metrics should be refreshed
// ignore: avoid_dynamic_calls
expect(state.numberLabelWidth - 46.0 < precisionErrorTolerance, isTrue);
expect(state.numberLabelWidth, lessThan(46.0 + precisionErrorTolerance));
// ignore: avoid_dynamic_calls
expect(state.numberLabelHeight - 23.0 < precisionErrorTolerance, isTrue);
expect(state.numberLabelHeight, lessThan(23.0 + precisionErrorTolerance));
// ignore: avoid_dynamic_calls
expect(state.numberLabelBaseline - 18.400070190429688 < precisionErrorTolerance, isTrue);
expect(state.numberLabelBaseline, lessThan(18.400070190429688 + precisionErrorTolerance));
final Element element = tester.element(find.byType(CupertinoTimerPicker));
expect(element.dirty, isTrue);
}, skip: isBrowser); // TODO(yjbanov): cupertino does not work on the Web yet: https://github.com/flutter/flutter/issues/41920
......
......@@ -1507,6 +1507,27 @@ void main() {
painter.dispose();
});
test('TextPainter line breaking does not round to integers', () {
if (! const bool.hasEnvironment('SKPARAGRAPH_REMOVE_ROUNDING_HACK')) {
return;
}
const double fontSize = 1.25;
const String text = '12345';
assert((fontSize * text.length).truncate() != fontSize * text.length);
final TextPainter painter = TextPainter(
textDirection: TextDirection.ltr,
text: const TextSpan(text: text, style: TextStyle(fontSize: fontSize)),
)..layout(maxWidth: text.length * fontSize);
expect(painter.maxIntrinsicWidth, text.length * fontSize);
switch (painter.computeLineMetrics()) {
case [ui.LineMetrics(width: final double width)]:
expect(width, text.length * fontSize);
case final List<ui.LineMetrics> metrics:
expect(metrics, hasLength(1));
}
}, skip: kIsWeb && !isCanvasKit); // [intended] Browsers seem to always round font/glyph metrics.
}
class MockCanvas extends Fake implements Canvas {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment