Bug 128420

Summary: AX: [GTK] Implement computedRoleString in AccessibilityUIElement
Product: WebKit Reporter: James Craig <jcraig>
Component: AccessibilityAssignee: Joanmarie Diggs <jdiggs>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, dpino, jdiggs, mario, samuel_white
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Bug Depends on: 127447    
Bug Blocks: 145255    
Attachments:
Description Flags
Patch
none
Patch none

Description James Craig 2014-02-07 17:14:17 PST
AX: Implement computedRoleString in AccessibilityUIElement
skipping accessibility/roles-computedRoleString.html in patch for bug 127447.
Comment 1 Radar WebKit Bug Importer 2014-02-07 17:15:31 PST
<rdar://problem/16017915>
Comment 2 Radar WebKit Bug Importer 2014-02-07 17:16:35 PST
<rdar://problem/16017921>
Comment 3 Diego Pino 2014-03-04 11:57:36 PST
Created attachment 225799 [details]
Patch
Comment 4 Diego Pino 2014-03-04 12:01:00 PST
When running accessibility/roles-computedRoleString.html, there are still some tests that fail, but at least now there's an implementation and there are more tests passing than before.

Here is a list of the tests that are failing:

FAIL: pre -> tabpanel. Expected: group.
FAIL: section -> . Expected: region.
FAIL: select:not([multiple]) -> combobox. Expected: .
FAIL: select[multiple] -> list. Expected: listbox.
FAIL: option -> listitem. Expected: option.
FAIL: optgroup -> listitem. Expected: option.
FAIL: option -> listitem. Expected: option.
FAIL: option -> listitem. Expected: option.
FAIL: textarea -> . Expected: textbox.
FAIL: div[role="alert"] -> alertdialog. Expected: alert.
FAIL: div[role="group"] -> tabpanel. Expected: group.
FAIL: div[role="listbox"] -> list. Expected: listbox.
FAIL: div[role="option"] -> listitem. Expected: option.
FAIL: div[role="radiogroup"] -> tabpanel. Expected: radiogroup.
FAIL: div[role="region"] -> . Expected: region.
Comment 5 Mario Sanchez Prada 2014-03-07 04:56:01 PST
Comment on attachment 225799 [details]
Patch

I'm not sure about this patch mainly because it tries to match an ATK object with the text that would be returned by AccessibilityObject::computedRoleString() if we were in the WebCore space, based only on the AtkRole for the object.

The problem with that approach is that you are basing the mapping in information that has been already filtered and mapped before (core a11y objects -> ATK objects) and so you might not have now all the required information now to be able to print what the Mac platform would print here, because the roles exposed by ATK (and even the a11y hierarchy, which is usually flattened) are not a 1:1 match with the roles from WebCore.

In the other hand, I do not see how to reliably extract that information in the GTK port without directly inspecting the AccessibilityObject wrapped by the ATK object, but by doing that you would break the encapsulation and I'm not sure what the benefit of testing such a thing would be in the end, other than testing what the Mac platform is testing already. And that would anyway be exclusively related to the needs of the WebInspector, not the needs of platform specific Assistive Technologies (such as ORCA), which is what we normally test in the other cases (and why we only use ATK APIs from here).

So, IMHO, this function should either remain unimplemented in ATK based ports or it should be implemented, as an exception, by breaking the encapsulation and getting the information directly from the wrapped WebCore::AccessibilityObject.
Comment 6 Diego Pino 2014-03-19 06:03:31 PDT
(In reply to comment #5)

> So, IMHO, this function should either remain unimplemented in ATK based ports or it should be implemented, as an exception, by breaking the encapsulation and getting the information directly from the wrapped WebCore::AccessibilityObject.

Thanks for the long explanation, now everything is much clearer.

So, it seems there's no good solution for this. I don't like the solution of breaking encapsulation either. 

Now the inspector includes an Accessibility tab (in the Node panel) that helps examining the aria information of a DOM element (for instance it prints out aria related information of a node (role, aria-required, aria-ignored, aria-checked, etc)). Without computedRoleString implemented it prints out "No exact ARIA role match". How much valuable is that information, I don't know. Is it worth to break encapsulation to get the role printed? And even in that case, it would be the Mac role, as you explained, which maybe useless for GTK.

In case of breaking encapsulation and relying on the computedRoleString function of WebCore I would append a label in the GTK port stating that the role is the role of MAC. Something like "Role: [MAC] link", for instance.

On the other hand, I think it would be good to differentiate the case of "No exact ARIA role match" from the case of not computing the role string. When the role is the empty string or "unknown" the inspector prints "No exact ARIA role match". I would print a "Not supported" string if computedRoleString returns is not implemented (it would simply return the empty string in that case). But that may be another bug.
Comment 7 Mario Sanchez Prada 2014-03-26 04:05:17 PDT
(In reply to comment #6)
> (In reply to comment #5)
> 
> > So, IMHO, this function should either remain unimplemented in ATK based ports or it should be implemented, as an exception, by breaking the encapsulation and getting the information directly from the wrapped WebCore::AccessibilityObject.
> 
> Thanks for the long explanation, now everything is much clearer.
> 

You're welcome

> So, it seems there's no good solution for this. I don't like the solution of breaking encapsulation either. 
> 
> Now the inspector includes an Accessibility tab (in the Node panel) that 
> helps examining the aria information of a DOM element (for instance it prints 
> out aria related information of a node (role, aria-required, aria-ignored, 
> aria-checked, etc)). Without computedRoleString implemented it prints out "No 
> exact ARIA role match". How much valuable is that information, I don't know. 
> Is it worth to break encapsulation to get the role printed? And even in that 
> case, it would be the Mac role, as you explained, which maybe useless for 
> GTK.
> 
Exactly. I'm not sure either about how useful that would be for GTK other than for easily inspecting the accessibility role given by WebCore to the a11y object wrapping a given element in the DOM. I guess this specific use case might be useful for debugging purposes: with Accerciser you could see the ATK role, but with the inspector you could see the WebCore accessibility role, which might be useful sometimes when you don't want to use gdb/printfs just to know the role that is being mapped.

But the thing is that, if the only interesting use case is this one (debugging purposes in ATK platforms), then it would probably be better to just give up on trying to match the computed role strings that are output in the Mac platform and just print the associated role names for the AtkRole of the object exposed.

In other words, just call atk_role_get_name(role) in WKTR's computeRoleString() and return either the output of it (when not null) or a hardcoded string saying "Not supported", "No exact role match" or the like.

Of course, you would need to add platform specific expectations for that test, but I think this would be cleaner than the previous approach, and maybe useful for debugging purposes as well (it would avoid devs to have to open Accerciser just to know the ATK role of an element)

> In case of breaking encapsulation and relying on the computedRoleString 
> function of WebCore I would append a label in the GTK port stating that the 
> role is the role of MAC. Something like "Role: [MAC] link", for instance.
> 
Honestly, I would not append anything and would just have the WebCore role showing up there, if any. But I finally don't think breaking encapsulation is worth it. If any, I would go for printing the name of the AtkRole and providing platform specific expectations (see above).

> On the other hand, I think it would be good to differentiate the case of "No
> exact ARIA role match" from the case of not computing the role string. When 
> the role is the empty string or "unknown" the inspector prints "No exact ARIA 
> role match". I would print a "Not supported" string if computedRoleString 
> returns is not implemented (it would simply return the empty string in that 
> case). But that may be another bug.

That's a good idea, but I think it could be integrated in this patch without much trouble, tbh.
Comment 8 Joanmarie Diggs 2015-05-21 18:56:36 PDT
Created attachment 253573 [details]
Patch
Comment 9 Joanmarie Diggs 2015-05-21 19:03:15 PDT
The attached does the following:

1. Exposes all ARIA role values via the "xml-roles" AtkObject attribute as per http://www.w3.org/TR/core-aam-1.1/#roleMappingGeneralRules

2. Exposes the computed role string value via the "computed-role" AtkObject attribute. That makes it available for testing via ATK/AT-SPI2. Object attributes are cheap and where you put stuff that doesn't belong anywhere else.

3. Implements computedRoleString, tests, etc.

Review appreciated.
Comment 10 WebKit Commit Bot 2015-05-21 23:12:17 PDT
Comment on attachment 253573 [details]
Patch

Clearing flags on attachment: 253573

Committed r184754: <http://trac.webkit.org/changeset/184754>
Comment 11 WebKit Commit Bot 2015-05-21 23:12:28 PDT
All reviewed patches have been landed.  Closing bug.