Bug 218316

<rdar://problem/70787809>

Note that this fix will require a WebKit and a VoiceOver change. The VoiceOver bug will not have visibility on this website

Created attachment 414697 [details]
patch

(In reply to chris fleizach from comment #3)
> Created attachment 414697 [details]
> patch

Why a Mac only test since seems you are adding it for iOS too?

(In reply to Andres Gonzalez from comment #4)
> (In reply to chris fleizach from comment #3)
> > Created attachment 414697 [details]
> > patch
> 
> Why a Mac only test since seems you are adding it for iOS too?

Will add one

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
@@ -2736,6 +2736,11 @@ bool AccessibilityObject::supportsRowCountChange() const
     }
 }

+bool AccessibilityObject::treeItemSupportsCheckedState() const
+{
+    return roleValue() == AccessibilityRole::TreeItem && hasAttribute(aria_checkedAttr);
+}
+

Shouldn't we add this to the AccessibilityTreeItem::supportsCheckedState instead of adding a new method to the interface?

(In reply to Andres Gonzalez from comment #6)
> --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> @@ -2736,6 +2736,11 @@ bool AccessibilityObject::supportsRowCountChange()
> const
>      }
>  }
> 
> +bool AccessibilityObject::treeItemSupportsCheckedState() const
> +{
> +    return roleValue() == AccessibilityRole::TreeItem &&
> hasAttribute(aria_checkedAttr);
> +}
> +
> 
> Shouldn't we add this to the AccessibilityTreeItem::supportsCheckedState
> instead of adding a new method to the interface?

Yes we can do that, I just get nervous about methods only in one class which sometimes causes confusion when we do a bad cast

(In reply to chris fleizach from comment #7)
> (In reply to Andres Gonzalez from comment #6)
> > --- a/Source/WebCore/accessibility/AccessibilityObject.cpp
> > +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp
> > @@ -2736,6 +2736,11 @@ bool AccessibilityObject::supportsRowCountChange()
> > const
> >      }
> >  }
> > 
> > +bool AccessibilityObject::treeItemSupportsCheckedState() const
> > +{
> > +    return roleValue() == AccessibilityRole::TreeItem &&
> > hasAttribute(aria_checkedAttr);
> > +}
> > +
> > 
> > Shouldn't we add this to the AccessibilityTreeItem::supportsCheckedState
> > instead of adding a new method to the interface?
> 
> Yes we can do that, I just get nervous about methods only in one class which
> sometimes causes confusion when we do a bad cast

It would be much cleaner. The need of casting AccessibilityObjects to its subclasses is greatly diminished because AXCoreObjects are not just AccessibilityObjects any more, but also AXIsolatedObjects. So as we move clients to using AXCoreObject, they cannot cast any more, which is a good thing.

When I said clients above I meant platform code as the client of the core classes.

Created attachment 414755 [details]
patch

Created attachment 414768 [details]
patch

Created attachment 415032 [details]
patch

Created attachment 415068 [details]
Patch

(In reply to chris fleizach from comment #13)
> Created attachment 415068 [details]
> Patch

Looks good. Just a couple of minor issues.

--- a/LayoutTests/accessibility/ios-simulator/checked-status-tree-items.html
+++ a/LayoutTests/accessibility/ios-simulator/checked-status-tree-items.html

+        var treeItemNotChecked = accessibilityController.accessibleElementById("treeitem_notchecked");

This is not used and it will be null since there is no element with that id attribute.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedObject.h

+    bool supportsCheckedState() const override { return boolAttributeValue(AXPropertyName::SupportsCheckedState); };

Extra ; at the end of the line.

Created attachment 415089 [details]
patch

Thanks Andres -- addressed.

Committed r270333: <https://trac.webkit.org/changeset/270333>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415089 [details].