[Webkit-unassigned] [Bug 86707] document.activeElement should not return a non-focusable element

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=86707





--- Comment #23 from Arpita Bahuguna <arpitabahuguna at gmail.com>  2012-07-16 01:38:13 PST ---
(In reply to comment #22)
> (From update of attachment 151923 [details])
> 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.

Thanks for the review Ryosuke.

There are two separate scenarios for which the supportsFocus() implementation in HTMLPlugInElement fails.
The second patch submitted by me (which did not contain any supportsFocus() implementation for plugin elements) failed the following test cases:
plugins/mouse-events-fixedpos.html
plugins/keyboard-events.html
plugins/mouse-events.html
This was because of having added the isFocusable() check in Document::setFocusedNode(). Ideally we expect that any element to be focused should be able to clear the isFocusable() check.(?)
The aforementioned testcases expect the plugin element to get the focus event which it was unable to do so since it lacked rare data and was thus failing the supportsFocus() check called from isFocusable() method.

Post this I made another patch which implemented supportsFocus() method for Plugin elements and was essentially just returning true.
But this fails the case: fast/events/tabindex-focus-blur-all.html
In this case, when trying to set the tabIndex() on the plugin element, supportsFocus() is called which redirects to the supportsFocus() implementation for plugin element (which returned true).
But for this particular testcase we expect supportsFocus() to return false since even though it has rare data it fails the tabIndexSetExplicitly() check.

Thus we have two separate scenarios wherein, if a plugin does not have rare data set for it, supportsFocus() still returns true (going by the assumption that all plugins should be able to get focus) and the second being that if a plugin has rare data set for it, supportsFocus() should run an additional check for tabIndexSetExplicitly().

I understand that the current implementation of supportsFocus() for plugins indeed does not seem right. I would appreciate any pointers towards a better solution here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.