Bug 30928 - MSAA: Accessibility of links is wrong
Summary: MSAA: Accessibility of links is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-10-29 14:01 PDT by Jon Honeycutt
Modified: 2009-10-30 14:39 PDT (History)
0 users

See Also:


Attachments
patch (10.33 KB, patch)
2009-10-29 14:19 PDT, Jon Honeycutt
darin: review-
Details | Formatted Diff | Diff
patch v2 (10.19 KB, patch)
2009-10-29 15:33 PDT, Jon Honeycutt
darin: review+
Details | Formatted Diff | Diff
DRT changes, layout test (10.96 KB, patch)
2009-10-29 20:51 PDT, Jon Honeycutt
darin: 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 14:01:48 PDT
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>
Comment 1 Jon Honeycutt 2009-10-29 14:19:09 PDT
Created attachment 42145 [details]
patch

Test to follow.
Comment 2 Darin Adler 2009-10-29 14:29:07 PDT
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
Comment 3 Alice Liu 2009-10-29 14:49:20 PDT
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.
Comment 4 Jon Honeycutt 2009-10-29 15:33:18 PDT
(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.
Comment 5 Jon Honeycutt 2009-10-29 15:33:29 PDT
Created attachment 42153 [details]
patch v2
Comment 6 Darin Adler 2009-10-29 15:44:24 PDT
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
Comment 7 Jon Honeycutt 2009-10-29 15:59:14 PDT
(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!
Comment 8 Jon Honeycutt 2009-10-29 20:51:08 PDT
Created attachment 42173 [details]
DRT changes, layout test
Comment 9 Jon Honeycutt 2009-10-30 14:39:18 PDT
Committed in r50353, r50355.