Bug 30937 - MSAA: Accessibility of headings is not correct
Summary: MSAA: Accessibility of headings is not correct
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-10-29 17:22 PDT by Jon Honeycutt
Modified: 2009-10-30 14:39 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Honeycutt 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>
Comment 1 Jon Honeycutt 2009-10-29 18:53:19 PDT
Created attachment 42167 [details]
patch

Test to follow.
Comment 2 Alice Liu 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
Comment 3 Jon Honeycutt 2009-10-29 21:13:17 PDT
Created attachment 42174 [details]
DRT changes, layout test
Comment 4 David Levin 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.
Comment 5 Adam Roben (:aroben) 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.
Comment 6 Jon Honeycutt 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!
Comment 7 Jon Honeycutt 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!
Comment 8 Jon Honeycutt 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!
Comment 9 Jon Honeycutt 2009-10-30 14:39:46 PDT
Committed in r50354, r50356.