Make AXPlatformNodeTextRangeProviderWin keep its positions valid.

This change makes the AXPlatformNodeTextRangeProviderWin endpoints
helper class a tree observer, allowing it to keep its stored |start_|
and |end_| position valid.

By observing the trees that the |start_| and |end_| position are in,
the AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints can adjust
its endpoints if the node they are on is deleted. When this is the case,
the position is then moved to the parent node.

Known issue:
In some cases, a node can get deleted alongside its parent. When this is
the case, moving the position to its parent wouldn't solve the issue of
the invalid position as the parent, itself deleted, wouldn't be a
valid anchor. I have not observed this issue in practice, but I think it
could be a good idea to keep that in mind.

Change-Id: Ib0fd7bef9040b8744a0b1e0d7ea2d6b58c7477d8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432805
Commit-Queue: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: Jacques Newman <janewman@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#815656}

content/browser/accessibility/ax_platform_node_textrangeprovider_win_browsertest.cc

@@ -2596,7 +2596,7 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
       /*expected_text*/ L"Text in iframe\nAfter frame",
       /*expected_count*/ 1);
 
-  // Validiate this selection with a waiter.
+  // Validate this selection with a waiter.
   AccessibilityNotificationWaiter waiter(
       shell()->web_contents(), ui::kAXModeComplete,
       ax::mojom::Event::kDocumentSelectionChanged);
@@ -2626,4 +2626,97 @@ IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
   EXPECT_HRESULT_SUCCEEDED(text_range_provider->Select());
 }
 
+IN_PROC_BROWSER_TEST_F(AXPlatformNodeTextRangeProviderWinBrowserTest,
+                       ReplaceStartAndEndEndpointNodeInMultipleTrees) {
+  LoadInitialAccessibilityTreeFromHtmlFilePath(
+      "/accessibility/html/replaced-node-across-trees.html");
+  auto* web_contents = static_cast<WebContentsImpl*>(shell()->web_contents());
+  WaitForAccessibilityTreeToContainNodeWithName(web_contents, "Text in iframe");
+
+  auto* before_frame_node =
+      FindNode(ax::mojom::Role::kStaticText, "Before frame");
+  ASSERT_NE(nullptr, before_frame_node);
+
+  // 1. Test when |start_| and |end_| are both not in the iframe.
+  {
+    ComPtr<ITextRangeProvider> text_range_provider;
+    GetTextRangeProviderFromTextNode(*before_frame_node, &text_range_provider);
+    ASSERT_NE(nullptr, text_range_provider.Get());
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame");
+
+    AccessibilityNotificationWaiter waiter(web_contents, ui::kAXModeComplete,
+                                           ax::mojom::Event::kChildrenChanged);
+
+    // Updating the style on that particular node is going to invalidate the
+    // leaf text node and will replace it with a new one with the updated style.
+    // We don't care about the style - we use it to trigger a node replacement.
+    EXPECT_TRUE(ExecuteScript(
+        web_contents,
+        "document.getElementById('s1').style.outline = '1px solid black';"));
+
+    waiter.WaitForNotification();
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame");
+  }
+
+  // 1. Test when |start_| is not in the iframe but |end_| is.
+  {
+    ComPtr<ITextRangeProvider> text_range_provider;
+    GetTextRangeProviderFromTextNode(*before_frame_node, &text_range_provider);
+    ASSERT_NE(nullptr, text_range_provider.Get());
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame");
+
+    // Move the range from "<B>efore frame<>" to "<B>efore frame/nText< >"
+    EXPECT_UIA_MOVE_ENDPOINT_BY_UNIT(
+        text_range_provider, TextPatternRangeEndpoint_End, TextUnit_Word,
+        /*count*/ 1,
+        /*expected_text*/ L"Before frame\nText ",
+        /*expected_count*/ 1);
+
+    AccessibilityNotificationWaiter waiter(web_contents, ui::kAXModeComplete,
+                                           ax::mojom::Event::kChildrenChanged);
+
+    // Updating the style on that particular node is going to invalidate the
+    // leaf text node and will replace it with a new one with the updated style.
+    // We don't care about the style - we use it to trigger a node replacement.
+    EXPECT_TRUE(ExecuteScript(
+        web_contents,
+        "document.getElementsByTagName('iframe')[0].contentWindow.document."
+        "getElementById('s1').style.outline = '1px solid black';"));
+
+    waiter.WaitForNotification();
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"Before frame\nText ");
+  }
+
+  // 1. Test when |start_| is in the iframe but |end_| is not.
+  {
+    ComPtr<ITextRangeProvider> text_range_provider;
+    auto* after_frame_node =
+        FindNode(ax::mojom::Role::kStaticText, "After frame");
+    ASSERT_NE(nullptr, after_frame_node);
+    GetTextRangeProviderFromTextNode(*after_frame_node, &text_range_provider);
+    ASSERT_NE(nullptr, text_range_provider.Get());
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"After frame");
+
+    // Move the range from "<B>efore frame<>" to "<B>efore frame/nText< >"
+    EXPECT_UIA_MOVE_ENDPOINT_BY_UNIT(
+        text_range_provider, TextPatternRangeEndpoint_Start, TextUnit_Word,
+        /*count*/ -1,
+        /*expected_text*/ L"iframe\nAfter frame",
+        /*expected_count*/ -1);
+
+    AccessibilityNotificationWaiter waiter(web_contents, ui::kAXModeComplete,
+                                           ax::mojom::Event::kChildrenChanged);
+
+    // Updating the style on that particular node is going to invalidate the
+    // leaf text node and will replace it with a new one with the updated style.
+    // We don't care about the style - we use it to trigger a node replacement.
+    EXPECT_TRUE(ExecuteScript(
+        web_contents,
+        "document.getElementById('s2').style.outline = '1px solid black';"));
+
+    waiter.WaitForNotification();
+    EXPECT_UIA_TEXTRANGE_EQ(text_range_provider, L"iframe\nAfter frame");
+  }
+}
+
 }  // namespace content

content/browser/accessibility/browser_accessibility_manager.cc

@@ -1427,6 +1427,14 @@ ui::AXNode* BrowserAccessibilityManager::GetNodeFromTree(
   return wrapper ? wrapper->node() : nullptr;
 }
 
+void BrowserAccessibilityManager::AddObserver(ui::AXTreeObserver* observer) {
+  ax_tree()->AddObserver(observer);
+}
+
+void BrowserAccessibilityManager::RemoveObserver(ui::AXTreeObserver* observer) {
+  ax_tree()->RemoveObserver(observer);
+}
+
 AXTreeID BrowserAccessibilityManager::GetTreeID() const {
   return ax_tree_id();
 }

content/browser/accessibility/browser_accessibility_manager.h

@@ -447,6 +447,8 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
   ui::AXNode* GetNodeFromTree(ui::AXTreeID tree_id,
                               ui::AXNode::AXID node_id) const override;
   ui::AXNode* GetNodeFromTree(ui::AXNode::AXID node_id) const override;
+  void AddObserver(ui::AXTreeObserver* observer) override;
+  void RemoveObserver(ui::AXTreeObserver* observer) override;
   AXTreeID GetTreeID() const override;
   AXTreeID GetParentTreeID() const override;
   ui::AXNode* GetRootAsAXNode() const override;

content/test/data/accessibility/html/frame/static_text_2.html

+<!DOCTYPE html>
+<html>
+
+<body><span id="s1">Text in iframe</span></body>
+
+</html>
\ No newline at end of file

content/test/data/accessibility/html/replaced-node-across-trees.html

+<!DOCTYPE html>
+<html>
+
+<body>
+  <span id="s1">Before frame</span>
+  <div>
+    <iframe aria-label="Cross-process iframe" src="frame/static_text_2.html">
+    </iframe>
+  </div>
+  <span id="s2">After frame</span>
+</body>
+
+</html>
\ No newline at end of file

ui/accessibility/ax_tree_manager.h

@@ -8,6 +8,7 @@
 #include "ui/accessibility/ax_export.h"
 #include "ui/accessibility/ax_node.h"
 #include "ui/accessibility/ax_tree_id.h"
+#include "ui/accessibility/ax_tree_observer.h"
 
 namespace ui {
 
@@ -26,6 +27,9 @@ class AX_EXPORT AXTreeManager {
   // Returns nullptr if |node_id| is not found.
   virtual AXNode* GetNodeFromTree(const AXNode::AXID node_id) const = 0;
 
+  virtual void AddObserver(AXTreeObserver* observer) {}
+  virtual void RemoveObserver(AXTreeObserver* observer) {}
+
   // Returns the tree id of the tree managed by this AXTreeManager.
   virtual AXTreeID GetTreeID() const = 0;
 

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.cc

@@ -961,6 +961,8 @@ HRESULT AXPlatformNodeTextRangeProviderWin::GetChildren(SAFEARRAY** children) {
   std::vector<gfx::NativeViewAccessible> descendants;
 
   const AXNode* common_anchor = start()->LowestCommonAnchor(*end());
+  if (!common_anchor)
+    return UIA_E_ELEMENTNOTAVAILABLE;
   const AXTreeID tree_id = common_anchor->tree()->GetAXTreeID();
   const AXNode::AXID node_id = common_anchor->id();
   AXPlatformNodeDelegate* delegate = GetDelegate(tree_id, node_id);
@@ -1419,7 +1421,90 @@ bool AXPlatformNodeTextRangeProviderWin::ShouldReleaseTextAttributeAsSafearray(
          !TextAttributeIsUiaReservedValue(attribute_value);
 }
 
-AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::TextRangeEndpoints() {}
+AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::TextRangeEndpoints() {
+  start_ = AXNodePosition::CreateNullPosition();
+  end_ = AXNodePosition::CreateNullPosition();
+}
+
 AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::~TextRangeEndpoints() {}
 
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::SetStart(
+    AXPositionInstance new_start) {
+  bool did_tree_change = start_->tree_id() != new_start->tree_id();
+  if (did_tree_change && !start_->IsNullPosition() &&
+      start_->tree_id() != end_->tree_id()) {
+    RemoveObserver(start_->tree_id());
+  }
+
+  start_ = std::move(new_start);
+
+  if (did_tree_change && !start_->IsNullPosition() &&
+      start_->tree_id() != end_->tree_id()) {
+    AddObserver(start_->tree_id());
+  }
+}
+
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::SetEnd(
+    AXPositionInstance new_end) {
+  bool did_tree_change = end_->tree_id() != new_end->tree_id();
+  if (did_tree_change && !end_->IsNullPosition() &&
+      end_->tree_id() != start_->tree_id()) {
+    RemoveObserver(end_->tree_id());
+  }
+
+  end_ = std::move(new_end);
+
+  if (did_tree_change && !end_->IsNullPosition() &&
+      start_->tree_id() != end_->tree_id()) {
+    AddObserver(end_->tree_id());
+  }
+}
+
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::AddObserver(
+    const AXTreeID tree_id) {
+  AXTreeManager* ax_tree_manager =
+      AXTreeManagerMap::GetInstance().GetManager(tree_id);
+  DCHECK(ax_tree_manager);
+  observer_.Add(ax_tree_manager);
+}
+
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::RemoveObserver(
+    const AXTreeID tree_id) {
+  AXTreeManager* ax_tree_manager =
+      AXTreeManagerMap::GetInstance().GetManager(tree_id);
+  DCHECK(ax_tree_manager);
+  observer_.Remove(ax_tree_manager);
+}
+
+void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::
+    OnNodeWillBeDeleted(AXTree* tree, AXNode* node) {
+  // If an endpoint is on a node that will be deleted, move endpoint up to a
+  // parent since we want to ensure that the endpoints of a text range provider
+  // are always valid positions. Otherwise, the range will be stuck on nodes
+  // that don't exist anymore.
+  DCHECK(tree);
+  DCHECK(node);
+  DCHECK_EQ(tree->GetAXTreeID(), node->tree()->GetAXTreeID());
+  if (tree->GetAXTreeID() == start_->tree_id() &&
+      node->id() == start_->anchor_id()) {
+    AXPositionInstance new_start = start_->CreateParentPosition();
+    // Create a degenerate range at |end_| if we have an inverted range -
+    // which occurs when the |end_| comes before the |start_|. However, if the
+    // |end_| is positioned on the deleted node, don't create a degenerate range
+    // yet as that position will be updated below.
+    if (node->id() != end_->anchor_id() && *end_ < *new_start)
+      new_start = end_->Clone();
+    SetStart(std::move(new_start));
+  }
+  if (tree->GetAXTreeID() == end_->tree_id() &&
+      node->id() == end_->anchor_id()) {
+    AXPositionInstance new_end = end_->CreateParentPosition();
+    // Create a degenerate range at |start_| if we have an inverted range -
+    // which occurs when the |end_| comes before the |start_|.
+    if (*new_end < *start_)
+      new_end = start_->Clone();
+    SetEnd(std::move(new_end));
+  }
+}
+
 }  // namespace ui

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.h

@@ -11,6 +11,7 @@
 #include <tuple>
 #include <vector>
 
+#include "base/scoped_observer.h"
 #include "ui/accessibility/ax_node_position.h"
 #include "ui/accessibility/ax_position.h"
 #include "ui/accessibility/ax_range.h"
@@ -187,20 +188,24 @@ class AX_EXPORT __declspec(uuid("3071e40d-a10d-45ff-a59f-6e8e1138e2c1"))
 
   Microsoft::WRL::ComPtr<AXPlatformNodeWin> owner_;
 
-  class TextRangeEndpoints {
+  class TextRangeEndpoints : public AXTreeObserver {
    public:
     TextRangeEndpoints();
-    ~TextRangeEndpoints();
+    ~TextRangeEndpoints() override;
     const AXPositionInstance& GetStart() const { return start_; }
     const AXPositionInstance& GetEnd() const { return end_; }
-    void SetStart(AXPositionInstance new_start) {
-      start_ = std::move(new_start);
-    }
-    void SetEnd(AXPositionInstance new_end) { end_ = std::move(new_end); }
+    void SetStart(AXPositionInstance new_start);
+    void SetEnd(AXPositionInstance new_end);
+
+    void AddObserver(const AXTreeID tree_id);
+    void RemoveObserver(const AXTreeID tree_id);
+    void OnNodeWillBeDeleted(AXTree* tree, AXNode* node) override;
 
    private:
     AXPositionInstance start_;
     AXPositionInstance end_;
+
+    ScopedObserver<AXTreeManager, AXTreeObserver> observer_{this};
   };
   TextRangeEndpoints endpoints_;
 };

ui/accessibility/platform/ax_platform_node_textrangeprovider_win_unittest.cc

@@ -5718,4 +5718,149 @@ TEST_F(AXPlatformNodeTextRangeProviderTest, TestValidateStartAndEnd) {
       /*expected_text*/ L"ome tex",
       /*expected_count*/ 1);
 }
+
+TEST_F(AXPlatformNodeTextRangeProviderTest,
+       TestReplaceStartAndEndEndpointNode) {
+  // This test updates the tree structure to ensure that the text range is still
+  // valid after a text node gets replaced by another one. This case occurs
+  // every time an AT's focus moves to a node whose style is affected by focus,
+  // thus generating a tree update.
+  //
+  // ++1 kRootWebArea
+  // ++++2 kStaticText/++++3 kStaticText (replacement node)
+  // ++++4 kStaticText/++++5 kStaticText (replacement node)
+  AXNodeData root_1;
+  AXNodeData text_2;
+  AXNodeData text_3;
+  AXNodeData text_4;
+  AXNodeData text_5;
+
+  root_1.id = 1;
+  text_2.id = 2;
+  text_3.id = 3;
+  text_4.id = 4;
+  text_5.id = 5;
+
+  root_1.role = ax::mojom::Role::kRootWebArea;
+  root_1.child_ids = {text_2.id, text_4.id};
+
+  text_2.role = ax::mojom::Role::kStaticText;
+  text_2.SetName("some text");
+
+  // Replacement node of |text_2|.
+  text_3.role = ax::mojom::Role::kStaticText;
+  text_3.SetName("some text");
+
+  text_4.role = ax::mojom::Role::kStaticText;
+  text_4.SetName("more text");
+
+  // Replacement node of |text_4|.
+  text_5.role = ax::mojom::Role::kStaticText;
+  text_5.SetName("more text");
+
+  ui::AXTreeUpdate update;
+  ui::AXTreeID tree_id = ui::AXTreeID::CreateNewAXTreeID();
+  update.root_id = root_1.id;
+  update.tree_data.tree_id = tree_id;
+  update.has_tree_data = true;
+  update.nodes = {root_1, text_2, text_4};
+  Init(update);
+
+  // Create a position at MaxTextOffset.
+  // Making |owner| AXID:1 so that |TestAXNodeWrapper::BuildAllWrappers|
+  // will build the entire tree.
+  AXPlatformNodeWin* owner = static_cast<AXPlatformNodeWin*>(
+      AXPlatformNodeFromNode(GetNodeFromTree(tree_id, 1)));
+
+  // start: TextPosition, anchor_id=2, text_offset=0, annotated_text=<s>ome text
+  // end  : TextPosition, anchor_id=4, text_offset=9, annotated_text=more text<>
+  ComPtr<AXPlatformNodeTextRangeProviderWin> range;
+  CreateTextRangeProviderWin(
+      range, owner, tree_id,
+      /*start_anchor_id*/ 2, /*start_offset*/ 0,
+      /*start_affinity*/ ax::mojom::TextAffinity::kDownstream,
+      /*end_anchor_id*/ 4, /*end_offset*/ 9,
+      /*end_affinity*/ ax::mojom::TextAffinity::kDownstream);
+
+  EXPECT_UIA_TEXTRANGE_EQ(range, /*expected_text*/ L"some textmore text");
+
+  // 1. Replace the node on which |start_| is.
+  {
+    // Replace node |text_2| with |text_3|.
+    root_1.child_ids = {text_3.id, text_4.id};
+    AXTreeUpdate test_update;
+    test_update.nodes = {root_1, text_3};
+    ASSERT_TRUE(GetTree()->Unserialize(test_update));
+
+    // Replacing that node shouldn't impact the range.
+    base::win::ScopedSafearray children;
+    range->GetChildren(children.Receive());
+    EXPECT_UIA_TEXTRANGE_EQ(range, /*expected_text*/ L"some textmore text");
+
+    // The |start_| endpoint should have moved to its parent.
+    EXPECT_EQ(1, GetStart(range.Get())->anchor_id());
+    EXPECT_EQ(0, GetStart(range.Get())->text_offset());
+
+    // The |end_| endpoint should not have moved.
+    EXPECT_EQ(4, GetEnd(range.Get())->anchor_id());
+    EXPECT_EQ(9, GetEnd(range.Get())->text_offset());
+  }
+
+  // 2. Replace the node on which |end_| is.
+  {
+    // Replace node |text_4| with |text_5|.
+    root_1.child_ids = {text_3.id, text_5.id};
+    AXTreeUpdate test_update;
+    test_update.nodes = {root_1, text_5};
+    ASSERT_TRUE(GetTree()->Unserialize(test_update));
+
+    // Replacing that node shouldn't impact the range.
+    base::win::ScopedSafearray children;
+    range->GetChildren(children.Receive());
+    EXPECT_UIA_TEXTRANGE_EQ(range, /*expected_text*/ L"some textmore text");
+
+    // The |start_| endpoint should still be on its parent.
+    EXPECT_EQ(1, GetStart(range.Get())->anchor_id());
+    EXPECT_EQ(0, GetStart(range.Get())->text_offset());
+
+    // The |end_| endpoint should have moved to its parent.
+    EXPECT_EQ(1, GetEnd(range.Get())->anchor_id());
+    EXPECT_EQ(18, GetEnd(range.Get())->text_offset());
+  }
+
+  // 3. Replace the node on which |end_| is.
+  {
+    // start: TextPosition, anchor_id=3, text_offset=0, annotated_text=<s>ome
+    // end  : TextPosition, anchor_id=3, text_offset=4, annotated_text=some<>
+    ComPtr<AXPlatformNodeTextRangeProviderWin> range_2;
+    CreateTextRangeProviderWin(
+        range_2, owner, tree_id,
+        /*start_anchor_id*/ 3, /*start_offset*/ 0,
+        /*start_affinity*/ ax::mojom::TextAffinity::kDownstream,
+        /*end_anchor_id*/ 3, /*end_offset*/ 4,
+        /*end_affinity*/ ax::mojom::TextAffinity::kDownstream);
+
+    EXPECT_UIA_TEXTRANGE_EQ(range_2, /*expected_text*/ L"some");
+
+    // Replace node |text_3| with |text_2|.
+    root_1.child_ids = {text_2.id, text_5.id};
+    AXTreeUpdate test_update;
+    test_update.nodes = {root_1, text_2};
+    ASSERT_TRUE(GetTree()->Unserialize(test_update));
+
+    // Replacing that node shouldn't impact the range.
+    base::win::ScopedSafearray children;
+    range_2->GetChildren(children.Receive());
+    EXPECT_UIA_TEXTRANGE_EQ(range_2, /*expected_text*/ L"some");
+
+    // The |start_| endpoint should have moved to its parent.
+    EXPECT_EQ(1, GetStart(range_2.Get())->anchor_id());
+    EXPECT_EQ(0, GetStart(range_2.Get())->text_offset());
+
+    // The |end_| endpoint should have moved to its parent.
+    EXPECT_EQ(1, GetEnd(range_2.Get())->anchor_id());
+    EXPECT_EQ(4, GetEnd(range_2.Get())->text_offset());
+  }
+}
+
 }  // namespace ui

ui/accessibility/test_ax_tree_manager.cc

@@ -53,6 +53,16 @@ AXNode* TestAXTreeManager::GetNodeFromTree(const AXNode::AXID node_id) const {
   return tree_ ? tree_->GetFromId(node_id) : nullptr;
 }
 
+void TestAXTreeManager::AddObserver(AXTreeObserver* observer) {
+  if (tree_)
+    tree_->AddObserver(observer);
+}
+
+void TestAXTreeManager::RemoveObserver(AXTreeObserver* observer) {
+  if (tree_)
+    tree_->RemoveObserver(observer);
+}
+
 AXTreeID TestAXTreeManager::GetTreeID() const {
   return tree_ ? tree_->data().tree_id : AXTreeIDUnknown();
 }

ui/accessibility/test_ax_tree_manager.h

@@ -43,6 +43,8 @@ class TestAXTreeManager : public AXTreeManager {
   AXNode* GetNodeFromTree(const AXTreeID tree_id,
                           const AXNode::AXID node_id) const override;
   AXNode* GetNodeFromTree(const AXNode::AXID node_id) const override;
+  void AddObserver(AXTreeObserver* observer) override;
+  void RemoveObserver(AXTreeObserver* observer) override;
   AXTreeID GetTreeID() const override;
   AXTreeID GetParentTreeID() const override;
   AXNode* GetRootAsAXNode() const override;