The accessibility description for an element with aria-labelledby should be equivalent to the description for an element with an aria-label of the same value as the element referred to by aria-labelledby. Currently, no description is returned. http://www.w3.org/TR/wai-aria/complete#textalternativecomputation says: "The aria-labelledby attribute takes precedence as the element's text alternative (...) If aria-labelledby is empty or undefined, the aria-label attribute, which defines an explicit text string, is used. (...)"
Created attachment 95866 [details] Patch
I noticed that aria-labelledby was being returned in a node's title, while aria-label was being returned in a node's description. This change seems to add aria-labelledby text to a node's description, but should it also be removed from the title? Also, I seem to remember the last time I added an accessibility layout test that this test needs to be suppressed from running on non-Mac platforms, because the accessibilityController implementation is currently Mac-specific.
Here is what the Accessibility Programming Guidelines for Cocoa (http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Accessibility/cocoaAXOverview/cocoaAXOverview.html#//apple_ref/doc/uid/20001057-BAJBCJHJ) say of the two currrently used attributes: NSAccessibilityTitleAttribute: "This attribute is required for any user interface object that displays a visible text title as part of its visual interface... Note that the title attribute is not intended to contain static text that serves as the title for a user interface element, but that is not part of its visual interface. For objects that are accompanied by separate static text titles and descriptions, use the NSAccessibilityTitleUIElementAttribute and NSAccessibilityServesAsTitleForUIElementsAttribute attributes (described in “Provide Descriptive Information for All Elements”)." (For a moment I thought NSAccessibilityTitleUIElementAttribute might be appropriate, but hacking it in didn't seem to work, even dodging the issue of having more than one value for aria-labelledby.) NSAccessibilityDescriptionAttribute: "Available in Mac OS X version 10.4 and later, this attribute contains a localized string that describes the object’s purpose. All user interface objects that do not display a text title (and therefore do not include the title attribute) should include the description attribute to allow an assistive application to describe the purpose of the object to the user." So it seems like it is appropriate to use the value from aria-labelledby, but not aria-label, as the NSAccessibilityTitle attribute; perhaps VoiceOver's ignoring it when generating the accessibility object hierarchy it is a quirk or bug? This also makes me wonder whether it would be better to have the AccessibilityObjectWrapper return the value of AccessibilityRenderObject::title() for NSAccessibilityDescriptionAttribute in the case where NSAccessibilityDescriptionAttribute is empty, rather than having the logic in AccessibilityRenderObject. I'd really appreciate some more guidance -- the ARIA spec gives virtually no helpful information about this case, and I can't find any documentation on what VoiceOver looks for. As for suppressing the test on non-Mac platforms; I'm not sure how to do that, is it something that is done in code?
I don't have a strong opinion on title vs description; I think it's more important that WebCore presents something consistent; the Mac port can choose to map those to the NSAccessibility APIs however it wants. Here's a simpler example: take this button: <button>Alpha</button> Right now WebKit returns "Alpha" as either the title or the description - I don't recall which one - but it's not both. Now, either of these two buttons should return "Beta" in place of whatever previously returned "Alpha": <button aria-label="Beta">Alpha</button> <button aria-labelledby="beta">Alpha</button> <span id="beta">Beta</span> The ARIA spec is pretty clear that this is what should happen. Be sure to think of this in layers; WebCore should implement something that's consistent with the ARIA spec, and each WebKit port then matches this up with the appropriate platform APIs - i.e. there's no reason each platform must have a concept of "title" and "description" that agree. For suppressing the test, look at files like LayoutTests/platform/win/Skipped - you'll need to add this test to each Skipped file for platforms that are unable to run this test. Currently that's every platform other than Mac.
Created attachment 96391 [details] Patch
Patch mostly unchanged; added another layout test to show the interaction between
Oops; meant to say, added another test to show the interaction between aria-label and aria-labelledby. After much thought, I think that this behaviour is correct; Chris, do you have any suggestions or thoughts? Still for me to do is to test this change in Chromium on Windows.
Comment on attachment 96391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96391&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1356 > + return ariaLabelledBy; This is used in title(); String ariaLabel = ariaLabeledByAttribute(); I think we need to re-organize what goes into title vs. description. I think we need to rename title to accessibilityName and use accessibilityDescription for aria-describedby But that's a big job. You should remove the call in ::title() so that it's only in ariaAXDescription. I'd guess you'd have some Mac test failures with this. I also suspect these test won't pass on GTK and Win, since they probably return different prefixes for title and description.
(In reply to comment #3) > Here is what the Accessibility Programming Guidelines for Cocoa (http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Accessibility/cocoaAXOverview/cocoaAXOverview.html#//apple_ref/doc/uid/20001057-BAJBCJHJ) say of the two currrently used attributes: > > NSAccessibilityTitleAttribute: "This attribute is required for any user interface object that displays a visible text title as part of its visual interface... Note that the title attribute is not intended to contain static text that serves as the title for a user interface element, but that is not part of its visual interface. For objects that are accompanied by separate static text titles and descriptions, use the NSAccessibilityTitleUIElementAttribute and NSAccessibilityServesAsTitleForUIElementsAttribute attributes (described in “Provide Descriptive Information for All Elements”)." > > (For a moment I thought NSAccessibilityTitleUIElementAttribute might be appropriate, but hacking it in didn't seem to work, even dodging the issue of having more than one value for aria-labelledby.) > > NSAccessibilityDescriptionAttribute: "Available in Mac OS X version 10.4 and later, this attribute contains a localized string that describes the object’s purpose. All user interface objects that do not display a text title (and therefore do not include the title attribute) should include the description attribute to allow an assistive application to describe the purpose of the object to the user." > > So it seems like it is appropriate to use the value from aria-labelledby, but not aria-label, as the NSAccessibilityTitle attribute; perhaps VoiceOver's ignoring it when generating the accessibility object hierarchy it is a quirk or bug? > > This also makes me wonder whether it would be better to have the AccessibilityObjectWrapper return the value of AccessibilityRenderObject::title() for NSAccessibilityDescriptionAttribute in the case where NSAccessibilityDescriptionAttribute is empty, rather than having the logic in AccessibilityRenderObject. I'd really appreciate some more guidance -- the ARIA spec gives virtually no helpful information about this case, and I can't find any documentation on what VoiceOver looks for. > > As for suppressing the test on non-Mac platforms; I'm not sure how to do that, is it something that is done in code? This is a tricky issue I have been thinking about for a long time. There's no good way to map between what ARIA states and what the mac platform expects without it being very ugly. Like you noted, what happens in you have an aria-label on an <button>. Does it become the AXTitle or AXDesc? If it's the AXDesc, does the original AXTitle get removed? those rules would be unique for many elements. My feeling is that we should have a method in AccessibilityObject called accessibilityName() -> that will map to AXDescription. and AXTitle will basically be empty. aria-describedby would then be mapped to TitleUIElement then we can have accessibilityDescription() which will probably just be aria-describedby and that can go to AXDescription
Oops, our comments crossed. Typing on a phone so sorry for brevity: your idea seems much like what i have been thinking, except I'm not clear on how you envisage aria-labelledby being handled.
Comment on attachment 96391 [details] Patch Attachment 96391 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8777629
(In reply to comment #8) > (From update of attachment 96391 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96391&action=review > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1356 > > + return ariaLabelledBy; > > This is used in title(); > String ariaLabel = ariaLabeledByAttribute(); > > I think we need to re-organize what goes into title vs. description. I think we need to rename title to accessibilityName and use accessibilityDescription for aria-describedby > > But that's a big job. You should remove the call in ::title() so that it's only in ariaAXDescription. I agree that file needs re-working and the mapping from aria attributes to VoiceOver attributes needs some more thought. I would love to help out with this effort, preferably with someone who has a better understanding of VoiceOver internals than me. > I'd guess you'd have some Mac test failures with this. I also suspect these test won't pass on GTK and Win, since they probably return different prefixes for title and description. Yes, it breaks: accessibility/aria-labelledby-on-input.html accessibility/aria-labelledby-overrides-label.html accessibility/aria-labelledby-stay-within.html - on mac. Would you suggest updating these tests to expect descriptions? I'm still working on testing the change in Chromium and on Windows.
Manually tested in Chromium on mac; works as expected.
(In reply to comment #12) > (In reply to comment #8) > > (From update of attachment 96391 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=96391&action=review > > > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1356 > > > + return ariaLabelledBy; > > > > This is used in title(); > > String ariaLabel = ariaLabeledByAttribute(); > > > > I think we need to re-organize what goes into title vs. description. I think we need to rename title to accessibilityName and use accessibilityDescription for aria-describedby > > > > But that's a big job. You should remove the call in ::title() so that it's only in ariaAXDescription. > > I agree that file needs re-working and the mapping from aria attributes to VoiceOver attributes needs some more thought. I would love to help out with this effort, preferably with someone who has a better understanding of VoiceOver internals than me. > > > I'd guess you'd have some Mac test failures with this. I also suspect these test won't pass on GTK and Win, since they probably return different prefixes for title and description. > > Yes, it breaks: > accessibility/aria-labelledby-on-input.html > accessibility/aria-labelledby-overrides-label.html > accessibility/aria-labelledby-stay-within.html > - on mac. > i'd have to see the actual result output to see if it's still OK > Would you suggest updating these tests to expect descriptions? > > I'm still working on testing the change in Chromium and on Windows.
Created attachment 97234 [details] Patch
I've adjusted the relevant tests in this patch; please let me know what you think.
Comment on attachment 97234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97234&action=review > LayoutTests/accessibility/aria-description-prefers-aria-labelledby.html:1 > +17390411<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> does this test provide any extra coverage that aria-labelledby-overrides-aria-label.html does not? I would combine both of these tests into one if they do indeed test something different. What they're testing does not seem to be different enough to warrant two tests on the same checkin > LayoutTests/accessibility/aria-labelledby-overrides-aria-label.html:21 > + description("This tests that if aria-labelledby is used, then aria-label attributes are not used."); I would also verify that aria-labeledby works as expected > LayoutTests/accessibility/aria-labelledby-overrides-aria-label.html:44 > + shouldBe("obj.description", '"AXDescription: "'); these comments in this section seem superfluous, in that they merely state what is happening, not why. i would just remove them. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1350 > + const AtomicString& ariaLabelledBy = ariaLabeledByAttribute(); looks like labeled is spelled "labeled" in the rest of the code, so we should stick with that
(In reply to comment #17) > (From update of attachment 97234 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97234&action=review > > > LayoutTests/accessibility/aria-description-prefers-aria-labelledby.html:1 > > +17390411<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > does this test provide any extra coverage that aria-labelledby-overrides-aria-label.html does not? I would combine both of these tests into one if they do indeed test something different. What they're testing does not seem to be different enough to warrant two tests on the same checkin You are right; removed this test. > > LayoutTests/accessibility/aria-labelledby-overrides-aria-label.html:21 > > + description("This tests that if aria-labelledby is used, then aria-label attributes are not used."); > > I would also verify that aria-labeledby works as expected Done. > > LayoutTests/accessibility/aria-labelledby-overrides-aria-label.html:44 > > + shouldBe("obj.description", '"AXDescription: "'); > > these comments in this section seem superfluous, in that they merely state what is happening, not why. i would just remove them. Done. > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1350 > > + const AtomicString& ariaLabelledBy = ariaLabeledByAttribute(); > > looks like labeled is spelled "labeled" in the rest of the code, so we should stick with that Ok, I'll keep it consistent for now, but I think it would be better if it were spelled "labelled" throughout, as that is the spelling used in the standard (see bug 62351).
Created attachment 97376 [details] Patch
Comment on attachment 97376 [details] Patch r=me
Comment on attachment 97376 [details] Patch Clearing flags on attachment: 97376 Committed r89204: <http://trac.webkit.org/changeset/89204>
All reviewed patches have been landed. Closing bug.