At least in the case of ATK, elements referenced by aria-labelledby should be exposed via ATK_RELATION_LABELLED_BY. This is consistent with the Core AAM spec, and is also something that will be tested in the implementations of the SVG AAM. We are currently only exposing elements referenced by aria-labelledby via the accessible name calculation.
<rdar://problem/25158925>
Created attachment 274067 [details] Patch
Chris: The attached patch only does the exposure for ATK, though I did update the layout test for the Mac which currently indicates the elements are not being exposed via AXTitleUIElement. According to the current expectations of the SVG Accessibility Task Force [1], there are a number of instances in which the elements should be exposed via AXTitleUIElement. Could you please either confirm that their expectations are correct (in which case I'll do a new patch that also handles exposure for the Mac), or let me know that they are wrong (in which case, I'll pass that along to them)? Thanks! [1] https://www.w3.org/wiki/SVG_Accessibility/Testing/Test_Assertions_with_Tables
Comment on attachment 274067 [details] Patch Attachment 274067 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/980045 New failing tests: accessibility/w3c-svg-name-calculation.html
Created attachment 274069 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 274067 [details] Patch Attachment 274067 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/980057 New failing tests: accessibility/w3c-svg-name-calculation.html
Created attachment 274070 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 274071 [details] Patch
(In reply to comment #3) > Chris: The attached patch only does the exposure for ATK, though I did > update the layout test for the Mac which currently indicates the elements > are not being exposed via AXTitleUIElement. > > According to the current expectations of the SVG Accessibility Task Force > [1], there are a number of instances in which the elements should be exposed > via AXTitleUIElement. Could you please either confirm that their > expectations are correct (in which case I'll do a new patch that also > handles exposure for the Mac), or let me know that they are wrong (in which > case, I'll pass that along to them)? > THere has been some talk of exposing labelledby- as elements in title UI Elements, but so far we've just take the string concatenation. We could fix that in the future, but i don't know if i've seen a real world case where it's made a difference yet > Thanks! > > [1] > https://www.w3.org/wiki/SVG_Accessibility/Testing/Test_Assertions_with_Tables
Comment on attachment 274071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274071&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1038 > + return !getAttribute(aria_labelledbyAttr).isEmpty() || !getAttribute(aria_labeledbyAttr).isEmpty(); seems like this can move into AXObject.cpp and then it doesn't need to be virtual > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1045 > + ariaElementsFromAttribute(ariaLabelledBy, aria_labeledbyAttr); ditto > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:256 > + } else if (coreObject->supportsARIALabelledBy()) { does having a supportsARIALabelledBy method give us any wins here? it seems like directly getting ariaLabelledByElements() will be just as fast when there's nothing there, and faster where there is something
(In reply to comment #10) Thanks for the review! Regarding this: > does having a supportsARIALabelledBy method give us any wins here? it seems > like directly getting ariaLabelledByElements() will be just as fast when > there's nothing there, and faster where there is something Makes sense. FWIW, I did it purely to be consistent with what was done for flowto, controls, and describedby without questioning why it was done for the others. Taking a look at them now, do any of them give us any wins? Most seem to be used just by ATK, the exception being supportsARIAFlowTo() (called by AccessibilityObject::supportsARIAAttributes()). But that could easily be replaced with hasAttribute(aria_flowtoAttr). So it seems to me that we might as well remove the supportsFoo() when only used prior to ariaFooElements(). If you agree, I'll do that as part of the revised patch. (In reply to comment #9) > THere has been some talk of exposing labelledby- as elements in title UI > Elements, but so far we've just take the string concatenation. We could fix > that in the future, but i don't know if i've seen a real world case where > it's made a difference yet The only thing I can think of is possibly eliminating double-presentation of text content. I'll see if I can come up with a test case to prove or disprove that with VoiceOver.
(In reply to comment #11) > (In reply to comment #10) > > Thanks for the review! Regarding this: > > > does having a supportsARIALabelledBy method give us any wins here? it seems > > like directly getting ariaLabelledByElements() will be just as fast when > > there's nothing there, and faster where there is something > > Makes sense. FWIW, I did it purely to be consistent with what was done for > flowto, controls, and describedby without questioning why it was done for > the others. > > Taking a look at them now, do any of them give us any wins? Most seem to be > used just by ATK, the exception being supportsARIAFlowTo() (called by > AccessibilityObject::supportsARIAAttributes()). But that could easily be > replaced with hasAttribute(aria_flowtoAttr). So it seems to me that we might > as well remove the supportsFoo() when only used prior to ariaFooElements(). > If you agree, I'll do that as part of the revised patch. I know there are some cases that I think gate the inclusion of attributes for the mac platform, so I guess I'll need to see the patch, but I did just check out aria-flowto and that seems like a ripe candidate indeed > > (In reply to comment #9) > > > THere has been some talk of exposing labelledby- as elements in title UI > > Elements, but so far we've just take the string concatenation. We could fix > > that in the future, but i don't know if i've seen a real world case where > > it's made a difference yet > > The only thing I can think of is possibly eliminating double-presentation of > text content. I'll see if I can come up with a test case to prove or > disprove that with VoiceOver.
Comment on attachment 274071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274071&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1038 >> + return !getAttribute(aria_labelledbyAttr).isEmpty() || !getAttribute(aria_labeledbyAttr).isEmpty(); > > seems like this can move into AXObject.cpp and then it doesn't need to be virtual On this one, we agree the method is not needed at all, but.... (to be continued) >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1045 >> + ariaElementsFromAttribute(ariaLabelledBy, aria_labeledbyAttr); > > ditto (continuation) To confirm: Move all the ariaFooElements() out of AccessibilityRenderObject and into AccessibilityObject? Are there AccessibilityObject items which will not be AccessibilityRenderObject instances and while have aria-{labelledby,describedby,flowto,controls,owns}?
(In reply to comment #13) > Comment on attachment 274071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=274071&action=review > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1038 > >> + return !getAttribute(aria_labelledbyAttr).isEmpty() || !getAttribute(aria_labeledbyAttr).isEmpty(); > > > > seems like this can move into AXObject.cpp and then it doesn't need to be virtual > > On this one, we agree the method is not needed at all, but.... (to be > continued) > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1045 > >> + ariaElementsFromAttribute(ariaLabelledBy, aria_labeledbyAttr); > > > > ditto > > (continuation) To confirm: Move all the ariaFooElements() out of > AccessibilityRenderObject and into AccessibilityObject? Are there > AccessibilityObject items which will not be AccessibilityRenderObject > instances and while have aria-{labelledby,describedby,flowto,controls,owns}? I guess its always possible there will be a fake element or a Node only element that has these attributes. Plus by virtue of being not virtual, it should be slightly faster
Created attachment 274140 [details] Patch
Comment on attachment 274140 [details] Patch Chris, I believe I addressed everything: * All of the supportsARIAFoo() methods with corresponding ariaFooElements() method seem unneeded EXCEPT supportsARIAOwns() which the Mac wrapper calls in additionalAccessibilityAttributeNames(). I've removed the unneeded ones, which turned out to also be used by the inspector in the same fashion as the ATK wrapper. * All of the ariaFooElements() methods have been moved from AccessibilityRenderObject to AccessibilityObject. * I tried to come up with a real-world use case in which VoiceOver was double-speaking text referenced by aria-labelledby in SVG. I failed. :) I'll check with the SVG Accessibility Task Force to see if verifying implementation via the name computation will be sufficient for CR testing. I also did a bit more cleanup in the ATK Wrapper's relation-set construction. Please review when you have a chance. Thanks again!
Comment on attachment 274140 [details] Patch Clearing flags on attachment: 274140 Committed r198254: <http://trac.webkit.org/changeset/198254>
All reviewed patches have been landed. Closing bug.