Bug 30937 – MSAA: Accessibility of headings is not correct

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

[Jon Honeycutt]

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>

[Jon Honeycutt]

Created attachment 42167 [details]
patch

Test to follow.

[Alice Liu]

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]

Created attachment 42174 [details]
DRT changes, layout test

[David Levin]

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 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]

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

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

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

Committed in r50354, r50356.