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&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>
Created attachment 41820 [details]
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 on attachment 41820 [details]
> - : m_areaElement(0),
> + : m_anchorElement(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->hasAttribute(altAttr) && m_anchorElement->getAttribute(altAttr).isEmpty())
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())
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.
made this bug to track allowing <a> children in a <map>
Created attachment 41838 [details]
added patch to just fix the crasher