Bug 97301 – AX: Layout tests would be easier to write if AccessibilityController could find an element by id

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

The idea would be to eliminate code like this (that's brittle and often fails on a different platform):

       var root = accessibilityController.rootElement;
       var table = root.childAtIndex(0).childAtIndex(0);

...and replace it with:

       var root = accessibilityController.accessibleElementById('table_0');

[Dominic Mazzoni]

Created attachment 165068 [details]
Patch

[chris fleizach]

Comment on attachment 165068 [details]
Patch

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

it seems like the overall approach could find the elementById and get a Node back, then return the ax object for that one, so that we don't have to iterate all the children to find the object

> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:106
> +    for (unsigned i = 0; i < obj.childCount(); i++) {

obj.childCount() should be in a variable so you don't have to call a method each time through the loop

> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:187
> +        result->setNull();

can you set result->setNull() at the top of the method and then just return right away on the error conditions?
that will make it a tad cleaner and remove the need for brackets

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:43
> +#define END_AX_OBJC_EXCEPTIONS } @catch(NSException *e) { if (![[e name] isEqualToString:NSAccessibilityException]) @throw; }

we should find a way to only define this once since it's also defined in AXUIElement

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:52
> +{

I believe this method already exists in AccessibilityUIElement... we should probably have it in just one location

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:54
> +        return NULL;

we should return nil for ObjC objects

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:103
> +    NSArray* children = [obj accessibilityAttributeValue:NSAccessibilityChildrenAttribute];

since this is an ObjC object the * should be next to the "children"
http://www.webkit.org/coding/coding-style.html#pointers-non-cpp

same goes for NSString* idAttribute

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:104
> +    for (NSUInteger i = 0; i < [children count]; ++i) {

[children count] should be in a variable so we don't have to call the method each time through the loop

> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:120
> +    else

no need for else here

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:51
> +    PassRefPtr<AccessibilityUIElement> accessibleElementById(JSStringRef attribute);

attribute should probably be "id"

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:61
> +    return [(NSString *)cfString autorelease];

same comments as DRT

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:108
> +    return 0;

same comments as DRT

[Dominic Mazzoni]

Created attachment 165138 [details]
Rebase only, still needs work

[Build Bot]

Comment on attachment 165138 [details]
Rebase only, still needs work

Attachment 165138 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13957690

[Dominic Mazzoni]

Created attachment 165148 [details]
Feedback addressed

[WebKit Review Bot]

Attachment 165148 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:43:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:56:  Extra space before [  [whitespace/braces] [5]
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:60:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:48:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:51:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:56:  Extra space before [  [whitespace/braces] [5]
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:60:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Mazzoni]

Comment on attachment 165068 [details]
Patch

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

> it seems like the overall approach could find the elementById and get a Node back,
> then return the ax object for that one, so that we don't have to iterate all the
> children to find the object

I thought about that, but I think this approach is better for tests:
* It tests the actual interface for walking the accessibility tree, rather than using a fake shortcut created just for testing
* It ensures anything found is actually reachable from the accessibility root
* Calling accessibleElementById with a nonexistent id is a handy way to touch every element in the accessibility tree once, which would be sufficient to repro a lot of crash bugs. (A few layout tests used to repro crash bugs walk every element in the tree using JS now, this would be far easier.)
* Finally, I don't think that speed is as important for code that's only used for testing; the accessibility layout tests are all pretty fast now and exploring the whole tree doesn't affect their runtime

>> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:106
>> +    for (unsigned i = 0; i < obj.childCount(); i++) {
> 
> obj.childCount() should be in a variable so you don't have to call a method each time through the loop

Done

>> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:187
>> +        result->setNull();
> 
> can you set result->setNull() at the top of the method and then just return right away on the error conditions?
> that will make it a tad cleaner and remove the need for brackets

Sure, that's cleaner.

>> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:43
>> +#define END_AX_OBJC_EXCEPTIONS } @catch(NSException *e) { if (![[e name] isEqualToString:NSAccessibilityException]) @throw; }
> 
> we should find a way to only define this once since it's also defined in AXUIElement

Moved to a common header file.

>> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:54
>> +        return NULL;
> 
> we should return nil for ObjC objects

Done

>> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:103
>> +    NSArray* children = [obj accessibilityAttributeValue:NSAccessibilityChildrenAttribute];
> 
> since this is an ObjC object the * should be next to the "children"
> http://www.webkit.org/coding/coding-style.html#pointers-non-cpp
> 
> same goes for NSString* idAttribute

Done

>> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:104
>> +    for (NSUInteger i = 0; i < [children count]; ++i) {
> 
> [children count] should be in a variable so we don't have to call the method each time through the loop

Done

>> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:120
>> +    else
> 
> no need for else here

Done

>> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:51
>> +    PassRefPtr<AccessibilityUIElement> accessibleElementById(JSStringRef attribute);
> 
> attribute should probably be "id"

Named it idAttribute to not conflict with the obj-C type "id".

>> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:61
>> +    return [(NSString *)cfString autorelease];
> 
> same comments as DRT

Done

[Dominic Mazzoni]

The style checker doesn't seem to realize that AccessibilityCommonMac.h is an objective-C file.

Should I name it .hh?

Why don't other obj-C header files ending in .h have the same problem? Or is there something else I'm missing?

[chris fleizach]

(In reply to comment #8)
> The style checker doesn't seem to realize that AccessibilityCommonMac.h is an objective-C file.
> 
> Should I name it .hh?
> 
> Why don't other obj-C header files ending in .h have the same problem? Or is there something else I'm missing?

Maybe a bug in the style checker? ObjC headers are always .h in all code I've ever seen

[WebKit Review Bot]

Comment on attachment 165148 [details]
Feedback addressed

Attachment 165148 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13951791

New failing tests:
accessibility/aria-hidden-with-elements.html

[Timothy Hatcher]

Comment on attachment 165148 [details]
Feedback addressed

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

>> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43
>> +@interface NSString (JSStringRefAdditions)
> 
> Extra space before ( in function call  [whitespace/parens] [4]

This does seem bogus. Just ignore it — StyleQueue isn't the final say.

> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:64
> +@implementation NSString (JSStringRefAdditions)
> +
> ++ (NSString *)stringWithJSStringRef:(JSStringRef)jsStringRef
> +{
> +    if (!jsStringRef)
> +        return nil;
> +
> +    CFStringRef cfString = JSStringCopyCFString(kCFAllocatorDefault, jsStringRef);
> +    return [(NSString *)cfString autorelease];
> +}
> +
> +- (JSStringRef)createJSStringRef
> +{
> +    return JSStringCreateWithCFString((CFStringRef)self);
> +}
> +
> +@end

The @implementation should not be in a .h file. If it was in a .m file it would be fine. I'm surprised this builds.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:64
> +@implementation NSString (JSStringRefAdditions)
> +
> ++ (NSString *)stringWithJSStringRef:(JSStringRef)jsStringRef
> +{
> +    if (!jsStringRef)
> +        return nil;
> +
> +    CFStringRef cfString = JSStringCopyCFString(kCFAllocatorDefault, jsStringRef);
> +    return [(NSString *)cfString autorelease];
> +}
> +
> +- (JSStringRef)createJSStringRef
> +{
> +    return JSStringCreateWithCFString((CFStringRef)self);
> +}
> +
> +@end

Ditto.

[Dominic Mazzoni]

Created attachment 165205 [details]
Patch

[Dominic Mazzoni]

Comment on attachment 165148 [details]
Feedback addressed

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

>>> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43
>>> +@interface NSString (JSStringRefAdditions)
>> 
>> Extra space before ( in function call  [whitespace/parens] [4]
> 
> This does seem bogus. Just ignore it — StyleQueue isn't the final say.

Thanks for taking a look! I'll ignore it, and now that the impl is in a .mm file, there are no other warnings.

>> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:64
>> +@end
> 
> The @implementation should not be in a .h file. If it was in a .m file it would be fine. I'm surprised this builds.

Fixed.

[WebKit Review Bot]

Attachment 165205 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1
Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:42:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:42:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 165205 [details]
Patch

looks good

[WebKit Review Bot]

Comment on attachment 165205 [details]
Patch

Attachment 165205 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13950806

New failing tests:
WebFilterOperationsTest.saveAndRestore
accessibility/aria-hidden-with-elements.html

[Dominic Mazzoni]

Created attachment 165248 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 165248 [details]
Patch for landing

Rejecting attachment 165248 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
rce/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Total errors found: 0 in 0 files

Full output: http://queues.webkit.org/results/13981413

[Dominic Mazzoni]

Created attachment 165263 [details]
Patch for landing

[Dominic Mazzoni]

Created attachment 165265 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 165265 [details]
Patch for landing

Clearing flags on attachment: 165265

Committed r129308: <http://trac.webkit.org/changeset/129308>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.