WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(13.28 KB, patch)
2012-06-25 15:21 PDT
,
Fady Samuel
no flags
Details
Formatted Diff
Diff
Patch
(12.06 KB, patch)
2012-08-07 15:32 PDT
,
Fady Samuel
no flags
Details
Formatted Diff
Diff
Patch
(13.35 KB, patch)
2012-08-07 16:00 PDT
,
Fady Samuel
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Fady Samuel
Comment 1
2012-06-12 22:15:43 PDT
Created
attachment 147227
[details]
Patch
Build Bot
Comment 2
2012-06-12 22:43:44 PDT
Comment on
attachment 147227
[details]
Patch
Attachment 147227
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12950240
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
Created
attachment 149369
[details]
Patch
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
Comment on
attachment 149369
[details]
Patch
Attachment 149369
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13094253
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
Created
attachment 157022
[details]
Patch
Fady Samuel
Comment 23
2012-08-07 16:00:50 PDT
Created
attachment 157031
[details]
Patch
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
Hopefully fixed
http://trac.webkit.org/changeset/124963
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.
Top of Page
Format For Printing
XML
Clone This Bug