Bug 203863 – Implement AXIsolatedTreeNode::roleDescription.

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

[Andres Gonzalez]

Implement AXIsolatedTreeNode::roleDescription.

[Andres Gonzalez]

Created attachment 382852 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56934435>

[chris fleizach]

../../Source/WebCore/accessibility/AccessibilityObject.cpp: In member function ‘virtual WTF::String WebCore::AccessibilityObject::ariaLandmarkRoleDescription() const’:
../../Source/WebCore/accessibility/AccessibilityObject.cpp:2622:16: error: ‘AXARIAContentGroupText’ was not declared in this scope

[Radar WebKit Bug Importer]

<rdar://problem/56934437>

[chris fleizach]

C:\Buildbot\WinCairo-EWS\build\Source\WebCore\accessibility/AccessibilityObject.cpp(2657): error C3861: 'AXARIAContentGroupText': identifier not found
C:\Buildbot\WinCairo-EWS\build\Source\WebCore\accessibility/AccessibilityObject.cpp(2659): error C3861: 'AXARIAContentGroupText': identifier not found
C:\Buildbot\WinCairo-EWS\build\Source\WebCore\accessibility/AccessibilityObject.cpp(2661): error C3861: 'AXARIAContentGroupText': identifier not found

[Radar WebKit Bug Importer]

<rdar://problem/56934443>

[chris fleizach]

Comment on attachment 382852 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        platforms are pending.

add new line after this sentence

> Source/WebCore/accessibility/AccessibilityObject.h:836
> +using PlatformRoleMap = HashMap<AccessibilityRole, String, DefaultHash<int>::Hash, WTF::UnsignedWithZeroKeyHashTraits<int>>;

should this be DefaultHash <int> or unsigned?

> Source/WebCore/accessibility/AccessibilityObject.h:838
> +PlatformRoleMap createPlatformRoleMap();

it doesn't seem like createPlatformRoleMap should be in the .h. It's meant to be created the first time roleToPlatformString is accessed

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:155
> +    NSString* axRole = rolePlatformString();

NSString *axRole
(pointer on other side)

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:264
> +        __unsafe_unretained NSString *string;

this file isn't ARC, so I don't think we need unsafe_unretained. that is the default behavior

> Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:413
> +        roleMap.add(static_cast<int>(role.value), (__bridge CFStringRef)role.string);

do we need this cast (__bridge CFStringRef)
maybe we just need a NSString * cast do get rid of the const NSString *

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2040
> +    NSString* string = m_object->rolePlatformString();

* on wrong side

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2270
> +    const String roleDescription = m_object->roleDescription();

the const seems unnecessary here

[Andres Gonzalez]

Created attachment 382924 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> Comment on attachment 382852 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382852&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        platforms are pending.
> 
> add new line after this sentence

Done.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:836
> > +using PlatformRoleMap = HashMap<AccessibilityRole, String, DefaultHash<int>::Hash, WTF::UnsignedWithZeroKeyHashTraits<int>>;
> 
> should this be DefaultHash <int> or unsigned?

Changed it to unsigned. Other existing RoleMap is using int and thus I thought to leave it as is, but we probably come back and change the other too.
 > 
> > Source/WebCore/accessibility/AccessibilityObject.h:838
> > +PlatformRoleMap createPlatformRoleMap();
> 
> it doesn't seem like createPlatformRoleMap should be in the .h. It's meant
> to be created the first time roleToPlatformString is accessed

The prototype has to be visible to both the core .cpp and the platform overrides, the .mm on Mac. So the options were to duplicate the prototype or to put it in a common header. Eventually, some of these methods should be moved to their own header file. I would leave it here for now unless you feel strongly about moving it somewhere else.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:155
> > +    NSString* axRole = rolePlatformString();
> 
> NSString *axRole
> (pointer on other side)

Done.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:264
> > +        __unsafe_unretained NSString *string;
> 
> this file isn't ARC, so I don't think we need unsafe_unretained. that is the
> default behavior

Done.
> 
> > Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm:413
> > +        roleMap.add(static_cast<int>(role.value), (__bridge CFStringRef)role.string);
> 
> do we need this cast (__bridge CFStringRef)
> maybe we just need a NSString * cast do get rid of the const NSString *

Got rid of it.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2040
> > +    NSString* string = m_object->rolePlatformString();
> 
> * on wrong side

Done.
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2270
> > +    const String roleDescription = m_object->roleDescription();
> 
> the const seems unnecessary here

Got rid of it.

Thanks!

[WebKit Commit Bot]

Comment on attachment 382924 [details]
Patch

Clearing flags on attachment: 382924

Committed r252181: <https://trac.webkit.org/changeset/252181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.