From 76dfaaa371f2a32b8985fe40c028f32714d52ca1 Mon Sep 17 00:00:00 2001
From: Aaron Leventhal <aleventhal@google.com>
Date: Wed, 6 Jul 2022 20:52:06 +0000
Subject: [PATCH] Clarify/enforce rules around child trees

* All child tree owning elements need a contentframe, whereas embed/object elements do not.
* Child tree elements cannot have children until later when trees are
merged in the browser process.

Enforcing this showed a flaw in our shadowdom-based <fencedframe> support, where an extra nested iframe was exposed in the AX tree.
The fix is that for shadowdom-based <fencedframe>s, only the
child <iframe> is exposed as an iframe. The parent <fencedframe>
is exposed as a group.

I've altered object/embed/etc. so that if the content is not supported,
then the object is marked disabled. It seems to make sense as a
semantic and also enables the rest of the code cleanup -- the dump
tree tests can use that as a signal to not wait for any child content.

Possible follow up:
- Merge kEmbedObject and kPluginObject roles.

Bug: 1311759
Cq-Include-Trybots: luci.chromium.try:linux-blink-web-tests-force-accessibility-rel
Change-Id: Idd66ace04e165e95b5f8593f92eb228b48380170
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3727378
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Auto-Submit: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1021364}
---
 .../dump_accessibility_browsertest_base.cc    |  3 +-
 .../html/embed-expected-android-external.txt  |  2 +-
 .../html/embed-expected-android.txt           |  2 +-
 .../html/embed-expected-blink.txt             |  2 +-
 .../html/embed-expected-uia-win.txt           |  2 +-
 ...me-scrollable-shadowdom-expected-blink.txt |  2 +-
 .../html/object-expected-android-external.txt |  2 +-
 .../html/object-expected-android.txt          |  2 +-
 .../html/object-expected-blink.txt            |  2 +-
 .../html/object-expected-uia-win.txt          |  2 +-
 .../html/object-image-expected-blink.txt      |  2 +-
 .../html/param-expected-android-external.txt  |  2 +-
 .../html/param-expected-blink.txt             |  2 +-
 .../modules/accessibility/ax_node_object.cc   |  7 ++
 .../modules/accessibility/ax_object.cc        | 76 +++++++++----------
 .../presubmit/audit_non_blink_usage.py        |  1 +
 ui/accessibility/ax_role_properties.cc        |  2 +
 17 files changed, 61 insertions(+), 52 deletions(-)

diff --git a/content/browser/accessibility/dump_accessibility_browsertest_base.cc b/content/browser/accessibility/dump_accessibility_browsertest_base.cc
index f93a4a6c1e97a4..87df1e43869d89 100644
--- a/content/browser/accessibility/dump_accessibility_browsertest_base.cc
+++ b/content/browser/accessibility/dump_accessibility_browsertest_base.cc
@@ -52,7 +52,8 @@ bool AccessibilityTreeContainsAllChildTrees(const ui::AXNode& node) {
   if (!num_children) {
     // No children. All content is contained unless there is supposed to be
     // a child tree for this node.
-    return !ui::IsChildTreeOwner(node.GetRole());
+    return !ui::IsChildTreeOwner(node.GetRole()) ||
+           node.data().GetRestriction() == ax::mojom::Restriction::kDisabled;
   }
 
   for (size_t i = 0; i < num_children; i++) {
diff --git a/content/test/data/accessibility/html/embed-expected-android-external.txt b/content/test/data/accessibility/html/embed-expected-android-external.txt
index 9d80d0188663ec..e0ec3cb87a8716 100644
--- a/content/test/data/accessibility/html/embed-expected-android-external.txt
+++ b/content/test/data/accessibility/html/embed-expected-android-external.txt
@@ -1,3 +1,3 @@
 WebView focusable focused scrollable actions:[CLEAR_FOCUS, AX_FOCUS] bundle:[chromeRole="rootWebArea"]
 ++View actions:[AX_FOCUS] bundle:[chromeRole="genericContainer"]
-++++View actions:[AX_FOCUS] bundle:[chromeRole="embeddedObject", roleDescription="object"]
\ No newline at end of file
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="embeddedObject", roleDescription="object"]
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/embed-expected-android.txt b/content/test/data/accessibility/html/embed-expected-android.txt
index 2771ff0b91307c..b19408545b7b64 100644
--- a/content/test/data/accessibility/html/embed-expected-android.txt
+++ b/content/test/data/accessibility/html/embed-expected-android.txt
@@ -1,3 +1,3 @@
 android.webkit.WebView focusable focused scrollable
 ++android.view.View
-++++android.view.View role_description='object'
+++++android.view.View role_description='object' disabled
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/embed-expected-blink.txt b/content/test/data/accessibility/html/embed-expected-blink.txt
index f477743023530f..1ea2acaad2d558 100644
--- a/content/test/data/accessibility/html/embed-expected-blink.txt
+++ b/content/test/data/accessibility/html/embed-expected-blink.txt
@@ -1,4 +1,4 @@
 rootWebArea
 ++genericContainer ignored
 ++++genericContainer
-++++++embeddedObject
\ No newline at end of file
+++++++embeddedObject restriction=disabled
diff --git a/content/test/data/accessibility/html/embed-expected-uia-win.txt b/content/test/data/accessibility/html/embed-expected-uia-win.txt
index 9fcd0cd52b28f9..e7415109c5ed4f 100644
--- a/content/test/data/accessibility/html/embed-expected-uia-win.txt
+++ b/content/test/data/accessibility/html/embed-expected-uia-win.txt
@@ -1,3 +1,3 @@
 Document
 ++Group IsControlElement=false
-++++Pane IsControlElement=false
+++++Pane IsEnabled=false IsControlElement=false
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/fencedframe-scrollable-shadowdom-expected-blink.txt b/content/test/data/accessibility/html/fencedframe-scrollable-shadowdom-expected-blink.txt
index 212b818d2fa987..22b09963d6972d 100644
--- a/content/test/data/accessibility/html/fencedframe-scrollable-shadowdom-expected-blink.txt
+++ b/content/test/data/accessibility/html/fencedframe-scrollable-shadowdom-expected-blink.txt
@@ -1,7 +1,7 @@
 rootWebArea scrollable=true
 ++genericContainer ignored
 ++++genericContainer
-++++++iframe name='Scrollable iframe'
+++++++group name='Scrollable iframe'
 ++++++++iframe
 ++++++++++rootWebArea scrollable=true
 ++++++++++++genericContainer ignored
diff --git a/content/test/data/accessibility/html/object-expected-android-external.txt b/content/test/data/accessibility/html/object-expected-android-external.txt
index e1ecb4e9f34b71..d30be048599ccc 100644
--- a/content/test/data/accessibility/html/object-expected-android-external.txt
+++ b/content/test/data/accessibility/html/object-expected-android-external.txt
@@ -1,3 +1,3 @@
 WebView focusable focused scrollable actions:[CLEAR_FOCUS, AX_FOCUS] bundle:[chromeRole="rootWebArea"]
 ++View actions:[AX_FOCUS] bundle:[chromeRole="genericContainer"]
-++++View actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
\ No newline at end of file
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/object-expected-android.txt b/content/test/data/accessibility/html/object-expected-android.txt
index 2771ff0b91307c..b19408545b7b64 100644
--- a/content/test/data/accessibility/html/object-expected-android.txt
+++ b/content/test/data/accessibility/html/object-expected-android.txt
@@ -1,3 +1,3 @@
 android.webkit.WebView focusable focused scrollable
 ++android.view.View
-++++android.view.View role_description='object'
+++++android.view.View role_description='object' disabled
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/object-expected-blink.txt b/content/test/data/accessibility/html/object-expected-blink.txt
index a50015957132ba..600cb12eafd002 100644
--- a/content/test/data/accessibility/html/object-expected-blink.txt
+++ b/content/test/data/accessibility/html/object-expected-blink.txt
@@ -1,4 +1,4 @@
 rootWebArea
 ++genericContainer ignored
 ++++genericContainer
-++++++pluginObject
+++++++pluginObject restriction=disabled
diff --git a/content/test/data/accessibility/html/object-expected-uia-win.txt b/content/test/data/accessibility/html/object-expected-uia-win.txt
index 3c480e6517f969..4f9152a21fad1f 100644
--- a/content/test/data/accessibility/html/object-expected-uia-win.txt
+++ b/content/test/data/accessibility/html/object-expected-uia-win.txt
@@ -1,3 +1,3 @@
 Document
 ++Group IsControlElement=false
-++++Document IsControlElement=false
+++++Document IsEnabled=false IsControlElement=false
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/object-image-expected-blink.txt b/content/test/data/accessibility/html/object-image-expected-blink.txt
index 868d4e41bafc7f..ccd8a469c7141f 100644
--- a/content/test/data/accessibility/html/object-image-expected-blink.txt
+++ b/content/test/data/accessibility/html/object-image-expected-blink.txt
@@ -2,7 +2,7 @@ rootWebArea
 ++genericContainer ignored
 ++++genericContainer ignored
 ++++++genericContainer
-++++++++pluginObject
+++++++++pluginObject restriction=disabled
 ++++++genericContainer
 ++++++++staticText name='complete'
 ++++++++++inlineTextBox name='complete'
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/param-expected-android-external.txt b/content/test/data/accessibility/html/param-expected-android-external.txt
index e1ecb4e9f34b71..d30be048599ccc 100644
--- a/content/test/data/accessibility/html/param-expected-android-external.txt
+++ b/content/test/data/accessibility/html/param-expected-android-external.txt
@@ -1,3 +1,3 @@
 WebView focusable focused scrollable actions:[CLEAR_FOCUS, AX_FOCUS] bundle:[chromeRole="rootWebArea"]
 ++View actions:[AX_FOCUS] bundle:[chromeRole="genericContainer"]
-++++View actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
\ No newline at end of file
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
\ No newline at end of file
diff --git a/content/test/data/accessibility/html/param-expected-blink.txt b/content/test/data/accessibility/html/param-expected-blink.txt
index a50015957132ba..4fd4408f39ecdf 100644
--- a/content/test/data/accessibility/html/param-expected-blink.txt
+++ b/content/test/data/accessibility/html/param-expected-blink.txt
@@ -1,4 +1,4 @@
 rootWebArea
 ++genericContainer ignored
 ++++genericContainer
-++++++pluginObject
+++++++pluginObject restriction=disabled
\ No newline at end of file
diff --git a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
index bd8148444cd00e..b079dd83c6dec1 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
+++ b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
@@ -1258,6 +1258,13 @@ ax::mojom::blink::Role AXNodeObject::NativeRoleIgnoringAria() const {
   if (IsFrame(GetNode()))
     return ax::mojom::blink::Role::kIframe;
 
+  if (IsA<HTMLFencedFrameElement>(GetNode())) {
+    // Shadow DOM <fencedframe>s are marked as a group, as they are not the
+    // child tree owner. The child tree owner is their <iframe> child.
+    DCHECK(blink::features::IsFencedFramesShadowDOMBased());
+    return ax::mojom::blink::Role::kGroup;
+  }
+
   // There should only be one banner/contentInfo per page. If header/footer are
   // being used within an article or section then it should not be exposed as
   // whole page's banner/contentInfo but as a generic container role.
diff --git a/third_party/blink/renderer/modules/accessibility/ax_object.cc b/third_party/blink/renderer/modules/accessibility/ax_object.cc
index c3c32907ef83c7..dd2074256e13a2 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_object.cc
+++ b/third_party/blink/renderer/modules/accessibility/ax_object.cc
@@ -80,6 +80,7 @@
 #include "third_party/blink/renderer/core/html/html_title_element.h"
 #include "third_party/blink/renderer/core/html/media/html_media_element.h"
 #include "third_party/blink/renderer/core/html/parser/html_parser_idioms.h"
+#include "third_party/blink/renderer/core/html/portal/html_portal_element.h"
 #include "third_party/blink/renderer/core/input/context_menu_allowed_scope.h"
 #include "third_party/blink/renderer/core/input/event_handler.h"
 #include "third_party/blink/renderer/core/input_type_names.h"
@@ -677,33 +678,6 @@ void AXObject::Init(AXObject* parent) {
     AXObjectCache().MaybeNewRelationTarget(*GetNode(), this);
 
   UpdateCachedAttributeValuesIfNeeded(false);
-
-#if defined(AX_FAIL_FAST_BUILD)
-  if (parent_ && parent_->RoleValue() == ax::mojom::blink::Role::kIframe &&
-      RoleValue() != ax::mojom::blink::Role::kDocument) {
-    // A frame/iframe can only have a document child.
-    // Make an exception for ShadowDOM based fenced frames. While they have
-    // the same role as a regular IFrame, they will have an inner iframe
-    // be the child of the outer iframe, rather than a document. This
-    // behavior is expected and the exception is carved out here.
-    if (!blink::features::IsFencedFramesEnabled() ||
-        !blink::features::IsFencedFramesShadowDOMBased() ||
-        !IsA<HTMLFencedFrameElement>(parent_->GetNode())) {
-      NOTREACHED() << "An iframe can only have a document child."
-                   << "\n* Child = " << ToString(true, true)
-                   << "\n* Parent =  " << parent_->ToString(true, true);
-    }
-  }
-
-  if (blink::features::IsFencedFramesEnabled() &&
-      blink::features::IsFencedFramesShadowDOMBased() && parent_ &&
-      IsA<HTMLFencedFrameElement>(parent_->GetNode()) &&
-      RoleValue() != ax::mojom::blink::Role::kIframe) {
-    NOTREACHED() << "A ShadowDOM fenced frame must have an iframe child."
-                 << "\n* Child = " << ToString(true, true)
-                 << "\n* Parent =  " << parent_->ToString(true, true);
-  }
-#endif
 }
 
 void AXObject::Detach() {
@@ -1324,18 +1298,33 @@ void AXObject::SerializeActionAttributes(ui::AXNodeData* node_data) {
 void AXObject::SerializeChildTreeID(ui::AXNodeData* node_data) {
   // If this is an HTMLFrameOwnerElement (such as an iframe), we may need
   // to embed the ID of the child frame.
-  if (auto* html_frame_owner_element =
-          DynamicTo<HTMLFrameOwnerElement>(GetElement())) {
-    if (Frame* child_frame = html_frame_owner_element->ContentFrame()) {
-      absl::optional<base::UnguessableToken> child_token =
-          child_frame->GetEmbeddingToken();
-      if (child_token && !(IsDetached() || ChildCountIncludingIgnored())) {
-        ui::AXTreeID child_tree_id =
-            ui::AXTreeID::FromToken(child_token.value());
-        node_data->AddChildTreeId(child_tree_id);
-      }
-    }
+  if (!ui::IsChildTreeOwner(RoleValue())) {
+    DCHECK(!IsA<HTMLFrameOwnerElement>(GetElement()) || !IsFrame(GetNode()))
+        << "Element was expected to be a child tree owner: " << GetElement();
+    return;
   }
+
+  auto* html_frame_owner_element = To<HTMLFrameOwnerElement>(GetElement());
+
+  Frame* child_frame = html_frame_owner_element->ContentFrame();
+  if (!child_frame) {
+    DCHECK(IsDisabled());
+    return;
+  }
+
+  absl::optional<base::UnguessableToken> child_token =
+      child_frame->GetEmbeddingToken();
+  if (!child_token)
+    return;  // No child token means that the connection isn't ready yet.
+
+  DCHECK_EQ(ChildCountIncludingIgnored(), 0)
+      << "Children won't exist until the trees are stitched together in the "
+         "browser process. A failure means that a child node was incorrectly "
+         "considered relevant by AXObjectCacheImpl. Element src = "
+      << GetAttribute(html_names::kSrcAttr);
+
+  ui::AXTreeID child_tree_id = ui::AXTreeID::FromToken(child_token.value());
+  node_data->AddChildTreeId(child_tree_id);
 }
 
 void AXObject::SerializeChooserPopupAttributes(ui::AXNodeData* node_data) {
@@ -4215,6 +4204,12 @@ const AtomicString& AXObject::LiveRegionRelevant() const {
 }
 
 bool AXObject::IsDisabled() const {
+  // <embed> or <object> with unsupported plugin, or more iframes than allowed.
+  if (ui::IsChildTreeOwner(RoleValue())) {
+    auto* html_frame_owner_element = To<HTMLFrameOwnerElement>(GetElement());
+    return !html_frame_owner_element->ContentFrame();
+  }
+
   // Check for HTML form control with the disabled attribute.
   if (GetElement() && GetElement()->IsDisabledFormControl())
     return true;
@@ -6000,8 +5995,11 @@ bool AXObject::IsFrame(const Node* node) {
   switch (frame_owner->OwnerType()) {
     case FrameOwnerElementType::kIframe:
     case FrameOwnerElementType::kFrame:
-    case FrameOwnerElementType::kFencedframe:
       return true;
+    case FrameOwnerElementType::kFencedframe:
+      // Shadow DOM <fencedframe>s have an <iframe> child, which will be the
+      // child tree owner.
+      return !blink::features::IsFencedFramesShadowDOMBased();
     case FrameOwnerElementType::kObject:
     case FrameOwnerElementType::kEmbed:
     case FrameOwnerElementType::kPortal:
diff --git a/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py b/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
index e66a68c08da5a4..c67dfb9aaf5246 100755
--- a/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
+++ b/third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py
@@ -665,6 +665,7 @@
             # serialization. Please keep alphabetized.
             'ui::CanHaveInlineTextBoxChildren',
             'ui::IsCellOrTableHeader',
+            'ui::IsChildTreeOwner',
             'ui::IsClickable',
             'ui::IsComboBox',
             'ui::IsContainerWithSelectableChildren',
diff --git a/ui/accessibility/ax_role_properties.cc b/ui/accessibility/ax_role_properties.cc
index 29c3ed797ef225..ae176cfbecebbf 100644
--- a/ui/accessibility/ax_role_properties.cc
+++ b/ui/accessibility/ax_role_properties.cc
@@ -105,8 +105,10 @@ bool IsCellOrTableHeader(const ax::mojom::Role role) {
 
 bool IsChildTreeOwner(const ax::mojom::Role role) {
   switch (role) {
+    case ax::mojom::Role::kEmbeddedObject:
     case ax::mojom::Role::kIframe:
     case ax::mojom::Role::kIframePresentational:
+    case ax::mojom::Role::kPluginObject:
     case ax::mojom::Role::kPortal:
       return true;
     default: