[ATK] Missing aria roles mappings.

[Lukasz Gajowy]

Based on: http://www.w3.org/TR/2009/WD-wai-aria-implementation-20090224/#mapping_role
most of missing roles can be mapped to coresponding ATK roles.

alert -> ATK_ROLE_ALERT
alertdialog -> ATK_ROLE_DIALOG
application -> ATK_ROLE_EMBEDDED
dialog -> ATK_ROLE_DIALOG
document -> ATK_ROLE_DOCUMENT_FRAME
marquee -> ATK_ROLE_PANE
status -> ATK_ROLE_STATUSBAR
tooltip -> ATK_ROLE_TOOL_TIP
tree -> ATK_ROLE_TREE
treegrid -> ATK_ROLE_TREE_TABLE
treeitem -> ATK_ROLE_LIST_ITEM

[Lukasz Gajowy]

Created attachment 205576 [details]
New mappings

I didn't map two roles: marquee and treegrid because i wasn't sure of that they should be mapped the way it is said in the document (http://www.w3.org/TR/2009/WD-wai-aria-implementation-20090224/#mapping_role). Also there is no such role as ATK_ROLE_PANE which is the proposed ATK role for marquee role mapping.

[chris fleizach]

Comment on attachment 205576 [details]
New mappings

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

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:595
> +    case DocumentRole:

i don't think DocumentRole is a web area role. It's its own separate thing in ARIA

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:601
> +    case LandmarkApplicationRole:

this one is probably wrong too. This is a way for websites to say that the containing elements behave like an application that uses its own keystrokes

[Mario Sanchez Prada]

Comment on attachment 205576 [details]
New mappings

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

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:595
>> +    case DocumentRole:
> 
> i don't think DocumentRole is a web area role. It's its own separate thing in ARIA

I think this code is not meant to be read as implying that Document role is a web area role, but that those two happen to be mapped to the same ATK role, even though they're different things.

For the case of the WebArea it has been historically mapped always to ATK_ROLE_DOCUMENT_FRAME because it represents the whole web page, the "document" to the eyes or ATs like Orca. For the other case, it seems to be the recommended mapping as per the W3C spec. So, I'd say this might be the right mapping after all for ATK.

>> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:601
>> +    case LandmarkApplicationRole:
> 
> this one is probably wrong too. This is a way for websites to say that the containing elements behave like an application that uses its own keystrokes

I agree with this concern too.

[Lukasz Gajowy]

Created attachment 206867 [details]
New mappings - fixed according to Mario's and Chris' leads.

I removed LandmarkApplicationRole mapping as it was advised.

[Lukasz Gajowy]

Created attachment 206868 [details]
New mappings - fixed according to Mario's and Chris' leads.

Previous patch had a failing test - my bad, sorry.

[Mario Sanchez Prada]

Comment on attachment 206868 [details]
New mappings - fixed according to Mario's and Chris' leads.

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

Unofficially reviewing the patch, see some comments below...

> LayoutTests/ChangeLog:11
> +        * platform/efl/accessibility/aria-mappings-expected.txt: Added.
> +        * platform/efl/accessibility/aria-mappings.html: Added.

It would be great if we could have these tests running as well in the GTK platform, since both that one and EFL share the ATK bits.

One option would be to duplicate the test and the associated expectations inside platform/gtk/accessibility, but perhaps it would be better if we just moved this test and its expectaions down to LayoutTests/accessibility and add a line to platform/mac/TestExpectations and platform/win/TestExpectations, since those would probably need to provide different expectations due to different role names, at least in the "win" platform.

> LayoutTests/platform/efl/accessibility/aria-mappings.html:31
> +var roles=new Array("'AXRole: AXAlert'",
> +                    "'AXRole: AXAlert'",
> +                    "'AXRole: AXDialog'",
> +                    "'AXRole: AXWebArea'",
> +                    "'AXRole: AXStatusBar'",
> +                    "'AXRole: AXUserInterfaceTooltip'",
> +                    "'AXRole: AXTree'",
> +                    "'AXRole: AXListItem'");

It might be just a personal preference, but I found clearer to use this literal strings where they are actually needed (below) than putting all of them together in an array and dereferencing them later from here. After all, each of them are only used once.

> LayoutTests/platform/efl/accessibility/aria-mappings.html:32
> +

I miss here (between description() and the check for accessibilityController), the usual snippet to explicitly check for the testRunner object and select the 'dumpAsText' mode:

if (window.testRunner) {
  testRunner.dumpAsText();

  if (window.accessibilityController) {

     ...
  }
}

> LayoutTests/platform/efl/accessibility/aria-mappings.html:34
> +    document.getElementById("body").focus();

You don't really need to do this, you can do document.body.focus() and remove the need to have an id set for the body tag.

> LayoutTests/platform/efl/accessibility/aria-mappings.html:38
> +    shouldBe("body.childAtIndex(0).role" , roles[0]);

In case we consider the option to move this test and its expectations to LayoutTests/accessibility, I think we should then avoid using the shouldBe() function for checking the role, since that output will be different depending on the platform (e.g. win outputs different strings than GTK and Mac).

My personal preference would be just to use debug() with something like this:

  debug("Role for 'alert' div is: " + body.childAtIndex(0).role);
  debug("Role for 'alertdialog' div is: " + body.childAtIndex(1).role);
  ...
  debug("Role for 'treeitem' div is: " + body.childAtIndex(7).role);

...and then update the expectations file accordingly to match this new behaviour.

> LayoutTests/platform/efl/accessibility/aria-mappings.html:58
> +    debug("\nrole=\"treeitem\" is an option item of a tree.");

Instead of prepending '\n' to force having that extra line between every two lines (for readability, I guess), you might consider just putting a debug("<br>") line between blocks, which is done in some more tests out there.

[chris fleizach]

Comment on attachment 206868 [details]
New mappings - fixed according to Mario's and Chris' leads.

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

>> LayoutTests/platform/efl/accessibility/aria-mappings.html:38
>> +    shouldBe("body.childAtIndex(0).role" , roles[0]);
> 
> In case we consider the option to move this test and its expectations to LayoutTests/accessibility, I think we should then avoid using the shouldBe() function for checking the role, since that output will be different depending on the platform (e.g. win outputs different strings than GTK and Mac).
> 
> My personal preference would be just to use debug() with something like this:
> 
>   debug("Role for 'alert' div is: " + body.childAtIndex(0).role);
>   debug("Role for 'alertdialog' div is: " + body.childAtIndex(1).role);
>   ...
>   debug("Role for 'treeitem' div is: " + body.childAtIndex(7).role);
> 
> ...and then update the expectations file accordingly to match this new behaviour.

i agree with this. if we change this to debug we can probably run this test on all platforms

[Lukasz Gajowy]

Created attachment 208123 [details]
Patch set #4

Added changes in DumpRenderTree same as in WebKitTestRunner so that tests wouldn't fail in WebKit 1.

[Mario Sanchez Prada]

Comment on attachment 208123 [details]
Patch set #4

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

Thanks for the new patch. Yet another informal review here...

> LayoutTests/accessibility/aria-mappings-expected.txt:28
> +role="document" is a region containing related information that is declared as document content, as opposed to a web application.
> +Role for 'document' div is: AXRole: AXWebArea

I just realized that this expectation is wrong. We should print "AXDocument" here instead of "AXWebArea", since that's a div with the document role, not an actual web area object.

Problem is that DRT/WKTR currently prints AXWebArea for anything with the DOCUMENT_FRAME role, so we probably would need to fix that in DRT/WKTR first, so they are at least a bit smarter when it comes to determine what it's actually a web area and what it's simply an element with the 'document' role.

We can probably incorporate that fix as part of this patch if it's a small change, since that situation was not an issue before as we did not handle the DocumentRole role until now in GTK/EFL

> LayoutTests/platform/mac/TestExpectations:1337
> +
> +

Extra lines not needed

> LayoutTests/platform/mac/TestExpectations:1344
> +webkit.org/b/117729 accessibility/aria-mappings.html [ Skip ]

I believe that we normally don't skip tests unless we have good reasons to do it (e.g. timing out), but set the expectations to Failure

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:60
> +    case ATK_ROLE_DIALOG:
> +        return "AXRole: AXDialog";

This patch won't apply against latest webkit since we no longer return a String with the "AXRole: " prefix here

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:132
> +    case ATK_ROLE_STATUSBAR:
> +            return "AXRole: AXStatusBar";

Ditto

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:174
> +    case ATK_ROLE_DIALOG:
> +        return "AXRole: AXDialog";

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:246
> +    case ATK_ROLE_STATUSBAR:
> +        return "AXRole: AXStatusBar";

Ditto.

[Lukasz Gajowy]

Created attachment 208256 [details]
Patch set #5

Thank you all for your effort! 
Here is another patch according to Mario's latest leads.

[WebKit Commit Bot]

Attachment 208256 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/aria-mappings-expected.txt', u'LayoutTests/accessibility/aria-mappings.html', u'LayoutTests/platform/mac/TestExpectations', u'LayoutTests/platform/win/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp']" exit_code: 1
Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lukasz Gajowy]

Created attachment 208258 [details]
Patch set #5 - corrected style

[Mario Sanchez Prada]

Comment on attachment 208258 [details]
Patch set #5 - corrected style

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

Thanks for the patch, although I feel like the proposed solution to show AXDocument role might not be entirely correct. See my comments below...

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:107
> -        return "AXWebArea";
> +        return "AXDocument";

This is wrong. We still want to the web area to show up with a "AXWebArea" role so we are consistent with other tests that already expect that result, and so I suspect this might be breaking other tests.

What I was thinking was something more like trying to figure out, for an object with role ATK_ROLE_DOCUMENT_FRAME, whether it has that role because of being the web area object or because of something else (e.g. specifiying the role attribute), and return the result accordingly.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:194
> -        return "AXWebArea";
> +        return "AXDocument";

Same consideration applies here.

[chris fleizach]

Comment on attachment 208258 [details]
Patch set #5 - corrected style

- review based on Marios last comments

[Lukasz Gajowy]

Created attachment 209081 [details]
Patch set #6.

Applied Mario's suggestions.

[chris fleizach]

Comment on attachment 209081 [details]
Patch set #6.

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

> LayoutTests/ChangeLog:10
> +

extra new lines not needed here

> LayoutTests/accessibility/aria-mappings.html:29
> +            var body = accessibilityController.focusedElement;

instead of doing the focus thing you can do

var body = accessibilityController.accessibleElementById("body");

> LayoutTests/platform/mac/TestExpectations:1308
> +webkit.org/b/117729 accessibility/aria-mappings.html [ Failure ]

is this a real failure on the Mac or just missing expectations? It looks like it just needs expected results in which case we probably don't need these lines. we can upload the patch, watch the bots and then get the new results

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:502
> +        attributeSet = addToAtkAttributeSet(attributeSet, "AXWebArea", "true");

all the other attributes are lowercase and use - between words. seems like this should follow suite and be something like "is-webarea"

[Lukasz Gajowy]

Comment on attachment 209081 [details]
Patch set #6.

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

>> LayoutTests/accessibility/aria-mappings.html:29
>> +            var body = accessibilityController.focusedElement;
> 
> instead of doing the focus thing you can do
> 
> var body = accessibilityController.accessibleElementById("body");

Doing:

document.body.focus();
var body = accessibilityController.focusedElement;

seems to be common behavior as I've found some tests in accessibility that obtain body this way. Furthermore, if we want to use:
var body = accessibilityController.accessibleElementById("body");

we need to add the role attribute to the <body> tag. Otherwise accessibilityController.accessibleElementById("body"); will return null and the test will crash. 
Also I haven't found tests using accessibilityController.accessibleElementById("body"); to obtain body object. 
Is it ok if I leave it as it is?

[chris fleizach]

Comment on attachment 209081 [details]
Patch set #6.

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

>>> LayoutTests/accessibility/aria-mappings.html:29
>>> +            var body = accessibilityController.focusedElement;
>> 
>> instead of doing the focus thing you can do
>> 
>> var body = accessibilityController.accessibleElementById("body");
> 
> Doing:
> 
> document.body.focus();
> var body = accessibilityController.focusedElement;
> 
> seems to be common behavior as I've found some tests in accessibility that obtain body this way. Furthermore, if we want to use:
> var body = accessibilityController.accessibleElementById("body");
> 
> we need to add the role attribute to the <body> tag. Otherwise accessibilityController.accessibleElementById("body"); will return null and the test will crash. 
> Also I haven't found tests using accessibilityController.accessibleElementById("body"); to obtain body object. 
> Is it ok if I leave it as it is?

It should work ok, but sometimes there are cross-platform issues with what is the body element so that childAtIndex(0) for body *may* not be the same thing.

Ideally you would just wrap your content in a <div> and reference that div.

It's not crucial to make that change though

[Lukasz Gajowy]

Created attachment 209332 [details]
Patch set #7

I modified the test that it should be cross-platform. 
Now all the accessibility elements have their id and their role is obtained by using accessibilityController.accessibleElementById(<element's id>).role methods. The body's id was unnecessary so i removed it too.

I also deleted the lines from TestExpectations files, as you advised.

[Build Bot]

Comment on attachment 209332 [details]
Patch set #7

Attachment 209332 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1538040

New failing tests:
accessibility/aria-mappings.html

[Build Bot]

Created attachment 209334 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 209332 [details]
Patch set #7

Attachment 209332 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1492064

New failing tests:
accessibility/aria-mappings.html

[Build Bot]

Created attachment 209339 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[chris fleizach]

Comment on attachment 209332 [details]
Patch set #7

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

> LayoutTests/accessibility/aria-mappings-expected.txt:4
> +document role

this probably needs to be a platforms specific result. We can gather the results for Mac and other platforms once it lands

[Build Bot]

Comment on attachment 209332 [details]
Patch set #7

Attachment 209332 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1543122

New failing tests:
accessibility/aria-mappings.html

[Build Bot]

Created attachment 209399 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[chris fleizach]

Comment on attachment 209332 [details]
Patch set #7

r- until we fix the layout tests

[Lukasz Gajowy]

Created attachment 211766 [details]
Patch set #8

Added aria-mappings-expected.txt in LayoutTests/platform/mac folder.

[chris fleizach]

Comment on attachment 211766 [details]
Patch set #8

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

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:601
> +    case DocumentRole:

Technically the DocumentRole is not the same as a frame... Screenreaders are supposed to do different things when seeing a DocumentRole
http://www.w3.org/TR/wai-aria/roles#document

However, GTK may not have that notion of the difference yet. I imagine this might be changed in the future

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:612
> +        roleStringWithPrefix.set(g_strdup_printf("AXRole: AXWebArea"));

can you make this an if/else block and then just have one JSStringCreateWith... method ?

[Joanmarie Diggs]

Just curious if the roles that were added in Atk 2.10 are being taken into consideration (https://developer.gnome.org/atk/stable/AtkObject.html#AtkRole). The W3C document looks a bit out of date -- and with at least one typo since, as you point out, there is no ATK_ROLE_PANE. :)

ATK_ROLE_TABLE_ROW
A row in a table. Since: ATK-2.1.0

ATK_ROLE_TREE_ITEM
An object that represents an element of a tree. Since: ATK-2.1.0

ATK_ROLE_DOCUMENT_SPREADSHEET
A document frame which contains a spreadsheet. Since: ATK-2.1.0

ATK_ROLE_DOCUMENT_PRESENTATION
A document frame which contains a presentation or slide content. Since: ATK-2.1.0

ATK_ROLE_DOCUMENT_TEXT
A document frame which contains textual content, such as found in a word processing application. Since: ATK-2.1.0

ATK_ROLE_DOCUMENT_WEB
A document frame which contains HTML or other markup suitable for display in a web browser. Since: ATK-2.1.0

ATK_ROLE_DOCUMENT_EMAIL
A document frame which contains email content to be displayed or composed either in plain text or HTML. Since: ATK-2.1.0

ATK_ROLE_COMMENT
An object found within a document and designed to present a comment, note, or other annotation. In some cases, this object might not be visible until activated. Since: ATK-2.1.0

ATK_ROLE_LIST_BOX
A non-collapsible list of choices the user can select from. Since: ATK-2.1.0

ATK_ROLE_GROUPING
A group of related widgets. This group typically has a label. Since: ATK-2.1.0

ATK_ROLE_IMAGE_MAP
An image map object. Usually a graphic with multiple hotspots, where each hotspot can be activated resulting in the loading of another document or section of a document. Since: ATK-2.1.0

ATK_ROLE_NOTIFICATION
A transitory object designed to present a message to the user, typically at the desktop level rather than inside a particular application. Since: ATK-2.1.0

ATK_ROLE_INFO_BAR
An object designed to present a message to the user within an existing window. Since: ATK-2.1.0

[Mario Sanchez Prada]

Comment on attachment 211766 [details]
Patch set #8

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

(In reply to comment #30)
> Just curious if the roles that were added in Atk 2.10 are being taken into consideration 
> (https://developer.gnome.org/atk/stable/AtkObject.html#AtkRole).

That's a good point, perhaps we should use ATK_ROLE_DOCUMENT_FRAME for 'article' and 'document' ARIA roles, as stated in [1], and move the WebArea to be exposed as ATK_ROLE_DOCUMENT_WEB instead of ATK_ROLE_DOCUMENT_FRAME. What do you think Joanie?

[1] http://www.w3.org/TR/wai-aria-implementation/

> The W3C document looks a bit out of date -- and with at least one typo since, as you point out, there is no ATK_ROLE_PANE. :)

I think he meant ATK_ROLE_PANEL anyway :)

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:502
> +    if (coreObject->isWebArea())
> +        attributeSet = addToAtkAttributeSet(attributeSet, "is-webarea", "true");

I'm not sure exposing an object attribute "is-webarea" is the right thing to do, although considering it would only apply to the accessibility object for the webview, it might be not that bad after all.

However, as I mentioned above, perhaps a better solution is to expose DocumentRole as ATK_ROLE_DOCUMENT_FRAME and move exposure of WebAreaRole to ATK_ROLE_DOCUMENT_WEB, which seems to be what other implementors like chromium and mozilla are doing.

[Joanmarie Diggs]

(In reply to comment #31)
 
> That's a good point, perhaps we should use ATK_ROLE_DOCUMENT_FRAME for 'article' and 'document' ARIA roles, as stated in [1], and move the WebArea to be exposed as ATK_ROLE_DOCUMENT_WEB instead of ATK_ROLE_DOCUMENT_FRAME. What do you think Joanie?

Short answer: Sure

Longer answer: The intent behind the other roles is that, for example, Google Docs could tell rendering engines that a spreadsheet has role spreadsheet, a presentation role presentation, a word processing document role text, etc. That doesn't seem to be something in the ARIA spec -- at least not yet. Sadly that means screen readers like Orca have to guess. But hopefully one day, there will be a means via ARIA for WebKit to know what type of document the 'role=document' object is. And when that happens, ATK_ROLE_DOCUMENT_FRAME will be wrong. But that day isn't here, so ATK_ROLE_DOCUMENT_FRAME for the ARIA role document seems good. :)

Lastly, Orca is not yet expecting this new role yet. I'll clue Orca into it. But if you try Orca stable with your changes, it likely will not work as you expect.

Finally, just to be sure, given that the original proposal in the opening report says:

> treeitem -> ATK_ROLE_LIST_ITEM

I wanted to be sure you saw the ATK_ROLE_TREE_ITEM.

Hope this helps. Thanks for asking! :)

[chris fleizach]

(In reply to comment #32)
> (In reply to comment #31)
> 
> > That's a good point, perhaps we should use ATK_ROLE_DOCUMENT_FRAME for 'article' and 'document' ARIA roles, as stated in [1], and move the WebArea to be exposed as ATK_ROLE_DOCUMENT_WEB instead of ATK_ROLE_DOCUMENT_FRAME. What do you think Joanie?
> 
> Short answer: Sure
> 
> Longer answer: The intent behind the other roles is that, for example, Google Docs could tell rendering engines that a spreadsheet has role spreadsheet, a presentation role presentation, a word processing document role text, etc. That doesn't seem to be something in the ARIA spec -- at least not yet. Sadly that means screen readers like Orca have to guess. But hopefully one day, there will be a means via ARIA for WebKit to know what type of document the 'role=document' object is. And when that happens, ATK_ROLE_DOCUMENT_FRAME will be wrong. But that day isn't here, so ATK_ROLE_DOCUMENT_FRAME for the ARIA role document seems good. :)
> 
> Lastly, Orca is not yet expecting this new role yet. I'll clue Orca into it. But if you try Orca stable with your changes, it likely will not work as you expect.
> 
> Finally, just to be sure, given that the original proposal in the opening report says:
> 
> > treeitem -> ATK_ROLE_LIST_ITEM
> 
> I wanted to be sure you saw the ATK_ROLE_TREE_ITEM.
> 
> Hope this helps. Thanks for asking! :)

So does this patch need to be updated again, or are these suggestions for future? (the tree item thing seems actionable now)

[Joanmarie Diggs]

I was pinged by Lukasz in IRC to respond to this, so....

(In reply to comment #33)

> So does this patch need to be updated again, or are these suggestions for future? (the tree item thing seems actionable now)

Yes, the tree item thing is indeed actionable now. And calling a tree item a list item is just plain wrong imho. So if nothing else I would expect this patch to be updated again to address that.

Changing the document role now or dealing with it in the future is something that remains to be determined I suppose. With my Orca hat on, as long as I know what the plan is, I can adjust accordingly.

[Lukasz Gajowy]

Created attachment 216289 [details]
Patch set #9 - to be discussed

I fixed it as you proposed... but:
I don't know how to test the WebAreaRole mapping. This is not a a role that is invoked explicitly by writing the test: <div role="foobar">foobar role</div>
How can i test it different way? Should I test it?
The second thing is that document role and article role both are mapped to ATK_ROLE_DOCUMENT_FRAME and because of that they both return "AXDocument" in test. Is this behavior correct? "alert" and "alertdialog" roles seem to behave similarly.

[Mario Sanchez Prada]

Comment on attachment 216289 [details]
Patch set #9 - to be discussed

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

This looks better now. Thanks!

Still, setting r- for now because of a couple of issues in the patch. See my comments below...

(In reply to comment #35)
> Created an attachment (id=216289) [details]
> Patch set #9 - to be discussed
> 
> I fixed it as you proposed... but:
> I don't know how to test the WebAreaRole mapping. This is not a a role that is invoked explicitly by writing the test: <div role="foobar">foobar role</div>
> How can i test it different way? Should I test it?

Just get the accessible element for the <body> element and query the role for it. That should be ATK_ROLE_DOCUMENT_WEB, therefore mapped to AXWebArea

> The second thing is that document role and article role both are mapped to ATK_ROLE_DOCUMENT_FRAME and because of that they both return "AXDocument" in test. Is this behavior correct?

According to the WAI ARIA mapping table, that is correct. The only difference between those is that the article should also expose the object attribute "xml-roles:article" (according to the very last revision from November the 5th), but I would not add that bit for now unless Joanie gives us green light to do it so. And in any case that's something we can easily add later on, so for now I'd say "forget about that xml-roles attribute and just expose ATK_ROLE_DOCUMENT_FRAME" for article too.

> "alert" and "alertdialog" roles seem to behave similarly.

I'd say that for "alert" you should expose it as ATK_ROLE_ALERT and for "alertdialog" as ATK_ROLE_DIALOG and ignore, at least for now, the fact that you can't distinguish between a "dialog" and an "alertdialog" here.

> LayoutTests/accessibility/aria-mappings-expected.txt:21
> +role="alertdialog" is a dialog which contains an alert message.
> +Role for 'alertdialog' div is: AXRole: AXAlert

AlertDialogRole should be mapped to a dialog role, according to http://www.w3.org/TR/wai-aria-implementation/#mapping_role_table

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:523
> +    case ApplicationAlertRole:
> +    case ApplicationAlertDialogRole:
> +        return ATK_ROLE_ALERT;

AlertDialogRole should be mapped to a ATK_ROLE_DIALOG, according to http://www.w3.org/TR/wai-aria-implementation/#mapping_role_table

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:194
> -        return "AXWebArea";
> +        return "AXDocument";

You forgot to map ATK_ROLE_DOCUMENT_WEB here

[Lukasz Gajowy]

Created attachment 216658 [details]
Patch set #10

I tested WebArea role in a different way because of the reason described here: https://bugs.webkit.org/show_bug.cgi?id=117729#c17
The rest of suggestions are applied the way you suggested.

[Mario Sanchez Prada]

Comment on attachment 216658 [details]
Patch set #10

(In reply to comment #37)
> Created an attachment (id=216658) [details]
> Patch set #10
> 
> I tested WebArea role in a different way because of the reason described here: https://bugs.webkit.org/show_bug.cgi?id=117729#c17

I believe all you have to do to apply Chris's suggestion is to use <body id="body">, but I might be missing something

> The rest of suggestions are applied the way you suggested.

The patch looks good to me now, but I'd like you to reconsider Chris's suggestion (see my previous comment) here too, if possible

[Lukasz Gajowy]

Reviewers, do you have any more suggestions? :)

[Lukasz Gajowy]

(In reply to comment #38)
> (From update of attachment 216658 [details])
> (In reply to comment #37)
> > Created an attachment (id=216658) [details] [details]
> > Patch set #10
> > 
> > I tested WebArea role in a different way because of the reason described here: https://bugs.webkit.org/show_bug.cgi?id=117729#c17
> 
> I believe all you have to do to apply Chris's suggestion is to use <body id="body">, but I might be missing something
> 
> > The rest of suggestions are applied the way you suggested.
> 
> The patch looks good to me now, but I'd like you to reconsider Chris's suggestion (see my previous comment) here too, if possible

I cannot apply to Chris suggestion, because it doesn't work like this on efl port (probably a bug). When i: 
1) provide id="body" to <body>
2) invoke var body = accessibilityControler.getAccessibleElementById("body")
3) debug(body.role)
I get the error "null is not an object". 

This is why i decided to do this the other way.

[Lukasz Gajowy]

On below link there is a bug report regarding to the accessibilityControler.getAccessibleElementById("body") problem:

https://bugs.webkit.org/show_bug.cgi?id=125343

[Mario Sanchez Prada]

Comment on attachment 216658 [details]
Patch set #10

(In reply to comment #40)
> [...]
> > The patch looks good to me now, but I'd like you to reconsider Chris's suggestion (see my previous comment) here too, if possible
> 
> I cannot apply to Chris suggestion, because it doesn't work like this on efl port (probably a bug). When i: 
> 1) provide id="body" to <body>
> 2) invoke var body = accessibilityControler.getAccessibleElementById("body")
> 3) debug(body.role)
> I get the error "null is not an object". 
> 
> This is why i decided to do this the other way.

Ok

(In reply to comment #41)
> On below link there is a bug report regarding to the accessibilityControler.getAccessibleElementById("body") problem:
> 
> https://bugs.webkit.org/show_bug.cgi?id=125343

Nice. Now I do not see any reason why we should not give this a try.

Thanks!

[WebKit Commit Bot]

Comment on attachment 216658 [details]
Patch set #10

Clearing flags on attachment: 216658

Committed r160220: <http://trac.webkit.org/changeset/160220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.