| Summary: | crash with AX on when an image map contains an anchor tag | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | Keywords: | InRadar | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
chris fleizach
2009-10-23 23:32:53 PDT
Created attachment 41820 [details]
patch
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] 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. made this bug to track allowing <a> children in a <map> https://bugs.webkit.org/show_bug.cgi?id=30760 Created attachment 41838 [details]
patch
added patch to just fix the crasher |