The aria-selected attribute is supported on gridcell, option, row, tab, columnheader, rowheader, and treeitem. As a result, AtkSelection should be implemented on the element with the required context role, and ATK_STATE_SELECTABLE and ATK_STATE_SELECTED should be supported on the selectable elements. With the exception of listbox and their children, this is not the case. In addition, ATK/AT-SPI2 menubars and menus are expected to have the aforementioned support. While aria-selected is not supported on menuitem roles, aria-activedescendant and focused state can be used instead.
<rdar://problem/29309753>
Created attachment 295049 [details] Patch
Comment on attachment 295049 [details] Patch Chris: I'm tossing this at EWS before officially asking for review. But if you could take a look when you have several spare moments, as opposed to just one. ;) 1. I took several guesses about what was right for your platform and the resulting changes can be seen in the Mac -expected.txt files. 2. I marked a couple of items with FIXME comments because there is code that seems deliberate (TableRole supports selected children, isChecked() is being called to find the selected tab), but whose correctness I question. Thanks much in advance!!
Comment on attachment 295049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295049&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3298 > + default: > + return false; There’s a pattern where we omit the "default" case from a switch and instead put the default behavior after the switch statement. The pattern works in a natural way when a switch has return statements instead of break statements in each case. Doing it that way makes it so that we get compiler warnings if we forget to list any enumeration values, especially useful in cases where we add a new enumeration value and don’t think about this function. Would that be a good choice here? Or is it better to just count on test coverage to make sure we get this right? > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3388 > + default: > + return; Same comment.
Comment on attachment 295049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295049&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3298 >> + return false; > > There’s a pattern where we omit the "default" case from a switch and instead put the default behavior after the switch statement. The pattern works in a natural way when a switch has return statements instead of break statements in each case. Doing it that way makes it so that we get compiler warnings if we forget to list any enumeration values, especially useful in cases where we add a new enumeration value and don’t think about this function. > > Would that be a good choice here? Or is it better to just count on test coverage to make sure we get this right? In this particular case, I don't think it would be a good choice. There are (currently) 134 AccessibilityRole values. >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3388 >> + return; > > Same comment. Similar response: 134 values. However, in this case, there's an early return. So what would make sense is to make the default case ASSERT_NOT_REACHED().
Comment on attachment 295049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=295049&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2180 > + // Elements that are considered selectable by assistive technologies this comment is probably superfluous > Source/WebCore/accessibility/AccessibilityObject.cpp:520 > + for (AccessibilityObject* parent = parentObject(); parent; parent = parent->parentObject()) { you could save like two lines by using return AccessibilityObject::matchedParent(*this, false, [] (const AccessibilityObject& object) { return object.roleValue() == role; }); > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3359 > + if (!canHaveSelectedChildren() && role != TableRole) I think we can remove table here, especially if no tests fail. on Mac, tables can have selected children. maybe that's why it was added here, but I also don't think we can have selected children if no layout tests fail, then I think we're good here >>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3388 >>> + return; >> >> Same comment. > > Similar response: 134 values. However, in this case, there's an early return. So what would make sense is to make the default case ASSERT_NOT_REACHED(). yea we shouldn't hit the default case here
Created attachment 295262 [details] Patch
(In reply to comment #6) > Comment on attachment 295049 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=295049&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2180 > > + // Elements that are considered selectable by assistive technologies > > this comment is probably superfluous Removed. > you could save like two lines by using > > return AccessibilityObject::matchedParent(*this, false, [] (const > AccessibilityObject& object) { return object.roleValue() == role; }); > > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3359 > > + if (!canHaveSelectedChildren() && role != TableRole) Done. > I think we can remove table here, especially if no tests fail. > on Mac, tables can have selected children. maybe that's why it was added > here, but I also don't think we can have selected children > if no layout tests fail, then I think we're good here Tables can have selected children on my platform too. But the sorts of tables that can are equivalent to what gets assigned the GridRole AccessibilityRole. Anyway, thank you for the explanation and suggestion. No tests failed on my Mac. Tossing it at EWS to see if anything else has failures as a result of removing table. > > Similar response: 134 values. However, in this case, there's an early return. So what would make sense is to make the default case ASSERT_NOT_REACHED(). > > yea we shouldn't hit the default case here ASSERT_NOT_REACHED() added. Thanks for the review!
Comment on attachment 295262 [details] Patch Clearing flags on attachment: 295262 Committed r208929: <http://trac.webkit.org/changeset/208929>
All reviewed patches have been landed. Closing bug.