WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
164865
AX: [ATK] Implement selection interface and states for elements supporting aria-selected and for menu roles
https://bugs.webkit.org/show_bug.cgi?id=164865
Summary
AX: [ATK] Implement selection interface and states for elements supporting ar...
Joanmarie Diggs
Reported
2016-11-17 05:33:39 PST
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.
Attachments
Patch
(30.84 KB, patch)
2016-11-17 05:58 PST
,
Joanmarie Diggs
no flags
Details
Formatted Diff
Diff
Patch
(30.52 KB, patch)
2016-11-19 02:40 PST
,
Joanmarie Diggs
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2016-11-17 05:33:52 PST
<
rdar://problem/29309753
>
Joanmarie Diggs
Comment 2
2016-11-17 05:58:29 PST
Created
attachment 295049
[details]
Patch
Joanmarie Diggs
Comment 3
2016-11-17 06:04:31 PST
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 4
2016-11-17 08:57:26 PST
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 5
2016-11-17 09:34:55 PST
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 6
2016-11-19 00:16:29 PST
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
Comment 7
2016-11-19 02:40:44 PST
Created
attachment 295262
[details]
Patch
Joanmarie Diggs
Comment 8
2016-11-19 02:47:11 PST
(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 9
2016-11-19 13:12:33 PST
Comment on
attachment 295262
[details]
Patch Clearing flags on attachment: 295262 Committed
r208929
: <
http://trac.webkit.org/changeset/208929
>
WebKit Commit Bot
Comment 10
2016-11-19 13:12:37 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug