Bug 155481 – AX: Expose pointers to SVG elements referenced by aria-labelledby

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

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.

[Radar WebKit Bug Importer]

<rdar://problem/25158925>

[Joanmarie Diggs]

Created attachment 274067 [details]
Patch

[Joanmarie Diggs]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Joanmarie Diggs]

Created attachment 274071 [details]
Patch

[chris fleizach]

(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

[chris fleizach]

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

[Joanmarie Diggs]

(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.

[chris fleizach]

(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.

[Joanmarie Diggs]

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}?

[chris fleizach]

(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

[Joanmarie Diggs]

Created attachment 274140 [details]
Patch

[Joanmarie Diggs]

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!

[WebKit Commit Bot]

Comment on attachment 274140 [details]
Patch

Clearing flags on attachment: 274140

Committed r198254: <http://trac.webkit.org/changeset/198254>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.