Bug 164865 – AX: [ATK] Implement selection interface and states for elements supporting aria-selected and for menu roles

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

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.

[Radar WebKit Bug Importer]

<rdar://problem/29309753>

[Joanmarie Diggs]

Created attachment 295049 [details]
Patch

[Joanmarie Diggs]

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!!

[Darin Adler]

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.

[Joanmarie Diggs]

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

[chris fleizach]

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

[Joanmarie Diggs]

Created attachment 295262 [details]
Patch

[Joanmarie Diggs]

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

[WebKit Commit Bot]

Comment on attachment 295262 [details]
Patch

Clearing flags on attachment: 295262

Committed r208929: <http://trac.webkit.org/changeset/208929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.