Bug 203863 - Implement AXIsolatedTreeNode::roleDescription.
Summary: Implement AXIsolatedTreeNode::roleDescription.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-05 14:08 PST by Andres Gonzalez
Modified: 2019-11-07 05:22 PST (History)
10 users (show)

See Also:


Attachments
Patch (52.00 KB, patch)
2019-11-05 14:38 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (61.67 KB, patch)
2019-11-06 06:49 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2019-11-05 14:08:44 PST
Implement AXIsolatedTreeNode::roleDescription.
Comment 1 Andres Gonzalez 2019-11-05 14:38:45 PST
Created attachment 382852 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-11-05 23:24:55 PST
<rdar://problem/56934435>
Comment 3 chris fleizach 2019-11-05 23:25:02 PST
../../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
Comment 4 Radar WebKit Bug Importer 2019-11-05 23:25:07 PST
<rdar://problem/56934437>
Comment 5 chris fleizach 2019-11-05 23:25:20 PST
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
Comment 6 Radar WebKit Bug Importer 2019-11-05 23:25:32 PST
<rdar://problem/56934443>
Comment 7 chris fleizach 2019-11-05 23:36:04 PST
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
Comment 8 Andres Gonzalez 2019-11-06 06:49:39 PST
Created attachment 382924 [details]
Patch
Comment 9 Andres Gonzalez 2019-11-06 07:17:12 PST
(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!
Comment 10 WebKit Commit Bot 2019-11-07 05:22:06 PST
Comment on attachment 382924 [details]
Patch

Clearing flags on attachment: 382924

Committed r252181: <https://trac.webkit.org/changeset/252181>
Comment 11 WebKit Commit Bot 2019-11-07 05:22:08 PST
All reviewed patches have been landed.  Closing bug.