[LayoutNG] Fix hyphens when rewinding occurs

This patch removes |NGLineInfo::LineEndFragment|, which was
introduced to support hyphens and ellipsis with the single
code. However, ellipsis was changed to use different code,
and |LineEndFragment| ended up with non-optimal way to handle
hyphens.

Although hyphens appear at most once in a line, storing this
to |NGLineInfo| makes harder to keep it in sync with the
hyphenated item. This patch changes it to a field in
|NGInlineItemResult|.

Bug: 1015297
Change-Id: I0706cf54bebfa967661dc40123c6dfd5887ff59c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868534
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708894}

third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.cc

@@ -220,18 +220,7 @@ LayoutUnit NGLineInfo::ComputeWidth() const {
   for (const NGInlineItemResult& item_result : Results())
     inline_size += item_result.inline_size;
 
-  if (UNLIKELY(line_end_fragment_)) {
-    inline_size += line_end_fragment_->Size()
-                       .ConvertToLogical(LineStyle().GetWritingMode())
-                       .inline_size;
-  }
-
   return inline_size;
 }
 
-void NGLineInfo::SetLineEndFragment(
-    scoped_refptr<const NGPhysicalTextFragment> fragment) {
-  line_end_fragment_ = std::move(fragment);
-}
-
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/inline/ng_inline_item_result.h

@@ -7,7 +7,6 @@
 
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_box_strut.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h"
-#include "third_party/blink/renderer/core/layout/ng/inline/ng_text_end_effect.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_layout_result.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_positioned_float.h"
 #include "third_party/blink/renderer/platform/fonts/shaping/shape_result.h"
@@ -37,6 +36,15 @@ struct CORE_EXPORT NGInlineItemResult {
     return end_offset - start_offset;
   }
 
+  LayoutUnit HyphenInlineSize() const {
+    return hyphen_shape_result->SnappedWidth().ClampNegativeToZero();
+  }
+
+  void ClearHyphen() {
+    hyphen_string = String();
+    hyphen_shape_result = nullptr;
+  }
+
   // The NGInlineItem and its index.
   const NGInlineItem* item;
   unsigned item_index;
@@ -52,6 +60,10 @@ struct CORE_EXPORT NGInlineItemResult {
   // is needed in the line breaker.
   scoped_refptr<const ShapeResultView> shape_result;
 
+  // Hyphen character and its |ShapeResult| if this text is hyphenated.
+  String hyphen_string;
+  scoped_refptr<const ShapeResult> hyphen_shape_result;
+
   // NGLayoutResult for atomic inline items.
   scoped_refptr<const NGLayoutResult> layout_result;
 
@@ -112,10 +124,6 @@ struct CORE_EXPORT NGInlineItemResult {
   // position) any unpositioned floats.
   bool has_unpositioned_floats = false;
 
-  // End effects for text items.
-  // The effects are included in |shape_result|, but not in text content.
-  NGTextEndEffect text_end_effect = NGTextEndEffect::kNone;
-
   NGInlineItemResult();
   NGInlineItemResult(const NGInlineItem*,
                      unsigned index,
@@ -242,12 +250,6 @@ class CORE_EXPORT NGLineInfo {
   // justify alignment.
   bool NeedsAccurateEndPosition() const { return needs_accurate_end_position_; }
 
-  // Fragment to append to the line end. Used by 'text-overflow: ellipsis'.
-  scoped_refptr<const NGPhysicalTextFragment>& LineEndFragment() {
-    return line_end_fragment_;
-  }
-  void SetLineEndFragment(scoped_refptr<const NGPhysicalTextFragment>);
-
  private:
   bool ComputeNeedsAccurateEndPosition() const;
 
@@ -258,7 +260,6 @@ class CORE_EXPORT NGLineInfo {
   const NGInlineItemsData* items_data_ = nullptr;
   const ComputedStyle* line_style_ = nullptr;
   NGInlineItemResults results_;
-  scoped_refptr<const NGPhysicalTextFragment> line_end_fragment_;
 
   NGBfcOffset bfc_offset_;
 

third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.cc

@@ -236,7 +236,7 @@ void NGInlineLayoutAlgorithm::CreateLine(
                                  baseline_type_);
       }
 
-      if (item.IsSymbolMarker()) {
+      if (UNLIKELY(item.IsSymbolMarker())) {
         text_builder.SetItem(NGPhysicalTextFragment::kSymbolMarker,
                              line_info->ItemsData(), &item_result,
                              box->text_height);
@@ -245,10 +245,19 @@ void NGInlineLayoutAlgorithm::CreateLine(
                              line_info->ItemsData(), &item_result,
                              box->text_height);
       }
-      line_box_.AddChild(text_builder.ToTextFragment(), box->text_top,
-                         item_result.inline_size, item.BidiLevel());
+      if (UNLIKELY(item_result.hyphen_shape_result)) {
+        LayoutUnit hyphen_inline_size = item_result.HyphenInlineSize();
+        line_box_.AddChild(text_builder.ToTextFragment(), box->text_top,
+                           item_result.inline_size - hyphen_inline_size,
+                           item.BidiLevel());
+        PlaceHyphen(item_result, hyphen_inline_size, box);
+      } else {
+        line_box_.AddChild(text_builder.ToTextFragment(), box->text_top,
+                           item_result.inline_size, item.BidiLevel());
+      }
       // Text boxes always need full paint invalidations.
       item.GetLayoutObject()->ClearNeedsLayoutWithFullPaintInvalidation();
+
     } else if (item.Type() == NGInlineItem::kControl) {
       PlaceControlItem(item, *line_info, &item_result, box);
     } else if (item.Type() == NGInlineItem::kOpenTag) {
@@ -284,13 +293,6 @@ void NGInlineLayoutAlgorithm::CreateLine(
     }
   }
 
-  if (line_info->LineEndFragment()) {
-    // Add a generated text fragment, hyphen or ellipsis, at the logical end.
-    // By using the paragraph bidi_level, it will appear at the visual end.
-    PlaceGeneratedContent(std::move(line_info->LineEndFragment()),
-                          IsLtr(line_info->BaseDirection()) ? 0 : 1, box);
-  }
-
   box_states_->OnEndPlaceItems(&line_box_, baseline_type_);
 
   if (UNLIKELY(Node().IsBidiEnabled())) {
@@ -425,32 +427,23 @@ void NGInlineLayoutAlgorithm::PlaceControlItem(const NGInlineItem& item,
                      item_result->inline_size, item.BidiLevel());
 }
 
-// Place a generated content that does not exist in DOM nor in LayoutObject
-// tree.
-void NGInlineLayoutAlgorithm::PlaceGeneratedContent(
-    scoped_refptr<const NGPhysicalTextFragment> fragment,
-    UBiDiLevel bidi_level,
-    NGInlineBoxState* box) {
-  LayoutUnit inline_size = IsHorizontalWritingMode() ? fragment->Size().width
-                                                     : fragment->Size().height;
-  const ComputedStyle& style = fragment->Style();
-  if (box->CanAddTextOfStyle(style)) {
-    if (UNLIKELY(quirks_mode_))
-      box->EnsureTextMetrics(style, baseline_type_);
-    DCHECK(!box->text_metrics.IsEmpty());
-    line_box_.AddChild(std::move(fragment), box->text_top, inline_size,
-                       bidi_level);
-  } else {
-    scoped_refptr<ComputedStyle> text_style =
-        ComputedStyle::CreateAnonymousStyleWithDisplay(style,
-                                                       EDisplay::kInline);
-    NGInlineBoxState* box = box_states_->OnOpenTag(*text_style, line_box_);
-    box->ComputeTextMetrics(*text_style, baseline_type_);
-    DCHECK(!box->text_metrics.IsEmpty());
-    line_box_.AddChild(std::move(fragment), box->text_top, inline_size,
-                       bidi_level);
-    box_states_->OnCloseTag(&line_box_, box, baseline_type_);
-  }
+void NGInlineLayoutAlgorithm::PlaceHyphen(const NGInlineItemResult& item_result,
+                                          LayoutUnit hyphen_inline_size,
+                                          NGInlineBoxState* box) {
+  DCHECK(item_result.item);
+  DCHECK(item_result.hyphen_string);
+  DCHECK(item_result.hyphen_shape_result);
+  DCHECK_EQ(hyphen_inline_size, item_result.HyphenInlineSize());
+  const NGInlineItem& item = *item_result.item;
+  const WritingMode writing_mode = ConstraintSpace().GetWritingMode();
+  NGTextFragmentBuilder builder(writing_mode);
+  builder.SetText(
+      item.GetLayoutObject(), item_result.hyphen_string, item.Style(),
+      /* is_ellipsis_style */ false,
+      ShapeResultView::Create(item_result.hyphen_shape_result.get()));
+  DCHECK(!box->text_metrics.IsEmpty());
+  line_box_.AddChild(builder.ToTextFragment(), box->text_top,
+                     hyphen_inline_size, item.BidiLevel());
 }
 
 NGInlineBoxState* NGInlineLayoutAlgorithm::PlaceAtomicInline(
@@ -694,8 +687,8 @@ bool NGInlineLayoutAlgorithm::ApplyJustify(LayoutUnit space,
   // matches to the |ShapeResult|.
   DCHECK(!line_info->Results().IsEmpty());
   const NGInlineItemResult& last_item_result = line_info->Results().back();
-  if (last_item_result.text_end_effect == NGTextEndEffect::kHyphen)
-    line_text_builder.Append(last_item_result.item->Style()->HyphenString());
+  if (last_item_result.hyphen_string)
+    line_text_builder.Append(last_item_result.hyphen_string);
 
   // Compute the spacing to justify.
   String line_text = line_text_builder.ToString();
@@ -714,14 +707,14 @@ bool NGInlineLayoutAlgorithm::ApplyJustify(LayoutUnit space,
       scoped_refptr<ShapeResult> shape_result =
           item_result.shape_result->CreateShapeResult();
       DCHECK_GE(item_result.start_offset, line_info->StartOffset());
-      // |shape_result| has more characters if it's hyphenated.
-      DCHECK(item_result.text_end_effect != NGTextEndEffect::kNone ||
-             shape_result->NumCharacters() ==
-                 item_result.end_offset - item_result.start_offset);
+      DCHECK_EQ(shape_result->NumCharacters(),
+                item_result.end_offset - item_result.start_offset);
       shape_result->ApplySpacing(spacing, item_result.start_offset -
                                               line_info->StartOffset() -
                                               shape_result->StartIndex());
       item_result.inline_size = shape_result->SnappedWidth();
+      if (UNLIKELY(item_result.hyphen_shape_result))
+        item_result.inline_size += item_result.HyphenInlineSize();
       item_result.shape_result = ShapeResultView::Create(shape_result.get());
     } else if (item_result.item->Type() == NGInlineItem::kAtomicInline) {
       float offset = 0.f;

third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.h

@@ -80,9 +80,9 @@ class CORE_EXPORT NGInlineLayoutAlgorithm final
                         const NGLineInfo&,
                         NGInlineItemResult*,
                         NGInlineBoxState*);
-  void PlaceGeneratedContent(scoped_refptr<const NGPhysicalTextFragment>,
-                             UBiDiLevel,
-                             NGInlineBoxState*);
+  void PlaceHyphen(const NGInlineItemResult&,
+                   LayoutUnit hyphen_inline_size,
+                   NGInlineBoxState*);
   NGInlineBoxState* PlaceAtomicInline(const NGInlineItem&,
                                       const NGLineInfo&,
                                       NGInlineItemResult*);

third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.cc

@@ -132,22 +132,16 @@ LayoutUnit ComputeFloatAncestorInlineEndSize(const NGConstraintSpace& space,
   return inline_end_size;
 }
 
-scoped_refptr<const NGPhysicalTextFragment> CreateHyphenFragment(
-    NGInlineNode node,
-    WritingMode writing_mode,
-    const NGInlineItem& item) {
+void CreateHyphen(NGInlineNode node,
+                  WritingMode writing_mode,
+                  const NGInlineItem& item,
+                  NGInlineItemResult* item_result) {
   DCHECK(item.Style());
   const ComputedStyle& style = *item.Style();
   TextDirection direction = style.Direction();
-  String hyphen_string = style.HyphenString();
-  HarfBuzzShaper shaper(hyphen_string);
-  scoped_refptr<ShapeResult> hyphen_result =
-      shaper.Shape(&style.GetFont(), direction);
-  NGTextFragmentBuilder builder(writing_mode);
-  builder.SetText(item.GetLayoutObject(), hyphen_string, &style,
-                  /* is_ellipsis_style */ false,
-                  ShapeResultView::Create(hyphen_result.get()));
-  return builder.ToTextFragment();
+  item_result->hyphen_string = style.HyphenString();
+  HarfBuzzShaper shaper(item_result->hyphen_string);
+  item_result->hyphen_shape_result = shaper.Shape(&style.GetFont(), direction);
 }
 
 inline void ClearNeedsLayout(const NGInlineItem& item) {
@@ -262,25 +256,6 @@ void NGLineBreaker::SetMaxSizeCache(MaxSizeCache* max_size_cache) {
   max_size_cache_ = max_size_cache;
 }
 
-LayoutUnit NGLineBreaker::SetLineEndFragment(
-    scoped_refptr<const NGPhysicalTextFragment> fragment,
-    NGLineInfo* line_info) {
-  LayoutUnit inline_size;
-  bool is_horizontal =
-      IsHorizontalWritingMode(constraint_space_.GetWritingMode());
-  if (line_info->LineEndFragment()) {
-    const PhysicalSize& size = line_info->LineEndFragment()->Size();
-    inline_size = is_horizontal ? -size.width : -size.height;
-  }
-  if (fragment) {
-    const PhysicalSize& size = fragment->Size();
-    inline_size = is_horizontal ? size.width : size.height;
-  }
-  line_info->SetLineEndFragment(std::move(fragment));
-  position_ += inline_size;
-  return inline_size;
-}
-
 // Compute the base direction for bidi algorithm for this line.
 void NGLineBreaker::ComputeBaseDirection() {
   // If 'unicode-bidi' is not 'plaintext', use the base direction of the block.
@@ -752,25 +727,24 @@ NGLineBreaker::BreakResult NGLineBreaker::BreakText(
     CHECK_GT(result.break_offset, item_result->start_offset);
 
     inline_size = shape_result->SnappedWidth().ClampNegativeToZero();
-    item_result->inline_size = inline_size;
     if (UNLIKELY(result.is_hyphenated)) {
       const WritingMode writing_mode = constraint_space_.GetWritingMode();
-      scoped_refptr<const NGPhysicalTextFragment> hyphen_fragment =
-          CreateHyphenFragment(node_, writing_mode, item);
+      CreateHyphen(node_, writing_mode, item, item_result);
+      DCHECK(item_result->hyphen_shape_result);
+      DCHECK(item_result->hyphen_string);
       LayoutUnit space_for_hyphen = available_width - inline_size;
-      LayoutUnit hyphen_inline_size = IsHorizontalWritingMode(writing_mode)
-                                          ? hyphen_fragment->Size().width
-                                          : hyphen_fragment->Size().height;
+      LayoutUnit hyphen_inline_size = item_result->HyphenInlineSize();
       // If the hyphen overflows, retry with the reduced available width.
       if (space_for_hyphen >= 0 && hyphen_inline_size > space_for_hyphen) {
         available_width -= hyphen_inline_size;
         continue;
       }
-      inline_size += SetLineEndFragment(std::move(hyphen_fragment), line_info);
-      item_result->text_end_effect = NGTextEndEffect::kHyphen;
+      inline_size += hyphen_inline_size;
+    } else if (UNLIKELY(item_result->hyphen_shape_result)) {
+      item_result->hyphen_shape_result = nullptr;
+      item_result->hyphen_string = String();
     }
-    item_result->inline_size =
-        shape_result->SnappedWidth().ClampNegativeToZero();
+    item_result->inline_size = inline_size;
     item_result->end_offset = result.break_offset;
     item_result->shape_result = std::move(shape_result);
     break;
@@ -1744,8 +1718,6 @@ void NGLineBreaker::HandleOverflow(NGLineInfo* line_info) {
           if (new_end == item_results->size()) {
             position_ =
                 available_width + width_to_rewind + item_result->inline_size;
-            if (line_info->LineEndFragment())
-              SetLineEndFragment(nullptr, line_info);
             DCHECK_EQ(position_, line_info->ComputeWidth());
             item_index_ = item_result->item_index;
             offset_ = item_result->end_offset;
@@ -1762,6 +1734,8 @@ void NGLineBreaker::HandleOverflow(NGLineInfo* line_info) {
           return;
         }
         SetCurrentStyle(*was_current_style);
+        if (i < item_results->size() - 1 && item_result->can_break_after)
+          break_before = i + 1;
         position_maybe_changed = true;
       }
     }
@@ -1814,6 +1788,23 @@ void NGLineBreaker::RewindOverflow(unsigned new_end, NGLineInfo* line_info) {
   const Vector<NGInlineItem>& items = Items();
   const NGInlineItemResults& item_results = line_info->Results();
   DCHECK_LT(new_end, item_results.size());
+
+  // |HandleOverflow()| may have tried to break the last text item. In that
+  // case, stop trying to include following items because there is a gap to the
+  // next item.
+  if (new_end) {
+    const NGInlineItemResult& item_result = item_results[new_end - 1];
+    DCHECK(item_result.item);
+    const NGInlineItem& item = *item_result.item;
+    if (item.Type() == NGInlineItem::kText &&
+        item.EndOffset() != item_result.end_offset) {
+      Rewind(new_end, line_info);
+      DCHECK_EQ(static_cast<unsigned>(&item - items.begin()), item_index_);
+      HandleTrailingSpaces(item, line_info);
+      return;
+    }
+  }
+
   unsigned open_tag_count = 0;
   const String& text = Text();
   for (unsigned index = new_end; index < item_results.size(); index++) {
@@ -1966,7 +1957,6 @@ void NGLineBreaker::Rewind(unsigned new_end, NGLineInfo* line_info) {
   item_results.Shrink(new_end);
 
   trailing_collapsible_space_.reset();
-  SetLineEndFragment(nullptr, line_info);
   position_ = line_info->ComputeWidth();
 }
 

third_party/blink/renderer/core/layout/ng/inline/ng_line_breaker.h

@@ -99,8 +99,6 @@ class CORE_EXPORT NGLineBreaker {
                               unsigned end_offset,
                               NGLineInfo*);
   NGInlineItemResult* AddItem(const NGInlineItem&, NGLineInfo*);
-  LayoutUnit SetLineEndFragment(scoped_refptr<const NGPhysicalTextFragment>,
-                                NGLineInfo*);
 
   void BreakLine(LayoutUnit percentage_resolution_block_size_for_min_max,
                  NGLineInfo*);

third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h

@@ -6,7 +6,6 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_INLINE_NG_PHYSICAL_TEXT_FRAGMENT_H_
 
 #include "third_party/blink/renderer/core/core_export.h"
-#include "third_party/blink/renderer/core/layout/ng/inline/ng_text_end_effect.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_ink_overflow.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_physical_fragment.h"
 #include "third_party/blink/renderer/platform/fonts/ng_text_fragment_paint_info.h"

third_party/blink/renderer/core/layout/ng/inline/ng_text_end_effect.h

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_INLINE_NG_TEXT_END_EFFECT_H_
-#define THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_INLINE_NG_TEXT_END_EFFECT_H_
-
-namespace blink {
-
-// Effects at the end of text fragments.
-enum class NGTextEndEffect {
-  kNone,
-  kHyphen,
-
-  // When adding new values, ensure NGPhysicalTextFragment has enough bits.
-};
-
-}  // namespace blink
-
-#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_LAYOUT_NG_INLINE_NG_TEXT_END_EFFECT_H_

third_party/blink/renderer/core/layout/ng/inline/ng_text_fragment_builder.h

@@ -8,7 +8,6 @@
 #include "third_party/blink/renderer/core/layout/geometry/logical_size.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h"
-#include "third_party/blink/renderer/core/layout/ng/inline/ng_text_end_effect.h"
 #include "third_party/blink/renderer/core/layout/ng/ng_fragment_builder.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 

third_party/blink/web_tests/external/wpt/css/css-text/hyphens/hyphens-overflow-001.html

@@ -9,6 +9,7 @@ div {
   font-size: 10px;
   font-family: Ahem;
   width: 5.1ch;
+  border: 1px solid blue;
 }
 </style>
 <body>
@@ -18,4 +19,7 @@ div {
   <div>1234&shy;xx</div>
   <div>12345&shy;xx</div>
   <div>123456&shy;xx</div>
+
+  <div style="width: 10ch"><span>ren&shy;for&shy;cer</span>99999</div>
+  <div><span>00&shy;1</span>222</div>
 </body>

third_party/blink/web_tests/external/wpt/css/css-text/hyphens/reference/hyphens-overflow-001-ref.html

@@ -4,6 +4,8 @@
 div {
   font-size: 10px;
   font-family: Ahem;
+  width: 5.1ch;
+  border: 1px solid blue;
 }
 </style>
 <body>
@@ -13,4 +15,7 @@ div {
   <div>1234-<br>xx</div>
   <div>12345-<br>xx</div>
   <div>123456-<br>xx</div>
+
+  <div style="width: 10ch">renfor-<br>cer99999</div>
+  <div>00-<br>1222</div>
 </body>