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}
qweri0p · Jul 6, 2022 · 76dfaaa · 76dfaaa
1 parent 4918f87
commit 76dfaaa
Show file tree

Hide file tree

Showing 17 changed files with 61 additions and 52 deletions.
diff --git 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
@@ -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"]
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="embeddedObject", roleDescription="object"]
diff --git 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
diff --git 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
+++++++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
@@ -1,3 +1,3 @@
 Document
 ++Group IsControlElement=false
-++++Pane IsControlElement=false
+++++Pane IsEnabled=false IsControlElement=false
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
@@ -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
@@ -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"]
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
diff --git 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
diff --git 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
@@ -1,3 +1,3 @@
 Document
 ++Group IsControlElement=false
-++++Document IsControlElement=false
+++++Document IsEnabled=false IsControlElement=false
diff --git 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'
diff --git 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"]
+++++View disabled actions:[AX_FOCUS] bundle:[chromeRole="pluginObject", roleDescription="object"]
diff --git 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
diff --git 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
@@ -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
@@ -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
@@ -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: