RESOLVED FIXED 203863
Implement AXIsolatedTreeNode::roleDescription.
https://bugs.webkit.org/show_bug.cgi?id=203863
Summary Implement AXIsolatedTreeNode::roleDescription.
Andres Gonzalez
Reported 2019-11-05 14:08:44 PST
Implement AXIsolatedTreeNode::roleDescription.
Attachments
Patch (52.00 KB, patch)
2019-11-05 14:38 PST, Andres Gonzalez
no flags
Patch (61.67 KB, patch)
2019-11-06 06:49 PST, Andres Gonzalez
no flags
Andres Gonzalez
Comment 1 2019-11-05 14:38:45 PST
Radar WebKit Bug Importer
Comment 2 2019-11-05 23:24:55 PST
chris fleizach
Comment 3 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
Radar WebKit Bug Importer
Comment 4 2019-11-05 23:25:07 PST
chris fleizach
Comment 5 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
Radar WebKit Bug Importer
Comment 6 2019-11-05 23:25:32 PST
chris fleizach
Comment 7 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
Andres Gonzalez
Comment 8 2019-11-06 06:49:39 PST
Andres Gonzalez
Comment 9 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!
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2019-11-07 05:22:08 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.