Summary: | AX: Implement missing/different accessibility API mappings for SVG | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||||||||
Component: | Accessibility | Assignee: | Joanmarie Diggs <jdiggs> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | cfleizach, commit-queue, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Joanmarie Diggs
2016-03-04 13:00:40 PST
Created attachment 273029 [details]
Patch
Created attachment 273045 [details]
Patch
Chris: When you get a chance, could you please review this? Thanks! 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 Created attachment 273077 [details]
Patch
Created attachment 273079 [details]
Patch
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 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 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
Created attachment 273082 [details]
Patch
Comment on attachment 273082 [details] Patch Clearing flags on attachment: 273082 Committed r197616: <http://trac.webkit.org/changeset/197616> All reviewed patches have been landed. Closing bug. |