| Summary: | AX: Support IOS Accessibility in WK2 | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
chris fleizach
2014-02-28 18:25:10 PST
Created attachment 225518 [details]
patch
This patch will need a corresponding change with WKSI, so it will not build until that piece lands
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). 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? Created attachment 225712 [details]
patch to avoid assertions
(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? (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? (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. Simon's review comments addressed in http://trac.webkit.org/changeset/165159 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). |