From 7014f7c89a943d75fa955461e8a1ba43067f205c Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 28 Feb 2024 15:32:05 -0800 Subject: [PATCH] Fix a few issues rendering text with Skwasm. This fixes https://github.com/flutter/flutter/issues/141001 This also fixes https://github.com/flutter/flutter/issues/143743 * We need to always call `setStrutEnabled(true)` on `StrutStyle`. * `getLineMetricsAt` had reversed ternary logic. * Ported unit tests from CanvasKit over to UI to cover Skwasm and HTML * The HTML renderer should return 0 line metrics for an empty paragraph. --- .../engine/skwasm/skwasm_impl/paragraph.dart | 6 +- .../raw/text/raw_paragraph_style.dart | 3 + .../src/engine/text/layout_fragmenter.dart | 3 + lib/web_ui/skwasm/text/paragraph_style.cpp | 5 + lib/web_ui/skwasm/text/strut_style.cpp | 4 +- lib/web_ui/test/canvaskit/text_test.dart | 127 --------------- lib/web_ui/test/ui/line_metrics_test.dart | 148 ++++++++++++++++++ lib/web_ui/test/ui/text_golden_test.dart | 10 ++ 8 files changed, 177 insertions(+), 129 deletions(-) create mode 100644 lib/web_ui/test/ui/line_metrics_test.dart diff --git a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart index f719326895f06..8117fa6ff2860 100644 --- a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart +++ b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart @@ -259,7 +259,7 @@ class SkwasmParagraph extends SkwasmObjectWrapper implements ui.Pa @override ui.LineMetrics? getLineMetricsAt(int index) { final LineMetricsHandle lineMetrics = paragraphGetLineMetricsAtIndex(handle, index); - return lineMetrics == nullptr ? SkwasmLineMetrics._(lineMetrics) : null; + return lineMetrics != nullptr ? SkwasmLineMetrics._(lineMetrics) : null; } } @@ -746,6 +746,10 @@ class SkwasmParagraphStyle extends SkwasmObjectWrapper implem skStringFree(localeHandle); } paragraphStyleSetTextStyle(handle, textStyleHandle); + + if (ui.ParagraphBuilder.shouldDisableRoundingHack) { + paragraphStyleSetApplyRoundingHack(handle, false); + } return SkwasmParagraphStyle._( handle, textStyle, diff --git a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph_style.dart b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph_style.dart index e65a5f7690066..77a7d64328638 100644 --- a/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph_style.dart +++ b/lib/web_ui/lib/src/engine/skwasm/skwasm_impl/raw/text/raw_paragraph_style.dart @@ -49,3 +49,6 @@ external void paragraphStyleSetStrutStyle(ParagraphStyleHandle handle, StrutStyl @Native(symbol: 'paragraphStyle_setTextStyle', isLeaf: true) external void paragraphStyleSetTextStyle(ParagraphStyleHandle handle, TextStyleHandle textStyle); + +@Native(symbol: 'paragraphStyle_setApplyRoundingHack', isLeaf: true) +external void paragraphStyleSetApplyRoundingHack(ParagraphStyleHandle handle, bool applyRoundingHack); diff --git a/lib/web_ui/lib/src/engine/text/layout_fragmenter.dart b/lib/web_ui/lib/src/engine/text/layout_fragmenter.dart index cf1eb647c2103..7bb9843ea0be2 100644 --- a/lib/web_ui/lib/src/engine/text/layout_fragmenter.dart +++ b/lib/web_ui/lib/src/engine/text/layout_fragmenter.dart @@ -25,6 +25,9 @@ class LayoutFragmenter extends TextFragmenter { @override List fragment() { + if (text.isEmpty) { + return []; + } final List fragments = []; int fragmentStart = 0; diff --git a/lib/web_ui/skwasm/text/paragraph_style.cpp b/lib/web_ui/skwasm/text/paragraph_style.cpp index 9a9dc82fac616..a319287bebeba 100644 --- a/lib/web_ui/skwasm/text/paragraph_style.cpp +++ b/lib/web_ui/skwasm/text/paragraph_style.cpp @@ -78,3 +78,8 @@ SKWASM_EXPORT void paragraphStyle_setTextStyle(ParagraphStyle* style, TextStyle* textStyle) { style->setTextStyle(*textStyle); } + +SKWASM_EXPORT void paragraphStyle_setApplyRoundingHack(ParagraphStyle* style, + bool applyRoundingHack) { + style->setApplyRoundingHack(applyRoundingHack); +} diff --git a/lib/web_ui/skwasm/text/strut_style.cpp b/lib/web_ui/skwasm/text/strut_style.cpp index f3c7ea0d2fa33..6d1de9b44cb00 100644 --- a/lib/web_ui/skwasm/text/strut_style.cpp +++ b/lib/web_ui/skwasm/text/strut_style.cpp @@ -8,7 +8,9 @@ using namespace skia::textlayout; SKWASM_EXPORT StrutStyle* strutStyle_create() { - return new StrutStyle(); + auto style = new StrutStyle(); + style->setStrutEnabled(true); + return style; } SKWASM_EXPORT void strutStyle_dispose(StrutStyle* style) { diff --git a/lib/web_ui/test/canvaskit/text_test.dart b/lib/web_ui/test/canvaskit/text_test.dart index bc0f58070125e..52532fc4ee4b5 100644 --- a/lib/web_ui/test/canvaskit/text_test.dart +++ b/lib/web_ui/test/canvaskit/text_test.dart @@ -125,133 +125,6 @@ void testMain() { } }); }); - - test('empty paragraph', () { - const double fontSize = 10.0; - final ui.Paragraph paragraph = ui.ParagraphBuilder(CkParagraphStyle( - fontSize: fontSize, - )).build(); - paragraph.layout(const ui.ParagraphConstraints(width: double.infinity)); - - expect(paragraph.getLineMetricsAt(0), isNull); - expect(paragraph.numberOfLines, 0); - expect(paragraph.getLineNumberAt(0), isNull); - }); - - test('Basic line related metrics', () { - const double fontSize = 10; - final ui.ParagraphBuilder builder = ui.ParagraphBuilder(CkParagraphStyle( - fontStyle: ui.FontStyle.normal, - fontWeight: ui.FontWeight.normal, - fontSize: fontSize, - maxLines: 1, - ellipsis: 'BBB', - ))..addText('A' * 100); - final ui.Paragraph paragraph = builder.build(); - paragraph.layout(const ui.ParagraphConstraints(width: 100.0)); - - expect(paragraph.numberOfLines, 1); - - expect(paragraph.getLineMetricsAt(-1), isNull); - expect(paragraph.getLineMetricsAt(0)?.lineNumber, 0); - expect(paragraph.getLineMetricsAt(1), isNull); - - expect(paragraph.getLineNumberAt(-1), isNull); - expect(paragraph.getLineNumberAt(0), 0); - expect(paragraph.getLineNumberAt(6), 0); - // The last 3 characters on the first line are ellipsized with BBB. - expect(paragraph.getLineMetricsAt(7), isNull); - }); - - test('Basic glyph metrics', () { - const double fontSize = 10; - final ui.ParagraphBuilder builder = ui.ParagraphBuilder(CkParagraphStyle( - fontStyle: ui.FontStyle.normal, - fontWeight: ui.FontWeight.normal, - fontFamily: 'FlutterTest', - fontSize: fontSize, - maxLines: 1, - ellipsis: 'BBB', - ))..addText('A' * 100); - final ui.Paragraph paragraph = builder.build(); - paragraph.layout(const ui.ParagraphConstraints(width: 100.0)); - - expect(paragraph.getGlyphInfoAt(-1), isNull); - - // The last 3 characters on the first line are ellipsized with BBB. - expect(paragraph.getGlyphInfoAt(0)?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 0, end: 1)); - expect(paragraph.getGlyphInfoAt(6)?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 6, end: 7)); - expect(paragraph.getGlyphInfoAt(7), isNull); - expect(paragraph.getGlyphInfoAt(200), isNull); - }); - - test('Basic glyph metrics - hit test', () { - const double fontSize = 10.0; - final ui.ParagraphBuilder builder = ui.ParagraphBuilder(CkParagraphStyle( - fontSize: fontSize, - fontFamily: 'FlutterTest', - ))..addText('Test\nTest'); - final ui.Paragraph paragraph = builder.build(); - paragraph.layout(const ui.ParagraphConstraints(width: double.infinity)); - - final ui.GlyphInfo? bottomRight = paragraph.getClosestGlyphInfoForOffset(const ui.Offset(99.0, 99.0)); - final ui.GlyphInfo? last = paragraph.getGlyphInfoAt(8); - expect(bottomRight, equals(last)); - expect(bottomRight, isNot(paragraph.getGlyphInfoAt(0))); - - expect(bottomRight?.graphemeClusterLayoutBounds, const ui.Rect.fromLTWH(30, 10, 10, 10)); - expect(bottomRight?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 8, end: 9)); - expect(bottomRight?.writingDirection, ui.TextDirection.ltr); - }); - - test('rounding hack disabled by default', () { - expect(ui.ParagraphBuilder.shouldDisableRoundingHack, isTrue); - - const double fontSize = 1.25; - const String text = '12345'; - assert((fontSize * text.length).truncate() != fontSize * text.length); - final ui.ParagraphBuilder builder = ui.ParagraphBuilder( - ui.ParagraphStyle(fontSize: fontSize, fontFamily: 'FlutterTest'), - ); - builder.addText(text); - final ui.Paragraph paragraph = builder.build() - ..layout(const ui.ParagraphConstraints(width: text.length * fontSize)); - - expect(paragraph.maxIntrinsicWidth, text.length * fontSize); - switch (paragraph.computeLineMetrics()) { - case [ui.LineMetrics(width: final double width)]: - expect(width, text.length * fontSize); - case final List metrics: - expect(metrics, hasLength(1)); - } - }); - - test('setDisableRoundinghHack to false works in tests', () { - bool assertsEnabled = false; - assert(() { - assertsEnabled = true; - return true; - }()); - if (!assertsEnabled){ - return; - } - - if (ui.ParagraphBuilder.shouldDisableRoundingHack) { - ui.ParagraphBuilder.setDisableRoundingHack(false); - addTearDown(() => ui.ParagraphBuilder.setDisableRoundingHack(true)); - } - - assert(!ui.ParagraphBuilder.shouldDisableRoundingHack); - const double fontSize = 1.25; - const String text = '12345'; - assert((fontSize * text.length).truncate() != fontSize * text.length); - final ui.ParagraphBuilder builder = ui.ParagraphBuilder(ui.ParagraphStyle(fontSize: fontSize, fontFamily: 'FlutterTest')); - builder.addText(text); - final ui.Paragraph paragraph = builder.build() - ..layout(const ui.ParagraphConstraints(width: text.length * fontSize)); - expect(paragraph.computeLineMetrics().length, greaterThan(1)); - }); - // TODO(hterkelsen): https://github.com/flutter/flutter/issues/71520 }, skip: isSafari || isFirefox); } diff --git a/lib/web_ui/test/ui/line_metrics_test.dart b/lib/web_ui/test/ui/line_metrics_test.dart new file mode 100644 index 0000000000000..cf79cfe237c40 --- /dev/null +++ b/lib/web_ui/test/ui/line_metrics_test.dart @@ -0,0 +1,148 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:test/bootstrap/browser.dart'; +import 'package:test/test.dart'; +import 'package:ui/src/engine.dart'; +import 'package:ui/ui.dart' as ui; + +import '../common/test_initialization.dart'; +import 'utils.dart'; + +void main() { + internalBootstrapBrowserTest(() => testMain); +} + +Future testMain() async { + setUpUnitTests( + withImplicitView: true, + setUpTestViewDimensions: false, + ); + + test('empty paragraph', () { + const double fontSize = 10.0; + final ui.Paragraph paragraph = ui.ParagraphBuilder(ui.ParagraphStyle( + fontSize: fontSize, + )).build(); + paragraph.layout(const ui.ParagraphConstraints(width: double.infinity)); + + expect(paragraph.getLineMetricsAt(0), isNull); + expect(paragraph.numberOfLines, 0); + expect(paragraph.getLineNumberAt(0), isNull); + }); + + test('Basic line related metrics', () { + const double fontSize = 10; + final ui.ParagraphBuilder builder = ui.ParagraphBuilder(ui.ParagraphStyle( + fontStyle: ui.FontStyle.normal, + fontWeight: ui.FontWeight.normal, + fontSize: fontSize, + maxLines: 1, + ellipsis: 'BBB', + ))..addText('A' * 100); + final ui.Paragraph paragraph = builder.build(); + paragraph.layout(const ui.ParagraphConstraints(width: 100.0)); + + expect(paragraph.numberOfLines, 1); + + expect(paragraph.getLineMetricsAt(-1), isNull); + expect(paragraph.getLineMetricsAt(0)?.lineNumber, 0); + expect(paragraph.getLineMetricsAt(1), isNull); + + expect(paragraph.getLineNumberAt(-1), isNull); + expect(paragraph.getLineNumberAt(0), 0); + expect(paragraph.getLineNumberAt(6), 0); + // The last 3 characters on the first line are ellipsized with BBB. + expect(paragraph.getLineMetricsAt(7), isNull); + }); + + test('Basic glyph metrics', () { + const double fontSize = 10; + final ui.ParagraphBuilder builder = ui.ParagraphBuilder(ui.ParagraphStyle( + fontStyle: ui.FontStyle.normal, + fontWeight: ui.FontWeight.normal, + fontFamily: 'FlutterTest', + fontSize: fontSize, + maxLines: 1, + ellipsis: 'BBB', + ))..addText('A' * 100); + final ui.Paragraph paragraph = builder.build(); + paragraph.layout(const ui.ParagraphConstraints(width: 100.0)); + + expect(paragraph.getGlyphInfoAt(-1), isNull); + + // The last 3 characters on the first line are ellipsized with BBB. + expect(paragraph.getGlyphInfoAt(0)?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 0, end: 1)); + expect(paragraph.getGlyphInfoAt(6)?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 6, end: 7)); + expect(paragraph.getGlyphInfoAt(7), isNull); + expect(paragraph.getGlyphInfoAt(200), isNull); + }); + + test('Basic glyph metrics - hit test', () { + const double fontSize = 10.0; + final ui.ParagraphBuilder builder = ui.ParagraphBuilder(ui.ParagraphStyle( + fontSize: fontSize, + fontFamily: 'FlutterTest', + ))..addText('Test\nTest'); + final ui.Paragraph paragraph = builder.build(); + paragraph.layout(const ui.ParagraphConstraints(width: double.infinity)); + + final ui.GlyphInfo? bottomRight = paragraph.getClosestGlyphInfoForOffset(const ui.Offset(99.0, 99.0)); + final ui.GlyphInfo? last = paragraph.getGlyphInfoAt(8); + expect(bottomRight, equals(last)); + expect(bottomRight, isNot(paragraph.getGlyphInfoAt(0))); + + expect(bottomRight?.graphemeClusterLayoutBounds, const ui.Rect.fromLTWH(30, 10, 10, 10)); + expect(bottomRight?.graphemeClusterCodeUnitRange, const ui.TextRange(start: 8, end: 9)); + expect(bottomRight?.writingDirection, ui.TextDirection.ltr); + }); + + test('rounding hack disabled by default', () { + expect(ui.ParagraphBuilder.shouldDisableRoundingHack, isTrue); + + const double fontSize = 1.25; + const String text = '12345'; + assert((fontSize * text.length).truncate() != fontSize * text.length); + final ui.ParagraphBuilder builder = ui.ParagraphBuilder( + ui.ParagraphStyle(fontSize: fontSize, fontFamily: 'FlutterTest'), + ); + builder.addText(text); + final ui.Paragraph paragraph = builder.build() + ..layout(const ui.ParagraphConstraints(width: text.length * fontSize)); + + expect(paragraph.maxIntrinsicWidth, text.length * fontSize); + switch (paragraph.computeLineMetrics()) { + case [ui.LineMetrics(width: final double width)]: + expect(width, text.length * fontSize); + case final List metrics: + expect(metrics, hasLength(1)); + } + }, skip: isHtml); // The rounding hack doesn't apply to the html renderer + + test('setDisableRoundingHack to false works in tests', () { + bool assertsEnabled = false; + assert(() { + assertsEnabled = true; + return true; + }()); + if (!assertsEnabled){ + return; + } + + if (ui.ParagraphBuilder.shouldDisableRoundingHack) { + ui.ParagraphBuilder.setDisableRoundingHack(false); + addTearDown(() => ui.ParagraphBuilder.setDisableRoundingHack(true)); + } + + assert(!ui.ParagraphBuilder.shouldDisableRoundingHack); + const double fontSize = 1.25; + const String text = '12345'; + assert((fontSize * text.length).truncate() != fontSize * text.length); + final ui.ParagraphBuilder builder = ui.ParagraphBuilder(ui.ParagraphStyle(fontSize: fontSize, fontFamily: 'FlutterTest')); + builder.addText(text); + final ui.Paragraph paragraph = builder.build() + ..layout(const ui.ParagraphConstraints(width: text.length * fontSize)); + expect(paragraph.computeLineMetrics().length, greaterThan(1)); + }, skip: isHtml); // The rounding hack doesn't apply to the html renderer +} diff --git a/lib/web_ui/test/ui/text_golden_test.dart b/lib/web_ui/test/ui/text_golden_test.dart index 66813ff44c418..e7a9307b6b52d 100644 --- a/lib/web_ui/test/ui/text_golden_test.dart +++ b/lib/web_ui/test/ui/text_golden_test.dart @@ -392,6 +392,16 @@ Future testMain() async { ); }); + test('strut style - override height', () async { + await testTextStyle( + 'strut style', + paragraphStrutStyle: ui.StrutStyle( + forceStrutHeight: true, + height: 2, + ), + ); + }); + test('sample Chinese text', () async { await testSampleText( 'chinese',