WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
DRT changes, layout test
(7.56 KB, patch)
2009-10-29 21:13 PDT
,
Jon Honeycutt
aroben
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug