RESOLVED FIXED 30937
MSAA: Accessibility of headings is not correct
https://bugs.webkit.org/show_bug.cgi?id=30937
Summary MSAA: Accessibility of headings is not correct
Jon Honeycutt
Reported 2009-10-29 17:22:50 PDT
The accessibility of heading elements on Windows is not correct. A heading element should return its tag name from IAccessible::get_accRole(), and IAccessible::get_accDescription() should include the level of the heading. <rdar://problem/7184566>
Attachments
patch (10.06 KB, patch)
2009-10-29 18:53 PDT, Jon Honeycutt
alice.barraclough: review+
DRT changes, layout test (7.56 KB, patch)
2009-10-29 21:13 PDT, Jon Honeycutt
aroben: review+
Jon Honeycutt
Comment 1 2009-10-29 18:53:19 PDT
Created attachment 42167 [details] patch Test to follow.
Alice Liu
Comment 2 2009-10-29 19:39:08 PDT
Comment on attachment 42167 [details] patch > +String AccessibilityRenderObject::stringRoleForMSAA() const > +{ ... > + // FIXME: Add the other tag names that should be returned as the role. > + Element* element = static_cast<Element*>(node); > + if (element->hasTagName(h1Tag) || element->hasTagName(h2Tag) || > + element->hasTagName(h3Tag) || element->hasTagName(h4Tag) || > + element->hasTagName(h5Tag) || element->hasTagName(h6Tag)) > + return element->tagName(); > + > + return String(); > +} I saw the list of remaining ones, and there aren't that many, so i would suggest just adding them. Factoring out the tag name check into a static function would make for more organized code, especially when you add the missing ones. r=me
Jon Honeycutt
Comment 3 2009-10-29 21:13:17 PDT
Created attachment 42174 [details] DRT changes, layout test
David Levin
Comment 4 2009-10-29 22:05:51 PDT
Just a comment in passing: VARIANT vRole; if (FAILED(m_element->get_accRole(self(), &vRole))) ... } else if (V_VT(&vRole) == VT_BSTR) result = wstring(V_BSTR(&vRole), ::SysStringLen(V_BSTR(&vRole))); You may want to add a VariantClear(&vRole); right before the return to clean up the bstr (or anything else in the Variant, if you ever get more exotic) or you could simply do the SysFreeString in the else.
Adam Roben (:aroben)
Comment 5 2009-10-30 08:00:07 PDT
Comment on attachment 42174 [details] DRT changes, layout test > + ASSERT(V_VT(&vRole) == VT_I4 || V_VT(&vRole) == VT_BSTR); > + > + wstring result; > + if (V_VT(&vRole) == VT_I4) { > + TCHAR roleText[64] = {0}; > + ::GetRoleText(V_I4(&vRole), roleText, ARRAYSIZE(roleText)); > + result = roleText; How do we know 64 characters is enough? > + } else if (V_VT(&vRole) == VT_BSTR) > + result = wstring(V_BSTR(&vRole), ::SysStringLen(V_BSTR(&vRole))); > + > + return JSStringCreateWithCharacters(result.data(), result.length()); As David says, you're leaking a BSTR here. r=me if you fix the leak.
Jon Honeycutt
Comment 6 2009-10-30 14:00:48 PDT
(In reply to comment #2) > (From update of attachment 42167 [details]) > I saw the list of remaining ones, and there aren't that many, so i would > suggest just adding them. Factoring out the tag name check into a static > function would make for more organized code, especially when you add the > missing ones. > > r=me I'll submit a follow-up patch to add the remaining tags. I moved the tag name check into a separate function as you suggested. Thanks for the review!
Jon Honeycutt
Comment 7 2009-10-30 14:01:23 PDT
(In reply to comment #4) > Just a comment in passing: > VARIANT vRole; > if (FAILED(m_element->get_accRole(self(), &vRole))) > ... > } else if (V_VT(&vRole) == VT_BSTR) > result = wstring(V_BSTR(&vRole), ::SysStringLen(V_BSTR(&vRole))); > > You may want to add a > VariantClear(&vRole); > right before the return to clean up the bstr (or anything else in the Variant, > if you ever get more exotic) or you could simply do the SysFreeString in the > else. I added a VariantClear(). Thanks!
Jon Honeycutt
Comment 8 2009-10-30 14:02:27 PDT
(In reply to comment #5) > (From update of attachment 42174 [details]) > > + ASSERT(V_VT(&vRole) == VT_I4 || V_VT(&vRole) == VT_BSTR); > > + > > + wstring result; > > + if (V_VT(&vRole) == VT_I4) { > > + TCHAR roleText[64] = {0}; > > + ::GetRoleText(V_I4(&vRole), roleText, ARRAYSIZE(roleText)); > > + result = roleText; > > How do we know 64 characters is enough? We don't. I'll post a patch to fix this. > > > + } else if (V_VT(&vRole) == VT_BSTR) > > + result = wstring(V_BSTR(&vRole), ::SysStringLen(V_BSTR(&vRole))); > > + > > + return JSStringCreateWithCharacters(result.data(), result.length()); > > As David says, you're leaking a BSTR here. > > r=me if you fix the leak. Fixed, thanks for the review!
Jon Honeycutt
Comment 9 2009-10-30 14:39:46 PDT
Committed in r50354, r50356.
Note You need to log in before you can comment on or make changes to this bug.