Bug 30739 – crash with AX on when an image map contains an anchor tag

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

[chris fleizach]

here's the offending code found on ebay.com

<div class="body"><img src="http://pics.ebaystatic.com/aw/pics/globalAssets/imgCCHPMeBAAGSignedOut.gif" height="79" width="613" border="0" usemap="#imgCCHPMeBAAGSignedOut"><map id="imgCCHPMeBAAGSignedOut" name="imgCCHPMeBAAGSignedOut"><area shape="rect" coords="97,45,212,75" href="https://signin.ebay.com/ws/eBayISAPI.dll?SignIn&amp;ru=http%3A%2F%2Fwww.ebay.com" alt=""><area shape="rect" coords="447,45,565,76" href="https://scgi.ebay.com/ws/eBayISAPI.dll?RegisterEnterInfo"><a href="https://scgi.ebay.com/ws/eBayISAPI.dll?RegisterEnterInfo" _sp="p3907.m247;Register"></a><area shape="default" nohref="nohref" alt=""></map></div>

[chris fleizach]

Created attachment 41820 [details]
patch

[chris fleizach]

the problem was that we were assuming all children of a <map> where <area>. But apparently, you can also stick in a <a> tag if you want

[Darin Adler]

Comment on attachment 41820 [details]
patch

>  AccessibilityImageMapLink::AccessibilityImageMapLink()
> -    : m_areaElement(0), 
> +    : m_anchorElement(0), 
>        m_mapElement(0)

This is not using the normal formatting for WebKit. We put the commas at the beginnings of the subsequent lines, lined up under the colons.

> +    // Ignore links that have a alt="" or nohref tag
> +    if (m_anchorElement 
> +        && ((m_anchorElement->hasAttribute(altAttr) && m_anchorElement->getAttribute(altAttr).isEmpty())
> +        || m_anchorElement->hasAttribute(nohrefAttr)))
> +        return true;
> +    
> +    return false; 

I suggest just using a return statement rather than an if. Also, I suggest indenting the || an additional tab level to indicate that it's in a nested expression.

Or you can keep this simpler by using early return a bit more.

    if (!m_anchorElement)
        return false;

    if (m_anchorElement->hasAttribute(altAttr) && m_anchorElement->getAttribute(altAttr).isEmpty())
        return true;

    if (m_anchorElement->hasAttribute(nohrefAttr))
        return true;

    return false;

Now that I write it that way, I can see it can be done more efficiently, too:

    const AtomicString& alt = m_anchorElement->getAttribute(altAttr);
    if (alt.isEmpty() && !alt.isNull())
        return true;

And you can write some additional comments explaining why this is the correct logic.

I see now where the crash came from -- it was from the check of isLink then assuming that meant HTMLAreaElement. Maybe it would have been best to have one simple patch fixing the crash that made that check do hasTagName(areaTag), and then another more sophisticated patch adding support for <a> inside <map>.

The test you added here simply tests for the crash. It does not test the newly added feature of having an <a> in a <map> or the new accessibilityIsIgnored check you added.

review- because I think you should do at least some of what I suggest above. In particular, I think the patch that just fixes the crash would be a good idea, and a new test that tests the new features you are adding is also called for.

[chris fleizach]

made this bug to track allowing <a> children in a <map>

https://bugs.webkit.org/show_bug.cgi?id=30760

[chris fleizach]

Created attachment 41838 [details]
patch

[chris fleizach]

added patch to just fix the crasher

[chris fleizach]

http://trac.webkit.org/changeset/50062