WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
129527
AX: Support IOS Accessibility in WK2
https://bugs.webkit.org/show_bug.cgi?id=129527
Summary
AX: Support IOS Accessibility in WK2
chris fleizach
Reported
2014-02-28 18:25:10 PST
Add in enough support to make this work
Attachments
patch
(54.78 KB, patch)
2014-02-28 18:33 PST
,
chris fleizach
sam
: review+
Details
Formatted Diff
Diff
patch to avoid assertions
(1.98 KB, patch)
2014-03-03 16:48 PST
,
chris fleizach
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
chris fleizach
Comment 1
2014-02-28 18:25:47 PST
<
rdar://problem/15062381
>
chris fleizach
Comment 2
2014-02-28 18:33:45 PST
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 3
2014-02-28 20:32:12 PST
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
Comment 4
2014-03-03 15:24:48 PST
http://trac.webkit.org/changeset/165014
Simon Fraser (smfr)
Comment 5
2014-03-03 16:26:39 PST
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
Comment 6
2014-03-03 16:48:19 PST
Created
attachment 225712
[details]
patch to avoid assertions
chris fleizach
Comment 7
2014-03-03 16:50:45 PST
(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
Comment 8
2014-03-03 16:53:39 PST
(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
Comment 9
2014-03-03 17:06:02 PST
(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
Comment 10
2014-03-05 18:46:29 PST
Simon's review comments addressed in
http://trac.webkit.org/changeset/165159
Csaba Osztrogonác
Comment 11
2014-06-04 00:50:46 PDT
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).
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug