Revert "[a11y-during-render] Call AddImageAnnotations from within Blink"

This reverts commit d70b84e.

Reason for revert: crbug.com/1346271 crbug.com/1346174 crbug.com/1346175

Original change's description:
> [a11y-during-render] Call AddImageAnnotations from within Blink
>
> Why: moves more of the serialization code out of BlinkAXTreeSource,
>   unblocking further refactoring. It also increases performance by
>   reducing copies and enabling use of the display lock memoizer.
>
> Why not move AXImageAnnotator entirely to Blink: I tried it, and it
> was going to be a *ton* of work given how much string manipluation
> is in there. Since there are not a whole lot of images on pages, it
> didn't seem worth it to avoid passing around a few more WebAXObjects.
>
> AX-Relnotes: n/a
> Bug: 1068668
>
> Change-Id: I8e2687b3a0ce338b2149ae256f7d5a4e13183d7d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3767642
> Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1026420}

Bug: 1068668, 1346271, 1346174, 1346175
Change-Id: I76ee73a1b49cdb7c1d95de3e53e07d6647f2c1e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3783136
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Vladimir Levin <vmpstr@chromium.org>
Owners-Override: Philip Rogers <pdr@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1027993}

content/renderer/accessibility/blink_ax_tree_source.cc

@@ -88,6 +88,56 @@ void CheckParentUnignoredOf(WebAXObject parent, WebAXObject child) {
 }
 #endif
 
+// Helper function that searches in the subtree of |obj| to a max
+// depth of |max_depth| for an image.
+//
+// Returns true on success, or false if it finds more than one image,
+// or any node with a name, or anything deeper than |max_depth|.
+bool SearchForExactlyOneInnerImage(WebAXObject obj,
+                                   WebAXObject* inner_image,
+                                   int max_depth) {
+  DCHECK(inner_image);
+
+  // If it's the first image, set |inner_image|. If we already
+  // found an image, fail.
+  if (ui::IsImage(obj.Role())) {
+    if (!inner_image->IsDetached())
+      return false;
+    *inner_image = obj;
+  } else {
+    // If we found something else with a name, fail.
+    if (!ui::IsPlatformDocument(obj.Role()) && !ui::IsLink(obj.Role())) {
+      blink::WebString web_name = obj.GetName();
+      if (!base::ContainsOnlyChars(web_name.Utf8(), base::kWhitespaceASCII)) {
+        return false;
+      }
+    }
+  }
+
+  // Fail if we recursed to |max_depth| and there's more of a subtree.
+  if (max_depth == 0 && obj.ChildCount())
+    return false;
+
+  // Don't count ignored nodes toward depth.
+  int next_depth = obj.AccessibilityIsIgnored() ? max_depth : max_depth - 1;
+
+  // Recurse.
+  for (unsigned int i = 0; i < obj.ChildCount(); i++) {
+    if (!SearchForExactlyOneInnerImage(obj.ChildAt(i), inner_image, next_depth))
+      return false;
+  }
+
+  return !inner_image->IsDetached();
+}
+
+// Return true if the subtree of |obj|, to a max depth of 3, contains
+// exactly one image. Return that image in |inner_image|.
+bool FindExactlyOneInnerImageInMaxDepthThree(WebAXObject obj,
+                                             WebAXObject* inner_image) {
+  DCHECK(inner_image);
+  return SearchForExactlyOneInnerImage(obj, inner_image, /* max_depth = */ 3);
+}
+
 // Ignore code that limits based on the protocol (like https, file, etc.)
 // to enable tests to run.
 bool g_ignore_protocol_checks_for_testing;
@@ -416,6 +466,20 @@ void BlinkAXTreeSource::SerializeNode(WebAXObject src,
     return;
   }
 
+  if (ui::IsImage(dst->role))
+    AddImageAnnotations(src, dst);
+
+  // If a link or web area isn't otherwise labeled and contains exactly one
+  // image (searching only to a max depth of 2), and the link doesn't have
+  // accessible text from an attribute like aria-label, then annotate the
+  // link/web area with the image's annotation, too.
+  if ((ui::IsLink(dst->role) || ui::IsPlatformDocument(dst->role)) &&
+      dst->GetNameFrom() != ax::mojom::NameFrom::kAttribute) {
+    WebAXObject inner_image;
+    if (FindExactlyOneInnerImageInMaxDepthThree(src, &inner_image))
+      AddImageAnnotations(inner_image, dst);
+  }
+
   if (dst->id == image_data_node_id_) {
     // In general, string attributes should be truncated using
     // TruncateAndAddStringAttribute, but ImageDataUrl contains a data url

content/renderer/accessibility/blink_ax_tree_source.h

@@ -123,8 +123,6 @@ class CONTENT_EXPORT BlinkAXTreeSource
   // to enable tests to run.
   static void IgnoreProtocolChecksForTesting();
 
-  void AddImageAnnotations(blink::WebAXObject& src, ui::AXNodeData* dst) const;
-
  private:
   const blink::WebDocument& document() const {
     DCHECK(frozen_);
@@ -152,6 +150,8 @@ class CONTENT_EXPORT BlinkAXTreeSource
       const std::string& value,
       uint32_t max_len = kMaxStringAttributeLength) const;
 
+  void AddImageAnnotations(blink::WebAXObject& src, ui::AXNodeData* dst) const;
+
   RenderFrameImpl* render_frame_;
 
   ui::AXMode accessibility_mode_;

content/renderer/accessibility/render_accessibility_impl.cc

@@ -489,11 +489,6 @@ void RenderAccessibilityImpl::MarkWebAXObjectDirty(
   ScheduleSendPendingAccessibilityEvents();
 }
 
-void RenderAccessibilityImpl::AddImageAnnotations(WebAXObject& obj,
-                                                  ui::AXNodeData* dst) {
-  tree_source_->AddImageAnnotations(obj, dst);
-}
-
 void RenderAccessibilityImpl::HandleAXEvent(const ui::AXEvent& event) {
   const WebDocument& document = GetMainDocument();
   if (document.IsNull())

content/renderer/accessibility/render_accessibility_impl.h

@@ -131,7 +131,6 @@ class CONTENT_EXPORT RenderAccessibilityImpl : public RenderAccessibility,
       ax::mojom::Action event_from_action = ax::mojom::Action::kNone,
       std::vector<ui::AXEventIntent> event_intents = {},
       ax::mojom::Event event_type = ax::mojom::Event::kNone);
-  void AddImageAnnotations(blink::WebAXObject&, ui::AXNodeData* dst);
 
   // Returns the main top-level document for this page, or NULL if there's
   // no view or frame.

content/renderer/render_frame_impl.cc

@@ -4513,15 +4513,6 @@ void RenderFrameImpl::MarkWebAXObjectDirty(
       ->MarkWebAXObjectDirty(obj, subtree, event_from, event_from_action);
 }
 
-void RenderFrameImpl::AddImageAnnotations(blink::WebAXObject& obj,
-                                          ui::AXNodeData* dst) {
-  if (!IsAccessibilityEnabled())
-    return;
-
-  render_accessibility_manager_->GetRenderAccessibilityImpl()
-      ->AddImageAnnotations(obj, dst);
-}
-
 void RenderFrameImpl::AddObserver(RenderFrameObserver* observer) {
   observers_.AddObserver(observer);
 }

content/renderer/render_frame_impl.h

@@ -618,7 +618,6 @@ class CONTENT_EXPORT RenderFrameImpl
                             bool subtree,
                             ax::mojom::EventFrom event_from,
                             ax::mojom::Action event_from_action) override;
-  void AddImageAnnotations(blink::WebAXObject&, ui::AXNodeData* dst) override;
   void CheckIfAudioSinkExistsAndIsAuthorized(
       const blink::WebString& sink_id,
       blink::WebSetSinkIdCompleteCallback callback) override;

third_party/blink/public/web/web_local_frame_client.h

@@ -669,10 +669,6 @@ class BLINK_EXPORT WebLocalFrameClient {
                                     ax::mojom::EventFrom event_from,
                                     ax::mojom::Action event_from_action) {}
 
-  // Adds automatic label annotations for |obj| and stores them
-  // in |dst|. This method assumes the object is an image.
-  virtual void AddImageAnnotations(WebAXObject& obj, ui::AXNodeData* dst) {}
-
   // Audio Output Devices API --------------------------------------------
 
   // Checks that the given audio sink exists and is authorized. The result is

third_party/blink/renderer/core/display_lock/display_lock_utilities.cc

@@ -832,37 +832,23 @@ bool DisplayLockUtilities::RevealHiddenUntilFoundAncestors(const Node& node) {
   return elements_to_reveal.size();
 }
 
-static bool CheckSelfIfInclusive(const Node* node, bool inclusive_check) {
-  if (inclusive_check) {
-    if (auto* element = DynamicTo<Element>(node)) {
-      if (auto* context = element->GetDisplayLockContext()) {
-        if (!context->ShouldPaintChildren())
-          return true;
-      }
-    }
-  }
-  return false;
-}
-
 bool DisplayLockUtilities::IsDisplayLockedPreventingPaint(
     const Node* node,
     bool inclusive_check) {
   // If we have a memoizer, consult with it to see if we already know the
   // result. Otherwise, fallback to get-element versions.
   if (memoizer_) {
     auto result = memoizer_->IsNodeLocked(node);
-    if (result) {
-      // The memoizer can only be used for non-inclusive checks.
-      return *result || CheckSelfIfInclusive(node, inclusive_check);
-    }
+    if (result)
+      return *result;
   } else {
     return inclusive_check
                ? DisplayLockUtilities::LockedInclusiveAncestorPreventingPaint(
                      *node)
                : DisplayLockUtilities::LockedAncestorPreventingPaint(*node);
   }
 
-  // Do some sanity checks that we can early out on.
+  // Do some sanity checks that we cwan early out on.
   if (!node->isConnected() ||
       node->GetDocument()
               .GetDisplayLockDocumentState()
@@ -874,8 +860,14 @@ bool DisplayLockUtilities::IsDisplayLockedPreventingPaint(
   // Handle the inclusive check -- that is, check the node itself. Note that
   // it's important not to memoize that since the memoization consists of
   // ancestor checks only.
-  if (CheckSelfIfInclusive(node, inclusive_check))
-    return true;
+  if (inclusive_check) {
+    if (auto* element = DynamicTo<Element>(node)) {
+      if (auto* context = element->GetDisplayLockContext()) {
+        if (!context->ShouldPaintChildren())
+          return true;
+      }
+    }
+  }
 
   // Walk up the ancestor chain, and consult with both the memoizer and check
   // directly if we're skipping paint. When we find a result (or finish the

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

@@ -1264,6 +1264,9 @@ void AXObject::Serialize(ui::AXNodeData* node_data,
     }
   }
 
+  if (accessibility_mode.has_mode(ui::AXMode::kScreenReader))
+    SerializeScreenReaderAttributes(node_data);
+
   SerializeUnignoredAttributes(node_data, accessibility_mode);
 
   if (accessibility_mode.has_mode(ui::AXMode::kPDF)) {
@@ -1602,87 +1605,7 @@ void AXObject::SerializeNameAndDescriptionAttributes(
       node_data, ax::mojom::blink::StringAttribute::kPlaceholder, placeholder);
 }
 
-// Helper function that searches in the subtree of |obj| to a max
-// depth of |max_depth| for an image.
-//
-// Returns true on success, or false if it finds more than one image,
-// or any node with a name, or anything deeper than |max_depth|.
-static AXObject* SearchForExactlyOneInnerImage(AXObject* obj,
-                                               AXObject* inner_image,
-                                               int max_depth) {
-  // If it's the first image, set |inner_image|. If we already
-  // found an image, fail.
-  if (ui::IsImage(obj->RoleValue())) {
-    if (inner_image)
-      return nullptr;
-    inner_image = obj;
-  } else {
-    // If we found something else with a name, fail.
-    if (!ui::IsPlatformDocument(obj->RoleValue()) &&
-        !ui::IsLink(obj->RoleValue())) {
-      ax::mojom::blink::NameFrom name_from;
-      HeapVector<Member<AXObject>> name_objects;
-      String name = obj->GetName(name_from, &name_objects);
-
-      if (!name.StripWhiteSpace().IsEmpty())
-        return nullptr;
-    }
-  }
-
-  // Fail if we recursed to |max_depth| and there's more of a subtree.
-  if (max_depth == 0 && obj->ChildCountIncludingIgnored())
-    return nullptr;
-
-  // Don't count ignored nodes toward depth.
-  int next_depth = obj->AccessibilityIsIgnored() ? max_depth : max_depth - 1;
-
-  // Recurse.
-  for (int i = 0; i < obj->ChildCountIncludingIgnored(); i++) {
-    inner_image = SearchForExactlyOneInnerImage(obj->ChildAtIncludingIgnored(i),
-                                                inner_image, next_depth);
-    if (!inner_image)
-      return nullptr;
-  }
-
-  return inner_image;
-}
-
-// Return true if the subtree of |obj|, to a max depth of 3, contains
-// exactly one image. Return that image in |inner_image|.
-static AXObject* FindExactlyOneInnerImageInMaxDepthThree(
-    AXObject& obj,
-    AXObject* inner_image) {
-  return SearchForExactlyOneInnerImage(&obj, inner_image, /* max_depth = */ 3);
-}
-
-String AXObject::KeyboardShortcut() const {
-  const AtomicString& access_key = AccessKey();
-  if (access_key.IsNull())
-    return String();
-
-  DEFINE_STATIC_LOCAL(String, modifier_string, ([] {
-                        unsigned modifiers =
-                            KeyboardEventManager::kAccessKeyModifiers;
-                        // Follow the same order as Mozilla MSAA implementation:
-                        // Ctrl+Alt+Shift+Meta+key. MSDN states that keyboard
-                        // shortcut strings should not be localized and defines
-                        // the separator as "+".
-                        StringBuilder modifier_string_builder;
-                        if (modifiers & WebInputEvent::kControlKey)
-                          modifier_string_builder.Append("Ctrl+");
-                        if (modifiers & WebInputEvent::kAltKey)
-                          modifier_string_builder.Append("Alt+");
-                        if (modifiers & WebInputEvent::kShiftKey)
-                          modifier_string_builder.Append("Shift+");
-                        if (modifiers & WebInputEvent::kMetaKey)
-                          modifier_string_builder.Append("Win+");
-                        return modifier_string_builder.ToString();
-                      }()));
-
-  return String(modifier_string + access_key);
-}
-
-void AXObject::SerializeOtherScreenReaderAttributes(ui::AXNodeData* node_data) {
+void AXObject::SerializeScreenReaderAttributes(ui::AXNodeData* node_data) {
   String display_style;
   Node* node = GetNode();
   if (node && !node->IsDocumentNode()) {
@@ -1714,21 +1637,6 @@ void AXObject::SerializeOtherScreenReaderAttributes(ui::AXNodeData* node_data) {
         active_descendant->AXObjectID());
   }
 
-  if (ui::IsImage(node_data->role))
-    AXObjectCache().AddImageAnnotations(this, node_data);
-
-  // If a link or web area isn't otherwise labeled and contains exactly one
-  // image (searching only to a max depth of 2), and the link doesn't have
-  // accessible text from an attribute like aria-label, then annotate the
-  // link/web area with the image's annotation, too.
-  if ((ui::IsLink(node_data->role) ||
-       ui::IsPlatformDocument(node_data->role)) &&
-      node_data->GetNameFrom() != ax::mojom::blink::NameFrom::kAttribute) {
-    if (AXObject* inner_image =
-            FindExactlyOneInnerImageInMaxDepthThree(*this, nullptr))
-      AXObjectCache().AddImageAnnotations(inner_image, node_data);
-  }
-
   if (Node* node = GetNode()) {
     if (node->IsElementNode()) {
       Element* element = To<Element>(node);
@@ -1741,7 +1649,36 @@ void AXObject::SerializeOtherScreenReaderAttributes(ui::AXNodeData* node_data) {
       }
     }
   }
+}
 
+String AXObject::KeyboardShortcut() const {
+  const AtomicString& access_key = AccessKey();
+  if (access_key.IsNull())
+    return String();
+
+  DEFINE_STATIC_LOCAL(String, modifier_string, ());
+  if (modifier_string.IsNull()) {
+    unsigned modifiers = KeyboardEventManager::kAccessKeyModifiers;
+    // Follow the same order as Mozilla MSAA implementation:
+    // Ctrl+Alt+Shift+Meta+key. MSDN states that keyboard shortcut strings
+    // should not be localized and defines the separator as "+".
+    StringBuilder modifier_string_builder;
+    if (modifiers & WebInputEvent::kControlKey)
+      modifier_string_builder.Append("Ctrl+");
+    if (modifiers & WebInputEvent::kAltKey)
+      modifier_string_builder.Append("Alt+");
+    if (modifiers & WebInputEvent::kShiftKey)
+      modifier_string_builder.Append("Shift+");
+    if (modifiers & WebInputEvent::kMetaKey)
+      modifier_string_builder.Append("Win+");
+    modifier_string = modifier_string_builder.ToString();
+  }
+
+  return String(modifier_string + access_key);
+}
+
+void AXObject::SerializeOtherScreenReaderAttributes(
+    ui::AXNodeData* node_data) const {
   DCHECK_NE(node_data->role, ax::mojom::blink::Role::kUnknown);
   DCHECK_NE(node_data->role, ax::mojom::blink::Role::kNone);
 

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

@@ -1417,7 +1417,7 @@ class MODULES_EXPORT AXObject : public GarbageCollected<AXObject> {
   void SerializeLiveRegionAttributes(ui::AXNodeData* node_data) const;
   void SerializeNameAndDescriptionAttributes(ui::AXMode accessibility_mode,
                                              ui::AXNodeData* node_data) const;
-  void SerializeOtherScreenReaderAttributes(ui::AXNodeData* node_data);
+  void SerializeOtherScreenReaderAttributes(ui::AXNodeData* node_data) const;
   void SerializeScreenReaderAttributes(ui::AXNodeData* node_data);
   void SerializeScrollAttributes(ui::AXNodeData* node_data);
   void SerializeSparseAttributes(ui::AXNodeData* node_data);

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

@@ -3542,16 +3542,6 @@ void AXObjectCacheImpl::MarkAXSubtreeDirtyWithCleanLayout(AXObject* obj) {
   MarkAXObjectDirtyWithCleanLayoutHelper(obj, true);
 }
 
-void AXObjectCacheImpl::AddImageAnnotations(AXObject* obj,
-                                            ui::AXNodeData* node_data) {
-  WebLocalFrameImpl* webframe = WebLocalFrameImpl::FromFrame(
-      obj->GetDocument()->AXObjectCacheOwner().GetFrame());
-  if (webframe && webframe->Client()) {
-    WebAXObject web_object(obj);
-    webframe->Client()->AddImageAnnotations(web_object, node_data);
-  }
-}
-
 void AXObjectCacheImpl::MarkAXObjectDirty(AXObject* obj) {
   if (!obj)
     return;