RESOLVED FIXED 88958
Allow plugins to decide whether they are keyboard focusable
https://bugs.webkit.org/show_bug.cgi?id=88958
Summary Allow plugins to decide whether they are keyboard focusable
Fady Samuel
Reported 2012-06-12 22:13:23 PDT
Allow ports to make plugins keyboard focusable
Attachments
Patch (7.09 KB, patch)
2012-06-12 22:15 PDT, Fady Samuel
no flags
Patch (13.28 KB, patch)
2012-06-25 15:21 PDT, Fady Samuel
no flags
Patch (12.06 KB, patch)
2012-08-07 15:32 PDT, Fady Samuel
no flags
Patch (13.35 KB, patch)
2012-08-07 16:00 PDT, Fady Samuel
no flags
Fady Samuel
Comment 1 2012-06-12 22:15:43 PDT
Build Bot
Comment 2 2012-06-12 22:43:44 PDT
Alexey Proskuryakov
Comment 3 2012-06-13 10:22:30 PDT
This sounds much like bug 87710. I had the below question in that bug: "This is surprising, because I can certainly focus Flash plug-ins to type in them in Safari on Mac and Windows. Keyboard also works in YouTube for controlling movies." I'm not sure if it has been answered or not - I did not see an answer that I could understand there.
Adam Barth
Comment 4 2012-06-13 10:29:12 PDT
Comment on attachment 147227 [details] Patch In addition to answer Alexey's question, this patch is missing an explanation of why we should make this change and a test that demonstrates the change in behavior.
Fady Samuel
Comment 5 2012-06-22 12:35:36 PDT
(In reply to comment #3) > This sounds much like bug 87710. I had the below question in that bug: > > "This is surprising, because I can certainly focus Flash plug-ins to type in them in Safari on Mac and Windows. Keyboard also works in YouTube for controlling movies." > > I'm not sure if it has been answered or not - I did not see an answer that I could understand there. On Chromium, we can click to focus plugins, but tab to focus does not seem to work. Plugins don't seem to participate in tab ordering. This patch addresses this. Is this a non-issue on Safari?
Fady Samuel
Comment 6 2012-06-22 13:29:31 PDT
(In reply to comment #5) > (In reply to comment #3) > > This sounds much like bug 87710. I had the below question in that bug: > > > > "This is surprising, because I can certainly focus Flash plug-ins to type in them in Safari on Mac and Windows. Keyboard also works in YouTube for controlling movies." > > > > I'm not sure if it has been answered or not - I did not see an answer that I could understand there. > > On Chromium, we can click to focus plugins, but tab to focus does not seem to work. Plugins don't seem to participate in tab ordering. This patch addresses this. Is this a non-issue on Safari? I just tested YouTube on Safari for Mac, it seems plugins are click-to-focus there as well, and not keyboard focusable (tab-to-focus). We'd like to add tab-to-focus for at least certain types of plugins.
Alexey Proskuryakov
Comment 7 2012-06-22 13:35:42 PDT
OK, I didn't read "keyboard focusable" as "focusable with keyboard" or "tab focusable", which it really is, hence the confusion.
Adam Barth
Comment 8 2012-06-22 14:40:04 PDT
Comment on attachment 147227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147227&action=review > Source/WebCore/html/HTMLPlugInElement.cpp:198 > + return chrome->client()->pluginsAreKeyboardFocusable(); I guess that raises the question of when a client wouldn't want plugins to be keyboard focusable. In what situations would someone want to return false here?
Fady Samuel
Comment 9 2012-06-22 15:32:30 PDT
(In reply to comment #8) > (From update of attachment 147227 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147227&action=review > > > Source/WebCore/html/HTMLPlugInElement.cpp:198 > > + return chrome->client()->pluginsAreKeyboardFocusable(); > > I guess that raises the question of when a client wouldn't want plugins to be keyboard focusable. In what situations would someone want to return false here? I'm considering adding some additional information about the element passed into pluginsAreKeyboardFocusable. This patch allows you to tab into a plugin, but you won't be able to tab out. I'm working towards enabling this on Chromium. Pepper does not currently support advancing the focus of the host page, the API has not yet been discussed or defined. However, some plugins do wish to participate in tabbing. Until we define how plugins can advance focus of the host page in general, I feel that this behavior should depend on the port and/or plugin. Thoughts?
Adam Barth
Comment 10 2012-06-22 15:37:22 PDT
Comment on attachment 147227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147227&action=review > Source/WebCore/html/HTMLPlugInElement.cpp:195 > + if (!document() || !document()->page()) I don't think document() can ever be null here.
Adam Barth
Comment 11 2012-06-22 15:39:32 PDT
Comment on attachment 147227 [details] Patch I mean, this feels a bit incomplete. It sounds like we're not going to make plugins keyboard focusable in general. We might make a particular plugin keyboard focusable (i.e., if it indicates that it knows how to give the focus back). In that case, wouldn't it make more sense to ask the plugin whether it support keyboard focusing rather than asking the ChromeClient ?
Fady Samuel
Comment 12 2012-06-25 15:21:46 PDT
Fady Samuel
Comment 13 2012-06-25 15:25:53 PDT
(In reply to comment #11) > (From update of attachment 147227 [details]) > I mean, this feels a bit incomplete. It sounds like we're not going to make plugins keyboard focusable in general. We might make a particular plugin keyboard focusable (i.e., if it indicates that it knows how to give the focus back). In that case, wouldn't it make more sense to ask the plugin whether it support keyboard focusing rather than asking the ChromeClient ? How does this new plumbing look to you, Adam? HTMLPlugInElement::isKeyboardFocusable asks its Widget (WebPluginContainerImpl in Chromium's case) whether it supports keyboard focus. On Chromium, WebPluginContainerImpl asks WebPlugin (a particular plugin type). Chromium's plugins extend WebPlugin and can individually choose whether or not to support keyboard focus (i.e. whether to participate in tab focusing).
WebKit Review Bot
Comment 14 2012-06-25 15:27:10 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Adam Barth
Comment 15 2012-06-25 15:29:03 PDT
Comment on attachment 149369 [details] Patch This looks a lot better, thanks.
Adam Barth
Comment 16 2012-06-25 15:35:19 PDT
Comment on attachment 149369 [details] Patch The API change LGTM.
Fady Samuel
Comment 17 2012-06-25 15:53:36 PDT
(In reply to comment #7) > OK, I didn't read "keyboard focusable" as "focusable with keyboard" or "tab focusable", which it really is, hence the confusion. Alexey, any objections (or suggestions) to this change? Thanks.
Build Bot
Comment 18 2012-06-25 16:00:40 PDT
Anders Carlsson
Comment 19 2012-06-26 10:52:29 PDT
FWIW, this looks good. For NPAPI plug-ins ports would have to implement the https://wiki.mozilla.org/NPAPI:AdvancedKeyHandling proposal to support keyboard focus.
Anders Carlsson
Comment 20 2012-06-26 10:56:05 PDT
Comment on attachment 149369 [details] Patch r=me, but please fix the Mac build before landing.
Arunprasad
Comment 21 2012-07-21 03:25:04 PDT
Currently you have handled focusing plugin for Tab keys(advanceFocusInDocumentOrder). But I think you missed out the spatial navigation(advanceFocusDirectionally). You can add the same implementation in as like above, FocusController::updateFocusCandidateIfNeeded. FocusController::advanceFocusDirectionallyInContainer. -Arun
Fady Samuel
Comment 22 2012-08-07 15:32:46 PDT
Fady Samuel
Comment 23 2012-08-07 16:00:50 PDT
Fady Samuel
Comment 24 2012-08-07 16:02:32 PDT
(In reply to comment #21) > Currently you have handled focusing plugin for Tab keys(advanceFocusInDocumentOrder). > > But I think you missed out the spatial navigation(advanceFocusDirectionally). > > You can add the same implementation in as like above, > FocusController::updateFocusCandidateIfNeeded. > FocusController::advanceFocusDirectionallyInContainer. > > -Arun I will address this in a separate patch. How is spatial navigation normally triggered in a browser?
WebKit Review Bot
Comment 25 2012-08-07 17:54:51 PDT
Comment on attachment 157031 [details] Patch Clearing flags on attachment: 157031 Committed r124954: <http://trac.webkit.org/changeset/124954>
WebKit Review Bot
Comment 26 2012-08-07 17:54:58 PDT
All reviewed patches have been landed. Closing bug.
Dean Jackson
Comment 27 2012-08-07 18:21:39 PDT
Caused a build failure in Mac (EWS was offline) Source/WebCore/html/HTMLPlugInElement.cpp:194:60: error: unused parameter 'event' [-Werror,-Wunused-parameter] bool HTMLPlugInElement::isKeyboardFocusable(KeyboardEvent* event) const ^ 1 error generated.
Dean Jackson
Comment 28 2012-08-07 18:22:57 PDT
Arunprasad
Comment 29 2012-08-12 02:21:51 PDT
>> Currently you have handled focusing plugin for Tab keys(advanceFocusInDocumentOrder). >> >> But I think you missed out the spatial navigation(advanceFocusDirectionally). >> >> You can add the same implementation in as like above, >> FocusController::updateFocusCandidateIfNeeded. >> FocusController::advanceFocusDirectionallyInContainer. >> >> -Arun >I will address this in a separate patch. How is spatial navigation normally >triggered in a browser? You can easily understand by looking FocusController::advanceFocusDirectionally. Also spatial navigation has to be enabled explicitly in Settings. -Arun
Note You need to log in before you can comment on or make changes to this bug.