Bug 262565 – AX: AXPropertyName::SupportsCurrent and AXPropertyName::SupportsKeyShortcuts are needlessly cached

• Component: Accessibility
• Status: NEW

[Tyler Wilcock]

We can compute these from the underlying value states (AXCoreObject::currentState and AXCoreObject::keyShortcuts()).

[Radar WebKit Bug Importer]

<rdar://problem/116416604>

[Tyler Wilcock]

Created attachment 468050 [details]
Patch

[Tyler Wilcock]

Created attachment 468056 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 468056 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXCoreObject.cpp b/Source/WebCore/accessibility/AXCoreObject.cpp
index cb731034e3c2..1a093d879201 100644
--- a/Source/WebCore/accessibility/AXCoreObject.cpp
+++ b/Source/WebCore/accessibility/AXCoreObject.cpp
@@ -383,4 +383,14 @@ AXCoreObject::AccessibilityChildrenVector AXCoreObject::selectedCells()
     return selectedCells;
 }

+bool AXCoreObject::supportsKeyShortcuts() const
+{
+    return !keyShortcuts().isEmpty();
+}
+
+bool AXCoreObject::supportsCurrent() const
+{
+    return currentState() != AccessibilityCurrentState::False;
+}
+

AG: The idea of having a supportsXXX() in the interface is that it is a cheaper call than calling the get the property method. Thus, it makes no sense to keep these if they are going to call the get property method. Otherwise the clients are likely to call the get property twice.

 } // namespace WebCore
diff --git a/Source/WebCore/accessibility/AXCoreObject.h b/Source/WebCore/accessibility/AXCoreObject.h
index 0c2df24ba1b2..0033bf27ca4c 100644
--- a/Source/WebCore/accessibility/AXCoreObject.h
+++ b/Source/WebCore/accessibility/AXCoreObject.h
@@ -1062,16 +1062,15 @@ public:
     virtual bool pressedIsPresent() const = 0;
     virtual String invalidStatus() const = 0;
     virtual bool supportsExpanded() const = 0;
-    virtual bool supportsChecked() const = 0;
     virtual AccessibilitySortDirection sortDirection() const = 0;
     virtual bool supportsRangeValue() const = 0;
     virtual String identifierAttribute() const = 0;
     virtual String linkRelValue() const = 0;
     virtual Vector<String> classList() const = 0;
     virtual AccessibilityCurrentState currentState() const = 0;
-    virtual bool supportsCurrent() const = 0;
+    bool supportsCurrent() const;
     String currentValue() const;
-    virtual bool supportsKeyShortcuts() const = 0;
+    bool supportsKeyShortcuts() const;
     virtual String keyShortcuts() const = 0;

     virtual bool isModalNode() const = 0;
diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
index 1922e7bbd960..6fbc81a29038 100644
--- a/Source/WebCore/accessibility/AXObjectCache.cpp
+++ b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -4105,6 +4105,7 @@ void AXObjectCache::updateIsolatedTree(const Vector<std::pair<RefPtr<Accessibili
             break;
         case AXCheckedStateChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::IsChecked);
+            tree->updateNodeProperty(*notification.first, AXPropertyName::SupportsCheckedState);

AG: it would be good to keep updating relating properties in one place. I think we do most of them in the AXIsolatedTree::updateXXX. I would do this in there instead of in the AXObjectCache.

             break;
         case AXCurrentStateChanged:
             tree->updateNodeProperty(*notification.first, AXPropertyName::CurrentState);
diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp
index 7d0f55b40908..792a050e91b3 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ b/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -1916,10 +1916,12 @@ bool AccessibilityObject::supportsCheckedState() const
 {
     auto role = roleValue();
     return isCheckboxOrRadio()
+#if !USE(ATSPI)
+    || isToggleButton()
+#endif
     || role == AccessibilityRole::MenuItemCheckbox
     || role == AccessibilityRole::MenuItemRadio
-    || role == AccessibilityRole::Switch
-    || isToggleButton();
+    || role == AccessibilityRole::Switch;
 }

 bool AccessibilityObject::supportsAutoComplete() const
@@ -2358,11 +2360,6 @@ String AccessibilityObject::invalidStatus() const
     return trueValue;
 }

-bool AccessibilityObject::supportsCurrent() const
-{
-    return hasAttribute(aria_currentAttr);
-}
-
 AccessibilityCurrentState AccessibilityObject::currentState() const
 {
     // aria-current can return false (default), true, page, step, location, date or time.
@@ -2940,11 +2937,6 @@ bool AccessibilityObject::isInlineText() const
     return is<RenderInline>(renderer());
 }

-bool AccessibilityObject::supportsKeyShortcuts() const
-{
-    return hasAttribute(aria_keyshortcutsAttr);
-}
-
 String AccessibilityObject::keyShortcuts() const
 {
     return getAttribute(aria_keyshortcutsAttr);
@@ -3414,20 +3406,6 @@ bool AccessibilityObject::isExpanded() const
     return false;
 }

-bool AccessibilityObject::supportsChecked() const
-{
-    switch (roleValue()) {
-    case AccessibilityRole::CheckBox:
-    case AccessibilityRole::MenuItemCheckbox:
-    case AccessibilityRole::MenuItemRadio:
-    case AccessibilityRole::RadioButton:
-    case AccessibilityRole::Switch:
-        return true;
-    default:
-        return false;
-    }
-}
-
 bool AccessibilityObject::supportsRowCountChange() const
 {
     switch (roleValue()) {
diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h
index d29dbcbb23d8..7f5dcc3ba8ad 100644
--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h
@@ -294,7 +294,6 @@ public:
     String invalidStatus() const override;
     bool supportsPressed() const;
     bool supportsExpanded() const override;
-    bool supportsChecked() const override;
     bool supportsRowCountChange() const;
     AccessibilitySortDirection sortDirection() const override;
     virtual bool canvasHasFallbackContent() const { return false; }
@@ -303,8 +302,6 @@ public:
     String linkRelValue() const override;
     Vector<String> classList() const override;
     AccessibilityCurrentState currentState() const override;
-    bool supportsCurrent() const override;
-    bool supportsKeyShortcuts() const override;
     String keyShortcuts() const override;

     // This function checks if the object should be ignored when there's a modal dialog displayed.
diff --git a/Source/WebCore/accessibility/AccessibilityTreeItem.h b/Source/WebCore/accessibility/AccessibilityTreeItem.h
index 084e2f1cf6cf..ee811408823d 100644
--- a/Source/WebCore/accessibility/AccessibilityTreeItem.h
+++ b/Source/WebCore/accessibility/AccessibilityTreeItem.h
@@ -38,7 +38,7 @@ public:
     static Ref<AccessibilityTreeItem> create(Node&);
     virtual ~AccessibilityTreeItem();

-    bool supportsCheckedState() const override;
+    bool supportsCheckedState() const final;

 private:
     explicit AccessibilityTreeItem(RenderObject*);
diff --git a/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp b/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
index 56a77358108f..33a15472fe68 100644
--- a/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
+++ b/Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp
@@ -773,7 +773,7 @@ OptionSet<Atspi::State> AccessibilityObjectAtspi::states() const
     }

     if (m_coreObject->canSetValueAttribute()) {
-        if (m_coreObject->supportsChecked())
+        if (m_coreObject->supportsCheckedState())
             states.add(Atspi::State::Checkable);

         if (m_coreObject->isTextControl() || m_coreObject->isNonNativeTextControl())
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
index 90d86087e862..ec321ac2a692 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.cpp
@@ -131,8 +131,6 @@ void AXIsolatedObject::initializeProperties(const Ref<AccessibilityObject>& axOb
     setProperty(AXPropertyName::IdentifierAttribute, object.identifierAttribute().isolatedCopy());
 #endif
     setProperty(AXPropertyName::CurrentState, static_cast<int>(object.currentState()));
-    setProperty(AXPropertyName::SupportsCurrent, object.supportsCurrent());
-    setProperty(AXPropertyName::SupportsKeyShortcuts, object.supportsKeyShortcuts());
     setProperty(AXPropertyName::KeyShortcuts, object.keyShortcuts().isolatedCopy());
     setProperty(AXPropertyName::SupportsSetSize, object.supportsSetSize());
     setProperty(AXPropertyName::SupportsPath, object.supportsPath());
@@ -1659,12 +1657,6 @@ bool AXIsolatedObject::supportsHasPopup() const
     return false;
 }

-bool AXIsolatedObject::supportsChecked() const
-{
-    ASSERT_NOT_REACHED();
-    return false;
-}
-
 bool AXIsolatedObject::isModalNode() const
 {
     ASSERT_NOT_REACHED();
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
index 47d3e7c6d7c4..25df5bbe6b9d 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
@@ -226,8 +226,6 @@ private:
     String linkRelValue() const override;
     Vector<String> classList() const override;
     AccessibilityCurrentState currentState() const override { return static_cast<AccessibilityCurrentState>(intAttributeValue(AXPropertyName::CurrentState)); }
-    bool supportsCurrent() const override { return boolAttributeValue(AXPropertyName::SupportsCurrent); }
-    bool supportsKeyShortcuts() const override { return boolAttributeValue(AXPropertyName::SupportsKeyShortcuts); }
     String keyShortcuts() const override { return stringAttributeValue(AXPropertyName::KeyShortcuts); }
     bool supportsSetSize() const override { return boolAttributeValue(AXPropertyName::SupportsSetSize); }
     bool supportsPosInSet() const override { return boolAttributeValue(AXPropertyName::SupportsPosInSet); }
@@ -464,7 +462,6 @@ private:

     bool supportsHasPopup() const override;
     bool supportsPressAction() const final { return boolAttributeValue(AXPropertyName::SupportsPressAction); }
-    bool supportsChecked() const override;
     bool isModalNode() const override;
     AXCoreObject* elementAccessibilityHitTest(const IntPoint&) const override;
     bool isDescendantOfRole(AccessibilityRole) const override;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
index 2f62a847ef15..b5b92cc93455 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
@@ -596,7 +596,6 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
             propertyMap.set(AXPropertyName::SortDirection, static_cast<int>(axObject.sortDirection()));
             break;
         case AXPropertyName::KeyShortcuts:
-            propertyMap.set(AXPropertyName::SupportsKeyShortcuts, axObject.supportsKeyShortcuts());
             propertyMap.set(AXPropertyName::KeyShortcuts, axObject.keyShortcuts().isolatedCopy());
             break;
         case AXPropertyName::SelectedChildren:
@@ -605,6 +604,9 @@ void AXIsolatedTree::updateNodeProperties(AXCoreObject& axObject, const Vector<A
         case AXPropertyName::SupportsARIAOwns:
             propertyMap.set(AXPropertyName::SupportsARIAOwns, axObject.supportsARIAOwns());
             break;
+        case AXPropertyName::SupportsCheckedState:
+            propertyMap.set(AXPropertyName::SupportsCheckedState, axObject.supportsCheckedState());
+            break;
         case AXPropertyName::SupportsExpanded:
             propertyMap.set(AXPropertyName::SupportsExpanded, axObject.supportsExpanded());
             break;
diff --git a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
index 3d91e290da4e..47b437f55617 100644
--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ b/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
@@ -211,11 +211,9 @@ enum class AXPropertyName : uint16_t {
     SupportsDropping,
     SupportsARIAOwns,
     SupportsCheckedState,
-    SupportsCurrent,
     SupportsDatetimeAttribute,
     SupportsExpanded,
     SupportsExpandedTextValue,
-    SupportsKeyShortcuts,
     SupportsPath,
     SupportsPosInSet,
     SupportsPressAction,
diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index 2ca9d7940a1b..f51677ff8ebc 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -869,7 +869,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN
 - (NSArray*)accessibilityAttributeNames
 ALLOW_DEPRECATED_IMPLEMENTATIONS_END
 {
-    RefPtr<AXCoreObject> backingObject = self.axBackingObject;
+    RefPtr<AXCoreObject> backingObject = self.updateObjectBackingStore;

AG: this may have a perf impact.

     if (!backingObject)
         return nil;

@@ -1140,6 +1140,10 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         [tempArray addObject:NSAccessibilityDisclosedByRowAttribute];
         [tempArray addObject:NSAccessibilityDisclosureLevelAttribute];
         [tempArray addObject:NSAccessibilityDisclosedRowsAttribute];
+        return tempArray;
+    }();
+    static NeverDestroyed outlineRowNoValueAttrs = [] {
+        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:outlineRowAttrs.get().get()]);
         [tempArray removeObject:NSAccessibilityValueAttribute];
         return tempArray;
     }();

AG: the naming and logic here is hard to follow. What about outlineRowAttrs and outlineCheckableRowAttrs, the latter being the former + value attribute.

@@ -1192,9 +1196,9 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         objectAttributes = outlineAttrs.get().get();
     else if (backingObject->isTreeItem()) {
         if (backingObject->supportsCheckedState())
-            objectAttributes = [outlineRowAttrs.get() arrayByAddingObject:NSAccessibilityValueAttribute];
-        else
             objectAttributes = outlineRowAttrs.get().get();
+        else
+            objectAttributes = outlineRowNoValueAttrs.get().get();
     }
     else if (backingObject->isListBox())
         objectAttributes = listBoxAttrs.get().get();
@@ -1240,7 +1244,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
         objectAttributes = [objectAttributes arrayByAddingObjectsFromArray:additionalAttributes];

     // Only expose AXARIACurrent attribute when the element is set to be current item.
-    if (backingObject->currentState() != AccessibilityCurrentState::False)
+    if (backingObject->supportsCurrent())
         objectAttributes = [objectAttributes arrayByAddingObjectsFromArray:@[ NSAccessibilityARIACurrentAttribute ]];

     // AppKit needs to know the screen height in order to do the coordinate conversion.

[Tyler Wilcock]

Created attachment 468067 [details]
Patch

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #4)
> (In reply to Tyler Wilcock from comment #3)
> > Created attachment 468056 [details]
> > Patch
> 
> diff --git a/Source/WebCore/accessibility/AXCoreObject.cpp
> b/Source/WebCore/accessibility/AXCoreObject.cpp
> index cb731034e3c2..1a093d879201 100644
> --- a/Source/WebCore/accessibility/AXCoreObject.cpp
> +++ b/Source/WebCore/accessibility/AXCoreObject.cpp
> @@ -383,4 +383,14 @@ AXCoreObject::AccessibilityChildrenVector
> AXCoreObject::selectedCells()
>      return selectedCells;
>  }
> 
> +bool AXCoreObject::supportsKeyShortcuts() const
> +{
> +    return !keyShortcuts().isEmpty();
> +}
> +
> +bool AXCoreObject::supportsCurrent() const
> +{
> +    return currentState() != AccessibilityCurrentState::False;
> +}
> +
> 
> AG: The idea of having a supportsXXX() in the interface is that it is a
> cheaper call than calling the get the property method. Thus, it makes no
> sense to keep these if they are going to call the get property method.
> Otherwise the clients are likely to call the get property twice.
What do you propose be changed?

> --- a/Source/WebCore/accessibility/AXObjectCache.cpp
> +++ b/Source/WebCore/accessibility/AXObjectCache.cpp
> @@ -4105,6 +4105,7 @@ void AXObjectCache::updateIsolatedTree(const
> Vector<std::pair<RefPtr<Accessibili
>              break;
>          case AXCheckedStateChanged:
>              tree->updateNodeProperty(*notification.first,
> AXPropertyName::IsChecked);
> +            tree->updateNodeProperty(*notification.first,
> AXPropertyName::SupportsCheckedState);
> 
> AG: it would be good to keep updating relating properties in one place. I
> think we do most of them in the AXIsolatedTree::updateXXX. I would do this
> in there instead of in the AXObjectCache.
That feels wrong to me. SupportsCheckedState changes because aria-checked changed (which posts AXCheckedStateChanged), not because of some inherent relation to IsChecked. But if you feels strongly about it then I'll move it.

> a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> index 2ca9d7940a1b..f51677ff8ebc 100644
> --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> +++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
> @@ -869,7 +869,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_BEGIN
>  - (NSArray*)accessibilityAttributeNames
>  ALLOW_DEPRECATED_IMPLEMENTATIONS_END
>  {
> -    RefPtr<AXCoreObject> backingObject = self.axBackingObject;
> +    RefPtr<AXCoreObject> backingObject = self.updateObjectBackingStore;
> 
> AG: this may have a perf impact.
It is necessary to fix the bug. Otherwise we could (and currently do prior to this patch) serve stale information, since some things in this function depend on properties being up-to-date. Do you have a suggested alternative?
 
> @@ -1140,6 +1140,10 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
>          [tempArray addObject:NSAccessibilityDisclosedByRowAttribute];
>          [tempArray addObject:NSAccessibilityDisclosureLevelAttribute];
>          [tempArray addObject:NSAccessibilityDisclosedRowsAttribute];
> +        return tempArray;
> +    }();
> +    static NeverDestroyed outlineRowNoValueAttrs = [] {
> +        auto tempArray = adoptNS([[NSMutableArray alloc]
> initWithArray:outlineRowAttrs.get().get()]);
>          [tempArray removeObject:NSAccessibilityValueAttribute];
>          return tempArray;
>      }();
> 
> AG: the naming and logic here is hard to follow. What about outlineRowAttrs
> and outlineCheckableRowAttrs, the latter being the former + value attribute.
Fixed.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> (In reply to Andres Gonzalez from comment #4)
> > (In reply to Tyler Wilcock from comment #3)
> > > Created attachment 468056 [details]
> > > Patch
> > 
> > diff --git a/Source/WebCore/accessibility/AXCoreObject.cpp
> > b/Source/WebCore/accessibility/AXCoreObject.cpp
> > index cb731034e3c2..1a093d879201 100644
> > --- a/Source/WebCore/accessibility/AXCoreObject.cpp
> > +++ b/Source/WebCore/accessibility/AXCoreObject.cpp
> > @@ -383,4 +383,14 @@ AXCoreObject::AccessibilityChildrenVector
> > AXCoreObject::selectedCells()
> >      return selectedCells;
> >  }
> > 
> > +bool AXCoreObject::supportsKeyShortcuts() const
> > +{
> > +    return !keyShortcuts().isEmpty();
> > +}
> > +
> > +bool AXCoreObject::supportsCurrent() const
> > +{
> > +    return currentState() != AccessibilityCurrentState::False;
> > +}
> > +
> > 
> > AG: The idea of having a supportsXXX() in the interface is that it is a
> > cheaper call than calling the get the property method. Thus, it makes no
> > sense to keep these if they are going to call the get property method.
> > Otherwise the clients are likely to call the get property twice.
> What do you propose be changed?

I propose we remove them from the interface, and have the caller check the return value of the corresponding method that actually returns the property.