Bug 155034 – AX: Implement missing/different accessibility API mappings for SVG

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

[Joanmarie Diggs]

The SVG Accessibility API Mappings spec [1] differs from the Core Accessibility API Mappings spec [2] in several areas, including:

* Inclusion in/Exclusion from the accessibility tree
* Sources and priority of text for the accessible name and accessible description mappings
* SVG-specific roles

As a result, quite a few of the tests to verify implementation [3] fail in WebKit.

[1] https://rawgit.com/w3c/aria/master/svg-aam/svg-aam.html
[2] https://rawgit.com/w3c/aria/master/core-aam/core-aam.html
[3] https://www.w3.org/wiki/SVG_Accessibility/Testing/Test_Assertions

[Radar WebKit Bug Importer]

<rdar://problem/24982046>

[Joanmarie Diggs]

Created attachment 273029 [details]
Patch

[Joanmarie Diggs]

Created attachment 273045 [details]
Patch

[Joanmarie Diggs]

Chris: When you get a chance, could you please review this? Thanks!

[chris fleizach]

Comment on attachment 273045 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1997
> +    if (role == PresentationalRole && !is<AccessibilitySVGElement>(*this) && supportsARIAAttributes())

can AccessibilitySVGElement just override determineAriaRoleAttribute?

> Source/WebCore/accessibility/AccessibilityObject.h:523
> +    virtual bool isSVGElement() const { return false; }

do we need both of these methods? isAccessibilitySVGElement and isSVGElement

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:126
> +        if (target)

if (AccessibilityObject* target = targetForUseElement())
  return target->...

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:151
> +    String describedBy = ariaDescribedByAttribute();

this looks like it duplicates the accessibility description in helpText. I think that will end up double speaking many things

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:169
> +            return target->helpText();

this looks ok

> Source/WebCore/accessibility/AccessibilitySVGElement.cpp:245
> +            svgTextualElementParents.get().add(SVGNames::textTag);

since there's only one tag name, it seems like we can avoid the overhead of making a static hash

[Joanmarie Diggs]

Created attachment 273077 [details]
Patch

[Joanmarie Diggs]

Created attachment 273079 [details]
Patch

[Joanmarie Diggs]

I'm still waiting for EWS to complete before asking for review again. In the meantime:

(In reply to comment #5)

> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1997
> > +    if (role == PresentationalRole && !is<AccessibilitySVGElement>(*this) && supportsARIAAttributes())
> 
> can AccessibilitySVGElement just override determineAriaRoleAttribute?

Yes. Done.
 
> > Source/WebCore/accessibility/AccessibilityObject.h:523
> > +    virtual bool isSVGElement() const { return false; }
> 
> do we need both of these methods? isAccessibilitySVGElement and isSVGElement

No we don't. Removed the latter.

> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:126
> > +        if (target)
> 
> if (AccessibilityObject* target = targetForUseElement())
>   return target->...

Thanks. Done.
 
> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:151
> > +    String describedBy = ariaDescribedByAttribute();
> 
> this looks like it duplicates the accessibility description in helpText. I
> think that will end up double speaking many things

This one I need clarification on. I haven't changed the code (yet) in response to your observation. What I have done, however, is modified the output of the name and description tests so that they always output AXTitle, AXDescription, and (on the mac only) AXHelp. That way we can see if there are duplicate exposures which would be, I assume, the source of any double speaking.

I'm not seeing duplicates.

> > Source/WebCore/accessibility/AccessibilitySVGElement.cpp:245
> > +            svgTextualElementParents.get().add(SVGNames::textTag);
> 
> since there's only one tag name, it seems like we can avoid the overhead of
> making a static hash

Done.

Thanks for the review!

[Build Bot]

Comment on attachment 273079 [details]
Patch

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

New failing tests:
accessibility/w3c-svg-elements-not-exposed.html
accessibility/w3c-svg-roles.html
accessibility/svg-labelledby.html
accessibility/w3c-svg-presentational-role.html
accessibility/container-node-delete-causes-crash.html
accessibility/svg-group-element-with-title.html
accessibility/mac/media-role-descriptions.html
accessibility/w3c-svg-name-calculation.html
accessibility/svg-bounds.html
accessibility/w3c-svg-description-calculation.html

[Build Bot]

Created attachment 273081 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Joanmarie Diggs]

Created attachment 273082 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 273082 [details]
Patch

Clearing flags on attachment: 273082

Committed r197616: <http://trac.webkit.org/changeset/197616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.