Fix wrong position created by AXPosition::ToPositionWithAffinity()

When calling AXPosition::ToPositionWithAffinity() on a position with a
|container_object_| of type AXInlineTextBox, the position returned is
always in the parent node, the StaticText object. Think of it as
a modification from a relative position to an absolute position, based
in the StaticText object.

However, when an InlineTextBox object has siblings (i.e. the parent
node has multiple InlineTextBox children), the position returned is
only valid for the first non-empty child. This bug was due to the fact
that we wrongfully assumed that the text offset in the child object
would be the same as the text offset in the parent position. This is
only true for the first non-empty child.

This CL introduces a fix for this bug. Instead of creating a position
in the parent node at the exact same offset, we add to it the sum of
the previous siblings MaxTextOffset().
It also adds a unit test for this case.

Bug: 928948
Change-Id: I1b52c7506e726956bca8b2d41d5a33bee3a84597
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1846563Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Commit-Queue: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#707817}

third_party/blink/renderer/core/layout/line/DEPS

@@ -4,6 +4,7 @@ include_rules = [
     "+third_party/blink/renderer/core/layout/api",
     "+third_party/blink/renderer/core/layout/line",
     "+third_party/blink/renderer/core/layout/logical_values.h",
+    "+third_party/blink/renderer/core/layout/ng/inline",
     "+third_party/blink/renderer/platform",
 
     "!third_party/blink/renderer/core/layout/bidi_run.h",

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

@@ -33,6 +33,8 @@
 #include "third_party/blink/renderer/core/accessibility/ax_object_cache.h"
 #include "third_party/blink/renderer/core/editing/ephemeral_range.h"
 #include "third_party/blink/renderer/core/editing/iterators/text_iterator.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/ng_offset_mapping.h"
 #include "third_party/blink/renderer/platform/text/text_break_iterator.h"
 
 namespace blink {
@@ -149,6 +151,28 @@ unsigned LegacyAbstractInlineTextBox::Len() const {
   return 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;
+
+  // 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();
+}
+
 AbstractInlineTextBox::Direction LegacyAbstractInlineTextBox::GetDirection()
     const {
   if (!inline_text_box_ || !GetLineLayoutItem())
@@ -279,4 +303,23 @@ bool LegacyAbstractInlineTextBox::IsLineBreak() const {
   return inline_text_box_->IsLineBreak();
 }
 
+const NGOffsetMapping* LegacyAbstractInlineTextBox::GetOffsetMapping() const {
+  const auto* text_node = DynamicTo<Text>(GetNode());
+
+  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;
+  }
+  return offset_mapping;
+}
+
 }  // namespace blink

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

@@ -42,6 +42,7 @@
 namespace blink {
 
 class InlineTextBox;
+class NGOffsetMapping;
 
 // High-level abstraction of InlineTextBox to allow the accessibility module to
 // get information about InlineTextBoxes without tight coupling.
@@ -66,6 +67,7 @@ class CORE_EXPORT AbstractInlineTextBox
   virtual scoped_refptr<AbstractInlineTextBox> NextInlineTextBox() const = 0;
   virtual LayoutRect LocalBounds() const = 0;
   virtual unsigned Len() const = 0;
+  virtual unsigned TextOffsetInContainer(unsigned) const = 0;
   virtual Direction GetDirection() const = 0;
   Node* GetNode() const;
   virtual void CharacterWidths(Vector<float>&) const = 0;
@@ -112,6 +114,7 @@ class CORE_EXPORT LegacyAbstractInlineTextBox final
   scoped_refptr<AbstractInlineTextBox> NextInlineTextBox() const final;
   LayoutRect LocalBounds() const final;
   unsigned Len() const final;
+  unsigned TextOffsetInContainer(unsigned offset) const final;
   Direction GetDirection() const final;
   void CharacterWidths(Vector<float>&) const final;
   String GetText() const final;
@@ -120,6 +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;
 
   InlineTextBox* inline_text_box_;
 

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

@@ -114,4 +114,37 @@ TEST_P(AbstractInlineTextBoxTest, GetTextWithLineBreakAtTrailingWhiteSpace) {
   EXPECT_EQ("abc: ", inline_text_box->GetText());
 }
 
+TEST_P(AbstractInlineTextBoxTest, GetTextOffsetInContainer) {
+  // "&#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>)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
+  // 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.
+  scoped_refptr<AbstractInlineTextBox> inline_text_box =
+      layout_text.FirstAbstractInlineTextBox();
+  String text = "First sentence of the";
+  EXPECT_EQ(LayoutNGEnabled() ? text : text + " ", inline_text_box->GetText());
+  EXPECT_EQ(0u, inline_text_box->TextOffsetInContainer(0));
+
+  // Need to jump over the line break AbstractInlineTextBox.
+  inline_text_box = inline_text_box->NextInlineTextBox()->NextInlineTextBox();
+  text = "paragraph. Second sentence of";
+  EXPECT_EQ(LayoutNGEnabled() ? text : text + " ", inline_text_box->GetText());
+  EXPECT_EQ(22u, inline_text_box->TextOffsetInContainer(0));
+
+  // 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));
+}
+
 }  // namespace blink

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

@@ -151,6 +151,12 @@ unsigned NGAbstractInlineTextBox::Len() const {
   return PhysicalTextFragment().TextLength();
 }
 
+unsigned NGAbstractInlineTextBox::TextOffsetInContainer(unsigned offset) const {
+  if (!fragment_)
+    return 0;
+  return PhysicalTextFragment().StartOffset() + offset;
+}
+
 AbstractInlineTextBox::Direction NGAbstractInlineTextBox::GetDirection() const {
   if (!fragment_ || !GetLineLayoutItem())
     return kLeftToRight;

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

@@ -43,6 +43,7 @@ class CORE_EXPORT NGAbstractInlineTextBox final : public AbstractInlineTextBox {
   scoped_refptr<AbstractInlineTextBox> NextInlineTextBox() const final;
   LayoutRect LocalBounds() const final;
   unsigned Len() const final;
+  unsigned TextOffsetInContainer(unsigned offset) const final;
   Direction GetDirection() const final;
   void CharacterWidths(Vector<float>&) const final;
   String GetText() const final;

third_party/blink/renderer/modules/accessibility/ax_inline_text_box.cc

@@ -131,6 +131,13 @@ void AXInlineTextBox::GetWordBoundaries(Vector<int>& word_starts,
   }
 }
 
+unsigned AXInlineTextBox::TextOffsetInContainer(unsigned offset) const {
+  if (!inline_text_box_)
+    return 0;
+
+  return inline_text_box_->TextOffsetInContainer(offset);
+}
+
 String AXInlineTextBox::GetName(ax::mojom::NameFrom& name_from,
                                 AXObject::AXObjectVector* name_objects) const {
   if (!inline_text_box_)

third_party/blink/renderer/modules/accessibility/ax_inline_text_box.h

@@ -59,6 +59,7 @@ class AXInlineTextBox final : public AXObject {
   void TextCharacterOffsets(Vector<int>&) const override;
   void GetWordBoundaries(Vector<int>& word_starts,
                          Vector<int>& word_ends) const override;
+  unsigned TextOffsetInContainer(unsigned offset) const override;
   void GetRelativeBounds(AXObject** out_container,
                          FloatRect& out_bounds_in_container,
                          SkMatrix44& out_container_transform,

third_party/blink/renderer/modules/accessibility/ax_object.h

@@ -687,6 +687,11 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
   // The start and end character offset of each word in the object's text.
   virtual void GetWordBoundaries(Vector<int>& word_starts,
                                  Vector<int>& word_ends) const;
+  // Returns the text offset (text offset as in AXPosition, not as in
+  // pixel offset) in the container of an inline text box.
+  virtual unsigned TextOffsetInContainer(unsigned offset) const {
+    return offset;
+  }
 
   // Properties of interactive elements.
   ax::mojom::DefaultActionVerb Action() const;

third_party/blink/renderer/modules/accessibility/ax_position.cc

@@ -822,8 +822,11 @@ const PositionWithAffinity AXPosition::ToPositionWithAffinity(
   const auto last_position = Position::LastPositionInNode(*container_node);
   CharacterIterator character_iterator(first_position, last_position,
                                        text_iterator_behavior);
+  unsigned text_offset_in_container =
+      adjusted_position.container_object_->TextOffsetInContainer(
+          adjusted_position.TextOffset());
   const EphemeralRange range = character_iterator.CalculateCharacterSubrange(
-      0, adjusted_position.text_offset_or_child_index_);
+      0, text_offset_in_container);
   return PositionWithAffinity(range.EndPosition(), affinity_);
 }
 

third_party/blink/renderer/modules/accessibility/ax_position_test.cc

@@ -1654,5 +1654,32 @@ TEST_F(AccessibilityTest, PositionInInvalidMapLayout) {
   EXPECT_EQ(0, position_after.GetPosition().OffsetInContainerNode());
 }
 
+TEST_P(ParameterizedAccessibilityTest,
+       ToPositionWithAffinityWithMultipleInlineTextBoxes) {
+  // "&#10" is a Line Feed ("\n").
+  SetBodyInnerHTML(
+      R"HTML(<style>p { white-space: pre-line; }</style>
+      <p id="paragraph">Hello &#10; world</p>)HTML");
+  const Node* text = GetElementById("paragraph")->firstChild();
+  ASSERT_NE(nullptr, text);
+  ASSERT_TRUE(text->IsTextNode());
+  AXObject* ax_static_text = GetAXObjectByElementId("paragraph")->FirstChild();
+
+  ASSERT_NE(nullptr, ax_static_text);
+  ASSERT_EQ(ax::mojom::Role::kStaticText, ax_static_text->RoleValue());
+
+  ax_static_text->LoadInlineTextBoxes();
+  ASSERT_EQ(3, ax_static_text->ChildCount());
+
+  // This test expects the starting offset of the last InlineTextBox object to
+  // equates the sum of the previous inline text boxes length, without the
+  // collapsed white-spaces.
+  const auto ax_position =
+      AXPosition::CreatePositionBeforeObject(*(ax_static_text->LastChild()));
+  const auto position = ax_position.ToPositionWithAffinity();
+  EXPECT_EQ(LayoutNGEnabled() ? 7 : 6,
+            position.GetPosition().OffsetInContainerNode());
+}
+
 }  // namespace test
 }  // namespace blink

third_party/blink/renderer/modules/accessibility/testing/accessibility_test.h

@@ -53,6 +53,18 @@ class AccessibilityTest : public RenderingTest {
                                         size_t level) const;
 };
 
+class ParameterizedAccessibilityTest : public testing::WithParamInterface<bool>,
+                                       private ScopedLayoutNGForTest,
+                                       public AccessibilityTest {
+ public:
+  ParameterizedAccessibilityTest() : ScopedLayoutNGForTest(GetParam()) {}
+
+ protected:
+  bool LayoutNGEnabled() const { return GetParam(); }
+};
+
+INSTANTIATE_TEST_SUITE_P(, ParameterizedAccessibilityTest, testing::Bool());
+
 }  // namespace test
 }  // namespace blink