Bug 30928 – MSAA: Accessibility of links is wrong

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

[Jon Honeycutt]

The accessibility of links is incorrect on Windows. An anchor element and its "linkable" children, such as text and images, should return the anchor's href from IAccessible::get_accValue(). Text nodes currently return their text from IAccessible::get_accValue(), but they should return their text from IAccessible::get_accName().

<rdar://problem/7342102>

[Jon Honeycutt]

Created attachment 42145 [details]
patch

Test to follow.

[Darin Adler]

Comment on attachment 42145 [details]
patch

> +#if PLATFORM(WIN)
> +    virtual String stringValueForMSAA() const { return String(); }
> +    virtual String nameForMSAA() const { return String(); }
> +#endif

Thanks for taking my suggestion of putting these here in this class instead of just in the Windows AccessibleBase. I also think they can be included unconditionally, even though the code won't be called on other platforms.

> +++ b/WebCore/accessibility/win/AccessibilityRenderObjectWin.cpp

Please take my suggestion above and thus don't add a new source file.

> +        Element* anchor = anchorElement();
> +        if (anchor && anchor->hasTagName(aTag))
> +            return static_cast<HTMLAnchorElement*>(anchor)->href();

Not all HTMLAnchorElement are <a> tags. It would be good to handle <area> too.

Otherwise looks good to me.

review- so you can consider my suggested approach of putting this code in cross-platform source files

[Alice Liu]

Comment on attachment 42145 [details]
patch


> +bool AccessibilityRenderObject::isLinked() const
> +{
> +    if (!isLinkable(*this))
> +        return false;
> +
> +    Element* anchor = anchorElement();
> +    if (!anchor || !anchor->hasTagName(aTag))
> +        return false;
> +
> +    return !static_cast<HTMLAnchorElement*>(anchor)->href().isEmpty();
> +}

Isn't this requiring that an image tag within an anchor have a non-empty href in order to be considered linked?  the mozilla documentation just says the img needs to be within an link, not that the link be considered STATE_LINKED as well.

[Jon Honeycutt]

(In reply to comment #3)
> 
> Isn't this requiring that an image tag within an anchor have a non-empty href
> in order to be considered linked?  the mozilla documentation just says the img
> needs to be within an link, not that the link be considered STATE_LINKED as
> well.

I tested with Firefox, and their implementation does require that the anchor element have an href before returning STATE_LINKED for an image.

[Jon Honeycutt]

Created attachment 42153 [details]
patch v2

[Darin Adler]

Comment on attachment 42153 [details]
patch v2

What about my <area> comment? You're checking specifically for the <a> tag, but <area> are also derived from HTMLAnchorElement.

> +    if (m_renderer && m_renderer->isText())
> +        return static_cast<RenderText*>(m_renderer)->text();

Is this really right? This won't take things like whitespace-collapsing into account. Lets investigate that further at some point.

r=me

[Jon Honeycutt]

(In reply to comment #6)
> (From update of attachment 42153 [details])
> What about my <area> comment? You're checking specifically for the <a> tag, but
> <area> are also derived from HTMLAnchorElement.

We create AccessibilityImageMapLinks for <area> elements. I added code to return the URL for the value property, and the alt text for the name property, of these objects.

> 
> > +    if (m_renderer && m_renderer->isText())
> > +        return static_cast<RenderText*>(m_renderer)->text();
> 
> Is this really right? This won't take things like whitespace-collapsing into
> account. Lets investigate that further at some point.
> 
> r=me

No, this wouldn't be right, then. There's an AccessibilityObject::textUnderElement() that is used elsewhere. I'll test and make that change if it fixes this issue. Thanks for the review!

[Jon Honeycutt]

Created attachment 42173 [details]
DRT changes, layout test

[Jon Honeycutt]

Committed in r50353, r50355.