[webkit-reviews] review denied: [Bug 54877] chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV at NULL (2f8e4496f8ea69da13971f5fddd09753) : [Attachment 84380] Patch attempt #2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Berend-Jan Wever
<skylined at chromium.org>'s request for review:
Bug 54877: chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV at NULL
(2f8e4496f8ea69da13971f5fddd09753)
https://bugs.webkit.org/show_bug.cgi?id=54877

Attachment 84380: Patch attempt #2
https://bugs.webkit.org/attachment.cgi?id=84380&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84380&action=review

Thanks for tackling this.

> Source/WebCore/html/HTMLAreaElement.cpp:234
> -    // If the AREA element was a link, it should support focus.
> +    // If the AREA element was a link and has a parent, it should support
focus.
>      // The inherited method is not used because it assumes that a render
object must exist 
>      // for the element to support focus. AREA elements do not have render
objects.
> -    return isLink();
> +    return isLink() && parentNode();

This check seems incorrect, too specific, and in the wrong place. For example,
an area element might have a parent, but its parent might in turn not be in the
document’s DOM tree.

I think the correct check would be not in the supportsFocus function, but
rather at the call sites. And the check would probably be inDocument, or
possibly a walk up the parent node chain until we reach the document that's
trying to do the focus or don’t.

I’m pretty sure that the function that needs the fix is Element::focus. It
doesn’t have the inDocument check that Node::isFocusable has, and the fix could
be as simple as adding that.