AX: AXObjectCache should try to use an unignored accessibilityObject when posting a selection notification when on the border between two accessibilityObjects

[Doug Russell]

When

1. AXObjectCache is asked to post a notification that selection state has changed
2. the selection is a caret
3. the selection is on a boundary between two accessibilityObjects and one of them is ignored

it is possible for AXObjectCache to try to resolve an observableObject() and end up with NULL because that element is ignored and won't have a useful tree.

This can be observed when tabbing between two links with a space in between them and observing that the accessibilityObject is the static text in between the links, not the second link.

The concrete fallout of this bug is difficulty correctly echoing links when tabbing between them.

This should be resolved by checking if the object is on a boundary and if the object on the other side of the boundary isn't ignored and using that instead.

[Radar WebKit Bug Importer]

<rdar://problem/21472191>

[Doug Russell]

Created attachment 255275 [details]
patch

[Darin Adler]

Comment on attachment 255275 [details]
patch

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

Same comments apply to all the copied and pasted versions in DumpRenderTree; I commented on the WebKitTestRunner code.

> Source/WebCore/accessibility/AXObjectCache.cpp:1020
> +    Node* node = position.deprecatedNode();

Why is it right to use deprecatedNode here? Ryosuke, is that right?

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:112
> +    static Class cls = nil;
> +    static dispatch_once_t onceToken;
> +    dispatch_once(&onceToken, ^{
> +        cls = NSClassFromString(@"WebAccessibilityObjectWrapper");
> +    });
> +    ASSERT(cls);
> +    return cls;

In WebKit code itself we often turn off the thread safety for globals. But if this is only used from one thread or if the thread safety for globals is turned on in WebKitTestRunner (which I think it could be), then this should just be:

    static Class wrapperClass = objc_getClass("WebAccessibilityObjectWrapper");
    return wrapperClass;

I also suggest using objc_getClass, since NSClassFromString just turns around and calls that.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:129
> +static JSValueRef MakeValueRefForValue(JSContextRef context, id value)

WebKit project does not capitalize function names.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:132
> +        return JSValueMakeString(context, JSRetainPtr<JSStringRef>(Adopt, [value createJSStringRef]).get());

We really need an adopt function to help write this kind of thing. The constructor syntax is super awkward.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:139
> +        return toJS(context, WTF::getPtr(WTR::AccessibilityUIElement::create(static_cast<PlatformUIElement>(value))));

No need to use WTF::getPtr here. Should just use the .get() function. The getPtr functions exists for generic programming that needs to work without knowing the type.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:147
> +static JSValueRef MakeArrayRefForArray(JSContextRef context, NSArray *array)

WebKit project does not capitalize function names.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:150
> +    JSValueRef arguments[count];

We normally don’t use the variable-sized array extension, but it’s probably oK for code only used in the test tool.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:158
> +static JSValueRef MakeObjectRefForDictionary(JSContextRef context, NSDictionary *dictionary)

WebKit project does not capitalize function names.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:164
> +
> +        JSRetainPtr<JSStringRef> propertyName = JSRetainPtr<JSStringRef>(Adopt, [key createJSStringRef]);

This should just be:

    JSRetainPtr<JSStringRef> propertyName { Adopt, [key createJSStringRef] };

But it would be even better if we had an adopt function so we could write:

    auto propertyName = adopt([key createJSStringRef]);

[Doug Russell]

Can adopt just be a free function in JSRetainPtr.h like

template<typename T> JSRetainPtr<T> adopt(T value)
{
    return JSRetainPtr<T>(Adopt, value);
}

or should it be a static method on JSRetainPtr (or something else)?

[Doug Russell]

Created attachment 255444 [details]
patch

[Darin Adler]

(In reply to comment #4)
> Can adopt just be a free function in JSRetainPtr.h like
> 
> template<typename T> JSRetainPtr<T> adopt(T value)
> {
>     return JSRetainPtr<T>(Adopt, value);
> }
> 
> or should it be a static method on JSRetainPtr (or something else)?

It can and should be something like that, but that template is too generic and would apply to too many different classes. Instead we should just do overloading, I think:

    inline JSRetainPtr<JSStringRef> adopt(JSStringRef string)
    {
        return JSRetainPtr<JSStringRef>(Adopt, string);
    }

We can overload for other types as needed. Not sure what types JSRetainPtr supports.

Later we will want to make the JSRetainPtr constructor for Adopt be private and move to this function exclusively.

[Doug Russell]

Created attachment 255452 [details]
patch

[Darin Adler]

Comment on attachment 255452 [details]
patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:1010
> +    AccessibilityObject* object = getOrCreate(node);
> +    postTextStateChangeNotification(object, intent, selection);

Would read better without the local variable.

> Source/WebCore/accessibility/AXObjectCache.cpp:1028
> +    if (object && object->accessibilityIsIgnored()) {

Does getOrCreate actually ever return null? The name makes it sound like it wouldn’t.

> Source/WebCore/accessibility/AXObjectCache.cpp:1030
> +            if (AccessibilityObject *nextSibling = object->nextSiblingUnignored(1))

Incorrect formatting for WebKit coding style; * is in the wrong place. Also, I suggest using auto or auto* for this.

> Source/WebCore/accessibility/AXObjectCache.cpp:1033
> +            if (AccessibilityObject *previousSibling = object->previousSiblingUnignored(1))

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:464
> +    if (limit < 0)
> +        limit = std::numeric_limits<int>::max();

This seems unnecessarily complex. If the default should be std::numeric_limits<int>::max(), then lets specify that default in the header, instead of making negative numbers magic. Also, we never use this feature, always passing 1 for limit, so this is dead untested code.

> Source/WebCore/accessibility/AccessibilityObject.cpp:465
> +    for (previous = this->previousSibling(); previous && previous->accessibilityIsIgnored(); previous = previous->previousSibling()) {

Normally we’d write this without this->

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:447
> +        if ((userInfo = dictionaryRemovingNonSupportedTypes(userInfo)))
> +            info = [NSDictionary dictionaryWithObjectsAndKeys:notificationName, @"notificationName", userInfo, @"userInfo", nil];
> +        else
>              info = [NSDictionary dictionaryWithObjectsAndKeys:notificationName, @"notificationName", nil];

Looks OK, but there is one sleazy shortcut we could take. No need to do the null check on userInfo because if it’s nil it will do the right thing, terminating the list early.

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:100
> +    static Class cls = nil;
> +    if (!cls)
> +        cls = NSClassFromString(@"WebAccessibilityObjectWrapper");

This is C++; no need for the if statement. This works:

    static Class cls = objc_getClass("WebAccessibilityObjectWrapper");

In fact, I would have just added "static" to the existing code instead of adding a new function. And even that change is not really needed; just makes this patch bigger for no significant benefit.

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:129
> +        return AccessibilityUIElement::makeJSAccessibilityUIElement(context, AccessibilityUIElement(value));

Is the explicit conversation to AccessibilityUIElement needed? Does it compile if you just leave it Oout?

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:152
> +    for (NSString *key in dictionary) {

Probably more efficient to use the method that enumerates with a block, since it gives both key and value and eliminates all the hash table lookups.

> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:154
> +        auto propertyName = adopt([key createJSStringRef]);

Not great to do this outside the if since it’s only used inside the if.

[Doug Russell]

Created attachment 255501 [details]
patch

[Doug Russell]

Created attachment 255502 [details]
patch

[WebKit Commit Bot]

Comment on attachment 255502 [details]
patch

Clearing flags on attachment: 255502

Committed r185924: <http://trac.webkit.org/changeset/185924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.