Bug 235414 – AX: Move Mac subrole logic to new subrolePlatformString AXCoreObject interface method

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Tyler Wilcock]

This causes string comparisons to fail unexpectedly not match here:

    if (backingObject->isStyleFormatGroup()) {
        using namespace HTMLNames;
        auto tagName = backingObject->tagName();
        if (tagName == kbdTag)
            return @"AXKeyboardInputStyleGroup";
        if (tagName == codeTag)
            return @"AXCodeStyleGroup";
        if (tagName == preTag)
            return @"AXPreformattedStyleGroup";
        if (tagName == sampTag)
            return @"AXSampleStyleGroup";
        if (tagName == varTag)
            return @"AXVariableStyleGroup";
        if (tagName == citeTag)
            return @"AXCiteStyleGroup";
        ASSERT_NOT_REACHED();
    }

[Radar WebKit Bug Importer]

<rdar://problem/87842864>

[Tyler Wilcock]

Created attachment 449601 [details]
Patch

[Tyler Wilcock]

Created attachment 449605 [details]
Patch

[chris fleizach]

Comment on attachment 449605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449605&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2020
> +        return Accessibility::retrieveValueFromMainThread<String>([protectedSelf = retainPtr(self)] () -> String {

should we cache this rather than hitting main thread?

[Tyler Wilcock]

Created attachment 449638 [details]
Patch

[Tyler Wilcock]

Created attachment 449639 [details]
Patch

[chris fleizach]

Comment on attachment 449639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449639&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1807
> +        && (!backingObject->isModel() || backingObject->modelElementChildren().isEmpty())

I think even if the model is empty we want to return the special subrole so that VO can land on it directly and know that it's a model

[Tyler Wilcock]

Created attachment 449658 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #7)
> Comment on attachment 449639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449639&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1807
> > +        && (!backingObject->isModel() || backingObject->modelElementChildren().isEmpty())
> 
> I think even if the model is empty we want to return the special subrole so
> that VO can land on it directly and know that it's a model
Fixed.

[Andres Gonzalez]

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

--- a/Source/WebCore/accessibility/AccessibilityObject.cpp
+++ a/Source/WebCore/accessibility/AccessibilityObject.cpp

+String AccessibilityObject::subrolePlatformString() const
+{
+    return String();
+}
+
I think we do these hallowed implementations as inlines in the header.

--- a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm
+++ a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm

+    setProperty(AXPropertyName::SubrolePlatformString, object.subrolePlatformString().isolatedCopy());

We can do this in the cross-platform initializeData since it is the same for all platforms.

--- a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
+++ a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm

+String AccessibilityObject::subrolePlatformString() const
+{

This can not be implemented twice in AccessibilityObject.cpp and Mac.mm.

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm

+    if (subrole != String())
+        return subrole;

Change to if (!subrole.isEmpty())

[Tyler Wilcock]

Created attachment 449684 [details]
Patch

[Tyler Wilcock]

> +String AccessibilityObject::subrolePlatformString() const
> +{
> +    return String();
> +}
> +
> I think we do these hallowed implementations as inlines in the header.
> --- a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
> +++ a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
> 
> +String AccessibilityObject::subrolePlatformString() const
> +{
> 
> This can not be implemented twice in AccessibilityObject.cpp and Mac.mm.
The patch diff doesn't show enough context for you to have seen this, but it's not implemented twice. I added the hollow implementation of subrolePlatformString to the .cpp source file to match how we do it for roles, as that is already wrapped in an #if !PLATFORM(MAC) guard:

#if !PLATFORM(MAC)
String AccessibilityObject::rolePlatformString() const
{
    return Accessibility::roleToPlatformString(roleValue());
}

String AccessibilityObject::rolePlatformDescription() const
{
    // FIXME: implement in other platforms.
    return String();
}

String AccessibilityObject::subrolePlatformString() const
{
    return String();
}
#endif

Then we provide the real implementation in AccessibilityObjectMac.mm.

I can still change it if you'd like, though.

Fixed your other two comments, thanks!

[Tyler Wilcock]

Created attachment 449686 [details]
Patch

[EWS]

Committed r288390 (246287@main): <https://commits.webkit.org/246287@main>

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