[webkit-reviews] review denied: [Bug 86707] document.activeElement should not return a non-focusable element : [Attachment 151923] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 86707: document.activeElement should not return a non-focusable element
https://bugs.webkit.org/show_bug.cgi?id=86707

Attachment 151923: Patch
https://bugs.webkit.org/attachment.cgi?id=151923&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151923&action=review


> Source/WebCore/ChangeLog:24
> +	   The supportsFocus() check (called from isFocusable() method in
Document::setFocusedNode())
> +	   fails for plugin elements if they do not have rareData set for them.
Previously plugin
> +	   elements were able to get the focus since no "focusable" check was
carried out in
> +	   setFocusedNode().

That's because Element::supportsFocus checks tabIndexSetExplicitly(), right?
Are you saying that the plugin element should always be focusable?
If so, then we should just return true in HTMLPlugInElement::supportsFocus. If
we do need to check the existence of render,
then it's odd that we don't have to do that when there is no rareData. e.g.
calling getElementsById will create a rare data. r- because of this.

> Source/WebCore/html/HTMLPlugInElement.cpp:115
> +    return hasRareData() ? HTMLElement::supportsFocus() : true;

This is the line I'm talking about, and we certainly need a test for this. If
there is an existing test that catches this problem,
then we should mention that in the change log.