[web] Multiple fixes to text layout and line breaks (#28642)

Author: mdebbar

lib/web_ui/dev/goldens_lock.yaml

@@ -1,2 +1,2 @@
 repository: https://github.com/flutter/goldens.git
-revision: 96b97105f7dc8ebae5babdf22b809ba3980061f6
+revision: 9c36f57f1a673a7ab444f4f20df16601dde15335

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,16 @@ 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.
+            //
+            // 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];
+            }
             lines.add(currentLine.build());
             currentLine = currentLine.nextLine();
           }
@@ -815,7 +822,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 +853,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 +883,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 +1049,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.
@@ -1099,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;
   }
 
@@ -1182,6 +1216,22 @@ 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();
+    }
+    // Make sure the line is not empty and still breakable after popping a few
+    // segments.
+    assert(isNotEmpty);
+    assert(isBreakable);
+  }
+
   LineBreakResult get _currentBoxStart {
     if (_boxes.isEmpty) {
       return start;
@@ -1360,13 +1410,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` 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.

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';
@@ -153,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":
 ///
@@ -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,34 @@ 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) {
+    //
+    // 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 ||
+            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) {
+    //
+    // 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;
     }
 
     // Do not break within ‘”[’, even with intervening spaces.
     // LB15: QU SP* × OP
-    if ((prev1 == LineCharProperty.QU ||
-            baseOfSpaceSequence == LineCharProperty.QU) &&
-        curr == LineCharProperty.OP) {
+    //
+    // 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;
     }
 

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) {