Boolean properties for Accessibility Object Model Phase 1

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

Index: third_party/WebKit/Source/modules/accessibility/AXObject.cpp
diff --git a/third_party/WebKit/Source/modules/accessibility/AXObject.cpp b/third_party/WebKit/Source/modules/accessibility/AXObject.cpp
index 3e18e556d466beaf250314eaee1346ca0c2f1955..4a20d1a95c5bb6829f76c6f17284d89e2aebe668 100644
--- a/third_party/WebKit/Source/modules/accessibility/AXObject.cpp
+++ b/third_party/WebKit/Source/modules/accessibility/AXObject.cpp
@@ -389,6 +389,39 @@ const AtomicString& AXObject::getAOMPropertyOrARIAAttribute(
   return AccessibleNode::getProperty(toElement(node), property);
 }
 
+bool AXObject::getAOMPropertyOrARIAAttribute(AOMBooleanProperty property,

[aboxhall, 2017/04/07 05:22:35]

Ok, now I see what you were talking about.

This is incredibly confusing since the form of getAOMPropertyOrARIAAttribute()
directly above returns the value, and this returns a boolean making it _look
like_ it's returning the value, but actually it's effectively returning the
result of hasAOMPropertyOrARIAAttribute. This seems like a hole waiting for
someone to fall in it :)

What about:
- making hasAOMPropertyOrARIAAttribute() work like this currently works (I see
it's not called anywhere currently?) - or maybe call it something like
canGetAOMPropertyOrARIAAttribute() (like "try to get this and let me know if you
got it")
- making a convenience version of hasAOMPropertyOrARIAAttribute() which doesn't
take a `result` reference but delegates to the one which does
- making getAOMPropertyOrARIAAttribute() work like AccessibleNode::getProperty()
(or just getting rid of it altogether and insisting on using the isTrue/isFalse
method)

i.e.
  bool getAOMPropertyOrARIAAttribute(AOMBooleanProperty, bool& isNull) const; 
// returns false if isNull gets set to true
  bool hasAOMPropertyOrARIAAttribute(AOMBooleanProperty, bool& value) const;
  bool hasAOMPropertyOrARIAAttribute(AOMBooleanProperty) const;  // delegates to
above

then the callsites look like

if (hasAOMPropertyOrARIAAttribute(AOMBooleanProperty::kModal, modal))
  return modal;

or

if (canGetAOMPropertyOrARIAAttribute(AOMBooleanProperty::kDisabled, disabled)) {
  if (disabled)
    return this;
  return nullptr;
}

There's still potentially a more minor footgun in that you have to know to check
isNull when calling getAOMPropertyOrARIAAttribute for a boolean attribute, but
this is consistent with the API on AccessibleNode.

[dmazzoni, 2017/04/21 07:11:37]

Thanks for the discussion.

I went with this form:

bool hasAOMPropertyOrARIAAttribute(AOMBooleanProperty, bool& value) const;

I don't think we need the version that just returns whether there's a value or
not, delegating to this
form, because it encourages calling it twice, which is unnecessarily expensive.

+                                             bool& result) const {
+  Node* node = this->getNode();

[aboxhall, 2017/04/07 05:22:35]

Would it help to make a convenience protected/private method
AXObject::GetElement which returns nullptr if node is null or not an element,
and an Element pointer otherwise?

[dmazzoni, 2017/04/21 07:11:37]

Good idea. Used it throughout the file.

+  if (!node || !node->isElementNode())
+    return false;
+
+  bool isNull = true;
+  result = AccessibleNode::getProperty(toElement(node), property, isNull);
+  return !isNull;
+}
+
+bool AXObject::hasAOMPropertyOrARIAAttribute(
+    AOMBooleanProperty property) const {
+  bool dummyResult;
+  return getAOMPropertyOrARIAAttribute(property, dummyResult);
+}
+
+bool AXObject::AOMPropertyOrARIAAttributeIsTrue(
+    AOMBooleanProperty property) const {
+  bool result;
+  if (getAOMPropertyOrARIAAttribute(property, result))
+    return result;
+  return false;
+}
+
+bool AXObject::AOMPropertyOrARIAAttributeIsFalse(
+    AOMBooleanProperty property) const {
+  bool result;
+  if (getAOMPropertyOrARIAAttribute(property, result))
+    return !result;
+  return false;
+}
+
 bool AXObject::isARIATextControl() const {
   return ariaRoleAttribute() == TextFieldRole ||
          ariaRoleAttribute() == SearchBoxRole ||
@@ -596,7 +629,7 @@ AXObject* AXObject::leafNodeAncestor() const {
 
 const AXObject* AXObject::ariaHiddenRoot() const {
   for (const AXObject* object = this; object; object = object->parentObject()) {
-    if (equalIgnoringASCIICase(object->getAttribute(aria_hiddenAttr), "true"))
+    if (object->AOMPropertyOrARIAAttributeIsTrue(AOMBooleanProperty::kHidden))
       return object;
   }
 
@@ -609,16 +642,17 @@ bool AXObject::isDescendantOfDisabledNode() const {
 }
 
 const AXObject* AXObject::disabledAncestor() const {
-  const AtomicString& disabled = getAttribute(aria_disabledAttr);
-  if (equalIgnoringASCIICase(disabled, "true"))
-    return this;
-  if (equalIgnoringASCIICase(disabled, "false"))
-    return 0;
+  bool disabled = false;
+  if (getAOMPropertyOrARIAAttribute(AOMBooleanProperty::kDisabled, disabled)) {
+    if (disabled)
+      return this;
+    return nullptr;
+  }
 
   if (AXObject* parent = parentObject())
     return parent->disabledAncestor();
 
-  return 0;
+  return nullptr;
 }
 
 bool AXObject::lastKnownIsIgnoredValue() {
@@ -723,7 +757,7 @@ String AXObject::recursiveTextAlternative(const AXObject& axObj,
 }
 
 bool AXObject::isHiddenForTextAlternativeCalculation() const {
-  if (equalIgnoringASCIICase(getAttribute(aria_hiddenAttr), "false"))
+  if (AOMPropertyOrARIAAttributeIsFalse(AOMBooleanProperty::kHidden))
     return false;
 
   if (getLayoutObject())
@@ -982,7 +1016,7 @@ bool AXObject::isMultiline() const {
   if (!isNativeTextControl() && !isNonNativeTextControl())
     return false;
 
-  return equalIgnoringASCIICase(getAttribute(aria_multilineAttr), "true");
+  return AOMPropertyOrARIAAttributeIsTrue(AOMBooleanProperty::kMultiline);
 }
 
 bool AXObject::ariaPressedIsPresent() const {