Bug 155494 – AX: [ATK] Implement missing AtkRelation types

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

While working on another bug, I discovered that we're not implementing certain relation types associated with ARIA attributes. This includes aria-owns and most of the reciprocal relations (e.g. description-for on elements referenced by aria-describedby).

While implementing support for the relation types set on the element is easy, it appears that implementing the reciprocal relations will require a more significant fix: We're currently doing all the work in WebKitAccessibleWrapper.cpp's setAtkRelationSetFromCoreObject(). At that point, it doesn't make sense to go on a hunt for elements which might have an ARIA attribute that references the current element. Thus we probably should cache these attributes and elements when creating the accessible tree.

In addition, we're not doing any real testing of the relations other than labelled-by (via AXTitleUIElement). The test runner will need some additions there....

[Radar WebKit Bug Importer]

<rdar://problem/25168402>

[Joanmarie Diggs]

Created attachment 311627 [details]
Patch

[Build Bot]

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


ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198:  'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199:  'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200:  'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201:  'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202:  'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203:  'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204:  'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
Total errors found: 7 in 13 files


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

[Joanmarie Diggs]

Comment on attachment 311627 [details]
Patch

Chris: What are your thoughts on the "incorrectly named" functions? I named the methods in my patch to be consistent with the existing methods. Beyond that, a general review would be much appreciated. Thanks!

[chris fleizach]

(In reply to Joanmarie Diggs (irc: joanie) from comment #4)
> Comment on attachment 311627 [details]
> Patch
> 
> Chris: What are your thoughts on the "incorrectly named" functions? I named
> the methods in my patch to be consistent with the existing methods. Beyond
> that, a general review would be much appreciated. Thanks!

those incorrectly named method warnings don't make any sense to me

[chris fleizach]

Comment on attachment 311627 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3291
> +    Node* node = this->node();

I think you can just do

Element* element = this->element() and save a check here

> Source/WebCore/accessibility/AccessibilityObject.cpp:3299
> +    AXObjectCache* cache = axObjectCache();

do we need to check if cache is nil here?

> Source/WebCore/accessibility/AccessibilityObject.cpp:3301
> +        String idList = element.attributeWithoutSynchronization(attribute).string();

can this be a const AtomicString&

> Source/WebCore/accessibility/AccessibilityObject.cpp:3306
> +        idList.replace("\n", " ").split(" ", ids);

should this be more inclusive of other whitespace? \t \r

[Build Bot]

Comment on attachment 311627 [details]
Patch

Attachment 311627 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3850428

New failing tests:
compositing/iframes/display-none-subframe.html
fast/css/target-fragment-match.html

[Build Bot]

Created attachment 311644 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Michael Catanzaro]

(In reply to Build Bot from comment #3)
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

I think the style checker is getting tripped up because the variable names contain "Ref". It would be smarter to search for " Ref<" or " RefPtr<" instead.

[Joanmarie Diggs]

Created attachment 311706 [details]
Patch

[Build Bot]

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


ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198:  'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199:  'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200:  'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201:  'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202:  'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203:  'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204:  'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
Total errors found: 7 in 13 files


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

[Darin Adler]

Comment on attachment 311706 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3291
> +    if (id.isNull() || id.isEmpty())

This should only check isEmpty, since null strings are also empty.

> Source/WebCore/accessibility/AccessibilityObject.cpp:3300
> +        if (idList.isNull() || idList.isEmpty())

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.cpp:3306
> +        Vector<String> ids;
> +        String(idList).simplifyWhiteSpace(isHTMLSpace).split(" ", ids);
> +        if (!ids.contains(id))
> +            continue;

There is a class to do this efficiently, called SpaceSplitString. That should be used rather than this much less efficient implementation, in part to be consistent with how the actual DOM implements this. One way to use it would be:

    if (!SpaceSplitString(idList, false).contains(id))
        continue;

If doing this, then the id local variable should be an AtomicString rather than a String. That leads me to question why identifierAttribute returns a String rather than a const AtomicString&; I can’t see any reason why it should, so that is worth fixing eventually for better efficiency.

[Build Bot]

Comment on attachment 311706 [details]
Patch

Attachment 311706 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3854196

New failing tests:
accessibility/aria-namefrom-author.html

[Build Bot]

Created attachment 311718 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joanmarie Diggs]

Created attachment 311720 [details]
Patch

[Joanmarie Diggs]

(In reply to Joanmarie Diggs (irc: joanie) from comment #15)
> Created attachment 311720 [details]
> Patch

Ignore this one as I didn't see the aria-namefrom-author.html failure.

[Build Bot]

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


ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198:  'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199:  'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200:  'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201:  'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202:  'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203:  'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204:  'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
Total errors found: 7 in 13 files


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

[Build Bot]

Comment on attachment 311720 [details]
Patch

Attachment 311720 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3855386

New failing tests:
accessibility/aria-namefrom-author.html

[Build Bot]

Created attachment 311755 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joanmarie Diggs]

Created attachment 311815 [details]
Patch

[Build Bot]

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


ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:198:  'ariaLabelledByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:199:  'ariaDescribedByElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:200:  'ariaOwnsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:201:  'ariaFlowToReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:202:  'ariaControlsReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:203:  'ariaLabelledByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:204:  'ariaDescribedByReferencingElementAtIndex' is incorrectly named. It should be named 'protector' or 'protectedUnsigned'.  [readability/naming/protected] [4]
Total errors found: 7 in 13 files


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

[Joanmarie Diggs]

(In reply to Darin Adler from comment #12)
 
> This should only check isEmpty, since null strings are also empty.

Done.

> Ditto.

Done.

> There is a class to do this efficiently, called SpaceSplitString. That
> should be used rather than this much less efficient implementation, in part
> to be consistent with how the actual DOM implements this. One way to use it
> would be:
> 
>     if (!SpaceSplitString(idList, false).contains(id))
>         continue;

Cool, thanks, and done. Also made a similar change in AccessibilityObject::elementsFromAttribute().
 
> If doing this, then the id local variable should be an AtomicString rather
> than a String. That leads me to question why identifierAttribute returns a
> String rather than a const AtomicString&; I can’t see any reason why it
> should, so that is worth fixing eventually for better efficiency.

Turns out it's only called on occasion, so also done.

Thanks!

[Joanmarie Diggs]

Chris or Darin, please review the current patch. Thanks!

[Darin Adler]

Comment on attachment 311815 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:3303
> +        if (idList.isEmpty())
> +            continue;

No need for this optimization; I think SpaceSplitString will handle this fine without this extra check.

[Joanmarie Diggs]

Created attachment 312761 [details]
Patch

patch for landing

[WebKit Commit Bot]

Comment on attachment 312761 [details]
Patch

Clearing flags on attachment: 312761

Committed r218177: <http://trac.webkit.org/changeset/218177>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.