Allow ports to make plugins keyboard focusable
Created attachment 147227 [details] Patch
Comment on attachment 147227 [details] Patch Attachment 147227 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12950240
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.
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.
(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?
(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.
OK, I didn't read "keyboard focusable" as "focusable with keyboard" or "tab focusable", which it really is, hence the confusion.
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?
(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?
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.
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 ?
Created attachment 149369 [details] Patch
(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).
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.
Comment on attachment 149369 [details] Patch This looks a lot better, thanks.
Comment on attachment 149369 [details] Patch The API change LGTM.
(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.
Comment on attachment 149369 [details] Patch Attachment 149369 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13094253
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.
Comment on attachment 149369 [details] Patch r=me, but please fix the Mac build before landing.
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
Created attachment 157022 [details] Patch
Created attachment 157031 [details] Patch
(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?
Comment on attachment 157031 [details] Patch Clearing flags on attachment: 157031 Committed r124954: <http://trac.webkit.org/changeset/124954>
All reviewed patches have been landed. Closing bug.
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.
Hopefully fixed http://trac.webkit.org/changeset/124963
>> 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