From 32b2f87caf12de21f6c99134f065eacf38750a89 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Tue, 26 Mar 2024 18:06:17 -0700 Subject: [PATCH 1/2] [Skwasm] Correctly handle paragraphs with empty text. --- .../engine/skwasm/skwasm_impl/paragraph.dart | 40 ++++++++++--------- lib/web_ui/test/ui/line_metrics_test.dart | 2 +- .../test/ui/paragraph_builder_test.dart | 7 ++++ 3 files changed, 30 insertions(+), 19 deletions(-) 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 6f6d724121136..f5c52fa737c32 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 @@ -940,27 +940,31 @@ class SkwasmParagraphBuilder extends SkwasmObjectWrapper im // just create both up front here. final Pointer outSize = scope.allocUint32Array(1); final Pointer utf8Data = paragraphBuilderGetUtf8Text(handle, outSize); + + final String text; + final JSString jsText; if (utf8Data == nullptr) { - return; + text = ''; + jsText = ''.toJS; + } else { + // TODO(jacksongardner): We could make a subclass of `List` here to + // avoid this copy. + final List codeUnitList = List.generate( + outSize.value, + (int index) => utf8Data[index] + ); + text = utf8.decode(codeUnitList); + jsText = _utf8Decoder.decode( + // In an ideal world we would just use a subview of wasm memory rather + // than a slice, but the TextDecoder API doesn't work on shared buffer + // sources yet. + // See https://bugs.chromium.org/p/chromium/issues/detail?id=1012656 + createUint8ArrayFromBuffer(skwasmInstance.wasmMemory.buffer).slice( + utf8Data.address.toJS, + (utf8Data.address + outSize.value).toJS + )); } - // TODO(jacksongardner): We could make a subclass of `List` here to - // avoid this copy. - final List codeUnitList = List.generate( - outSize.value, - (int index) => utf8Data[index] - ); - final String text = utf8.decode(codeUnitList); - final JSString jsText = _utf8Decoder.decode( - // In an ideal world we would just use a subview of wasm memory rather - // than a slice, but the TextDecoder API doesn't work on shared buffer - // sources yet. - // See https://bugs.chromium.org/p/chromium/issues/detail?id=1012656 - createUint8ArrayFromBuffer(skwasmInstance.wasmMemory.buffer).slice( - utf8Data.address.toJS, - (utf8Data.address + outSize.value).toJS - )); - _addGraphemeBreakData(text, jsText); _addWordBreakData(text, jsText); _addLineBreakData(text, jsText); diff --git a/lib/web_ui/test/ui/line_metrics_test.dart b/lib/web_ui/test/ui/line_metrics_test.dart index e287cb6d79cd6..678f5d0920edb 100644 --- a/lib/web_ui/test/ui/line_metrics_test.dart +++ b/lib/web_ui/test/ui/line_metrics_test.dart @@ -176,5 +176,5 @@ Future testMain() async { // In Roboto, the width should be 11 here. In the test font, it would be square (16 points) expect(metrics!.width, 11); - }, solo: true); + }); } diff --git a/lib/web_ui/test/ui/paragraph_builder_test.dart b/lib/web_ui/test/ui/paragraph_builder_test.dart index 72f733ec69516..2ef2d54323d31 100644 --- a/lib/web_ui/test/ui/paragraph_builder_test.dart +++ b/lib/web_ui/test/ui/paragraph_builder_test.dart @@ -39,4 +39,11 @@ Future testMain() async { expect(() => builder.build(), returnsNormally); }); + + test('build and layout a paragraph with an empty addText', () { + final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle()); + builder.addText(''); + final Paragraph paragraph = builder.build(); + paragraph.layout(const ParagraphConstraints(width: double.infinity)); + }); } From 5c7a6327cb7ec479b2a52b1e80912b2fb8f39101 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Wed, 27 Mar 2024 12:27:50 -0700 Subject: [PATCH 2/2] Address comments. --- lib/web_ui/lib/src/engine/skwasm/skwasm_impl/paragraph.dart | 2 -- lib/web_ui/test/ui/paragraph_builder_test.dart | 5 ++++- 2 files changed, 4 insertions(+), 3 deletions(-) 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 f5c52fa737c32..b83233189b55a 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 @@ -947,8 +947,6 @@ class SkwasmParagraphBuilder extends SkwasmObjectWrapper im text = ''; jsText = ''.toJS; } else { - // TODO(jacksongardner): We could make a subclass of `List` here to - // avoid this copy. final List codeUnitList = List.generate( outSize.value, (int index) => utf8Data[index] diff --git a/lib/web_ui/test/ui/paragraph_builder_test.dart b/lib/web_ui/test/ui/paragraph_builder_test.dart index 2ef2d54323d31..368b653a5e49e 100644 --- a/lib/web_ui/test/ui/paragraph_builder_test.dart +++ b/lib/web_ui/test/ui/paragraph_builder_test.dart @@ -44,6 +44,9 @@ Future testMain() async { final ParagraphBuilder builder = ParagraphBuilder(ParagraphStyle()); builder.addText(''); final Paragraph paragraph = builder.build(); - paragraph.layout(const ParagraphConstraints(width: double.infinity)); + expect( + () => paragraph.layout(const ParagraphConstraints(width: double.infinity)), + returnsNormally, + ); }); }