[DevTools] Accessibility: Show siblings and children of selected node

third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp

Index: third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
diff --git a/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp b/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
index 0d7ac6ba8ff4044ffa0f1f26b66fe6f6a7ba8f16..a94638bcb3df2566ea4a94d06f0bfabd2c0a4817 100644
--- a/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp
@@ -8,6 +8,8 @@
 #include "core/dom/AXObjectCache.h"
 #include "core/dom/DOMNodeIds.h"
 #include "core/dom/Element.h"
+#include "core/dom/Node.h"
+#include "core/dom/NodeList.h"
 #include "core/inspector/IdentifiersFactory.h"
 #include "core/inspector/InspectorDOMAgent.h"
 #include "core/inspector/InspectorStyleSheet.h"
@@ -36,98 +38,76 @@ using namespace HTMLNames;
 
 namespace {
 
-void fillCoreProperties(AXObject* axObject, AXNode* nodeObject) {
-  // Description (secondary to the accessible name).
-  AXNameFrom nameFrom;
-  AXObject::AXObjectVector nameObjects;
-  axObject->name(nameFrom, &nameObjects);
-  AXDescriptionFrom descriptionFrom;
-  AXObject::AXObjectVector descriptionObjects;
-  String description =
-      axObject->description(nameFrom, descriptionFrom, &descriptionObjects);
-  if (!description.isEmpty())
-    nodeObject->setDescription(
-        createValue(description, AXValueTypeEnum::ComputedString));
-
-  // Value.
-  if (axObject->supportsRangeValue()) {
-    nodeObject->setValue(createValue(axObject->valueForRange()));
-  } else {
-    String stringValue = axObject->stringValue();
-    if (!stringValue.isEmpty())
-      nodeObject->setValue(createValue(stringValue));
-  }
-}
-
-void fillLiveRegionProperties(AXObject* axObject,
-                              protocol::Array<AXProperty>* properties) {
-  if (!axObject->liveRegionRoot())
+void fillLiveRegionProperties(AXObject& axObject,
+                              protocol::Array<AXProperty>& properties) {
+  if (!axObject.liveRegionRoot())
     return;
 
-  properties->addItem(
+  properties.addItem(
       createProperty(AXLiveRegionAttributesEnum::Live,
-                     createValue(axObject->containerLiveRegionStatus(),
+                     createValue(axObject.containerLiveRegionStatus(),
                                  AXValueTypeEnum::Token)));
-  properties->addItem(createProperty(
-      AXLiveRegionAttributesEnum::Atomic,
-      createBooleanValue(axObject->containerLiveRegionAtomic())));
-  properties->addItem(
+  properties.addItem(
+      createProperty(AXLiveRegionAttributesEnum::Atomic,
+                     createBooleanValue(axObject.containerLiveRegionAtomic())));
+  properties.addItem(
       createProperty(AXLiveRegionAttributesEnum::Relevant,
-                     createValue(axObject->containerLiveRegionRelevant(),
+                     createValue(axObject.containerLiveRegionRelevant(),
                                  AXValueTypeEnum::TokenList)));
-  properties->addItem(
+  properties.addItem(
       createProperty(AXLiveRegionAttributesEnum::Busy,
-                     createBooleanValue(axObject->containerLiveRegionBusy())));
+                     createBooleanValue(axObject.containerLiveRegionBusy())));
 
-  if (!axObject->isLiveRegion())
-    properties->addItem(
-        createProperty(AXLiveRegionAttributesEnum::Root,
-                       createRelatedNodeListValue(axObject->liveRegionRoot())));
+  if (!axObject.isLiveRegion()) {
+    properties.addItem(createProperty(
+        AXLiveRegionAttributesEnum::Root,
+        createRelatedNodeListValue(*(axObject.liveRegionRoot()))));
+  }
 }
 
-void fillGlobalStates(AXObject* axObject,
-                      protocol::Array<AXProperty>* properties) {
-  if (!axObject->isEnabled())
-    properties->addItem(
+void fillGlobalStates(AXObject& axObject,
+                      protocol::Array<AXProperty>& properties) {
+  if (!axObject.isEnabled())
+    properties.addItem(
         createProperty(AXGlobalStatesEnum::Disabled, createBooleanValue(true)));
 
-  if (const AXObject* hiddenRoot = axObject->ariaHiddenRoot()) {
-    properties->addItem(
+  if (const AXObject* hiddenRoot = axObject.ariaHiddenRoot()) {
+    properties.addItem(
         createProperty(AXGlobalStatesEnum::Hidden, createBooleanValue(true)));
-    properties->addItem(createProperty(AXGlobalStatesEnum::HiddenRoot,
-                                       createRelatedNodeListValue(hiddenRoot)));
+    properties.addItem(createProperty(AXGlobalStatesEnum::HiddenRoot,
+                                      createRelatedNodeListValue(*hiddenRoot)));
   }
 
-  InvalidState invalidState = axObject->getInvalidState();
+  InvalidState invalidState = axObject.getInvalidState();
   switch (invalidState) {
     case InvalidStateUndefined:
       break;
     case InvalidStateFalse:
-      properties->addItem(
+      properties.addItem(
           createProperty(AXGlobalStatesEnum::Invalid,
                          createValue("false", AXValueTypeEnum::Token)));
       break;
     case InvalidStateTrue:
-      properties->addItem(
+      properties.addItem(
           createProperty(AXGlobalStatesEnum::Invalid,
                          createValue("true", AXValueTypeEnum::Token)));
       break;
     case InvalidStateSpelling:
-      properties->addItem(
+      properties.addItem(
           createProperty(AXGlobalStatesEnum::Invalid,
                          createValue("spelling", AXValueTypeEnum::Token)));
       break;
     case InvalidStateGrammar:
-      properties->addItem(
+      properties.addItem(
           createProperty(AXGlobalStatesEnum::Invalid,
                          createValue("grammar", AXValueTypeEnum::Token)));
       break;
     default:
       // TODO(aboxhall): expose invalid: <nothing> and source: aria-invalid as
       // invalid value
-      properties->addItem(createProperty(
+      properties.addItem(createProperty(
           AXGlobalStatesEnum::Invalid,
-          createValue(axObject->ariaInvalidValue(), AXValueTypeEnum::String)));
+          createValue(axObject.ariaInvalidValue(), AXValueTypeEnum::String)));
       break;
   }
 }
@@ -173,47 +153,49 @@ bool roleAllowsSelected(AccessibilityRole role) {
          role == RowHeaderRole || role == TreeItemRole;
 }
 
-void fillWidgetProperties(AXObject* axObject,
-                          protocol::Array<AXProperty>* properties) {
-  AccessibilityRole role = axObject->roleValue();
-  String autocomplete = axObject->ariaAutoComplete();
+void fillWidgetProperties(AXObject& axObject,

[dmazzoni, 2016/10/19 16:33:49]

You could land a quick change to replace pointers with references for all
existing APIs
first to cut down on the size of this review

[aboxhall, 2016/10/19 23:49:05]

Done.

+                          protocol::Array<AXProperty>& properties) {
+  AccessibilityRole role = axObject.roleValue();
+  String autocomplete = axObject.ariaAutoComplete();
   if (!autocomplete.isEmpty())
-    properties->addItem(
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Autocomplete,
                        createValue(autocomplete, AXValueTypeEnum::Token)));
 
-  if (axObject->hasAttribute(HTMLNames::aria_haspopupAttr)) {
-    bool hasPopup = axObject->ariaHasPopup();
-    properties->addItem(createProperty(AXWidgetAttributesEnum::Haspopup,
-                                       createBooleanValue(hasPopup)));
+  if (axObject.hasAttribute(HTMLNames::aria_haspopupAttr)) {
+    bool hasPopup = axObject.ariaHasPopup();
+    properties.addItem(createProperty(AXWidgetAttributesEnum::Haspopup,
+                                      createBooleanValue(hasPopup)));
   }
 
-  int headingLevel = axObject->headingLevel();
-  if (headingLevel > 0)
-    properties->addItem(createProperty(AXWidgetAttributesEnum::Level,
-                                       createValue(headingLevel)));
-  int hierarchicalLevel = axObject->hierarchicalLevel();
+  int headingLevel = axObject.headingLevel();
+  if (headingLevel > 0) {

[dmazzoni, 2016/10/19 16:33:49]

Did this come from "git cl format" or some other automated pass?

You could land another change to add braces to existing code
first to cut down on the size of this change

[aboxhall, 2016/10/19 23:49:05]

Done.

+    properties.addItem(createProperty(AXWidgetAttributesEnum::Level,
+                                      createValue(headingLevel)));
+  }
+  int hierarchicalLevel = axObject.hierarchicalLevel();
   if (hierarchicalLevel > 0 ||
-      axObject->hasAttribute(HTMLNames::aria_levelAttr))
-    properties->addItem(createProperty(AXWidgetAttributesEnum::Level,
-                                       createValue(hierarchicalLevel)));
+      axObject.hasAttribute(HTMLNames::aria_levelAttr)) {
+    properties.addItem(createProperty(AXWidgetAttributesEnum::Level,
+                                      createValue(hierarchicalLevel)));
+  }
 
   if (roleAllowsMultiselectable(role)) {
-    bool multiselectable = axObject->isMultiSelectable();
-    properties->addItem(createProperty(AXWidgetAttributesEnum::Multiselectable,
-                                       createBooleanValue(multiselectable)));
+    bool multiselectable = axObject.isMultiSelectable();
+    properties.addItem(createProperty(AXWidgetAttributesEnum::Multiselectable,
+                                      createBooleanValue(multiselectable)));
   }
 
   if (roleAllowsOrientation(role)) {
-    AccessibilityOrientation orientation = axObject->orientation();
+    AccessibilityOrientation orientation = axObject.orientation();
     switch (orientation) {
       case AccessibilityOrientationVertical:
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetAttributesEnum::Orientation,
                            createValue("vertical", AXValueTypeEnum::Token)));
         break;
       case AccessibilityOrientationHorizontal:
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetAttributesEnum::Orientation,
                            createValue("horizontal", AXValueTypeEnum::Token)));
         break;
@@ -222,103 +204,104 @@ void fillWidgetProperties(AXObject* axObject,
     }
   }
 
-  if (role == TextFieldRole)
-    properties->addItem(
+  if (role == TextFieldRole) {
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Multiline,
-                       createBooleanValue(axObject->isMultiline())));
+                       createBooleanValue(axObject.isMultiline())));
+  }
 
   if (roleAllowsReadonly(role)) {
-    properties->addItem(
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Readonly,
-                       createBooleanValue(axObject->isReadOnly())));
+                       createBooleanValue(axObject.isReadOnly())));
   }
 
   if (roleAllowsRequired(role)) {
-    properties->addItem(
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Required,
-                       createBooleanValue(axObject->isRequired())));
+                       createBooleanValue(axObject.isRequired())));
   }
 
   if (roleAllowsSort(role)) {
     // TODO(aboxhall): sort
   }
 
-  if (axObject->isRange()) {
-    properties->addItem(
+  if (axObject.isRange()) {
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Valuemin,
-                       createValue(axObject->minValueForRange())));
-    properties->addItem(
+                       createValue(axObject.minValueForRange())));
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Valuemax,
-                       createValue(axObject->maxValueForRange())));
-    properties->addItem(
+                       createValue(axObject.maxValueForRange())));
+    properties.addItem(
         createProperty(AXWidgetAttributesEnum::Valuetext,
-                       createValue(axObject->valueDescription())));
+                       createValue(axObject.valueDescription())));
   }
 }
 
-void fillWidgetStates(AXObject* axObject,
-                      protocol::Array<AXProperty>* properties) {
-  AccessibilityRole role = axObject->roleValue();
+void fillWidgetStates(AXObject& axObject,
+                      protocol::Array<AXProperty>& properties) {
+  AccessibilityRole role = axObject.roleValue();
   if (roleAllowsChecked(role)) {
-    AccessibilityButtonState checked = axObject->checkboxOrRadioValue();
+    AccessibilityButtonState checked = axObject.checkboxOrRadioValue();
     switch (checked) {
       case ButtonStateOff:
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetStatesEnum::Checked,
                            createValue("false", AXValueTypeEnum::Tristate)));
         break;
       case ButtonStateOn:
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetStatesEnum::Checked,
                            createValue("true", AXValueTypeEnum::Tristate)));
         break;
       case ButtonStateMixed:
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetStatesEnum::Checked,
                            createValue("mixed", AXValueTypeEnum::Tristate)));
         break;
     }
   }
 
-  AccessibilityExpanded expanded = axObject->isExpanded();
+  AccessibilityExpanded expanded = axObject.isExpanded();
   switch (expanded) {
     case ExpandedUndefined:
       break;
     case ExpandedCollapsed:
-      properties->addItem(createProperty(
+      properties.addItem(createProperty(
           AXWidgetStatesEnum::Expanded,
           createBooleanValue(false, AXValueTypeEnum::BooleanOrUndefined)));
       break;
     case ExpandedExpanded:
-      properties->addItem(createProperty(
+      properties.addItem(createProperty(
           AXWidgetStatesEnum::Expanded,
           createBooleanValue(true, AXValueTypeEnum::BooleanOrUndefined)));
       break;
   }
 
   if (role == ToggleButtonRole) {
-    if (!axObject->isPressed()) {
-      properties->addItem(
+    if (!axObject.isPressed()) {
+      properties.addItem(
           createProperty(AXWidgetStatesEnum::Pressed,
                          createValue("false", AXValueTypeEnum::Tristate)));
     } else {
       const AtomicString& pressedAttr =
-          axObject->getAttribute(HTMLNames::aria_pressedAttr);
+          axObject.getAttribute(HTMLNames::aria_pressedAttr);
       if (equalIgnoringCase(pressedAttr, "mixed"))
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetStatesEnum::Pressed,
                            createValue("mixed", AXValueTypeEnum::Tristate)));
       else
-        properties->addItem(
+        properties.addItem(
             createProperty(AXWidgetStatesEnum::Pressed,
                            createValue("true", AXValueTypeEnum::Tristate)));
     }
   }
 
   if (roleAllowsSelected(role)) {
-    properties->addItem(
+    properties.addItem(
         createProperty(AXWidgetStatesEnum::Selected,
-                       createBooleanValue(axObject->isSelected())));
+                       createBooleanValue(axObject.isSelected())));
   }
 }
 
@@ -334,46 +317,46 @@ std::unique_ptr<AXProperty> createRelatedNodeListProperty(
     const String& key,
     AXObject::AXObjectVector& nodes,
     const QualifiedName& attr,
-    AXObject* axObject) {
+    AXObject& axObject) {
   std::unique_ptr<AXValue> nodeListValue = createRelatedNodeListValue(nodes);
-  const AtomicString& attrValue = axObject->getAttribute(attr);
+  const AtomicString& attrValue = axObject.getAttribute(attr);
   nodeListValue->setValue(protocol::StringValue::create(attrValue));
   return createProperty(key, std::move(nodeListValue));
 }
 
-void fillRelationships(AXObject* axObject,
-                       protocol::Array<AXProperty>* properties) {
-  if (AXObject* activeDescendant = axObject->activeDescendant()) {
-    properties->addItem(
+void fillRelationships(AXObject& axObject,
+                       protocol::Array<AXProperty>& properties) {
+  if (AXObject* activeDescendant = axObject.activeDescendant()) {
+    properties.addItem(
         createProperty(AXRelationshipAttributesEnum::Activedescendant,
-                       createRelatedNodeListValue(activeDescendant)));
+                       createRelatedNodeListValue(*activeDescendant)));
   }
 
   AXObject::AXObjectVector results;
-  axObject->ariaFlowToElements(results);
+  axObject.ariaFlowToElements(results);
   if (!results.isEmpty())
-    properties->addItem(
+    properties.addItem(
         createRelatedNodeListProperty(AXRelationshipAttributesEnum::Flowto,
                                       results, aria_flowtoAttr, axObject));
   results.clear();
 
-  axObject->ariaControlsElements(results);
+  axObject.ariaControlsElements(results);
   if (!results.isEmpty())
-    properties->addItem(
+    properties.addItem(
         createRelatedNodeListProperty(AXRelationshipAttributesEnum::Controls,
                                       results, aria_controlsAttr, axObject));
   results.clear();
 
-  axObject->ariaDescribedbyElements(results);
+  axObject.ariaDescribedbyElements(results);
   if (!results.isEmpty())
-    properties->addItem(
+    properties.addItem(
         createRelatedNodeListProperty(AXRelationshipAttributesEnum::Describedby,
                                       results, aria_describedbyAttr, axObject));
   results.clear();
 
-  axObject->ariaOwnsElements(results);
+  axObject.ariaOwnsElements(results);
   if (!results.isEmpty())
-    properties->addItem(createRelatedNodeListProperty(
+    properties.addItem(createRelatedNodeListProperty(
         AXRelationshipAttributesEnum::Owns, results, aria_ownsAttr, axObject));
   results.clear();
 }
@@ -390,22 +373,109 @@ std::unique_ptr<AXValue> createRoleNameValue(AccessibilityRole role) {
   return roleNameValue;
 }
 
-std::unique_ptr<AXNode> buildObjectForIgnoredNode(Node* node,
-                                                  const AXObject* axObject) {
+bool isAncestorOf(AXObject* possibleAncestor, AXObject* descendant) {
+  if (!possibleAncestor || !descendant)
+    return false;
+
+  AXObject* ancestor = descendant;
+  while (ancestor) {
+    if (ancestor == possibleAncestor)
+      return true;
+    ancestor = ancestor->parentObject();
+  }
+  return false;
+}
+
+}  // namespace
+
+InspectorAccessibilityAgent::InspectorAccessibilityAgent(
+    Page* page,
+    InspectorDOMAgent* domAgent)
+    : m_page(page), m_domAgent(domAgent) {}
+
+void InspectorAccessibilityAgent::getPartialAXTree(
+    ErrorString* errorString,
+    int domNodeId,
+    std::unique_ptr<protocol::Array<AXNode>>* nodes) {
+  if (!m_domAgent->enabled()) {
+    *errorString = "DOM agent must be enabled";
+    return;
+  }
+  Node* domNode = m_domAgent->assertNode(errorString, domNodeId);
+  if (!domNode)
+    return;
+
+  Document& document = domNode->document();
+  document.updateStyleAndLayoutIgnorePendingStylesheets();
+  DocumentLifecycle::DisallowTransitionScope disallowTransition(
+      document.lifecycle());
+  LocalFrame* localFrame = document.frame();
+  if (!localFrame) {
+    *errorString = "Frame is detached.";
+    return;
+  }
+  std::unique_ptr<ScopedAXObjectCache> scopedCache =
+      ScopedAXObjectCache::create(document);
+  AXObjectCacheImpl* cache = toAXObjectCacheImpl(scopedCache->get());
+
+  AXObject* inspectedAXObject = cache->getOrCreate(domNode);
+  *nodes = protocol::Array<protocol::Accessibility::AXNode>::create();
+  if (!inspectedAXObject || inspectedAXObject->accessibilityIsIgnored()) {
+    (*nodes)->addItem(
+        buildObjectForIgnoredNode(domNode, inspectedAXObject, *nodes, *cache));
+  } else {
+    (*nodes)->addItem(buildProtocolAXObject(*inspectedAXObject,
+                                            inspectedAXObject, *nodes, *cache));
+  }
+
+  if (!inspectedAXObject)
+    return;
+
+  AXObject* parent = inspectedAXObject->parentObjectUnignored();

[dmazzoni, 2016/10/19 16:33:48]

Will it be confusing if you can click on a node and see it's ignored, but when
you explore the partial tree you won't see ignored parents? Just making sure
that's what you intended.

[aboxhall, 2016/10/19 23:49:05]

Yeah, I think it's either that (i.e. show ignored nodes iff they map to the
inspected DOM node) or just include *all* ignored nodes in the tree, which would
be really noisy. What do you think?

+  if (!parent)
+    return;
+
+  std::unique_ptr<AXNode> parentNodeObject =
+      buildProtocolAXObject(*parent, inspectedAXObject, *nodes, *cache);
+
+  (*nodes)->addItem(std::move(parentNodeObject));

[dmazzoni, 2016/10/19 16:33:48]

Are you using parentNodeObject? If not, maybe just do:

    (*nodes)->addItem(
        buildProtocolAXObject(*parent, inspectedAXObject, *nodes, *cache));

[aboxhall, 2016/10/19 23:49:05]

Done.

+  AXObject* ancestor = parent->parentObjectUnignored();
+  while (ancestor) {
+    (*nodes)->addItem(
+        buildProtocolAXObject(*ancestor, inspectedAXObject, *nodes, *cache));

[dmazzoni, 2016/10/19 16:33:48]

I think you can eliminate a few lines of code here - you handle the first parent
and then the rest of the ancestors, but I think you can replace it with just one
loop that does all of the ancestors

[aboxhall, 2016/10/19 23:49:05]

Done (yeah, it was originally special-casing the parent which is why it was
pulled out into a variable as well).

+    ancestor = ancestor->parentObjectUnignored();
+  }
+}
+
+std::unique_ptr<AXNode> InspectorAccessibilityAgent::buildObjectForIgnoredNode(
+    Node* domNode,
+    AXObject* axObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
   AXObject::IgnoredReasons ignoredReasons;
 
   AXID axID = 0;
+  if (axObject)
+    axID = axObject->axObjectID();
   std::unique_ptr<AXNode> ignoredNodeObject =
       AXNode::create().setNodeId(String::number(axID)).setIgnored(true).build();
   if (axObject) {
     axObject->computeAccessibilityIsIgnored(&ignoredReasons);
     axID = axObject->axObjectID();

[dmazzoni, 2016/10/19 16:33:49]

Duplicate, you just computed it above

[aboxhall, 2016/10/19 23:49:05]

Done.

-    AccessibilityRole role = axObject->roleValue();
+    AccessibilityRole role = AccessibilityRole::IgnoredRole;
     ignoredNodeObject->setRole(createRoleNameValue(role));
-  } else if (node && !node->layoutObject()) {
+
+    populateRelatives(*axObject, axObject, *(ignoredNodeObject.get()), nodes,
+                      cache);
+  } else if (domNode && !domNode->layoutObject()) {
+    populateDOMNodeRelatives(*domNode, *(ignoredNodeObject.get()), nodes,
+                             cache);
     ignoredReasons.append(IgnoredReason(AXNotRendered));
   }
 
+  if (domNode)
+    ignoredNodeObject->setDomNodeId(DOMNodeIds::idForNode(domNode));
+
   std::unique_ptr<protocol::Array<AXProperty>> ignoredReasonProperties =
       protocol::Array<AXProperty>::create();
   for (size_t i = 0; i < ignoredReasons.size(); i++)
@@ -415,25 +485,95 @@ std::unique_ptr<AXNode> buildObjectForIgnoredNode(Node* node,
   return ignoredNodeObject;
 }
 
-std::unique_ptr<AXNode> buildProtocolAXObject(AXObject* axObject) {
-  AccessibilityRole role = axObject->roleValue();
+void InspectorAccessibilityAgent::populateDOMNodeRelatives(
+    Node& inspectedDOMNode,
+    AXNode& nodeObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  // Walk up parents until an AXObject can be found.
+  Node* parentNode = inspectedDOMNode.parentNode();

[dmazzoni, 2016/10/19 16:33:48]

I'm not an expert at all, but I suspect you may want to use
FlatTreeTraversal::parent(inspectedDOMNode) here in order to properly walk up
through shadow roots. Or check with someone on the core Blink team to better
understand the differences here.

[aboxhall, 2016/10/19 23:49:05]

That makes sense, thanks for catching.

+  AXObject* parentAXObject = cache.getOrCreate(parentNode);
+  while (!parentAXObject) {
+    parentNode = parentNode->parentNode();
+    parentAXObject = cache.getOrCreate(parentNode);
+  };
+  if (parentAXObject) {
+    // Populate parent and siblings.
+    nodeObject.setParentId(String::number(parentAXObject->axObjectID()));
+    std::unique_ptr<AXNode> parentNodeObject =
+        buildProtocolAXObject(*parentAXObject, nullptr, nodes, cache);
+    std::unique_ptr<protocol::Array<AXNodeId>> siblingIds =
+        protocol::Array<AXNodeId>::create();
+    findDOMNodeChildren(siblingIds, *parentNode, inspectedDOMNode, nodes,
+                        cache);
+    parentNodeObject->setChildIds(std::move(siblingIds));
+    nodes->addItem(std::move(parentNodeObject));
+  }
+  // Then populate children.
+  std::unique_ptr<protocol::Array<AXNodeId>> childIds =
+      protocol::Array<AXNodeId>::create();
+  findDOMNodeChildren(childIds, inspectedDOMNode, inspectedDOMNode, nodes,
+                      cache);
+}
+
+void InspectorAccessibilityAgent::findDOMNodeChildren(
+    std::unique_ptr<protocol::Array<AXNodeId>>& childIds,
+    Node& parentNode,
+    Node& inspectedDOMNode,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  NodeList* childNodes = parentNode.childNodes();
+  for (size_t i = 0; i < childNodes->length(); ++i) {
+    Node* childNode = childNodes->item(i);
+    if (childNode == &inspectedDOMNode) {
+      childIds->addItem(String::number(0));

[dmazzoni, 2016/10/19 16:33:48]

Can you document this magic number? Why is the inspected node id 0 and not its
actual id?

[aboxhall, 2016/10/19 23:49:05]

It doesn't have an actual id - there is no AXNode for the inspected DOM node in
this case, so there's no ID to use.

+      continue;
+    }
+    AXObject* childAXObject = cache.getOrCreate(childNode);
+    if (childAXObject && !childAXObject->accessibilityIsIgnored()) {
+      addChild(childIds, *childAXObject, nullptr, nodes, cache);
+      continue;
+    }
+
+    if (!childAXObject ||
+        childNode->isShadowIncludingInclusiveAncestorOf(&inspectedDOMNode)) {
+      // If the inspected node may be a descendant of this node, keep walking
+      // recursively until we find its actual siblings.
+      findDOMNodeChildren(childIds, *childNode, inspectedDOMNode, nodes, cache);
+      continue;
+    }
+
+    // Otherwise, just add the un-ignored children.

[dmazzoni, 2016/10/19 16:33:48]

Reading through all of the special cases for building a tree based on a DOM node
(because it has no AXObject) vs building a tree based on an AXObject directly
makes me worried that it will be confusing to see a subtly different tree
depending on what the current inspected DOM node is.

In particular, think about how an AXObject builds its list of children: when one
of its children is ignored, it calls addChildren recursively on that ignored
child, so they all look like direct children of the ancestor. Now here you've
got a DOM node whose AXObject is ignored (otherwise you wouldn't be calling
findDOMNodeChildren), but you're taking one of its children and adding its
unignored children...but these could all actually be children of some other
ancestor AXObject that's had them added as its own children.

Sorry if my explanation is confusing.

I guess what I'm leaning towards is maybe simplifying what you try to do when
exploring a DOM node that's ignored. Perhaps you should ONLY display the parent
chain in that case. That'd be more foolproof.

Another idea: what if we got rid of the tiny number of cases where
AXObjectCacheImpl::getOrCreate(Node*) returns null? Just have it return an
object that's ignored. It looks like that would simplify your code here a lot if
you only had to deal with ignored and unignored, rather than ignored, unignored,
and null.

[aboxhall, 2016/10/19 23:49:05]

Not quite accurate - this is only called if there is *no* AXObject. However, the
same basic strategy is used if the AXObject is ignored. Effectively, it
temporarily "reinstates" the ignored node so that you can see the shape of the
AX tree around it.
Not at all, this code was extremely confusing to write :\
I think that would be confusing to use.
That would be much easier! I'll give this a go.

[aboxhall, 2016/10/21 17:28:23]

Ok, I tried that out and it actually doesn't buy us much, because the AXTree is
built up based on the layout tree, so if something doesn't have a layout object
it still ends up "weird". So I'd prefer to leave this code in if possible.

If you have the time, it'd be great if you could patch this change in and have a
play to see how it all works together; meanwhile I'll add some more screenshots
to the bug to show the flow for ignored/unrendered nodes.

+    const AXObject::AXObjectVector& indirectChildren =
+        childAXObject->children();
+    for (unsigned i = 0; i < indirectChildren.size(); ++i)
+      addChild(childIds, *(indirectChildren[i]), nullptr, nodes, cache);
+  }
+}
+
+std::unique_ptr<AXNode> InspectorAccessibilityAgent::buildProtocolAXObject(
+    AXObject& axObject,
+    AXObject* inspectedAXObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  AccessibilityRole role = axObject.roleValue();
   std::unique_ptr<AXNode> nodeObject =
       AXNode::create()
-          .setNodeId(String::number(axObject->axObjectID()))
+          .setNodeId(String::number(axObject.axObjectID()))
           .setIgnored(false)
           .build();
   nodeObject->setRole(createRoleNameValue(role));
 
   std::unique_ptr<protocol::Array<AXProperty>> properties =
       protocol::Array<AXProperty>::create();
-  fillLiveRegionProperties(axObject, properties.get());
-  fillGlobalStates(axObject, properties.get());
-  fillWidgetProperties(axObject, properties.get());
-  fillWidgetStates(axObject, properties.get());
-  fillRelationships(axObject, properties.get());
+  fillLiveRegionProperties(axObject, *(properties.get()));
+  fillGlobalStates(axObject, *(properties.get()));
+  fillWidgetProperties(axObject, *(properties.get()));
+  fillWidgetStates(axObject, *(properties.get()));
+  fillRelationships(axObject, *(properties.get()));
 
   AXObject::NameSources nameSources;
-  String computedName = axObject->name(&nameSources);
+  String computedName = axObject.name(&nameSources);
   if (!nameSources.isEmpty()) {
     std::unique_ptr<AXValue> name =
         createValue(computedName, AXValueTypeEnum::ComputedString);
@@ -459,55 +599,132 @@ std::unique_ptr<AXNode> buildProtocolAXObject(AXObject* axObject) {
     nodeObject->setProperties(std::move(properties));
   }
 
-  fillCoreProperties(axObject, nodeObject.get());
+  fillCoreProperties(axObject, inspectedAXObject, *(nodeObject.get()), nodes,
+                     cache);
   return nodeObject;
 }
 
-}  // namespace
-
-InspectorAccessibilityAgent::InspectorAccessibilityAgent(
-    Page* page,
-    InspectorDOMAgent* domAgent)
-    : m_page(page), m_domAgent(domAgent) {}
+void InspectorAccessibilityAgent::fillCoreProperties(
+    AXObject& axObject,
+    AXObject* inspectedAXObject,
+    AXNode& nodeObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  AXNameFrom nameFrom;
+  AXObject::AXObjectVector nameObjects;
+  axObject.name(nameFrom, &nameObjects);
 
-void InspectorAccessibilityAgent::getAXNodeChain(
-    ErrorString* errorString,
-    int domNodeId,
-    bool fetchAncestors,
-    std::unique_ptr<protocol::Array<protocol::Accessibility::AXNode>>* nodes) {
-  if (!m_domAgent->enabled()) {
-    *errorString = "DOM agent must be enabled";
-    return;
+  AXDescriptionFrom descriptionFrom;
+  AXObject::AXObjectVector descriptionObjects;
+  String description =
+      axObject.description(nameFrom, descriptionFrom, &descriptionObjects);
+  if (!description.isEmpty()) {
+    nodeObject.setDescription(
+        createValue(description, AXValueTypeEnum::ComputedString));
+  }
+  // Value.
+  if (axObject.supportsRangeValue()) {
+    nodeObject.setValue(createValue(axObject.valueForRange()));
+  } else {
+    String stringValue = axObject.stringValue();
+    if (!stringValue.isEmpty())
+      nodeObject.setValue(createValue(stringValue));
   }
-  Node* node = m_domAgent->assertNode(errorString, domNodeId);
-  if (!node)
-    return;
 
-  Document& document = node->document();
-  document.updateStyleAndLayoutIgnorePendingStylesheets();
-  DocumentLifecycle::DisallowTransitionScope disallowTransition(
-      document.lifecycle());
-  LocalFrame* localFrame = document.frame();
-  if (!localFrame) {
-    *errorString = "Frame is detached.";
+  populateRelatives(axObject, inspectedAXObject, nodeObject, nodes, cache);
+
+  Node* node = axObject.getNode();
+  if (node)
+    nodeObject.setDomNodeId(DOMNodeIds::idForNode(node));
+}
+
+void InspectorAccessibilityAgent::populateRelatives(
+    AXObject& axObject,
+    AXObject* inspectedAXObject,
+    AXNode& nodeObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  AXObject* parentObject = axObject.parentObject();
+  if (parentObject && parentObject != inspectedAXObject) {
+    // Use unignored parent unless parent is inspected ignored object.
+    parentObject = axObject.parentObjectUnignored();
+  }
+  if (parentObject)
+    nodeObject.setParentId(String::number(parentObject->axObjectID()));
+
+  std::unique_ptr<protocol::Array<AXNodeId>> childIds =
+      protocol::Array<AXNodeId>::create();
+  if (!inspectedAXObject) {
+    // If there is no AXObject for the inspected node, the caller will populate
+    // its relatives.
     return;
   }
-  std::unique_ptr<ScopedAXObjectCache> scopedCache =
-      ScopedAXObjectCache::create(document);
-  AXObjectCacheImpl* cache = toAXObjectCacheImpl(scopedCache->get());
-  AXObject* axObject = cache->getOrCreate(node);
-  *nodes = protocol::Array<protocol::Accessibility::AXNode>::create();
-  if (!axObject || axObject->accessibilityIsIgnored()) {
-    (*nodes)->addItem(buildObjectForIgnoredNode(node, axObject));
+
+  if (&axObject == inspectedAXObject->parentObjectUnignored() &&
+      inspectedAXObject->accessibilityIsIgnored()) {
+    // This is the parent of an ignored object, so search for its siblings.
+    addSiblingsOfIgnored(childIds, axObject, inspectedAXObject, nodes, cache);
   } else {
-    (*nodes)->addItem(buildProtocolAXObject(axObject));
+    addChildren(axObject, *inspectedAXObject, childIds, nodes, cache);
   }
+  nodeObject.setChildIds(std::move(childIds));
+}
+
+void InspectorAccessibilityAgent::addSiblingsOfIgnored(
+    std::unique_ptr<protocol::Array<AXNodeId>>& childIds,
+    AXObject& parentAXObject,
+    AXObject* inspectedAXObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  for (AXObject* childAXObject = parentAXObject.rawFirstChild(); childAXObject;
+       childAXObject = childAXObject->rawNextSibling()) {
+    if (childAXObject != inspectedAXObject &&
+        childAXObject->accessibilityIsIgnored()) {
+      if (isAncestorOf(childAXObject, inspectedAXObject)) {
+        addSiblingsOfIgnored(childIds, *childAXObject, inspectedAXObject, nodes,
+                             cache);
+        continue;
+      }
+      const AXObject::AXObjectVector& indirectChildren =
+          childAXObject->children();
+      for (unsigned i = 0; i < indirectChildren.size(); ++i) {
+        addChild(childIds, *(indirectChildren[i]), inspectedAXObject, nodes,
+                 cache);
+      }
+      continue;
+    }
+    addChild(childIds, *childAXObject, inspectedAXObject, nodes, cache);
+  }
+}
+
+void InspectorAccessibilityAgent::addChild(
+    std::unique_ptr<protocol::Array<AXNodeId>>& childIds,
+    AXObject& childAXObject,
+    AXObject* inspectedAXObject,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  childIds->addItem(String::number(childAXObject.axObjectID()));
+  if (&childAXObject == inspectedAXObject)
+    return;
+  nodes->addItem(
+      buildProtocolAXObject(childAXObject, inspectedAXObject, nodes, cache));
+}
 
-  if (fetchAncestors && axObject) {
-    AXObject* parent = axObject->parentObjectUnignored();
-    while (parent) {
-      (*nodes)->addItem(buildProtocolAXObject(parent));
-      parent = parent->parentObjectUnignored();
+void InspectorAccessibilityAgent::addChildren(
+    AXObject& axObject,
+    AXObject& inspectedAXObject,
+    std::unique_ptr<protocol::Array<AXNodeId>>& childIds,
+    std::unique_ptr<protocol::Array<AXNode>>& nodes,
+    AXObjectCacheImpl& cache) const {
+  const AXObject::AXObjectVector& children = axObject.children();
+  for (unsigned i = 0; i < children.size(); i++) {
+    AXObject& childAXObject = *children[i].get();
+    childIds->addItem(String::number(childAXObject.axObjectID()));
+    if (&axObject == &inspectedAXObject ||
+        &axObject == inspectedAXObject.parentObjectUnignored()) {
+      // Only add children/siblings of inspected node to returned nodes.
+      nodes->addItem(buildProtocolAXObject(childAXObject, &inspectedAXObject,
+                                           nodes, cache));
     }
   }
 }