Fixes CharacterWidths() and text Len(), and simplifies OffsetInContainer() in AbstractInlineTextBox

The text length of an abstract inline text box was sometimes reported as one character short because
the trailing white space was not included, whilst it was included in the actual text.
Similarly, the character widths did not include the width of any trailing white space.
Also simplified the calculation of OffsetInContainer by using some existing helper methods in Layout NG
and improved tests.

AX-Relnotes: n/a.

R=dmazzoni@chromium.org, aleventhal@chromium.org

Change-Id: Ia5af8a475c028ecc1980b7cbe12695807164824d
Bug: 1008031
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346289Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797185}

third_party/blink/renderer/core/layout/line/abstract_inline_text_box.cc

@@ -146,31 +146,38 @@ LayoutRect LegacyAbstractInlineTextBox::LocalBounds() const {
 
 unsigned LegacyAbstractInlineTextBox::Len() const {
   if (!inline_text_box_)
-    return 0;
+    return 0u;
 
-  return inline_text_box_->Len();
+  return NeedsTrailingSpace() ? inline_text_box_->Len() + 1
+                              : inline_text_box_->Len();
 }
 
 unsigned LegacyAbstractInlineTextBox::TextOffsetInContainer(
     unsigned offset) const {
   if (!inline_text_box_)
-    return 0;
-
-  unsigned offset_in_container = inline_text_box_->Start() + offset;
-
-  const NGOffsetMapping* offset_mapping = GetOffsetMapping();
-  if (!offset_mapping)
-    return offset_in_container;
+    return 0U;
 
   // The start offset of the inline text box returned by
   // inline_text_box_->Start() includes the collapsed white-spaces. Here, we
   // want the position in the parent node after white-space collapsing.
   // NGOffsetMapping can map an offset before whites-spaces are collapsed to the
   // offset after white-spaces are collapsed.
-  Position position(GetNode(), offset_in_container);
-  const NGOffsetMappingUnit* unit =
-      offset_mapping->GetMappingUnitForPosition(position);
-  return offset_in_container - unit->DOMStart() + unit->TextContentStart();
+  unsigned int offset_in_container = inline_text_box_->Start() + offset;
+  const Position position(GetNode(), offset_in_container);
+  LayoutBlockFlow* formatting_context =
+      NGOffsetMapping::GetInlineFormattingContextOf(position);
+  if (!formatting_context)
+    return offset_in_container;
+
+  // If "formatting_context" is not a Layout NG object, the offset mappings will
+  // be computed on demand and cached.
+  const NGOffsetMapping* offset_mapping =
+      NGInlineNode::GetOffsetMapping(formatting_context);
+  if (!offset_mapping)
+    return offset_in_container;
+
+  return offset_mapping->GetTextContentOffset(position).value_or(
+      offset_in_container);
 }
 
 AbstractInlineTextBox::Direction LegacyAbstractInlineTextBox::GetDirection()
@@ -198,6 +205,8 @@ void LegacyAbstractInlineTextBox::CharacterWidths(Vector<float>& widths) const {
     return;
 
   inline_text_box_->CharacterWidths(widths);
+  if (NeedsTrailingSpace())
+    widths.push_back(inline_text_box_->NewlineSpaceWidth());
 }
 
 void AbstractInlineTextBox::GetWordBoundaries(
@@ -289,13 +298,8 @@ String LegacyAbstractInlineTextBox::GetText() const {
   }
 
   // Insert a space at the end of this if necessary.
-  if (InlineTextBox* next = inline_text_box_->NextForSameLayoutObject()) {
-    if (next->Start() > inline_text_box_->Start() + inline_text_box_->Len() &&
-        result.length() && !result.Right(1).ContainsOnlyWhitespaceOrEmpty() &&
-        next->GetText().length() &&
-        !next->GetText().Left(1).ContainsOnlyWhitespaceOrEmpty())
-      return result + " ";
-  }
+  if (NeedsTrailingSpace())
+    return result + " ";
 
   return result;
 }
@@ -351,25 +355,18 @@ bool LegacyAbstractInlineTextBox::IsLineBreak() const {
   return inline_text_box_->IsLineBreak();
 }
 
-const NGOffsetMapping* LegacyAbstractInlineTextBox::GetOffsetMapping() const {
-  const auto* text_node = DynamicTo<Text>(GetNode());
-  if (!text_node)
-    return nullptr;
-
-  LayoutBlockFlow& block_flow = *NGOffsetMapping::GetInlineFormattingContextOf(
-      *text_node->GetLayoutObject());
-  const NGOffsetMapping* offset_mapping =
-      NGInlineNode::GetOffsetMapping(&block_flow);
-
-  if (UNLIKELY(!offset_mapping)) {
-    // TODO(crbug.com/955678): There are certain cases where we fail to
-    // compute // |NGOffsetMapping| due to failures in layout. As the root
-    // cause is hard to fix at the moment, we work around it here so that the
-    // production build doesn't crash.
-    NOTREACHED();
-    return nullptr;
+bool LegacyAbstractInlineTextBox::NeedsTrailingSpace() const {
+  if (const InlineTextBox* next = inline_text_box_->NextForSameLayoutObject()) {
+    return next->Start() >
+               inline_text_box_->Start() + inline_text_box_->Len() &&
+           inline_text_box_->GetText().length() &&
+           !inline_text_box_->GetText()
+                .Right(1)
+                .ContainsOnlyWhitespaceOrEmpty() &&
+           next->GetText().length() &&
+           !next->GetText().Left(1).ContainsOnlyWhitespaceOrEmpty();
   }
-  return offset_mapping;
+  return false;
 }
 
 }  // namespace blink

third_party/blink/renderer/core/layout/line/abstract_inline_text_box.h

@@ -42,7 +42,6 @@
 namespace blink {
 
 class InlineTextBox;
-class NGOffsetMapping;
 
 // High-level abstraction of InlineTextBox to allow the accessibility module to
 // get information about InlineTextBoxes without tight coupling.
@@ -78,6 +77,7 @@ class CORE_EXPORT AbstractInlineTextBox
   virtual scoped_refptr<AbstractInlineTextBox> NextOnLine() const = 0;
   virtual scoped_refptr<AbstractInlineTextBox> PreviousOnLine() const = 0;
   virtual bool IsLineBreak() const = 0;
+  virtual bool NeedsTrailingSpace() const = 0;
 
  protected:
   explicit AbstractInlineTextBox(LineLayoutText line_layout_item);
@@ -123,7 +123,7 @@ class CORE_EXPORT LegacyAbstractInlineTextBox final
   scoped_refptr<AbstractInlineTextBox> NextOnLine() const final;
   scoped_refptr<AbstractInlineTextBox> PreviousOnLine() const final;
   bool IsLineBreak() const final;
-  const NGOffsetMapping* GetOffsetMapping() const;
+  bool NeedsTrailingSpace() const final;
 
   InlineTextBox* inline_text_box_;
 

third_party/blink/renderer/core/layout/line/abstract_inline_text_box_test.cc

@@ -39,6 +39,8 @@ TEST_P(AbstractInlineTextBoxTest, GetTextWithCollapsedWhiteSpace) {
       layout_text.FirstAbstractInlineTextBox();
 
   EXPECT_EQ("abc", inline_text_box->GetText());
+  EXPECT_EQ(3u, inline_text_box->Len());
+  EXPECT_FALSE(inline_text_box->NeedsTrailingSpace());
 }
 
 // For DumpAccessibilityTreeTest.AccessibilityInputTextValue/blink
@@ -55,12 +57,14 @@ TEST_P(AbstractInlineTextBoxTest, GetTextWithLineBreakAtCollapsedWhiteSpace) {
       layout_text.FirstAbstractInlineTextBox();
 
   EXPECT_EQ("abc:", inline_text_box->GetText());
+  EXPECT_EQ(4u, inline_text_box->Len());
+  EXPECT_FALSE(inline_text_box->NeedsTrailingSpace());
 }
 
 // For "web_tests/accessibility/inline-text-change-style.html"
 TEST_P(AbstractInlineTextBoxTest,
        GetTextWithLineBreakAtMiddleCollapsedWhiteSpace) {
-  // Line break at a space after "012".
+  // There should be a line break at the space after "012".
   SetBodyInnerHTML(R"HTML(
     <style>* { font-size: 10px; }</style>
     <div id="target" style="width: 0ch">012 345</div>)HTML");
@@ -72,12 +76,14 @@ TEST_P(AbstractInlineTextBoxTest,
       layout_text.FirstAbstractInlineTextBox();
 
   EXPECT_EQ("012 ", inline_text_box->GetText());
+  EXPECT_EQ(4u, inline_text_box->Len());
+  EXPECT_TRUE(inline_text_box->NeedsTrailingSpace());
 }
 
 // DumpAccessibilityTreeTest.AccessibilitySpanLineBreak/blink
 TEST_P(AbstractInlineTextBoxTest,
        GetTextWithLineBreakAtSpanCollapsedWhiteSpace) {
-  // Line break at a space in <span>.
+  // There should be a line break at the space in <span>.
   SetBodyInnerHTML(R"HTML(
     <style>* { font-size: 10px; }</style>
     <p id="t1" style="width: 0ch">012<span id="t2"> </span>345</p>)HTML");
@@ -89,6 +95,8 @@ TEST_P(AbstractInlineTextBoxTest,
       layout_text1.FirstAbstractInlineTextBox();
 
   EXPECT_EQ("012", inline_text_box1->GetText());
+  EXPECT_EQ(3u, inline_text_box1->Len());
+  EXPECT_FALSE(inline_text_box1->NeedsTrailingSpace());
 
   const Element& target2 = *GetDocument().getElementById("t2");
   LayoutText& layout_text2 =
@@ -96,13 +104,14 @@ TEST_P(AbstractInlineTextBoxTest,
   scoped_refptr<AbstractInlineTextBox> inline_text_box2 =
       layout_text2.FirstAbstractInlineTextBox();
 
-  EXPECT_FALSE(inline_text_box2) << "We don't have inline box when <span> "
-                                    "contains only collapsed white spaces.";
+  EXPECT_EQ(nullptr, inline_text_box2)
+      << "We don't have inline box when <span> "
+         "contains only collapsed white spaces.";
 }
 
 // For DumpAccessibilityTreeTest.AccessibilityInputTypes/blink
 TEST_P(AbstractInlineTextBoxTest, GetTextWithLineBreakAtTrailingWhiteSpace) {
-  // Line break at a space of "abc: ".
+  // There should be a line break at the space of "abc: ".
   SetBodyInnerHTML(R"HTML(
     <style>* { font-size: 10px; }</style>
     <div style="width: 10ch"><label id=label>abc: <input></label></div>)HTML");
@@ -114,40 +123,56 @@ TEST_P(AbstractInlineTextBoxTest, GetTextWithLineBreakAtTrailingWhiteSpace) {
       layout_text.FirstAbstractInlineTextBox();
 
   EXPECT_EQ("abc: ", inline_text_box->GetText());
+  EXPECT_EQ(5u, inline_text_box->Len());
+  if (LayoutNGEnabled()) {
+    EXPECT_TRUE(inline_text_box->NeedsTrailingSpace());
+  } else {
+    EXPECT_FALSE(inline_text_box->NeedsTrailingSpace());
+  }
 }
 
 TEST_P(AbstractInlineTextBoxTest, GetTextOffsetInContainer) {
-  // "&#10" is a Line Feed ("\n").
+  // The span should not affect the offset in container of the following inline
+  // text boxes in the paragraph.
+  //
+  // Note that "&#10" is a Line Feed, ("\n").
   SetBodyInnerHTML(R"HTML(
     <style>p { white-space: pre-line; }</style>
-    <p id="paragraph">First sentence of the &#10; paragraph. Second sentence of &#10; the paragraph. </p>
-    <br id='br'>)HTML");
+    <p id="paragraph"><span>Offset</span>First sentence &#10; of the paragraph. Second sentence of &#10; the paragraph.</p>
+    <br id="br">)HTML");
 
   const Element& paragraph = *GetDocument().getElementById("paragraph");
-  LayoutText& layout_text =
-      *ToLayoutText(paragraph.firstChild()->GetLayoutObject());
-
-  // This test has 5 AbstractInlineTextBox. 1.text 2.\n 3.text 4.\n 5.text.
-  // The AbstractInlineTextBoxes are all child of the same text node and an
+  const Node& text_node = *paragraph.firstChild()->nextSibling();
+  LayoutText& layout_text = *ToLayoutText(text_node.GetLayoutObject());
+
+  // The above "layout_text" should create five AbstractInlineTextBoxes:
+  // 1. "First sentence "
+  // 2. "\n"
+  // 3. "of the paragraph. Second sentence of "
+  // 4." \n"
+  // 5. "the paragraph."
+  //
+  // The AbstractInlineTextBoxes are all children of the same text node and an
   // an offset calculated in the container node should always be the same for
   // both LayoutNG and Legacy, even though Legacy doesn't collapse the
-  // white-spaces at the end of an AbstractInlineTextBox.
+  // white spaces at the end of an AbstractInlineTextBox. White spaces at the
+  // beginning of the third and fifth inline text box should be collapsed.
   scoped_refptr<AbstractInlineTextBox> inline_text_box =
       layout_text.FirstAbstractInlineTextBox();
-  String text = "First sentence of the";
+  String text = "First sentence";
   EXPECT_EQ(LayoutNGEnabled() ? text : text + " ", inline_text_box->GetText());
-  EXPECT_EQ(0u, inline_text_box->TextOffsetInContainer(0));
+  EXPECT_EQ(6u, inline_text_box->TextOffsetInContainer(0));
 
-  // Need to jump over the line break AbstractInlineTextBox.
+  // We need to jump over the AbstractInlineTextBox with the line break.
   inline_text_box = inline_text_box->NextInlineTextBox()->NextInlineTextBox();
-  text = "paragraph. Second sentence of";
+  text = "of the paragraph. Second sentence of";
   EXPECT_EQ(LayoutNGEnabled() ? text : text + " ", inline_text_box->GetText());
-  EXPECT_EQ(22u, inline_text_box->TextOffsetInContainer(0));
+  EXPECT_EQ(21u, inline_text_box->TextOffsetInContainer(0u));
 
   // See comment above.
   inline_text_box = inline_text_box->NextInlineTextBox()->NextInlineTextBox();
   EXPECT_EQ("the paragraph.", inline_text_box->GetText());
-  EXPECT_EQ(52u, inline_text_box->TextOffsetInContainer(0));
+  EXPECT_EQ(58u, inline_text_box->TextOffsetInContainer(0u));
 
   // Ensure that calling TextOffsetInContainer on a br gives the correct result.
   const Element& br_element = *GetDocument().getElementById("br");
@@ -157,4 +182,23 @@ TEST_P(AbstractInlineTextBoxTest, GetTextOffsetInContainer) {
   EXPECT_EQ(0u, inline_text_box->TextOffsetInContainer(0));
 }
 
+TEST_P(AbstractInlineTextBoxTest, CharacterWidths) {
+  // There should be a line break at the space after "012".
+  SetBodyInnerHTML(R"HTML(
+    <style>* { font-size: 10px; }</style>
+    <div id="div" style="width: 0ch">012 345</div>)HTML");
+
+  const Element& div = *GetDocument().getElementById("div");
+  LayoutText& layout_text = *ToLayoutText(div.firstChild()->GetLayoutObject());
+  scoped_refptr<AbstractInlineTextBox> inline_text_box =
+      layout_text.FirstAbstractInlineTextBox();
+
+  Vector<float> widths;
+  inline_text_box->CharacterWidths(widths);
+  // There should be four elements in the "widths" vector, not three, because
+  // the width of the trailing space should be included.
+  EXPECT_EQ(4u, widths.size());
+  EXPECT_TRUE(inline_text_box->NeedsTrailingSpace());
+}
+
 }  // namespace blink

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

@@ -38,7 +38,6 @@ class CORE_EXPORT NGAbstractInlineTextBox final : public AbstractInlineTextBox {
   NGInlineCursor GetCursor() const;
   NGInlineCursor GetCursorOnLine() const;
   String GetTextContent() const;
-  bool NeedsTrailingSpace() const;
 
   // Implementations of AbstractInlineTextBox member functions.
   void Detach() final;
@@ -54,6 +53,7 @@ class CORE_EXPORT NGAbstractInlineTextBox final : public AbstractInlineTextBox {
   scoped_refptr<AbstractInlineTextBox> NextOnLine() const final;
   scoped_refptr<AbstractInlineTextBox> PreviousOnLine() const final;
   bool IsLineBreak() const final;
+  bool NeedsTrailingSpace() const final;
 
   union {
     const NGPaintFragment* fragment_;

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

@@ -99,7 +99,7 @@ class CORE_EXPORT NGInlineNode : public NGLayoutInputNode {
 
   // Returns the DOM to text content offset mapping of this block. If it is not
   // computed before, compute and store it in NGInlineNodeData.
-  // This funciton must be called with clean layout.
+  // This function must be called with clean layout.
   const NGOffsetMapping* ComputeOffsetMappingIfNeeded() const;
 
   // Get |NGOffsetMapping| for the |layout_block_flow|. |layout_block_flow|