Boolean properties for Accessibility Object Model Phase 1

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

 /*
  * Copyright (C) 2008, 2009, 2011 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 371 matching lines @@
 
 const AtomicString& AXObject::getAOMPropertyOrARIAAttribute(
     AOMStringProperty property) const {
   Node* node = this->getNode();
   if (!node || !node->isElementNode())
     return nullAtom;
 
   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 ||
          ariaRoleAttribute() == ComboBoxRole;
 }
 
 bool AXObject::isButton() const {
   AccessibilityRole role = roleValue();
 
   return role == ButtonRole || role == PopUpButtonRole ||
@@ skipping 187 matching lines @@
       return parent;
 
     return parent->leafNodeAncestor();
   }
 
   return 0;
 }
 
 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;
   }
 
   return 0;
 }
 
 bool AXObject::isDescendantOfDisabledNode() const {
   updateCachedAttributeValuesIfNeeded();
   return m_cachedIsDescendantOfDisabledNode;
 }
 
 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() {
   if (m_lastKnownIsIgnoredValue == DefaultBehavior)
     m_lastKnownIsIgnoredValue =
         accessibilityIsIgnored() ? IgnoreObject : IncludeObject;
 
   return m_lastKnownIsIgnoredValue == IgnoreObject;
 }
 
@@ skipping 84 matching lines @@
                                           AXObjectSet& visited) {
   if (visited.contains(&axObj) && !inAriaLabelledByTraversal)
     return String();
 
   AXNameFrom tmpNameFrom;
   return axObj.textAlternative(true, inAriaLabelledByTraversal, visited,
                                tmpNameFrom, nullptr, nullptr);
 }
 
 bool AXObject::isHiddenForTextAlternativeCalculation() const {
-  if (equalIgnoringASCIICase(getAttribute(aria_hiddenAttr), "false"))
+  if (AOMPropertyOrARIAAttributeIsFalse(AOMBooleanProperty::kHidden))
     return false;
 
   if (getLayoutObject())
     return getLayoutObject()->style()->visibility() != EVisibility::kVisible;
 
   // This is an obscure corner case: if a node has no LayoutObject, that means
   // it's not rendered, but we still may be exploring it as part of a text
   // alternative calculation, for example if it was explicitly referenced by
   // aria-labelledby. So we need to explicitly call the style resolver to check
   // whether it's invisible or display:none, rather than relying on the style
@@ skipping 238 matching lines @@
 
   if (isHTMLTextAreaElement(*node))
     return true;
 
   if (hasEditableStyle(*node))
     return true;
 
   if (!isNativeTextControl() && !isNonNativeTextControl())
     return false;
 
-  return equalIgnoringASCIICase(getAttribute(aria_multilineAttr), "true");
+  return AOMPropertyOrARIAAttributeIsTrue(AOMBooleanProperty::kMultiline);
 }
 
 bool AXObject::ariaPressedIsPresent() const {
   return !getAttribute(aria_pressedAttr).isEmpty();
 }
 
 bool AXObject::supportsActiveDescendant() const {
   // According to the ARIA Spec, all ARIA composite widgets, ARIA text boxes
   // and ARIA groups should be able to expose an active descendant.
   // Implicitly, <input> and <textarea> elements should also have this ability.
@@ skipping 791 matching lines @@
 }
 
 DEFINE_TRACE(AXObject) {
   visitor->trace(m_children);
   visitor->trace(m_parent);
   visitor->trace(m_cachedLiveRegionRoot);
   visitor->trace(m_axObjectCache);
 }
 
 }  // namespace blink