Bug 155034 - AX: Implement missing/different accessibility API mappings for SVG
Summary: AX: Implement missing/different accessibility API mappings for SVG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-04 13:00 PST by Joanmarie Diggs (irc: joanie)
Modified: 2016-03-05 13:35 PST (History)
3 users (show)

See Also:


Attachments
Patch (115.62 KB, patch)
2016-03-04 13:23 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (112.48 KB, patch)
2016-03-04 14:40 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (115.70 KB, patch)
2016-03-05 01:27 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (115.61 KB, patch)
2016-03-05 01:38 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.46 MB, application/zip)
2016-03-05 02:45 PST, Build Bot
no flags Details
Patch (115.74 KB, patch)
2016-03-05 03:06 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2016-03-04 13:00:40 PST
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
Comment 1 Radar WebKit Bug Importer 2016-03-04 13:03:24 PST
<rdar://problem/24982046>
Comment 2 Joanmarie Diggs (irc: joanie) 2016-03-04 13:23:50 PST
Created attachment 273029 [details]
Patch
Comment 3 Joanmarie Diggs (irc: joanie) 2016-03-04 14:40:05 PST
Created attachment 273045 [details]
Patch
Comment 4 Joanmarie Diggs (irc: joanie) 2016-03-04 15:58:42 PST
Chris: When you get a chance, could you please review this? Thanks!
Comment 5 chris fleizach 2016-03-04 18:16:46 PST
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
Comment 6 Joanmarie Diggs (irc: joanie) 2016-03-05 01:27:25 PST
Created attachment 273077 [details]
Patch
Comment 7 Joanmarie Diggs (irc: joanie) 2016-03-05 01:38:25 PST
Created attachment 273079 [details]
Patch
Comment 8 Joanmarie Diggs (irc: joanie) 2016-03-05 02:00:00 PST
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!
Comment 9 Build Bot 2016-03-05 02:45:33 PST
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
Comment 10 Build Bot 2016-03-05 02:45:36 PST
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
Comment 11 Joanmarie Diggs (irc: joanie) 2016-03-05 03:06:48 PST
Created attachment 273082 [details]
Patch
Comment 12 WebKit Commit Bot 2016-03-05 13:35:49 PST
Comment on attachment 273082 [details]
Patch

Clearing flags on attachment: 273082

Committed r197616: <http://trac.webkit.org/changeset/197616>
Comment 13 WebKit Commit Bot 2016-03-05 13:35:53 PST
All reviewed patches have been landed.  Closing bug.