From a2fb14c1df093737a7f4872e2ea99cce2421fae5 Mon Sep 17 00:00:00 2001
From: Mouad Debbar <mdebbar@google.com>
Date: Thu, 9 Sep 2021 10:23:37 -0400
Subject: [PATCH 1/4] [web] Multiple fixes to text layout and line breaks

---
 .../lib/src/engine/text/layout_service.dart   | 64 +++++++++++++++----
 .../lib/src/engine/text/line_breaker.dart     | 48 ++++++++++----
 lib/web_ui/lib/src/engine/text/paragraph.dart | 37 ++++++-----
 .../test/text/layout_service_helper.dart      | 28 ++++----
 .../test/text/layout_service_rich_test.dart   | 53 +++++++++++----
 lib/web_ui/test/text/line_breaker_test.dart   | 19 +++++-
 .../test/text/line_breaker_test_helper.dart   | 23 ++++++-
 7 files changed, 202 insertions(+), 70 deletions(-)

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 6d3c374523d05..934708d044081 100644
--- a/lib/web_ui/lib/src/engine/text/layout_service.dart
+++ b/lib/web_ui/lib/src/engine/text/layout_service.dart
@@ -121,7 +121,7 @@ class TextLayoutService {
       // *** THE MAIN MEASUREMENT PART *** //
       // ********************************* //
 
-      final ParagraphSpan span = paragraph.spans[spanIndex];
+      ParagraphSpan span = paragraph.spans[spanIndex];
 
       if (span is PlaceholderSpan) {
         if (currentLine.widthIncludingSpace + span.width <= constraints.width) {
@@ -143,9 +143,6 @@ class TextLayoutService {
             currentLine.getAdditionalWidthTo(nextBreak.lineBreak);
 
         if (currentLine.width + additionalWidth <= constraints.width) {
-          // TODO(mdebbar): Handle the case when `nextBreak` is just a span end
-          //                that shouldn't extend the line yet.
-
           // The line can extend to `nextBreak` without overflowing.
           currentLine.extendTo(nextBreak);
           if (nextBreak.type == LineBreakType.mandatory) {
@@ -168,8 +165,8 @@ class TextLayoutService {
             );
             lines.add(currentLine.build(ellipsis: ellipsis));
             break;
-          } else if (currentLine.isEmpty) {
-            // The current line is still empty, which means we are dealing
+          } else if (currentLine.isNotBreakable) {
+            // The entire line is unbreakable, which means we are dealing
             // with a single block of text that doesn't fit in a single line.
             // We need to force-break it without adding an ellipsis.
 
@@ -178,6 +175,13 @@ class TextLayoutService {
             currentLine = currentLine.nextLine();
           } else {
             // Normal line break.
+            currentLine.revertToLastBreakOpportunity();
+            // If a revert had occurred in the line, we need to revert the span
+            // index accordingly.
+            final ParagraphSpan revertedToSpan = currentLine.lastSegment.span;
+            while (span != revertedToSpan) {
+              span = paragraph.spans[--spanIndex];
+            }
             lines.add(currentLine.build());
             currentLine = currentLine.nextLine();
           }
@@ -815,7 +819,7 @@ class LineBuilder {
     required this.start,
     required this.lineNumber,
     required this.accumulatedHeight,
-  }) : end = start;
+  }) : _end = start;
 
   /// Creates a [LineBuilder] for the first line in a paragraph.
   factory LineBuilder.first(
@@ -846,7 +850,14 @@ class LineBuilder {
   final double accumulatedHeight;
 
   /// The index of the end of the line so far.
-  LineBreakResult end;
+  LineBreakResult get end => _end;
+  LineBreakResult _end;
+  set end(LineBreakResult value) {
+    if (value.type != LineBreakType.prohibited) {
+      isBreakable = true;
+    }
+    _end = value;
+  }
 
   /// The width of the line so far, excluding trailing white space.
   double width = 0.0;
@@ -869,6 +880,15 @@ class LineBuilder {
   /// The last segment in this line.
   LineSegment get lastSegment => _segments.last;
 
+  /// Returns true if there is at least one break opportunity in the line.
+  bool isBreakable = false;
+
+  /// Returns true if there's no break opportunity in the line.
+  bool get isNotBreakable => !isBreakable;
+
+  /// Whether the end of this line is a prohibited break.
+  bool get isEndProhibited => end.type == LineBreakType.prohibited;
+
   bool get isEmpty => _segments.isEmpty;
   bool get isNotEmpty => _segments.isNotEmpty;
 
@@ -1026,6 +1046,8 @@ class LineBuilder {
       boxDirection: _currentBoxDirection,
     ));
     _currentBoxStartOffset = widthIncludingSpace;
+    // Breaking is always allowed after a placeholder.
+    isBreakable = true;
   }
 
   /// Creates a new segment to be appended to the end of this line.
@@ -1182,6 +1204,18 @@ class LineBuilder {
     extendTo(nextBreak.copyWithIndex(breakingPoint));
   }
 
+  /// Looks for the last break opportunity in the line and reverts the line to
+  /// that point.
+  ///
+  /// If the line already ends with a break opportunity, this method does
+  /// nothing.
+  void revertToLastBreakOpportunity() {
+    assert(isBreakable);
+    while (isEndProhibited) {
+      _popSegment();
+    }
+  }
+
   LineBreakResult get _currentBoxStart {
     if (_boxes.isEmpty) {
       return start;
@@ -1360,13 +1394,21 @@ class LineBuilder {
     return cumulativeWidth;
   }
 
+  LineBreakResult? _cachedNextBreak;
+
   /// Finds the next line break after the end of this line.
   DirectionalPosition findNextBreak() {
+    LineBreakResult? nextBreak = _cachedNextBreak;
     final String text = paragraph.toPlainText();
-    final int maxEnd = spanometer.currentSpan.end;
-    final LineBreakResult result = nextLineBreak(text, end.index, maxEnd: maxEnd);
+    // Don't recompute the `nextBreak` again until the line has reached the
+    // previously computed `nextBreak`.
+    if (nextBreak == null || end.index >= nextBreak.index) {
+      final int maxEnd = spanometer.currentSpan.end;
+      nextBreak = nextLineBreak(text, end.index, maxEnd: maxEnd);
+      _cachedNextBreak = nextBreak;
+    }
     // The current end of the line is the beginning of the next block.
-    return getDirectionalBlockEnd(text, end, result);
+    return getDirectionalBlockEnd(text, end, nextBreak);
   }
 
   /// Creates a new [LineBuilder] to build the next line in the paragraph.
diff --git a/lib/web_ui/lib/src/engine/text/line_breaker.dart b/lib/web_ui/lib/src/engine/text/line_breaker.dart
index 11ff7e963109a..1589cf68574ce 100644
--- a/lib/web_ui/lib/src/engine/text/line_breaker.dart
+++ b/lib/web_ui/lib/src/engine/text/line_breaker.dart
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:math' as math;
+
 import 'package:ui/ui.dart' as ui;
 
 import '../util.dart';
@@ -170,6 +172,19 @@ bool _hasEastAsianWidthFWH(int charCode) {
 /// * https://www.unicode.org/reports/tr14/tr14-45.html#Algorithm
 /// * https://www.unicode.org/Public/11.0.0/ucd/LineBreak.txt
 LineBreakResult nextLineBreak(String text, int index, {int? maxEnd}) {
+  final LineBreakResult unsafeResult = _unsafeNextLineBreak(text, index, maxEnd: maxEnd);
+  if (maxEnd != null && unsafeResult.index > maxEnd) {
+    return LineBreakResult(
+      maxEnd,
+      math.min(maxEnd, unsafeResult.indexWithoutTrailingNewlines),
+      math.min(maxEnd, unsafeResult.indexWithoutTrailingSpaces),
+      LineBreakType.prohibited,
+    );
+  }
+  return unsafeResult;
+}
+
+LineBreakResult _unsafeNextLineBreak(String text, int index, {int? maxEnd}) {
   int? codePoint = getCodePoint(text, index);
   LineCharProperty curr = lineLookup.findForChar(codePoint);
 
@@ -208,11 +223,11 @@ LineBreakResult nextLineBreak(String text, int index, {int? maxEnd}) {
   // Always break at the end of text.
   // LB3: ! eot
   while (index < text.length) {
-    if (index == maxEnd) {
+    if (maxEnd != null && index > maxEnd) {
       return LineBreakResult(
-        index,
-        lastNonNewlineIndex,
-        lastNonSpaceIndex,
+        maxEnd,
+        math.min(maxEnd, lastNonNewlineIndex),
+        math.min(maxEnd, lastNonSpaceIndex),
         LineBreakType.prohibited,
       );
     }
@@ -389,26 +404,31 @@ LineBreakResult nextLineBreak(String text, int index, {int? maxEnd}) {
     //       × EX
     //       × IS
     //       × SY
-    if (curr == LineCharProperty.CL ||
-        curr == LineCharProperty.CP ||
-        curr == LineCharProperty.EX ||
-        curr == LineCharProperty.IS ||
-        curr == LineCharProperty.SY) {
+    //
+    // Modification: when these characters are preceded by a space, they
+    // shouldn't prevent a line break.
+    if (prev1 != LineCharProperty.SP &&
+        (curr == LineCharProperty.CL ||
+            curr == LineCharProperty.CP ||
+            curr == LineCharProperty.EX ||
+            curr == LineCharProperty.IS ||
+            curr == LineCharProperty.SY)) {
       continue;
     }
 
     // Do not break after ‘[’, even after spaces.
     // LB14: OP SP* ×
-    if (prev1 == LineCharProperty.OP ||
-        baseOfSpaceSequence == LineCharProperty.OP) {
+    //
+    // Modification: allow breaks when there are spaces.
+    if (prev1 == LineCharProperty.OP) {
       continue;
     }
 
     // Do not break within ‘”[’, even with intervening spaces.
     // LB15: QU SP* × OP
-    if ((prev1 == LineCharProperty.QU ||
-            baseOfSpaceSequence == LineCharProperty.QU) &&
-        curr == LineCharProperty.OP) {
+    //
+    // Modification: allow breaks when there are spaces.
+    if (prev1 == LineCharProperty.QU && curr == LineCharProperty.OP) {
       continue;
     }
 
diff --git a/lib/web_ui/lib/src/engine/text/paragraph.dart b/lib/web_ui/lib/src/engine/text/paragraph.dart
index 11e747c030cd4..3ebc48d918b07 100644
--- a/lib/web_ui/lib/src/engine/text/paragraph.dart
+++ b/lib/web_ui/lib/src/engine/text/paragraph.dart
@@ -268,19 +268,19 @@ class EngineParagraphStyle implements ui.ParagraphStyle {
   double? get lineHeight {
     // TODO(mdebbar): Implement proper support for strut styles.
     // https://github.com/flutter/flutter/issues/32243
-    if (_strutStyle == null ||
-        _strutStyle!._height == null ||
-        _strutStyle!._height == 0) {
+    final EngineStrutStyle? strutStyle = _strutStyle;
+    final double? strutHeight = strutStyle?._height;
+    if (strutStyle == null || strutHeight == null || strutHeight == 0) {
       // When there's no strut height, always use paragraph style height.
       return height;
     }
-    if (_strutStyle!._forceStrutHeight == true) {
+    if (strutStyle._forceStrutHeight == true) {
       // When strut height is forced, ignore paragraph style height.
-      return _strutStyle!._height;
+      return strutHeight;
     }
     // In this case, strut height acts as a minimum height for all parts of the
     // paragraph. So we take the max of strut height and paragraph style height.
-    return math.max(_strutStyle!._height!, height ?? 0.0);
+    return math.max(strutHeight, height ?? 0.0);
   }
 
   @override
@@ -324,6 +324,8 @@ class EngineParagraphStyle implements ui.ParagraphStyle {
   @override
   String toString() {
     if (assertionsEnabled) {
+      final double? fontSize = this.fontSize;
+      final double? height = this.height;
       return 'ParagraphStyle('
           'textAlign: ${textAlign ?? "unspecified"}, '
           'textDirection: ${textDirection ?? "unspecified"}, '
@@ -332,8 +334,8 @@ class EngineParagraphStyle implements ui.ParagraphStyle {
           'maxLines: ${maxLines ?? "unspecified"}, '
           'textHeightBehavior: ${_textHeightBehavior ?? "unspecified"}, '
           'fontFamily: ${fontFamily ?? "unspecified"}, '
-          'fontSize: ${fontSize != null ? fontSize!.toStringAsFixed(1) : "unspecified"}, '
-          'height: ${height != null ? "${height!.toStringAsFixed(1)}x" : "unspecified"}, '
+          'fontSize: ${fontSize != null ? fontSize.toStringAsFixed(1) : "unspecified"}, '
+          'height: ${height != null ? "${height.toStringAsFixed(1)}x" : "unspecified"}, '
           'ellipsis: ${ellipsis != null ? "\"$ellipsis\"" : "unspecified"}, '
           'locale: ${locale ?? "unspecified"}'
           ')';
@@ -533,6 +535,9 @@ class EngineTextStyle implements ui.TextStyle {
   @override
   String toString() {
     if (assertionsEnabled) {
+      final List<String>? fontFamilyFallback = this.fontFamilyFallback;
+      final double? fontSize = this.fontSize;
+      final double? height = this.height;
       return 'TextStyle('
           'color: ${color ?? "unspecified"}, '
           'decoration: ${decoration ?? "unspecified"}, '
@@ -543,11 +548,11 @@ class EngineTextStyle implements ui.TextStyle {
           'fontStyle: ${fontStyle ?? "unspecified"}, '
           'textBaseline: ${textBaseline ?? "unspecified"}, '
           'fontFamily: ${isFontFamilyProvided && fontFamily != '' ? fontFamily : "unspecified"}, '
-          'fontFamilyFallback: ${isFontFamilyProvided && fontFamilyFallback != null && fontFamilyFallback!.isNotEmpty ? fontFamilyFallback : "unspecified"}, '
-          'fontSize: ${fontSize != null ? fontSize!.toStringAsFixed(1) : "unspecified"}, '
+          'fontFamilyFallback: ${isFontFamilyProvided && fontFamilyFallback != null && fontFamilyFallback.isNotEmpty ? fontFamilyFallback : "unspecified"}, '
+          'fontSize: ${fontSize != null ? fontSize.toStringAsFixed(1) : "unspecified"}, '
           'letterSpacing: ${letterSpacing != null ? "${letterSpacing}x" : "unspecified"}, '
           'wordSpacing: ${wordSpacing != null ? "${wordSpacing}x" : "unspecified"}, '
-          'height: ${height != null ? "${height!.toStringAsFixed(1)}x" : "unspecified"}, '
+          'height: ${height != null ? "${height.toStringAsFixed(1)}x" : "unspecified"}, '
           'locale: ${locale ?? "unspecified"}, '
           'background: ${background ?? "unspecified"}, '
           'foreground: ${foreground ?? "unspecified"}, '
@@ -722,8 +727,9 @@ void applyTextStyleToElement({
   if (style.height != null) {
     cssStyle.lineHeight = '${style.height}';
   }
-  if (style.fontSize != null) {
-    cssStyle.fontSize = '${style.fontSize!.floor()}px';
+  final double? fontSize = style.fontSize;
+  if (fontSize != null) {
+    cssStyle.fontSize = '${fontSize.floor()}px';
   }
   if (style.fontWeight != null) {
     cssStyle.fontWeight = fontWeightToCss(style.fontWeight);
@@ -748,8 +754,9 @@ void applyTextStyleToElement({
   if (style.decoration != null) {
     updateDecoration = true;
   }
-  if (style.shadows != null) {
-    cssStyle.textShadow = _shadowListToCss(style.shadows!);
+  final List<ui.Shadow>? shadows = style.shadows;
+  if (shadows != null) {
+    cssStyle.textShadow = _shadowListToCss(shadows);
   }
 
   if (updateDecoration) {
diff --git a/lib/web_ui/test/text/layout_service_helper.dart b/lib/web_ui/test/text/layout_service_helper.dart
index 798c6e8e34766..632fd3b90b3a3 100644
--- a/lib/web_ui/test/text/layout_service_helper.dart
+++ b/lib/web_ui/test/text/layout_service_helper.dart
@@ -43,17 +43,19 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
     expect(
       line.lineNumber,
       i,
-      reason: '${i}th line had the wrong `lineNumber`. Expected: $i. Actual: ${line.lineNumber}',
+      reason: 'line #$i had the wrong `lineNumber`. Expected: $i. Actual: ${line.lineNumber}',
     );
     if (expectedLine.displayText != null) {
-      final String substring =
+      String displayText =
           text.substring(line.startIndex, line.endIndexWithoutNewlines);
-      final String ellipsis = line.ellipsis ?? '';
+      if (line.ellipsis != null) {
+        displayText += line.ellipsis!;
+      }
       expect(
-        substring + ellipsis,
+        displayText,
         expectedLine.displayText,
         reason:
-            '${i}th line had a different `displayText` value: "${line.displayText}" vs. "${expectedLine.displayText}"',
+            'line #$i had a different `displayText` value: "$displayText" vs. "${expectedLine.displayText}"',
       );
     }
     if (expectedLine.startIndex != null) {
@@ -61,7 +63,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.startIndex,
         expectedLine.startIndex,
         reason:
-            '${i}th line had a different `startIndex` value: "${line.startIndex}" vs. "${expectedLine.startIndex}"',
+            'line #$i had a different `startIndex` value: "${line.startIndex}" vs. "${expectedLine.startIndex}"',
       );
     }
     if (expectedLine.endIndex != null) {
@@ -69,7 +71,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.endIndex,
         expectedLine.endIndex,
         reason:
-            '${i}th line had a different `endIndex` value: "${line.endIndex}" vs. "${expectedLine.endIndex}"',
+            'line #$i had a different `endIndex` value: "${line.endIndex}" vs. "${expectedLine.endIndex}"',
       );
     }
     if (expectedLine.endIndexWithoutNewlines != null) {
@@ -77,7 +79,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.endIndexWithoutNewlines,
         expectedLine.endIndexWithoutNewlines,
         reason:
-            '${i}th line had a different `endIndexWithoutNewlines` value: "${line.endIndexWithoutNewlines}" vs. "${expectedLine.endIndexWithoutNewlines}"',
+            'line #$i had a different `endIndexWithoutNewlines` value: "${line.endIndexWithoutNewlines}" vs. "${expectedLine.endIndexWithoutNewlines}"',
       );
     }
     if (expectedLine.hardBreak != null) {
@@ -85,7 +87,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.hardBreak,
         expectedLine.hardBreak,
         reason:
-            '${i}th line had a different `hardBreak` value: "${line.hardBreak}" vs. "${expectedLine.hardBreak}"',
+            'line #$i had a different `hardBreak` value: "${line.hardBreak}" vs. "${expectedLine.hardBreak}"',
       );
     }
     if (expectedLine.height != null) {
@@ -93,7 +95,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.height,
         expectedLine.height,
         reason:
-            '${i}th line had a different `height` value: "${line.height}" vs. "${expectedLine.height}"',
+            'line #$i had a different `height` value: "${line.height}" vs. "${expectedLine.height}"',
       );
     }
     if (expectedLine.width != null) {
@@ -101,7 +103,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.width,
         expectedLine.width,
         reason:
-            '${i}th line had a different `width` value: "${line.width}" vs. "${expectedLine.width}"',
+            'line #$i had a different `width` value: "${line.width}" vs. "${expectedLine.width}"',
       );
     }
     if (expectedLine.widthWithTrailingSpaces != null) {
@@ -109,7 +111,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.widthWithTrailingSpaces,
         expectedLine.widthWithTrailingSpaces,
         reason:
-            '${i}th line had a different `widthWithTrailingSpaces` value: "${line.widthWithTrailingSpaces}" vs. "${expectedLine.widthWithTrailingSpaces}"',
+            'line #$i had a different `widthWithTrailingSpaces` value: "${line.widthWithTrailingSpaces}" vs. "${expectedLine.widthWithTrailingSpaces}"',
       );
     }
     if (expectedLine.left != null) {
@@ -117,7 +119,7 @@ void expectLines(CanvasParagraph paragraph, List<TestLine> expectedLines) {
         line.left,
         expectedLine.left,
         reason:
-            '${i}th line had a different `left` value: "${line.left}" vs. "${expectedLine.left}"',
+            'line #$i had a different `left` value: "${line.left}" vs. "${expectedLine.left}"',
       );
     }
   }
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 b59755edc3eea..f29d4db32a4a3 100644
--- a/lib/web_ui/test/text/layout_service_rich_test.dart
+++ b/lib/web_ui/test/text/layout_service_rich_test.dart
@@ -8,6 +8,7 @@ import 'package:test/test.dart';
 import 'package:ui/src/engine.dart';
 import 'package:ui/ui.dart' as ui;
 
+import '../html/paragraph/helper.dart';
 import 'layout_service_helper.dart';
 
 const ui.Color white = ui.Color(0xFFFFFFFF);
@@ -21,19 +22,6 @@ final EngineParagraphStyle ahemStyle = EngineParagraphStyle(
   fontSize: 10,
 );
 
-ui.ParagraphConstraints constrain(double width) {
-  return ui.ParagraphConstraints(width: width);
-}
-
-CanvasParagraph rich(
-  EngineParagraphStyle style,
-  void Function(CanvasParagraphBuilder) callback,
-) {
-  final CanvasParagraphBuilder builder = CanvasParagraphBuilder(style);
-  callback(builder);
-  return builder.build();
-}
-
 void main() {
   internalBootstrapBrowserTest(() => testMain);
 }
@@ -223,4 +211,43 @@ Future<void> testMain() async {
       l('ipsum', 6, 11, hardBreak: true, width: 50.0, height: 10.0, left: 125.0),
     ]);
   });
+
+  test('correctly force-breaks consecutive non-breakable spans', () {
+    final CanvasParagraph paragraph = rich(ahemStyle, (ui.ParagraphBuilder builder) {
+      builder.pushStyle(EngineTextStyle.only(fontSize: 1));
+      builder.addText('A');
+      builder.pop(); // Back to fontSize: 10
+      builder.addText('A' * 20);
+      builder.pushStyle(EngineTextStyle.only(fontSize: 1));
+      builder.addText('A');
+    });
+    paragraph.layout(constrain(200));
+
+    expect(paragraph.maxIntrinsicWidth, 200.0 + 2.0);
+    expect(paragraph.height, 20.0);
+    expectLines(paragraph, <TestLine>[
+      // 1x small "A" + 19x big "A"
+      l('A' * 20, 0, 20, hardBreak: false, width: 1.0 + 190.0, height: 10.0),
+      // 1x big "A" + 1x small "A"
+      l('AA', 20, 22, hardBreak: true, width: 10.0 + 1.0, height: 10.0),
+    ]);
+  });
+
+  test('does not take prohibited line breaks', () {
+    final CanvasParagraph paragraph = rich(ahemStyle, (ui.ParagraphBuilder builder) {
+      builder.pushStyle(EngineTextStyle.only(color: blue));
+      builder.addText('AAA B');
+      builder.pushStyle(EngineTextStyle.only(color: green));
+      builder.addText('BB ');
+      builder.pushStyle(EngineTextStyle.only(color: green));
+      builder.addText('CC');
+    });
+    paragraph.layout(constrain(60));
+
+    expect(paragraph.maxIntrinsicWidth, 100.0);
+    expectLines(paragraph, <TestLine>[
+      l('AAA ', 0, 4, hardBreak: false, width: 30.0),
+      l('BBB CC', 4, 10, hardBreak: true, width: 60.0),
+    ]);
+  });
 }
diff --git a/lib/web_ui/test/text/line_breaker_test.dart b/lib/web_ui/test/text/line_breaker_test.dart
index 8545b8cab6662..32673dc91233a 100644
--- a/lib/web_ui/test/text/line_breaker_test.dart
+++ b/lib/web_ui/test/text/line_breaker_test.dart
@@ -223,6 +223,23 @@ void testMain() {
       );
     });
 
+    test('whitespace before the last character', () {
+      const String text = 'Lorem sit .';
+      const LineBreakResult expectedResult =
+          LineBreakResult(10, 10, 9, LineBreakType.opportunity);
+
+      LineBreakResult result;
+
+      result = nextLineBreak(text, 6);
+      expect(result, expectedResult);
+
+      result = nextLineBreak(text, 9);
+      expect(result, expectedResult);
+
+      result = nextLineBreak(text, 9, maxEnd: 10);
+      expect(result, expectedResult);
+    });
+
     test('comprehensive test', () {
       final List<TestCase> testCollection = parseRawTestData(rawLineBreakTestData);
       for (int t = 0; t < testCollection.length; t++) {
@@ -269,7 +286,7 @@ void testMain() {
               reason: 'Failed at test case number $t:\n'
                   '${testCase.toString()}\n'
                   '"$text"\n'
-                  '\nUnexpected line break found at {$lastLineBreak - $i}.',
+                  '\nUnexpected line break found at {$lastLineBreak - ${result.index}}.',
             );
 
             // Since this isn't a line break, passing it as a `maxEnd` should
diff --git a/lib/web_ui/test/text/line_breaker_test_helper.dart b/lib/web_ui/test/text/line_breaker_test_helper.dart
index 7af7122050609..7fa153dfb3805 100644
--- a/lib/web_ui/test/text/line_breaker_test_helper.dart
+++ b/lib/web_ui/test/text/line_breaker_test_helper.dart
@@ -2,7 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-
 // The following test cases contradict rule LB25, so we are replacing them
 // with correct expectations.
 const Map<String, String> _replacements = <String, String>{
@@ -91,8 +90,26 @@ bool isValidTestCase(String line) {
 }
 
 String _checkReplacement(String line) {
-  final String? replacement = _replacements[line];
-  return replacement ?? line;
+  String replacement = _replacements[line] ?? line;
+  // Special case for rule LB13 to allow line breaks after spaces.
+  if (replacement.contains('SPACE (SP) × [13.')) {
+    replacement = replacement
+        .replaceAll('0020 ×', '0020 ÷')
+        .replaceFirst('SPACE (SP) × [13.', 'SPACE (SP) ÷ [13.');
+  }
+  // Special case for rule LB14 to allow line breaks after spaces.
+  if (replacement.contains('SPACE (SP) × [14.')) {
+    replacement = replacement
+        .replaceAll('0020 ×', '0020 ÷')
+        .replaceAll('SPACE (SP) × [14.', 'SPACE (SP) ÷ [14.');
+  }
+  // Special case for rule LB15 to allow line breaks after spaces.
+  if (replacement.contains('SPACE (SP) × [15.')) {
+    replacement = replacement
+        .replaceAll('0020 ×', '0020 ÷')
+        .replaceAll('SPACE (SP) × [15.', 'SPACE (SP) ÷ [15.');
+  }
+  return replacement;
 }
 
 final RegExp spaceRegex = RegExp(r'\s+');

From 8ce654060f945cc44b127c2465428a07a88751e2 Mon Sep 17 00:00:00 2001
From: Mouad Debbar <mdebbar@google.com>
Date: Thu, 16 Sep 2021 13:59:50 -0400
Subject: [PATCH 2/4] update goldens

---
 lib/web_ui/dev/goldens_lock.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/web_ui/dev/goldens_lock.yaml b/lib/web_ui/dev/goldens_lock.yaml
index d70560b437375..216e25c043ba2 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: 96b97105f7dc8ebae5babdf22b809ba3980061f6
+revision: 9c36f57f1a673a7ab444f4f20df16601dde15335

From 42dc58930cbdbc35e8e197c129f55685279c9b28 Mon Sep 17 00:00:00 2001
From: Mouad Debbar <mdebbar@google.com>
Date: Mon, 20 Sep 2021 14:05:21 -0400
Subject: [PATCH 3/4] address review comments

---
 lib/web_ui/lib/src/engine/text/layout_service.dart | 11 +++++++++--
 lib/web_ui/lib/src/engine/text/line_breaker.dart   | 13 ++++++++-----
 lib/web_ui/test/text/layout_service_rich_test.dart |  2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

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 934708d044081..f54b9b36e15ae 100644
--- a/lib/web_ui/lib/src/engine/text/layout_service.dart
+++ b/lib/web_ui/lib/src/engine/text/layout_service.dart
@@ -178,6 +178,9 @@ class TextLayoutService {
             currentLine.revertToLastBreakOpportunity();
             // If a revert had occurred in the line, we need to revert the span
             // index accordingly.
+            //
+            // If no revert occurred, then `revertedToSpan` will be equal to
+            // `span` and the following while loop won't do anything.
             final ParagraphSpan revertedToSpan = currentLine.lastSegment.span;
             while (span != revertedToSpan) {
               span = paragraph.spans[--spanIndex];
@@ -1214,6 +1217,10 @@ class LineBuilder {
     while (isEndProhibited) {
       _popSegment();
     }
+    // Make sure the line is not empty and still breakable after popping a few
+    // segments.
+    assert(isNotEmpty);
+    assert(isBreakable);
   }
 
   LineBreakResult get _currentBoxStart {
@@ -1400,8 +1407,8 @@ class LineBuilder {
   DirectionalPosition findNextBreak() {
     LineBreakResult? nextBreak = _cachedNextBreak;
     final String text = paragraph.toPlainText();
-    // Don't recompute the `nextBreak` again until the line has reached the
-    // previously computed `nextBreak`.
+    // Don't recompute the `nextBreak` until the line has reached the previously
+    // computed `nextBreak`.
     if (nextBreak == null || end.index >= nextBreak.index) {
       final int maxEnd = spanometer.currentSpan.end;
       nextBreak = nextLineBreak(text, end.index, maxEnd: maxEnd);
diff --git a/lib/web_ui/lib/src/engine/text/line_breaker.dart b/lib/web_ui/lib/src/engine/text/line_breaker.dart
index 1589cf68574ce..c8232d4d84188 100644
--- a/lib/web_ui/lib/src/engine/text/line_breaker.dart
+++ b/lib/web_ui/lib/src/engine/text/line_breaker.dart
@@ -155,7 +155,7 @@ bool _hasEastAsianWidthFWH(int charCode) {
 
 /// Finds the next line break in the given [text] starting from [index].
 ///
-/// Wethink about indices as pointing between characters, and they go all the
+/// We think about indices as pointing between characters, and they go all the
 /// way from 0 to the string length. For example, here are the indices for the
 /// string "foo bar":
 ///
@@ -405,8 +405,9 @@ LineBreakResult _unsafeNextLineBreak(String text, int index, {int? maxEnd}) {
     //       × IS
     //       × SY
     //
-    // Modification: when these characters are preceded by a space, they
-    // shouldn't prevent a line break.
+    // The above is a quote from unicode.org. In our implementation, we did the
+    // following modification: When there are spaces present, we consider it a
+    // line break opportunity.
     if (prev1 != LineCharProperty.SP &&
         (curr == LineCharProperty.CL ||
             curr == LineCharProperty.CP ||
@@ -419,7 +420,8 @@ LineBreakResult _unsafeNextLineBreak(String text, int index, {int? maxEnd}) {
     // Do not break after ‘[’, even after spaces.
     // LB14: OP SP* ×
     //
-    // Modification: allow breaks when there are spaces.
+    // The above is a quote from unicode.org. In our implementation, we did the
+    // following modification: Allow breaks when there are spaces.
     if (prev1 == LineCharProperty.OP) {
       continue;
     }
@@ -427,7 +429,8 @@ LineBreakResult _unsafeNextLineBreak(String text, int index, {int? maxEnd}) {
     // Do not break within ‘”[’, even with intervening spaces.
     // LB15: QU SP* × OP
     //
-    // Modification: allow breaks when there are spaces.
+    // The above is a quote from unicode.org. In our implementation, we did the
+    // following modification: Allow breaks when there are spaces.
     if (prev1 == LineCharProperty.QU && curr == LineCharProperty.OP) {
       continue;
     }
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 f29d4db32a4a3..e022830908d84 100644
--- a/lib/web_ui/test/text/layout_service_rich_test.dart
+++ b/lib/web_ui/test/text/layout_service_rich_test.dart
@@ -233,7 +233,7 @@ Future<void> testMain() async {
     ]);
   });
 
-  test('does not take prohibited line breaks', () {
+  test('does not make prohibited line breaks', () {
     final CanvasParagraph paragraph = rich(ahemStyle, (ui.ParagraphBuilder builder) {
       builder.pushStyle(EngineTextStyle.only(color: blue));
       builder.addText('AAA B');

From 01331c0caa2f9365088bd8aaf6faa41e502e957d Mon Sep 17 00:00:00 2001
From: Mouad Debbar <mdebbar@google.com>
Date: Mon, 20 Sep 2021 14:05:42 -0400
Subject: [PATCH 4/4] revert boxes too when necessary

---
 .../lib/src/engine/text/layout_service.dart   |  9 ++++
 .../test/text/canvas_paragraph_test.dart      | 53 +++++++++++++++++++
 2 files changed, 62 insertions(+)

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 f54b9b36e15ae..c4c62dfad48cc 100644
--- a/lib/web_ui/lib/src/engine/text/layout_service.dart
+++ b/lib/web_ui/lib/src/engine/text/layout_service.dart
@@ -1124,6 +1124,15 @@ class LineBuilder {
       }
     }
 
+    // Now let's fixes boxes if they need fixing.
+    //
+    // If we popped a segment of an already created box, we should pop the box
+    // too.
+    if (_currentBoxStart.index > poppedSegment.start.index) {
+      final RangeBox poppedBox = _boxes.removeLast();
+      _currentBoxStartOffset -= poppedBox.width;
+    }
+
     return poppedSegment;
   }
 
diff --git a/lib/web_ui/test/text/canvas_paragraph_test.dart b/lib/web_ui/test/text/canvas_paragraph_test.dart
index 6d287261782d3..fff138c43ccbb 100644
--- a/lib/web_ui/test/text/canvas_paragraph_test.dart
+++ b/lib/web_ui/test/text/canvas_paragraph_test.dart
@@ -369,6 +369,59 @@ Future<void> testMain() async {
         ],
       );
     });
+
+    test('pops boxes when segments are popped', () {
+      final CanvasParagraph paragraph = rich(ahemStyle, (ui.ParagraphBuilder builder) {
+        // Lines:
+        //   "AAA "
+        //   "B_C DD"
+        builder.pushStyle(EngineTextStyle.only(color: blue));
+        builder.addText('AAA B');
+        builder.pushStyle(EngineTextStyle.only(color: green));
+        builder.addText('_C ');
+        builder.pushStyle(EngineTextStyle.only(color: green));
+        builder.addText('DD');
+      });
+
+      // The layout algorithm will keep appending segments to the line builder
+      // until it reaches: "AAA B_". At that point, it'll try to add the "C" but
+      // realizes there isn't enough width in the line. Since the line already
+      // had a line break opportunity, the algorithm tries to revert the line to
+      // that opportunity (i.e. "AAA ") and pops the segments "_" and "B".
+      //
+      // Because the segments "B" and "_" have different directionality
+      // preferences (LTR and no-preferenece, respectively), the algorithm
+      // should've already created a box for "B". When the "B" segment is popped
+      // we want to make sure that the "B" box is also popped.
+      paragraph.layout(constrain(60));
+
+      final EngineLineMetrics firstLine = paragraph.computeLineMetrics()[0];
+      final EngineLineMetrics secondLine = paragraph.computeLineMetrics()[1];
+
+      // There should be no "B" in the first line's boxes.
+      expect(firstLine.boxes, hasLength(2));
+
+      expect((firstLine.boxes![0] as SpanBox).toText(), 'AAA');
+      expect((firstLine.boxes![0] as SpanBox).left, 0.0);
+
+      expect((firstLine.boxes![1] as SpanBox).toText(), ' ');
+      expect((firstLine.boxes![1] as SpanBox).left, 30.0);
+
+      // Make sure the second line isn't missing any boxes.
+      expect(secondLine.boxes, hasLength(4));
+
+      expect((secondLine.boxes![0] as SpanBox).toText(), 'B');
+      expect((secondLine.boxes![0] as SpanBox).left, 0.0);
+
+      expect((secondLine.boxes![1] as SpanBox).toText(), '_C');
+      expect((secondLine.boxes![1] as SpanBox).left, 10.0);
+
+      expect((secondLine.boxes![2] as SpanBox).toText(), ' ');
+      expect((secondLine.boxes![2] as SpanBox).left, 30.0);
+
+      expect((secondLine.boxes![3] as SpanBox).toText(), 'DD');
+      expect((secondLine.boxes![3] as SpanBox).left, 40.0);
+    });
   });
 
   group('$CanvasParagraph.getPositionForOffset', () {