Allow plugins to decide whether they are keyboard focusable

• Component: New Bugs
• Status: RESOLVED FIXED

[Fady Samuel]

Allow ports to make plugins keyboard focusable

[Fady Samuel]

Created attachment 147227 [details]
Patch

[Build Bot]

Comment on attachment 147227 [details]
Patch

Attachment 147227 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12950240

[Alexey Proskuryakov]

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 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]

(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]

(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]

OK, I didn't read "keyboard focusable" as "focusable with keyboard" or "tab focusable", which it really is, hence the confusion.

[Adam Barth]

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]

(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 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 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]

Created attachment 149369 [details]
Patch

[Fady Samuel]

(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]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 149369 [details]
Patch

This looks a lot better, thanks.

[Adam Barth]

Comment on attachment 149369 [details]
Patch

The API change LGTM.

[Fady Samuel]

(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 on attachment 149369 [details]
Patch

Attachment 149369 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13094253

[Anders Carlsson]

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 on attachment 149369 [details]
Patch

r=me, but please fix the Mac build before landing.

[Arunprasad]

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]

Created attachment 157022 [details]
Patch

[Fady Samuel]

Created attachment 157031 [details]
Patch

[Fady Samuel]

(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 on attachment 157031 [details]
Patch

Clearing flags on attachment: 157031

Committed r124954: <http://trac.webkit.org/changeset/124954>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

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]

Hopefully fixed http://trac.webkit.org/changeset/124963

[Arunprasad]

>> 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