Bug 23610 – Create a new MSAA role for List Item elements.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Sankar Aditya Tanguturi]

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.

[Sankar Aditya Tanguturi]

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.

[chris fleizach]

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 =)

[Sankar Aditya Tanguturi]

@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
> =)
> 

[chris fleizach]

indentation != intention

[Sankar Aditya Tanguturi]

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.

[chris fleizach]

that looks ok to me

[Eric Seidel (no email)]

This sounds like something jhoneycutt needs to review.

[Jon Honeycutt]

Comment on attachment 27341 [details]
Patch to populate role attribute for list item elements.

Looks good to me. Thanks, Sankar!

[Eric Seidel (no email)]

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

[Sankar Aditya Tanguturi]

Thanks Eric for committing the patch. It almost 9 months for me to get this patch landed.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Failure:
http://build.webkit.org/results/Tiger%20Intel%20Release/r43020%20(662)/accessibility/lists-pretty-diff.html

[chris fleizach]

that doesn't look like an important difference, although its a little odd it showed up after this patch

[Sankar Aditya Tanguturi]

(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.

[James Craig]

I think this may be resolved with:
http://trac.webkit.org/changeset/52351

[James Craig]

No response in at least 6 months. Closing