Bug 128296 - AX: AccessibilityObject::computedRoleString() should cover additional cases like option, etc.
Summary: AX: AccessibilityObject::computedRoleString() should cover additional cases l...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar, WPTImpact
Depends on: 127447
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-05 21:38 PST by James Craig
Modified: 2023-03-21 00:17 PDT (History)
10 users (show)

See Also:


Attachments
Patch (30.92 KB, patch)
2016-07-16 17:49 PDT, Joanmarie Diggs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2014-02-05 21:38:12 PST
AccessibilityObject::computedRoleString() from bug 127447 should cover additional cases like option, etc. that can't currently be returned from the ReverseRoleMap.
Comment 1 Radar WebKit Bug Importer 2014-02-05 21:38:43 PST
<rdar://problem/15998500>
Comment 2 James Craig 2014-02-07 15:15:30 PST
Some other unexpected results from LayoutTests/accessibility/roles-computedRoleString.html

footer -> . 
form -> . 
th -> gridcell. 
[role=directory] -> list.
[role=columnheader] -> gridcell. 
[role=rowheader] -> gridcell. 
[role=treegrid] -> grid.


Additional issues noticed in Web Accessibility Node Inspector:

body -> group.
Comment 3 James Craig 2014-02-10 18:26:00 PST
also check presentation role and layout tables
Comment 4 Joanmarie Diggs 2016-07-16 17:49:53 PDT
Created attachment 283856 [details]
Patch
Comment 5 Joanmarie Diggs 2016-07-16 18:04:36 PDT
Handling option appears to have been done. Ditto for:

> form -> . 
> th -> gridcell. 
> [role=columnheader] -> gridcell. 
> [role=rowheader] -> gridcell. 

Remaining from your list were:

> footer -> . 
> [role=directory] -> list.
> [role=treegrid] -> grid.

And also textfield. The attached patch handles those.

As for this:

> Additional issues noticed in Web Accessibility Node Inspector:
>
> body -> group.

Confirmed. However, immediately above the role string, there is an indication that the element is "ignored". If it were not ignored for some reason, I do believe GroupRole would be the right mapping. That said, I see your point.

I'm not sure how it should be addressed, however. If AccessibilityObject::computedRoleString() just returns an empty string for items for which accessibilityIsIgnored() is true, the inspector still displays the UI for the role, but indicates there was no exact ARIA role match. *Maybe* that's what we want to do for body (even though I would argue there IS an exact ARIA role match). But what about an img element which is ignored? There IS an exact ARIA role match for img, and to say otherwise might be just as confusing (if not more).

So I think we either need to assume (hope) that consumers of the role will not be confused because they will also see "Ignored" immediately above the role. Or I think we need to completely remove the role UI (and what about UI for things like children?) for ignored elements.
Comment 6 Darin Adler 2016-07-17 09:59:41 PDT
Comment on attachment 283856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283856&action=review

Changes all look OK assuming that we have enough test coverage.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2196
> +    if (role == DivRole && is<RenderTableCell>(renderer()))

What guarantees the renderer is valid when this function is called? Normally we have to perform a layout before looking at a renderer.
Comment 7 Joanmarie Diggs 2016-07-17 12:52:39 PDT
Thanks for the review! Regarding this:

> What guarantees the renderer is valid when this function is called? Normally
> we have to perform a layout before looking at a renderer.

computedRoleString() just looks up an existing WebCore AccessibilityRole in a hash table and returns the corresponding ARIA role name, if one exists. There are two consumers of this information: people using the inspector and (some) assistive technologies. In the former case, shouldn't it be safe to assume that the renderer for the object being inspected is valid?

In the latter case, if the renderer is not valid at the time we consider creating an AccessibilityObject for it, I believe we wouldn't create the AccessibilityObject and thus computedRoleString() would not be called; if the renderer was valid but no longer is, I'm not convinced we'd want to return a different value for the role string. But I'll give it some more thought for the general case.

(For the specific case you're asking about, I added the lines of code in question just to facilitate hassle-free shared accessibility layout tests. "Group" on an AtkObject with ATK_ROLE_SECTION (aka DivRole) would be ignored by ATs on my platform because a real ARIA group would have ATK_ROLE_PANEL. And in terms of the inspector, what matters is that the table cell is not exposed to ATs as an accessible table cell. That it's not exposed as a cell can be expressed by "group" or by "no exact ARIA role match.")
Comment 8 Joanmarie Diggs 2016-07-17 14:35:00 PDT
Comment on attachment 283856 [details]
Patch

(Clearing the r+ because I think I'll do something for the general case of non-valid renderers, objects being detached/destroyed, etc.)
Comment 9 James Craig 2016-07-19 01:37:55 PDT
Reassigning the bug from Aaron to Joanie so there is no work duplication.
Comment 10 James Craig 2023-03-21 00:17:45 PDT
Tagging WPTImpact because this may fail with some of the impending role tests coming with the Interop 2023 Accessibility Investigation.