Bug 30739 - crash with AX on when an image map contains an anchor tag
Summary: crash with AX on when an image map contains an anchor tag
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
Keywords: InRadar
Depends on:
Reported: 2009-10-23 23:32 PDT by chris fleizach
Modified: 2009-10-26 07:50 PDT (History)
0 users

See Also:

patch (12.81 KB, patch)
2009-10-25 10:06 PDT, chris fleizach
darin: review-
Details | Formatted Diff | Diff
patch (4.20 KB, patch)
2009-10-25 20:17 PDT, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2009-10-23 23:32:53 PDT
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>
Comment 1 chris fleizach 2009-10-25 10:06:04 PDT
Created attachment 41820 [details]
Comment 2 chris fleizach 2009-10-25 10:06:34 PDT
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
Comment 3 Darin Adler 2009-10-25 17:56:47 PDT
Comment on attachment 41820 [details]

>  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.
Comment 4 chris fleizach 2009-10-25 18:05:09 PDT
made this bug to track allowing <a> children in a <map>

Comment 5 chris fleizach 2009-10-25 20:17:24 PDT
Created attachment 41838 [details]
Comment 6 chris fleizach 2009-10-25 20:17:41 PDT
added patch to just fix the crasher
Comment 7 chris fleizach 2009-10-26 07:50:29 PDT