[A11y] Actions must update style and layout for content-visibility: a…

…uto targets first

- Reduce the number of public interfaces for actions down to 2 methods.
- Both methods will UpdateStyleAndLayoutForNode() on the target node
- If that causes |this| to be detached, will use the new object (see comment for more details)
- Removes unused RequestSetSelected() method and related actions
- Fixes failing tests that were surfaced while I was working on CL:4528515

Bug: none
Change-Id: Idae6c725e23980d4bed335a29e30e3fc55d309eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4566526
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1149459}

components/pdf/renderer/pdf_accessibility_tree_browsertest.cc

@@ -1958,8 +1958,6 @@ TEST_F(PdfAccessibilityTreeTest, TestEmptyPdfAxActions) {
   EXPECT_EQ(point.x(), 0);
   EXPECT_EQ(point.y(), 0);
 
-  EXPECT_FALSE(pdf_action_target->SetSelected(true));
-  EXPECT_FALSE(pdf_action_target->SetSelected(false));
   EXPECT_FALSE(pdf_action_target->ScrollToMakeVisible());
 }
 

components/pdf/renderer/pdf_ax_action_target.cc

@@ -114,10 +114,6 @@ gfx::Point PdfAXActionTarget::MaximumScrollOffset() const {
 
 void PdfAXActionTarget::SetScrollOffset(const gfx::Point& point) const {}
 
-bool PdfAXActionTarget::SetSelected(bool selected) const {
-  return false;
-}
-
 bool PdfAXActionTarget::SetSelection(const ui::AXActionTarget* anchor_object,
                                      int anchor_offset,
                                      const ui::AXActionTarget* focus_object,

components/pdf/renderer/pdf_ax_action_target.h

@@ -36,7 +36,6 @@ class PdfAXActionTarget : public ui::AXActionTarget {
   gfx::Point MinimumScrollOffset() const override;
   gfx::Point MaximumScrollOffset() const override;
   void SetScrollOffset(const gfx::Point& point) const override;
-  bool SetSelected(bool selected) const override;
   bool SetSelection(const ui::AXActionTarget* anchor_object,
                     int anchor_offset,
                     const ui::AXActionTarget* focus_object,

content/renderer/accessibility/blink_ax_action_target.cc

@@ -66,10 +66,6 @@ void BlinkAXActionTarget::SetScrollOffset(const gfx::Point& point) const {
   web_ax_object_.SetScrollOffset(point);
 }
 
-bool BlinkAXActionTarget::SetSelected(bool selected) const {
-  return web_ax_object_.SetSelected(selected);
-}
-
 bool BlinkAXActionTarget::SetSelection(const ui::AXActionTarget* anchor_object,
                                        int anchor_offset,
                                        const ui::AXActionTarget* focus_object,

content/renderer/accessibility/blink_ax_action_target.h

@@ -29,7 +29,6 @@ class BlinkAXActionTarget : public ui::AXActionTarget {
   gfx::Point MinimumScrollOffset() const override;
   gfx::Point MaximumScrollOffset() const override;
   void SetScrollOffset(const gfx::Point& point) const override;
-  bool SetSelected(bool selected) const override;
   bool SetSelection(const ui::AXActionTarget* anchor_object,
                     int anchor_offset,
                     const ui::AXActionTarget* focus_object,

content/renderer/accessibility/render_accessibility_impl_browsertest.cc

@@ -65,18 +65,6 @@ using blink::WebAXObject;
 using blink::WebDocument;
 using testing::ElementsAre;
 
-namespace {
-
-#if !BUILDFLAG(IS_ANDROID)
-bool IsSelected(const WebAXObject& obj) {
-  ui::AXNodeData node_data;
-  obj.Serialize(&node_data, ui::kAXModeComplete);
-  return node_data.GetBoolAttribute(ax::mojom::BoolAttribute::kSelected);
-}
-#endif  // !BUILDFLAG(IS_ANDROID)
-
-}  // namespace
-
 class TestAXImageAnnotator : public AXImageAnnotator {
  public:
   TestAXImageAnnotator(
@@ -1128,15 +1116,6 @@ TEST_F(BlinkAXActionTargetTest, TestMethods) {
   EXPECT_EQ(gfx::Point(0, 0), scroller_action_target->MinimumScrollOffset());
   EXPECT_GE(scroller_action_target->MaximumScrollOffset().y(), 900);
 
-  // Android does not produce accessible items for option elements.
-#if !BUILDFLAG(IS_ANDROID)
-  EXPECT_FALSE(IsSelected(option));
-  EXPECT_TRUE(option_action_target->SetSelected(true));
-  // Selecting option requires layout to be clean.
-  GetRenderAccessibilityImpl()->GetAXContext()->UpdateAXForAllDocuments();
-  EXPECT_TRUE(IsSelected(option));
-#endif
-
   std::string value_to_set("test-value");
   {
     ui::AXActionData action_data;

third_party/blink/public/web/web_ax_object.h

@@ -206,7 +206,6 @@ class BLINK_EXPORT WebAXObject {
   //
   // OLD: the od way is that we had separate APIs for every individual
   // action. We're migrating to use PerformAction() for everything.
-  bool SetSelected(bool) const;
   bool SetSelection(const WebAXObject& anchor_object,
                     int anchor_offset,
                     const WebAXObject& focus_object,

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

@@ -4743,9 +4743,6 @@ bool AXNodeObject::OnNativeBlurAction() {
     return false;
   }
 
-  document->UpdateStyleAndLayoutTreeForNode(
-      node, DocumentUpdateReason::kAccessibility);
-
   // An AXObject's node will always be of type `Element`, `Document` or
   // `Text`. If the object we're currently on is associated with the currently
   // focused element or the document object, we want to clear the focus.
@@ -4774,9 +4771,6 @@ bool AXNodeObject::OnNativeFocusAction() {
   if (!document || !node)
     return false;
 
-  document->UpdateStyleAndLayoutTreeForNode(
-      node, DocumentUpdateReason::kAccessibility);
-
   if (!CanSetFocusAttribute())
     return false;
 

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

@@ -6352,6 +6352,32 @@ LayoutRect AXObject::GetBoundsInFrameCoordinates() const {
 //
 
 bool AXObject::PerformAction(const ui::AXActionData& action_data) {
+  Document* document = GetDocument();
+  if (!document) {
+    return false;
+  }
+  AXObjectCacheImpl& cache = AXObjectCache();
+  Node* node = GetNode();
+  if (!node) {
+    node = GetClosestElement();
+  }
+
+  // In most cases, UpdateAllLifecyclePhasesExceptPaint() is enough, but if
+  // the action is part of a display locked node, that will not update the node
+  // because it's not part of the layout update cycle yet. In that case, calling
+  // UpdateStyleAndLayoutTreeForNode() is also necessary.
+  document->UpdateStyleAndLayoutTreeForNode(
+      node, DocumentUpdateReason::kAccessibility);
+  document->View()->UpdateAllLifecyclePhasesExceptPaint(
+      DocumentUpdateReason::kAccessibility);
+
+  // Updating style and layout for the node can cause it to gain layout,
+  // detaching an AXNodeObject to make room for an AXLayoutObject.
+  if (IsDetached()) {
+    AXObject* new_object = cache.GetOrCreate(node);
+    return new_object ? new_object->PerformAction(action_data) : false;
+  }
+
   switch (action_data.action) {
     case ax::mojom::blink::Action::kBlur:
       return OnNativeBlurAction();
@@ -6383,6 +6409,8 @@ bool AXObject::PerformAction(const ui::AXActionData& action_data) {
           WTF::String::FromUTF8(action_data.value.c_str()));
     case ax::mojom::blink::Action::kShowContextMenu:
       return RequestShowContextMenuAction();
+    case ax::mojom::blink::Action::kScrollToMakeVisible:
+      return RequestScrollToMakeVisibleAction();
     case ax::mojom::blink::Action::kScrollBackward:
     case ax::mojom::blink::Action::kScrollDown:
     case ax::mojom::blink::Action::kScrollForward:
@@ -6401,7 +6429,6 @@ bool AXObject::PerformAction(const ui::AXActionData& action_data) {
     case ax::mojom::blink::Action::kLoadInlineTextBoxes:
     case ax::mojom::blink::Action::kNone:
     case ax::mojom::blink::Action::kReplaceSelectedText:
-    case ax::mojom::blink::Action::kScrollToMakeVisible:
     case ax::mojom::blink::Action::kSetSelection:
     case ax::mojom::blink::Action::kShowTooltip:
     case ax::mojom::blink::Action::kSignalEndOfTest:
@@ -6511,6 +6538,36 @@ bool AXObject::RequestScrollToMakeVisibleWithSubFocusAction(
     const gfx::Rect& subfocus,
     blink::mojom::blink::ScrollAlignment horizontal_scroll_alignment,
     blink::mojom::blink::ScrollAlignment vertical_scroll_alignment) {
+  Document* document = GetDocument();
+  if (!document) {
+    return false;
+  }
+  AXObjectCacheImpl& cache = AXObjectCache();
+  Node* node = GetNode();
+  if (!node) {
+    node = GetClosestElement();
+  }
+
+  // In most cases, UpdateAllLifecyclePhasesExceptPaint() is enough, but if
+  // focus is is moving to a display locked node, that will not update the node
+  // because it's not part of the layout update cycle yet. In that case, calling
+  // UpdateStyleAndLayoutTreeForNode() is also necessary.
+  document->UpdateStyleAndLayoutTreeForNode(
+      node, DocumentUpdateReason::kAccessibility);
+  document->View()->UpdateAllLifecyclePhasesExceptPaint(
+      DocumentUpdateReason::kAccessibility);
+
+  // Updating style and layout for the node can cause it to gain layout,
+  // detaching an AXNodeObject to make room for an AXLayoutObject.
+  if (IsDetached()) {
+    AXObject* new_object = cache.GetOrCreate(node);
+    return new_object
+               ? new_object->OnNativeScrollToMakeVisibleWithSubFocusAction(
+                     subfocus, horizontal_scroll_alignment,
+                     vertical_scroll_alignment)
+               : false;
+  }
+
   return OnNativeScrollToMakeVisibleWithSubFocusAction(
       subfocus, horizontal_scroll_alignment, vertical_scroll_alignment);
 }

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

@@ -1273,37 +1273,13 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
     return nullptr;
   }
 
-  // Modify or take an action on an object.
-  //
-  // These are the public interfaces, called from outside of Blink.
-  // Each one first tries to fire an Accessibility Object Model event,
-  // if applicable, and if that isn't handled, falls back on the
-  // native implementation via a virtual member function, below.
-  //
-  // For example, |RequestIncrementAction| fires the AOM event and if
-  // that isn't handled it calls |DoNativeIncrement|.
-  //
-  // These all return true if handled.
-  //
-  // Note: we're migrating to have PerformAction() be the only public
-  // interface, the others will all be private.
+  // Modify or take an action on an object. Returns true if handled.
   bool PerformAction(const ui::AXActionData&);
-  bool RequestDecrementAction();
-  bool RequestClickAction();
-  bool RequestFocusAction();
-  bool RequestIncrementAction();
-  bool RequestScrollToGlobalPointAction(const gfx::Point&);
-  bool RequestScrollToMakeVisibleAction();
+  // TODO(accessibility) Do this through PerformAction() and move to private.
   bool RequestScrollToMakeVisibleWithSubFocusAction(
       const gfx::Rect&,
       blink::mojom::blink::ScrollAlignment horizontal_scroll_alignment,
       blink::mojom::blink::ScrollAlignment vertical_scroll_alignment);
-  bool RequestSetSelectedAction(bool);
-  bool RequestSetSequentialFocusNavigationStartingPointAction();
-  bool RequestSetValueAction(const String&);
-  bool RequestShowContextMenuAction();
-  bool RequestExpandAction();
-  bool RequestCollapseAction();
 
   // These are actions, just like the actions above, and they allow us
   // to keep track of nodes that gain or lose accessibility focus, but
@@ -1551,6 +1527,20 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
   bool ComputeIsInertViaStyle(const ComputedStyle*,
                               IgnoredReasons* = nullptr) const;
 
+  // Private action interfaces. Return bool if action is performed.
+  bool RequestDecrementAction();
+  bool RequestClickAction();
+  bool RequestFocusAction();
+  bool RequestIncrementAction();
+  bool RequestScrollToGlobalPointAction(const gfx::Point&);
+  bool RequestScrollToMakeVisibleAction();
+  bool RequestSetSelectedAction(bool);
+  bool RequestSetSequentialFocusNavigationStartingPointAction();
+  bool RequestSetValueAction(const String&);
+  bool RequestShowContextMenuAction();
+  bool RequestExpandAction();
+  bool RequestCollapseAction();
+
   // This returns true if the element associated with this AXObject is has
   // focusable style, meaning that it is visible. Note that we prefer to rely on
   // `Element::IsFocusableStyle()` for this, but sometimes it isn't available

third_party/blink/renderer/modules/exported/web_ax_object.cc

@@ -539,15 +539,6 @@ void WebAXObject::Selection(bool& is_selection_backward,
   }
 }
 
-bool WebAXObject::SetSelected(bool selected) const {
-  if (IsDetached())
-    return false;
-
-  ScopedActionAnnotator annotater(private_.Get(),
-                                  ax::mojom::blink::Action::kSetSelection);
-  return private_->RequestSetSelectedAction(selected);
-}
-
 bool WebAXObject::SetSelection(const WebAXObject& anchor_object,
                                int anchor_offset,
                                const WebAXObject& focus_object,
@@ -935,7 +926,9 @@ bool WebAXObject::ScrollToMakeVisible() const {
 
   ScopedActionAnnotator annotater(
       private_.Get(), ax::mojom::blink::Action::kScrollToMakeVisible);
-  return private_->RequestScrollToMakeVisibleAction();
+  ui::AXActionData action_data;
+  action_data.action = ax::mojom::blink::Action::kScrollToMakeVisible;
+  return private_->PerformAction(action_data);
 }
 
 bool WebAXObject::ScrollToMakeVisibleWithSubFocus(
@@ -966,6 +959,7 @@ bool WebAXObject::ScrollToMakeVisibleWithSubFocus(
       visible_horizontal_behavior, horizontal_behavior, horizontal_behavior};
   blink::mojom::blink::ScrollAlignment blink_vertical_scroll_alignment = {
       visible_vertical_behavior, vertical_behavior, vertical_behavior};
+
   return private_->RequestScrollToMakeVisibleWithSubFocusAction(
       subfocus, blink_horizontal_scroll_alignment,
       blink_vertical_scroll_alignment);

ui/accessibility/ax_action_target.h

@@ -30,7 +30,6 @@ class AXActionTarget {
   virtual gfx::Point MinimumScrollOffset() const = 0;
   virtual gfx::Point MaximumScrollOffset() const = 0;
   virtual void SetScrollOffset(const gfx::Point& point) const = 0;
-  virtual bool SetSelected(bool selected) const = 0;
   virtual bool SetSelection(const AXActionTarget* anchor_object,
                             int anchor_offset,
                             const AXActionTarget* focus_object,

ui/accessibility/null_ax_action_target.cc

@@ -33,10 +33,6 @@ gfx::Point NullAXActionTarget::MaximumScrollOffset() const {
 
 void NullAXActionTarget::SetScrollOffset(const gfx::Point& point) const {}
 
-bool NullAXActionTarget::SetSelected(bool selected) const {
-  return false;
-}
-
 bool NullAXActionTarget::SetSelection(const AXActionTarget* anchor_object,
                                       int anchor_offset,
                                       const AXActionTarget* focus_object,

ui/accessibility/null_ax_action_target.h

@@ -25,7 +25,6 @@ class AX_EXPORT NullAXActionTarget : public AXActionTarget {
   gfx::Point MinimumScrollOffset() const override;
   gfx::Point MaximumScrollOffset() const override;
   void SetScrollOffset(const gfx::Point& point) const override;
-  bool SetSelected(bool selected) const override;
   bool SetSelection(const AXActionTarget* anchor_object,
                     int anchor_offset,
                     const AXActionTarget* focus_object,

ui/accessibility/null_ax_action_target_unittest.cc

@@ -22,7 +22,6 @@ TEST(NullAXActionTargetTest, TestMethods) {
   EXPECT_EQ(gfx::Point(), action_target->GetScrollOffset());
   EXPECT_EQ(gfx::Point(), action_target->MinimumScrollOffset());
   EXPECT_EQ(gfx::Point(), action_target->MaximumScrollOffset());
-  EXPECT_FALSE(action_target->SetSelected(false));
   EXPECT_FALSE(action_target->SetSelection(nullptr, 0, nullptr, 0));
   EXPECT_FALSE(action_target->ScrollToMakeVisible());
   EXPECT_FALSE(action_target->ScrollToMakeVisibleWithSubFocus(