When a web page is rendered using WebKit and inspect32 is invoked on windows, MSAA role attribute is not properly set for list item elements. On Firefox 2.0.0.14, it is properly set to ListItemRole (ROLE_SYSTEM_LISTITEM). ~Thanks Sankar.
Created attachment 27139 [details] Patch to populate role attribute for list item elements. This is a very simple patch. As jon asked, this patch is uploaded to just populate the role attribute of list elements appropriately.
I am not sure that a list item should have casSetFocus through accessibility. Can a list item be focused on? You probably also want to map { ListItemRole, NSAccessibilityGroupRole } in AccessibilityObjectWrapper.mm so that mac users will not experience any problems immediately your indentation in your layout test is also incorrect. it seems like this layout test will fail on mac because the string "list item" appearing as the role will not be the value returned on the mac. perhaps make this a windows only test. this is an *official* review, just comments from the mac side of accessibility =)
@Chris, Yeah, list item can be focused. You can check it using inspect32 tool on Firefox 2.0.0.14 on windows. I agree that test case should be placed in windows specific directory. I will make changes to the patch. Other than that, I don't find any issue with the test case. Can you explain me why do you think that intention in the test case is not correct. Accessibility support on windows for running layout tests is not good. So, I had to focus the body element and walk till the list item element. ~ Thanks. (In reply to comment #2) > I am not sure that a list item should have casSetFocus through accessibility. > Can a list item be focused on? > > You probably also want to map > > { ListItemRole, NSAccessibilityGroupRole } > in AccessibilityObjectWrapper.mm > > so that mac users will not experience any problems immediately > > your indentation in your layout test is also incorrect. > > it seems like this layout test will fail on mac because the string "list item" > appearing as the role will not be the value returned on the mac. perhaps make > this a windows only test. > > this is an *official* review, just comments from the mac side of accessibility > =) >
indentation != intention
Created attachment 27341 [details] Patch to populate role attribute for list item elements. Implemented all changes as specified by Chris in his review comments. ~ Thanks.
that looks ok to me
This sounds like something jhoneycutt needs to review.
Comment on attachment 27341 [details] Patch to populate role attribute for list item elements. Looks good to me. Thanks, Sankar!
Comment on attachment 27341 [details] Patch to populate role attribute for list item elements. I had to fix your ChangeLogs when landing. There was also a stray tab character in AccessibilityObject.h (which causes the commit to fail). Otherwise it committed fine. http://trac.webkit.org/changeset/43020 Please set your REAL_NAME environment variable to your full name (or be sure to clean up the ChangeLogs manually after running prepare-ChangeLog). Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/platform/win/accessibility/listitem-role-expected.txt A LayoutTests/platform/win/accessibility/listitem-role.html M WebCore/ChangeLog M WebCore/page/AccessibilityObject.h M WebCore/page/AccessibilityRenderObject.cpp M WebCore/page/mac/AccessibilityObjectWrapper.mm M WebKit/win/AccessibleBase.cpp M WebKit/win/ChangeLog Committed r43020
Thanks Eric for committing the patch. It almost 9 months for me to get this patch landed.
Rolled out due to failure: Committing to http://svn.webkit.org/repository/webkit/trunk ... D LayoutTests/platform/win/accessibility/listitem-role-expected.txt D LayoutTests/platform/win/accessibility/listitem-role.html M LayoutTests/ChangeLog M WebCore/ChangeLog M WebCore/page/AccessibilityObject.h M WebCore/page/AccessibilityRenderObject.cpp M WebCore/page/mac/AccessibilityObjectWrapper.mm M WebKit/win/AccessibleBase.cpp M WebKit/win/ChangeLog Committed r43027
Failure: http://build.webkit.org/results/Tiger%20Intel%20Release/r43020%20(662)/accessibility/lists-pretty-diff.html
that doesn't look like an important difference, although its a little odd it showed up after this patch
(In reply to comment #13) > that doesn't look like an important difference, although its a little odd it > showed up after this patch > Can you tell me what I need to do from my side to get this patch landed.
I think this may be resolved with: http://trac.webkit.org/changeset/52351
No response in at least 6 months. Closing