Bug 129527 – AX: Support IOS Accessibility in WK2

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Add in enough support to make this work

[chris fleizach]

<rdar://problem/15062381>

[chris fleizach]

Created attachment 225518 [details]
patch

This patch will need a corresponding change with WKSI, so it will not build until that piece lands

[Sam Weinig]

Comment on attachment 225518 [details]
patch

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

> Source/WebKit2/Platform/IPC/Connection.h:177
> +    // Used by accessibility to know the mach_port.

I don't think this comment adds much.

> Source/WebKit2/Platform/IPC/mac/ConnectionMac.cpp:500
> +IPC::Connection::Identifier Connection::identifier() const
> +{
> +    return Identifier(m_isServer ? m_receivePort : m_sendPort);
> +}

The Identifier should probably also include the xpc_connection if there is one.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:237
> +    CFUUIDRef uuid = CFUUIDCreate(kCFAllocatorDefault);

Please use a RetainPtr<> here.

> Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.h:35
> +#ifndef WKAccessibilityWebPageObjectIOS_h
> +#define WKAccessibilityWebPageObjectIOS_h
> +
> +#include "WKAccessibilityWebPageObjectBase.h"
> +
> +@interface WKAccessibilityWebPageObject : WKAccessibilityWebPageObjectBase
> +
> +@property(nonatomic, retain) NSData *remoteTokenData;
> +
> +@end

You should probably wrap this file in #if PLATFORM(IOS)

> Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:35
> +#import "config.h"
> +#import "WKAccessibilityWebPageObjectIOS.h"
> +
> +#import "WebFrame.h"
> +#import "WebPage.h"
> +#import <WebCore/FrameView.h>
> +
> +/* 
> + The implementation of this class will be augmented by an accesibility bundle that is loaded only when accessibility is requested to be enabled.
> + */

You should probably wrap this file in #if PLATFORM(IOS)

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:34
> +<<<<<<< .mine
> +#import "DataReference.h"
> +=======
>  #if PLATFORM(IOS)
>  
> +>>>>>>> .r164901

I don't think you meant this.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:100
> +    CFUUIDRef uuid = CFUUIDCreate(kCFAllocatorDefault);
> +    NSData *remoteToken = WKAXRemoteToken(uuid);
> +    CFRelease(uuid);

RetainPtr<>.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.h:38
> +#ifndef WKAccessibilityWebPageObjectMac_h
> +#define WKAccessibilityWebPageObjectMac_h
> +
> +#include "WKAccessibilityWebPageObjectBase.h"
> +
> +@interface WKAccessibilityWebPageObject : WKAccessibilityWebPageObjectBase {
> +    NSArray* m_attributeNames;
> +    NSMutableArray* m_accessibilityChildren;
> +}
> +
> +- (NSPoint)convertScreenPointToRootView:(NSPoint)point;
> +
> +@end

This should probably be wrapped in PLATFORM(MAC).

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:65
> +
> +using namespace WebCore;
> +using namespace WebKit;
> +
> +@implementation WKAccessibilityWebPageObject
> +
> +- (void)dealloc
> +{
> +    WKUnregisterUniqueIdForElement(self);
> +    [m_accessibilityChildren release];
> +    [m_attributeNames release];
> +    [m_parent release];
> +    [super dealloc];
> +}
> +
> +- (BOOL)accessibilityIsIgnored
> +{
> +    return NO;
> +}
> +
> +- (NSArray *)accessibilityAttributeNames

This file should probably be wrapped in PLATFORM(MAC).

[chris fleizach]

http://trac.webkit.org/changeset/165014

[Simon Fraser (smfr)]

Comment on attachment 225518 [details]
patch

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

>> Source/WebKit2/UIProcess/ios/WKContentView.mm:237
>> +    CFUUIDRef uuid = CFUUIDCreate(kCFAllocatorDefault);
> 
> Please use a RetainPtr<> here.

Your committed code leaks.

> Source/WebKit2/UIProcess/ios/WKContentView.mm:239
> +    IPC::Connection* connection = _page->process().connection();

This asserts in a debug build. Did you test it?

> Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:39
> +- (id)init

Return instancetype.

> Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:65
> +- (void)dealloc
> +{
> +    [[NSNotificationCenter defaultCenter] removeObserver:self];
> +    self.remoteTokenData = nil;
> +    [super dealloc];
> +}

Please move this up under the -init.

You should not use property syntax in the dealloc method.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:95
> +    m_mockAccessibilityElement = [[[WKAccessibilityWebPageObject alloc] init] autorelease];

m_mockAccessibilityElement = adoptNS([[WKAccessibilityWebPageObject alloc] init]).

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:100
>> +    CFRelease(uuid);
> 
> RetainPtr<>.

Without leaking!

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.h:33
> +    NSArray* m_attributeNames;
> +    NSMutableArray* m_accessibilityChildren;

What is the ownership model for these? Should they be RetainPtr?

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:102
> +    return;

Remove.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:140
> +    if ([attribute isEqualToString:NSAccessibilityParentAttribute])
> +        return m_parent;
> +    if ([attribute isEqualToString:NSAccessibilityWindowAttribute])
> +        return [m_parent accessibilityAttributeValue:NSAccessibilityWindowAttribute];
> +    if ([attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute])
> +        return [m_parent accessibilityAttributeValue:NSAccessibilityTopLevelUIElementAttribute];
> +    if ([attribute isEqualToString:NSAccessibilityRoleAttribute])
> +        return NSAccessibilityGroupRole;
> +    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute])
> +        return NSAccessibilityRoleDescription(NSAccessibilityGroupRole, nil);
> +    if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
> +        return [NSNumber numberWithBool:NO];

Some spaces would make this more readable.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:163
> +    if ([parameter isKindOfClass:[NSValue class]] && strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) == 0) {

Isn't there a better way than strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) ?

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:176
> +        return CFBridgingRelease(WKStringCopyCFString(kCFAllocatorDefault, (WKStringRef)result.get()));
> +    else if (toImpl(result.get())->type() == API::Boolean::APIType)
> +        return [NSNumber numberWithBool:WKBooleanGetValue(static_cast<WKBooleanRef>(result.get()))];

No else after return.

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:187
> +#pragma clang diagnostic push
> +#pragma clang diagnostic ignored "-Wdeprecated-declarations"

Can this be removed at some point?

> Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:210
> +    // Hit-test point comes in as bottom-screen coordinates. Needs to be normalized to the frame of the web page.
> +    NSPoint remotePosition = [[self accessibilityAttributeValue:NSAccessibilityPositionAttribute] pointValue];
> +    NSSize remoteSize = [[self accessibilityAttributeValue:NSAccessibilitySizeAttribute] sizeValue];
> +    
> +    // Get the y position of the WKView (we have to screen-flip and go from bottom left to top left).
> +    CGFloat screenHeight = [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height;
> +    remotePosition.y = (screenHeight - remotePosition.y) - remoteSize.height;
> +    
> +    point.y = screenHeight - point.y;
> +    
> +    // Re-center point into the web page's frame.
> +    point.y -= remotePosition.y;
> +    point.x -= remotePosition.x;
> +    
> +    WebCore::FrameView* frameView = m_page ? m_page->mainFrameView() : 0;
> +    if (frameView) {
> +        point.y += frameView->scrollPosition().y();
> +        point.x += frameView->scrollPosition().x();
> +    }
> +    
> +    return [[self accessibilityRootObjectWrapper] accessibilityHitTest:point];

We have "screen to view" conversions on ScrollView (screenToContents() etc). Can this use it?

[chris fleizach]

Created attachment 225712 [details]
patch to avoid assertions

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 225518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225518&action=review
> 
> >> Source/WebKit2/UIProcess/ios/WKContentView.mm:237
> >> +    CFUUIDRef uuid = CFUUIDCreate(kCFAllocatorDefault);
> > 
> > Please use a RetainPtr<> here.
> 
> Your committed code leaks.
> 
> > Source/WebKit2/UIProcess/ios/WKContentView.mm:239
> > +    IPC::Connection* connection = _page->process().connection();
> 
> This asserts in a debug build. Did you test it?

Yes I've been testing. I've never hit a crash thus far on device

> 
> > Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:39
> > +- (id)init
> 
> Return instancetype.
> 
> > Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:65
> > +- (void)dealloc
> > +{
> > +    [[NSNotificationCenter defaultCenter] removeObserver:self];
> > +    self.remoteTokenData = nil;
> > +    [super dealloc];
> > +}
> 
> Please move this up under the -init.
> 
> You should not use property syntax in the dealloc method.
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:95
> > +    m_mockAccessibilityElement = [[[WKAccessibilityWebPageObject alloc] init] autorelease];
> 
> m_mockAccessibilityElement = adoptNS([[WKAccessibilityWebPageObject alloc] init]).
> 
> >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:100
> >> +    CFRelease(uuid);
> > 
> > RetainPtr<>.
> 
> Without leaking!
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.h:33
> > +    NSArray* m_attributeNames;
> > +    NSMutableArray* m_accessibilityChildren;
> 
> What is the ownership model for these? Should they be RetainPtr?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:102
> > +    return;
> 
> Remove.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:140
> > +    if ([attribute isEqualToString:NSAccessibilityParentAttribute])
> > +        return m_parent;
> > +    if ([attribute isEqualToString:NSAccessibilityWindowAttribute])
> > +        return [m_parent accessibilityAttributeValue:NSAccessibilityWindowAttribute];
> > +    if ([attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute])
> > +        return [m_parent accessibilityAttributeValue:NSAccessibilityTopLevelUIElementAttribute];
> > +    if ([attribute isEqualToString:NSAccessibilityRoleAttribute])
> > +        return NSAccessibilityGroupRole;
> > +    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute])
> > +        return NSAccessibilityRoleDescription(NSAccessibilityGroupRole, nil);
> > +    if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
> > +        return [NSNumber numberWithBool:NO];
> 
> Some spaces would make this more readable.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:163
> > +    if ([parameter isKindOfClass:[NSValue class]] && strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) == 0) {
> 
> Isn't there a better way than strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) ?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:176
> > +        return CFBridgingRelease(WKStringCopyCFString(kCFAllocatorDefault, (WKStringRef)result.get()));
> > +    else if (toImpl(result.get())->type() == API::Boolean::APIType)
> > +        return [NSNumber numberWithBool:WKBooleanGetValue(static_cast<WKBooleanRef>(result.get()))];
> 
> No else after return.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:187
> > +#pragma clang diagnostic push
> > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> 
> Can this be removed at some point?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:210
> > +    // Hit-test point comes in as bottom-screen coordinates. Needs to be normalized to the frame of the web page.
> > +    NSPoint remotePosition = [[self accessibilityAttributeValue:NSAccessibilityPositionAttribute] pointValue];
> > +    NSSize remoteSize = [[self accessibilityAttributeValue:NSAccessibilitySizeAttribute] sizeValue];
> > +    
> > +    // Get the y position of the WKView (we have to screen-flip and go from bottom left to top left).
> > +    CGFloat screenHeight = [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height;
> > +    remotePosition.y = (screenHeight - remotePosition.y) - remoteSize.height;
> > +    
> > +    point.y = screenHeight - point.y;
> > +    
> > +    // Re-center point into the web page's frame.
> > +    point.y -= remotePosition.y;
> > +    point.x -= remotePosition.x;
> > +    
> > +    WebCore::FrameView* frameView = m_page ? m_page->mainFrameView() : 0;
> > +    if (frameView) {
> > +        point.y += frameView->scrollPosition().y();
> > +        point.x += frameView->scrollPosition().x();
> > +    }
> > +    
> > +    return [[self accessibilityRootObjectWrapper] accessibilityHitTest:point];
> 
> We have "screen to view" conversions on ScrollView (screenToContents() etc). Can this use it?

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 225518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225518&action=review
> 
> >> Source/WebKit2/UIProcess/ios/WKContentView.mm:237
> >> +    CFUUIDRef uuid = CFUUIDCreate(kCFAllocatorDefault);
> > 
> > Please use a RetainPtr<> here.
> 
> Your committed code leaks.
> 
> > Source/WebKit2/UIProcess/ios/WKContentView.mm:239
> > +    IPC::Connection* connection = _page->process().connection();
> 
> This asserts in a debug build. Did you test it?
> 
> > Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:39
> > +- (id)init
> 
> Return instancetype.
> 
> > Source/WebKit2/WebProcess/WebPage/WKAccessibilityWebPageObjectIOS.mm:65
> > +- (void)dealloc
> > +{
> > +    [[NSNotificationCenter defaultCenter] removeObserver:self];
> > +    self.remoteTokenData = nil;
> > +    [super dealloc];
> > +}
> 
> Please move this up under the -init.
> 
> You should not use property syntax in the dealloc method.
> 
> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:95
> > +    m_mockAccessibilityElement = [[[WKAccessibilityWebPageObject alloc] init] autorelease];
> 
> m_mockAccessibilityElement = adoptNS([[WKAccessibilityWebPageObject alloc] init]).
> 
> >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:100
> >> +    CFRelease(uuid);
> > 
> > RetainPtr<>.
> 
> Without leaking!
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.h:33
> > +    NSArray* m_attributeNames;
> > +    NSMutableArray* m_accessibilityChildren;
> 
> What is the ownership model for these? Should they be RetainPtr?

This Mac related code has been here for maybe two years. It's looking a little crufty. I'll clean it up

> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:102
> > +    return;
> 
> Remove.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:140
> > +    if ([attribute isEqualToString:NSAccessibilityParentAttribute])
> > +        return m_parent;
> > +    if ([attribute isEqualToString:NSAccessibilityWindowAttribute])
> > +        return [m_parent accessibilityAttributeValue:NSAccessibilityWindowAttribute];
> > +    if ([attribute isEqualToString:NSAccessibilityTopLevelUIElementAttribute])
> > +        return [m_parent accessibilityAttributeValue:NSAccessibilityTopLevelUIElementAttribute];
> > +    if ([attribute isEqualToString:NSAccessibilityRoleAttribute])
> > +        return NSAccessibilityGroupRole;
> > +    if ([attribute isEqualToString:NSAccessibilityRoleDescriptionAttribute])
> > +        return NSAccessibilityRoleDescription(NSAccessibilityGroupRole, nil);
> > +    if ([attribute isEqualToString:NSAccessibilityFocusedAttribute])
> > +        return [NSNumber numberWithBool:NO];
> 
> Some spaces would make this more readable.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:163
> > +    if ([parameter isKindOfClass:[NSValue class]] && strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) == 0) {
> 
> Isn't there a better way than strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) ?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:176
> > +        return CFBridgingRelease(WKStringCopyCFString(kCFAllocatorDefault, (WKStringRef)result.get()));
> > +    else if (toImpl(result.get())->type() == API::Boolean::APIType)
> > +        return [NSNumber numberWithBool:WKBooleanGetValue(static_cast<WKBooleanRef>(result.get()))];
> 
> No else after return.
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:187
> > +#pragma clang diagnostic push
> > +#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> 
> Can this be removed at some point?
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:210
> > +    // Hit-test point comes in as bottom-screen coordinates. Needs to be normalized to the frame of the web page.
> > +    NSPoint remotePosition = [[self accessibilityAttributeValue:NSAccessibilityPositionAttribute] pointValue];
> > +    NSSize remoteSize = [[self accessibilityAttributeValue:NSAccessibilitySizeAttribute] sizeValue];
> > +    
> > +    // Get the y position of the WKView (we have to screen-flip and go from bottom left to top left).
> > +    CGFloat screenHeight = [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height;
> > +    remotePosition.y = (screenHeight - remotePosition.y) - remoteSize.height;
> > +    
> > +    point.y = screenHeight - point.y;
> > +    
> > +    // Re-center point into the web page's frame.
> > +    point.y -= remotePosition.y;
> > +    point.x -= remotePosition.x;
> > +    
> > +    WebCore::FrameView* frameView = m_page ? m_page->mainFrameView() : 0;
> > +    if (frameView) {
> > +        point.y += frameView->scrollPosition().y();
> > +        point.x += frameView->scrollPosition().x();
> > +    }
> > +    
> > +    return [[self accessibilityRootObjectWrapper] accessibilityHitTest:point];
> 
> We have "screen to view" conversions on ScrollView (screenToContents() etc). Can this use it?

[chris fleizach]

(In reply to comment #5)
> (From update of attachment 225518 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225518&action=review
> 
> 
> > Source/WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectMac.mm:163
> > +    if ([parameter isKindOfClass:[NSValue class]] && strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) == 0) {
> 
> Isn't there a better way than strcmp([(NSValue*)parameter objCType], @encode(NSPoint)) ?
> 

I looked through a bunch of projects. It looks like this is what everyone is doing.

> > +    NSSize remoteSize = [[self accessibilityAttributeValue:NSAccessibilitySizeAttribute] sizeValue];
> > +    
> > +    // Get the y position of the WKView (we have to screen-flip and go from bottom left to top left).
> > +    CGFloat screenHeight = [(NSScreen *)[[NSScreen screens] objectAtIndex:0] frame].size.height;
> > +    remotePosition.y = (screenHeight - remotePosition.y) - remoteSize.height;
> > +    
> > +    point.y = screenHeight - point.y;
> > +    
> > +    // Re-center point into the web page's frame.
> > +    point.y -= remotePosition.y;
> > +    point.x -= remotePosition.x;
> > +    
> > +    WebCore::FrameView* frameView = m_page ? m_page->mainFrameView() : 0;
> > +    if (frameView) {
> > +        point.y += frameView->scrollPosition().y();
> > +        point.x += frameView->scrollPosition().x();
> > +    }
> > +    
> > +    return [[self accessibilityRootObjectWrapper] accessibilityHitTest:point];
> 
> We have "screen to view" conversions on ScrollView (screenToContents() etc). Can this use it?

Filed https://bugs.webkit.org/show_bug.cgi?id=129643 to take care of that suggestion.

[chris fleizach]

Simon's review comments addressed in
http://trac.webkit.org/changeset/165159

[Csaba Osztrogonác]

Comment on attachment 225712 [details]
patch to avoid assertions

Cleared review? from attachment 225712 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).