Fixes crashes in AXPosition

We recently migrated to the NGOffsetMapping class to convert from DOM offsets, which might have uncompressed white-space, to text offsets
which are used in the accessibility tree.
However, the assumption was that NGOffsetMapping supports returning text offsets for
all block flow elements, which is untrue. Only block flow elements that are at inline or inline-block level are supported.
Unfortunately, allowing block-level elements through to the NGOffsetMapping class could cause a serious crash in Blink.

AX-Relnotes: n/a.

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

Change-Id: I311be3cbc01abd8d1819c405bf7c63aaa62785d5
Bug: 1124963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436748
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Auto-Submit: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811915}

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

@@ -3113,24 +3113,36 @@ int AXNodeObject::TextOffsetInFormattingContext(int offset) const {
     return AXObject::TextOffsetInFormattingContext(offset);
 
   // We support calculating the text offset from the start of the formatting
-  // contexts of the following layout objects: (Note that in the following
-  // examples, the paragraph is the formatting context.
+  // contexts of the following layout objects, provided that they are at
+  // inline-level, (display=inline) or "display=inline-block":
+  //
+  // (Note that in the following examples, the paragraph is the formatting
+  // context.
   //
   // Layout replaced, e.g. <p><img></p>.
   // Layout inline with a layout text child, e.g. <p><a href="#">link</a></p>.
   // Layout block flow, e.g. <p><b style="display: inline-block;"></b></p>.
   // Layout text, e.g. <p>Hello</p>.
   // Layout br (subclass of layout text), e.g. <p><br></p>.
+
   if (layout_obj->IsLayoutInline()) {
     // The NGOffsetMapping class doesn't map layout inline objects to their text
-    // mappings. We need to retrieve the first layout text or layout replaced
-    // child.
+    // mappings because such an operation could be ambiguous. An inline object
+    // may have another inline object inside it. For example,
+    // <span><span>Inner</span outer</span>. We need to recursively retrieve the
+    // first layout text or layout replaced child so that any potential
+    // ambiguity would be removed.
     const AXObject* first_child = FirstChildIncludingIgnored();
     return first_child ? first_child->TextOffsetInFormattingContext(offset)
                        : offset;
   }
-  if (!layout_obj->IsLayoutReplaced() && !layout_obj->IsLayoutBlockFlow() &&
-      !layout_obj->IsText()) {
+
+  // TODO(crbug.com/567964): LayoutObject::IsAtomicInlineLevel() also includes
+  // block-level replaced elements. We need to explicitly exclude them via
+  // LayoutObject::IsInline().
+  const bool is_atomic_inline_level =
+      layout_obj->IsInline() && layout_obj->IsAtomicInlineLevel();
+  if (!is_atomic_inline_level && !layout_obj->IsText()) {
     // Not in a formatting context in which text offsets are meaningful.
     return AXObject::TextOffsetInFormattingContext(offset);
   }

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

@@ -24,6 +24,7 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_replaced->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_replaced->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
@@ -40,10 +41,11 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_inline->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_inline->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
-       TextOffsetInFormattingContextWithLayoutBlockFlow) {
+       TextOffsetInFormattingContextWithLayoutBlockFlowAtInlineLevel) {
   SetBodyInnerHTML(R"HTML(
       <p>
         Before
@@ -57,6 +59,28 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_block_flow->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_block_flow->TextOffsetInFormattingContext(1));
+}
+
+TEST_P(ParameterizedAccessibilityTest,
+       TextOffsetInFormattingContextWithLayoutBlockFlowAtBlockLevel) {
+  // NGOffsetMapping does not support block flow objects that are at
+  // block-level, so we do not support them as well.
+  SetBodyInnerHTML(R"HTML(
+      <p>
+        Before
+        <b id="block-flow" style="display: block;">block flow</b>
+        after.
+      </p>)HTML");
+
+  const AXObject* ax_block_flow = GetAXObjectByElementId("block-flow");
+  ASSERT_NE(nullptr, ax_block_flow);
+  ASSERT_EQ(ax::mojom::Role::kGenericContainer, ax_block_flow->RoleValue());
+  // Since block-level elements do not expose a count of the number of
+  // characters from the beginning of their formatting context, we return the
+  // same offset that was passed in.
+  EXPECT_EQ(0, ax_block_flow->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(1, ax_block_flow->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
@@ -74,6 +98,7 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_text->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_text->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
@@ -89,6 +114,7 @@ TEST_P(ParameterizedAccessibilityTest,
   ASSERT_EQ("\n", ax_br->ComputedName());
   // After white space is compressed, the word "before" is of length 6.
   EXPECT_EQ(6, ax_br->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(7, ax_br->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
@@ -111,6 +137,7 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_first_letter->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_first_letter->TextOffsetInFormattingContext(1));
 }
 
 TEST_P(ParameterizedAccessibilityTest,
@@ -136,6 +163,7 @@ TEST_P(ParameterizedAccessibilityTest,
   // After white space is compressed, the word "before" plus a single white
   // space is of length 7.
   EXPECT_EQ(7, ax_css_generated->TextOffsetInFormattingContext(0));
+  EXPECT_EQ(8, ax_css_generated->TextOffsetInFormattingContext(1));
 }
 
 }  // namespace test

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

@@ -716,22 +716,26 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
 
   virtual int TextLength() const { return 0; }
 
-  // Supported on layout inline, layout text, layout replaced, layout block flow
-  // and native text field. For all other object types, returns |offset|.
+  // Supported on layout inline, layout text, layout replaced, and layout block
+  // flow, provided that they are at inline-level, i.e. "display=inline" or
+  // "display=inline-block". Also supported on native text fields. For all other
+  // object types, returns |offset|.
   //
   // For layout inline, text, replaced, and block flow: Translates the given
   // character offset to the equivalent offset in the object's formatting
-  // context. This is the deepest block flow ancestor, (excluding the current
-  // object), e.g. a paragraph. If this object is somehow not a descendant of a
-  // block flow in the layout tree, returns the given offset.
+  // context. The formatting context is the deepest block flow ancestor,
+  // (excluding the current object), e.g. the containing paragraph. If this
+  // object is somehow not a descendant of a block flow in the layout tree,
+  // returns |offset|.
   //
-  // For example, if the given offset is 0, this would return the number of
-  // characters, excluding any collapsed white space found in the DOM, from the
-  // start of the layout inline's deepest block flow ancestor, e.g. the
-  // beginning of the paragraph in which a span is found.
+  // For example, if this object is a span, and |offset| is 0, this method would
+  // return the number of characters, excluding any collapsed white space found
+  // in the DOM, from the start of the layout inline's deepest block flow
+  // ancestor, e.g. the beginning of the paragraph in which the span is found.
   //
   // For native text fields: Simply returns |offset|, because native text fields
-  // have no collapsed white space and so no translation is necessary.
+  // have no collapsed white space and so no translation from a DOM to an
+  // accessible text offset is necessary.
   virtual int TextOffsetInFormattingContext(int offset) const;
 
   // For all inline text boxes and native text fields. For all other object
@@ -747,7 +751,8 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
   // start of the inline text box's static text parent.
   //
   // For native text fields: Simply returns |offset|, because native text fields
-  // have no collapsed white space and so no translation is necessary.
+  // have no collapsed white space and so no translation from a DOM to an
+  // accessible text offset is necessary.
   virtual int TextOffsetInContainer(int offset) const;
 
   // Properties of interactive elements.

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

@@ -238,9 +238,12 @@ const AXPosition AXPosition::FromPosition(
     AXPosition ax_position(*container);
     // Convert from a DOM offset that may have uncompressed white space to a
     // character offset.
+    //
+    // Note that NGOffsetMapping::GetInlineFormattingContextOf will reject DOM
+    // positions that it does not support, so we don't need to explicitly check
+    // this before calling the method.)
     LayoutBlockFlow* formatting_context =
-        NGOffsetMapping::GetInlineFormattingContextOf(
-            *container_node->GetLayoutObject());
+        NGOffsetMapping::GetInlineFormattingContextOf(parent_anchored_position);
     const NGOffsetMapping* container_offset_mapping =
         formatting_context ? NGInlineNode::GetOffsetMapping(formatting_context)
                            : nullptr;
@@ -429,8 +432,15 @@ int AXPosition::MaxTextOffset() const {
   }
 
   const LayoutObject* layout_object = container_node->GetLayoutObject();
-  if (!layout_object || !layout_object->IsText())
+  if (!layout_object)
     return container_object_->ComputedName().length();
+  // TODO(nektar): Remove all this logic once we switch to
+  // AXObject::TextLength().
+  const bool is_atomic_inline_level =
+      layout_object->IsInline() && layout_object->IsAtomicInlineLevel();
+  if (!is_atomic_inline_level && !layout_object->IsText())
+    return container_object_->ComputedName().length();
+
   LayoutBlockFlow* formatting_context =
       NGOffsetMapping::GetInlineFormattingContextOf(*layout_object);
   const NGOffsetMapping* container_offset_mapping =
@@ -888,14 +898,29 @@ const PositionWithAffinity AXPosition::ToPositionWithAffinity(
                                 affinity_);
   }
 
-  // Convert from a text offset, which may have white space collapsed, to a DOM
-  // offset which should have uncompressed white space.
-  LayoutBlockFlow* formatting_context =
-      NGOffsetMapping::GetInlineFormattingContextOf(
-          *container_node->GetLayoutObject());
-  const NGOffsetMapping* container_offset_mapping =
-      formatting_context ? NGInlineNode::GetOffsetMapping(formatting_context)
-                         : nullptr;
+  // If NGOffsetMapping supports it, convert from a text offset, which may have
+  // white space collapsed, to a DOM offset which should have uncompressed white
+  // space. NGOffsetMapping supports layout text, layout replaced, ruby runs,
+  // list markers, and layout block flow at inline-level, i.e. "display=inline"
+  // or "display=inline-block". It also supports out-of-flow elements, which
+  // should not be relevant to text positions in the accessibility tree.
+  const LayoutObject* layout_object = container_node->GetLayoutObject();
+  // TODO(crbug.com/567964): LayoutObject::IsAtomicInlineLevel() also includes
+  // block-level replaced elements. We need to explicitly exclude them via
+  // LayoutObject::IsInline().
+  const bool supports_ng_offset_mapping =
+      layout_object &&
+      ((layout_object->IsInline() && layout_object->IsAtomicInlineLevel()) ||
+       layout_object->IsText());
+  const NGOffsetMapping* container_offset_mapping = nullptr;
+  if (supports_ng_offset_mapping) {
+    LayoutBlockFlow* formatting_context =
+        NGOffsetMapping::GetInlineFormattingContextOf(*layout_object);
+    container_offset_mapping =
+        formatting_context ? NGInlineNode::GetOffsetMapping(formatting_context)
+                           : nullptr;
+  }
+
   if (!container_offset_mapping) {
     // We are unable to compute the text offset in the accessibility tree that
     // corresponds to the DOM offset. We do the next best thing by returning