From 07000dc692ad137ab819e32be9570ce3acdb3aed Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Wed, 3 Mar 2021 21:23:19 -0800 Subject: [PATCH] Revert "[web] Fix painting of last placeholder in paragraph (#24716)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This broke the Linux Web Tests target in the framework tests. Example failure: https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20web_tests/11335/overview Failure log: ``` 01:09 +107 ~2 -1: test/widgets/rich_text_test.dart: WidgetSpan calculate correct intrinsic heights [E] Test failed. See exception logs above. The test description was: WidgetSpan calculate correct intrinsic heights 01:09 +107 ~2 -1: test/widgets/rich_text_test.dart: RichText implements debugFillProperties 01:09 +107 ~2 -1: test/widgets/rich_text_test.dart: RichText implements debugFillProperties ══╡ EXCEPTION CAUGHT BY RENDERING LIBRARY ╞═════════════════════════════════════════════════════════ The following assertion was thrown during performLayout(): Assertion failed: file:///b/s/w/ir/k/flutter/packages/flutter/lib/src/rendering/box.dart:2237:16 !RenderObject.debugCheckingIntrinsics is not true The relevant error-causing widget was: Text file:///b/s/w/ir/k/flutter/packages/flutter_test/lib/src/binding.dart:567:12 When the exception was thrown, this was the stack: ../dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 236:49 throw_ ../dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 29:3 assertFailed ../packages/flutter/src/rendering/layer.dart.js 4485:79 ../packages/flutter/src/rendering/layer.dart.js 4551:26 debugAssertDoesMeetConstraints ../packages/flutter/src/rendering/layer.dart.js 4380:14 ../packages/flutter/src/rendering/layer.dart.js 4382:26 set size ../packages/flutter/src/rendering/paragraph.dart.js 786:17 performLayout ../packages/flutter/src/rendering/layer.dart.js 3392:14 layout ../packages/flutter/src/rendering/shifted_box.dart.js 451:36 performLayout ../packages/flutter/src/rendering/layer.dart.js 3392:14 layout ../packages/flutter/src/rendering/layer.dart.js 4875:58 performLayout ../packages/flutter/src/rendering/layer.dart.js 3279:14 [_layoutWithoutResize] ../packages/flutter/src/rendering/layer.dart.js 6876:107 flushLayout ../packages/flutter_test/src/_matchers_web.dart.js 3990:30 drawFrame ../packages/flutter/src/rendering/layer.dart.js 6128:12 [_handlePersistentFrameCallback] ../packages/flutter/src/scheduler/binding.dart.js 764:9 [_invokeFrameCallback] ../packages/flutter/src/scheduler/binding.dart.js 732:37 handleDrawFrame ../packages/flutter_test/src/_matchers_web.dart.js 3954:12 scheduleWarmUpFrame ../packages/flutter/src/widgets/widget_span.dart.js 52123:11 ../packages/flutter/src/widgets/widget_span.dart.js 52125:7 runApp ../packages/flutter_test/src/_matchers_web.dart.js 3647:18 _runTestBody ../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 84:54 runBody ../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 123:5 _async ../packages/flutter_test/src/_matchers_web.dart.js 3644:20 [_runTestBody] ../dart-sdk/lib/async/zone.dart 1386:13 _rootRunBinary ../dart-sdk/lib/async/zone.dart 1272:19 runBinary ../packages/flutter_test/src/_matchers_web.dart.js 3636:16 [_runTest] ../packages/flutter_test/src/_matchers_web.dart.js 4072:44 ../packages/fake_async/fake_async.dart.js 141:96 ../dart-sdk/lib/async/zone.dart 1354:13 _rootRun ../dart-sdk/lib/async/zone.dart 1258:19 run ../dart-sdk/lib/async/zone.dart 1788:67 _runZoned ../dart-sdk/lib/async/zone.dart 1711:10 runZoned ../packages/clock/src/stopwatch.dart.js 364:18 withClock ../packages/fake_async/fake_async.dart.js 141:55 ../dart-sdk/lib/async/zone.dart 1354:13 _rootRun ../dart-sdk/lib/async/zone.dart 1258:19 run ../dart-sdk/lib/async/zone.dart 1788:67 _runZoned ../dart-sdk/lib/async/zone.dart 1711:10 runZoned ../packages/fake_async/fake_async.dart.js 141:20 run ../packages/flutter_test/src/_matchers_web.dart.js 4067:17 runTest ../packages/flutter_test/src/_matchers_web.dart.js 5312:24 ../packages/test_api/src/frontend/async_matcher.dart.js 862:17 ../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50 ../packages/stack_trace/src/stack_zone_specification.dart.js 179:98 ../packages/stack_trace/src/stack_zone_specification.dart.js 247:16 [_run] ../packages/stack_trace/src/stack_zone_specification.dart.js 179:80 ../dart-sdk/lib/async/zone.dart 1362:47 _rootRunUnary ../dart-sdk/lib/async/zone.dart 1265:19 runUnary ../dart-sdk/lib/async/future_impl.dart 155:18 handleValue ../dart-sdk/lib/async/future_impl.dart 707:44 handleValueCallback ../dart-sdk/lib/async/future_impl.dart 736:13 _propagateToListeners ../dart-sdk/lib/async/future_impl.dart 406:9 ../packages/stack_trace/src/stack_zone_specification.dart.js 247:16 [_run] ../packages/stack_trace/src/stack_zone_specification.dart.js 170:71 ../dart-sdk/lib/async/zone.dart 1354:13 _rootRun ../dart-sdk/lib/async/zone.dart 1258:19 run ../dart-sdk/lib/async/zone.dart 1162:7 runGuarded ../dart-sdk/lib/async/zone.dart 1202:23 callback ../dart-sdk/lib/async/schedule_microtask.dart 40:11 _microtaskLoop ../dart-sdk/lib/async/schedule_microtask.dart 49:5 _startMicrotaskLoop ../dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 166:15 The following RenderObject was being processed when the exception was fired: RenderParagraph#e6e8f relayoutBoundary=up1 NEEDS-PAINT: creator: RichText ← Text ← Center ← Container-[#9a020] ← [root] parentData: offset=Offset(0.0, 0.0) (can use size) constraints: BoxConstraints(0.0<=w<=800.0, 0.0<=h<=600.0) size: Size(640.0, 40.0) textAlign: start textDirection: ltr softWrap: wrapping at box width overflow: clip maxLines: unlimited This RenderObject had the following child: text: TextSpan ``` This reverts commit aa3bb5e3306b688123ec5234936715ddacd6f726. --- lib/web_ui/dev/goldens_lock.yaml | 2 +- .../lib/src/engine/text/layout_service.dart | 105 ++++++++---------- .../canvas_paragraph/placeholders_test.dart | 90 ++------------- .../test/text/layout_service_rich_test.dart | 21 +--- 4 files changed, 55 insertions(+), 163 deletions(-) diff --git a/lib/web_ui/dev/goldens_lock.yaml b/lib/web_ui/dev/goldens_lock.yaml index c848de7900609..3ddfa4a9ca48c 100644 --- a/lib/web_ui/dev/goldens_lock.yaml +++ b/lib/web_ui/dev/goldens_lock.yaml @@ -1,2 +1,2 @@ repository: https://github.com/flutter/goldens.git -revision: d4599aade933fe5c9c74d4c4acc08649ab6146bc +revision: bb55871d3803337053f7200b8690a4c1322e82ea diff --git a/lib/web_ui/lib/src/engine/text/layout_service.dart b/lib/web_ui/lib/src/engine/text/layout_service.dart index 745f93ae0b15c..5a47175da03e0 100644 --- a/lib/web_ui/lib/src/engine/text/layout_service.dart +++ b/lib/web_ui/lib/src/engine/text/layout_service.dart @@ -78,6 +78,7 @@ class TextLayoutService { final Spanometer spanometer = Spanometer(paragraph, context); int spanIndex = 0; + ParagraphSpan span = paragraph.spans[0]; LineBuilder currentLine = LineBuilder.first(paragraph, spanometer, maxWidth: constraints.width); @@ -85,33 +86,28 @@ class TextLayoutService { // statements (e.g. when we reach `endOfText`, when ellipsis has been // appended). while (true) { - // ************************** // - // *** HANDLE END OF TEXT *** // - // ************************** // - - // All spans have been consumed. - final bool reachedEnd = spanIndex == spanCount; - if (reachedEnd) { - // In some cases, we need to extend the line to the end of text and - // build it: - // - // 1. Line is not empty. This could happen when the last span is a - // placeholder. - // - // 2. We haven't reached `LineBreakType.endOfText` yet. This could - // happen when the last character is a new line. - if (currentLine.isNotEmpty || currentLine.end.type != LineBreakType.endOfText) { - currentLine.extendToEndOfText(); + // *********************************************** // + // *** HANDLE HARD LINE BREAKS AND END OF TEXT *** // + // *********************************************** // + + if (currentLine.end.isHard) { + if (currentLine.isNotEmpty) { lines.add(currentLine.build()); + if (currentLine.end.type != LineBreakType.endOfText) { + currentLine = currentLine.nextLine(); + } + } + + if (currentLine.end.type == LineBreakType.endOfText) { + break; } - break; } // ********************************* // // *** THE MAIN MEASUREMENT PART *** // // ********************************* // - final ParagraphSpan span = paragraph.spans[spanIndex]; + final isLastSpan = spanIndex == spanCount - 1; if (span is PlaceholderSpan) { if (currentLine.widthIncludingSpace + span.width <= constraints.width) { @@ -125,7 +121,11 @@ class TextLayoutService { } currentLine.addPlaceholder(span); } - spanIndex++; + + if (isLastSpan) { + lines.add(currentLine.build()); + break; + } } else if (span is FlatTextSpan) { spanometer.currentSpan = span; final LineBreakResult nextBreak = currentLine.findNextBreak(span.end); @@ -138,10 +138,6 @@ class TextLayoutService { // The line can extend to `nextBreak` without overflowing. currentLine.extendTo(nextBreak); - if (nextBreak.type == LineBreakType.mandatory) { - lines.add(currentLine.build()); - currentLine = currentLine.nextLine(); - } } else { // The chunk of text can't fit into the current line. final bool isLastLine = @@ -169,12 +165,6 @@ class TextLayoutService { currentLine = currentLine.nextLine(); } } - - // Only go to the next span if we've reached the end of this span. - if (currentLine.end.index >= span.end) { - currentLine.createBox(); - ++spanIndex; - } } else { throw UnimplementedError('Unknown span type: ${span.runtimeType}'); } @@ -182,6 +172,16 @@ class TextLayoutService { if (lines.length == maxLines) { break; } + + // ********************************************* // + // *** ADVANCE TO THE NEXT SPAN IF NECESSARY *** // + // ********************************************* // + + // Only go to the next span if we've reached the end of this span. + if (currentLine.end.index >= span.end && spanIndex < spanCount - 1) { + currentLine.createBox(); + span = paragraph.spans[++spanIndex]; + } } // ************************************************** // @@ -205,15 +205,13 @@ class TextLayoutService { // ******************************** // spanIndex = 0; + span = paragraph.spans[0]; currentLine = LineBuilder.first(paragraph, spanometer, maxWidth: constraints.width); - while (spanIndex < spanCount) { - final ParagraphSpan span = paragraph.spans[spanIndex]; - + while (currentLine.end.type != LineBreakType.endOfText) { if (span is PlaceholderSpan) { currentLine.addPlaceholder(span); - spanIndex++; } else if (span is FlatTextSpan) { spanometer.currentSpan = span; final LineBreakResult nextBreak = currentLine.findNextBreak(span.end); @@ -221,11 +219,6 @@ class TextLayoutService { // For the purpose of max intrinsic width, we don't care if the line // fits within the constraints or not. So we always extend it. currentLine.extendTo(nextBreak); - - // Only go to the next span if we've reached the end of this span. - if (currentLine.end.index >= span.end) { - spanIndex++; - } } final double widthOfLastSegment = currentLine.lastSegment.width; @@ -238,9 +231,19 @@ class TextLayoutService { maxIntrinsicWidth = currentLine.widthIncludingSpace; } - if (currentLine.end.type == LineBreakType.mandatory) { + if (currentLine.end.isHard) { currentLine = currentLine.nextLine(); } + + // Only go to the next span if we've reached the end of this span. + if (currentLine.end.index >= span.end) { + if (spanIndex < spanCount - 1) { + span = paragraph.spans[++spanIndex]; + } else { + // We reached the end of the last span in the paragraph. + break; + } + } } } @@ -773,23 +776,6 @@ class LineBuilder { _addSegment(_createSegment(newEnd)); } - void extendToEndOfText() { - final LineBreakResult endOfText = LineBreakResult.sameIndex( - paragraph.toPlainText().length, - LineBreakType.endOfText, - ); - - // The spanometer may not be ready in some cases. E.g. when the paragraph - // is made up of only placeholders and no text. - if (spanometer.isReady) { - ascent = math.max(ascent, spanometer.ascent); - descent = math.max(descent, spanometer.descent); - _addSegment(_createSegment(endOfText)); - } else { - end = endOfText; - } - } - void addPlaceholder(PlaceholderSpan placeholder) { // Increase the line's height to fit the placeholder, if necessary. final double ascent, descent; @@ -1038,7 +1024,7 @@ class LineBuilder { final LineBreakResult boxEnd = end; // Avoid creating empty boxes. This could happen when the end of a span // coincides with the end of a line. In this case, `createBox` is called twice. - if (boxStart.index == boxEnd.index) { + if (boxStart == boxEnd) { return; } @@ -1164,9 +1150,6 @@ class Spanometer { } } - /// Whether the spanometer is ready to take measurements. - bool get isReady => _currentSpan != null; - /// The distance from the top of the current span to the alphabetic baseline. double get ascent => _currentRuler!.alphabeticBaseline; diff --git a/lib/web_ui/test/golden_tests/engine/canvas_paragraph/placeholders_test.dart b/lib/web_ui/test/golden_tests/engine/canvas_paragraph/placeholders_test.dart index a1764aa33ddd8..45440b03dc3a9 100644 --- a/lib/web_ui/test/golden_tests/engine/canvas_paragraph/placeholders_test.dart +++ b/lib/web_ui/test/golden_tests/engine/canvas_paragraph/placeholders_test.dart @@ -50,7 +50,9 @@ void testMain() async { canvas.drawParagraph(paragraph, offset); // Then fill the placeholders. - fillPlaceholder(canvas, offset, paragraph); + final TextBox placeholderBox = paragraph.getBoxesForPlaceholders().single; + final SurfacePaint redPaint = Paint()..color = red; + canvas.drawRect(placeholderBox.toRect().shift(offset), redPaint.paintData); offset = offset.translate(0.0, paragraph.height + 30.0); } @@ -84,7 +86,9 @@ void testMain() async { canvas.drawParagraph(paragraph, offset); // Then fill the placeholders. - fillPlaceholder(canvas, offset, paragraph); + final TextBox placeholderBox = paragraph.getBoxesForPlaceholders().single; + final SurfacePaint redPaint = Paint()..color = red; + canvas.drawRect(placeholderBox.toRect().shift(offset), redPaint.paintData); offset = offset.translate(0.0, paragraph.height + 30.0); } @@ -118,89 +122,13 @@ void testMain() async { canvas.drawParagraph(paragraph, offset); // Then fill the placeholders. - fillPlaceholder(canvas, offset, paragraph); + final TextBox placeholderBox = paragraph.getBoxesForPlaceholders().single; + final SurfacePaint redPaint = Paint()..color = red; + canvas.drawRect(placeholderBox.toRect().shift(offset), redPaint.paintData); offset = offset.translate(0.0, paragraph.height + 30.0); } return takeScreenshot(canvas, bounds, 'canvas_paragraph_placeholders_align_dom'); }); - - test('draws paragraphs starting or ending with a placeholder', () { - const Rect bounds = Rect.fromLTWH(0, 0, 420, 300); - final canvas = BitmapCanvas(bounds, RenderStrategy()); - - Offset offset = Offset(10, 10); - - // First paragraph with a placeholder at the beginning. - final CanvasParagraph paragraph1 = rich( - ParagraphStyle(fontFamily: 'Roboto', fontSize: 24.0, textAlign: TextAlign.center), - (builder) { - builder.addPlaceholder(80.0, 50.0, PlaceholderAlignment.baseline, baseline: TextBaseline.alphabetic); - builder.pushStyle(TextStyle(color: black)); - builder.addText(' Lorem ipsum.'); - }, - )..layout(constrain(400.0)); - - // Draw the paragraph. - canvas.drawParagraph(paragraph1, offset); - fillPlaceholder(canvas, offset, paragraph1); - surroundParagraph(canvas, offset, paragraph1); - - offset = offset.translate(0.0, paragraph1.height + 30.0); - - // Second paragraph with a placeholder at the end. - final CanvasParagraph paragraph2 = rich( - ParagraphStyle(fontFamily: 'Roboto', fontSize: 24.0, textAlign: TextAlign.center), - (builder) { - builder.pushStyle(TextStyle(color: black)); - builder.addText('Lorem ipsum '); - builder.addPlaceholder(80.0, 50.0, PlaceholderAlignment.baseline, baseline: TextBaseline.alphabetic); - }, - )..layout(constrain(400.0)); - - // Draw the paragraph. - canvas.drawParagraph(paragraph2, offset); - fillPlaceholder(canvas, offset, paragraph2); - surroundParagraph(canvas, offset, paragraph2); - - offset = offset.translate(0.0, paragraph2.height + 30.0); - - // Third paragraph with a placeholder alone in the second line. - final CanvasParagraph paragraph3 = rich( - ParagraphStyle(fontFamily: 'Roboto', fontSize: 24.0, textAlign: TextAlign.center), - (builder) { - builder.pushStyle(TextStyle(color: black)); - builder.addText('Lorem ipsum '); - builder.addPlaceholder(80.0, 50.0, PlaceholderAlignment.baseline, baseline: TextBaseline.alphabetic); - }, - )..layout(constrain(200.0)); - - // Draw the paragraph. - canvas.drawParagraph(paragraph3, offset); - fillPlaceholder(canvas, offset, paragraph3); - surroundParagraph(canvas, offset, paragraph3); - - return takeScreenshot(canvas, bounds, 'canvas_paragraph_placeholders_start_and_end'); - }); -} - -void surroundParagraph( - EngineCanvas canvas, - Offset offset, - CanvasParagraph paragraph, -) { - final Rect rect = offset & Size(paragraph.width, paragraph.height); - final SurfacePaint paint = Paint()..color = blue..style = PaintingStyle.stroke; - canvas.drawRect(rect, paint.paintData); -} - -void fillPlaceholder( - EngineCanvas canvas, - Offset offset, - CanvasParagraph paragraph, -) { - final TextBox placeholderBox = paragraph.getBoxesForPlaceholders().single; - final SurfacePaint paint = Paint()..color = red; - canvas.drawRect(placeholderBox.toRect().shift(offset), paint.paintData); } diff --git a/lib/web_ui/test/text/layout_service_rich_test.dart b/lib/web_ui/test/text/layout_service_rich_test.dart index c7786429e7b21..5978658c21390 100644 --- a/lib/web_ui/test/text/layout_service_rich_test.dart +++ b/lib/web_ui/test/text/layout_service_rich_test.dart @@ -164,26 +164,7 @@ void testMain() async { expect(paragraph.minIntrinsicWidth, 300.0); expect(paragraph.height, 50.0); expectLines(paragraph, [ - l('', 0, 0, hardBreak: true, width: 300.0, left: 100.0), - ]); - }); - - test('correct maxIntrinsicWidth when paragraph ends with placeholder', () { - final EngineParagraphStyle paragraphStyle = EngineParagraphStyle( - fontFamily: 'ahem', - fontSize: 10, - textAlign: ui.TextAlign.center, - ); - final CanvasParagraph paragraph = rich(paragraphStyle, (builder) { - builder.addText('abcd'); - builder.addPlaceholder(300.0, 50.0, ui.PlaceholderAlignment.bottom); - })..layout(constrain(400.0)); - - expect(paragraph.maxIntrinsicWidth, 340.0); - expect(paragraph.minIntrinsicWidth, 300.0); - expect(paragraph.height, 50.0); - expectLines(paragraph, [ - l('abcd', 0, 4, hardBreak: true, width: 340.0, left: 30.0), + l('', 0, 0, hardBreak: false, width: 300.0, left: 100.0), ]); }); }