Bug 75158

Summary: Access keys do not work for frames that are not focused
Product: WebKit Reporter: Cem Kocagil <cem.kocagil+webkit>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: aboxhall, ap, dglazkov, rniwa, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 19820    
Attachments:
Description Flags
Test case
none
Patch
cem.kocagil+webkit: review-, webkit.review.bot: commit-queue-
Patch (skips the layout test)
none
Patch
none
Patch
none
Patch rniwa: review-

Description Cem Kocagil 2011-12-22 19:17:14 PST
If the element targeted by the accesskey is not in a focused frame, it doesn't get selected (tested on Chromium and Safari). IE, Firefox and Opera focus on the item.

This behavior depends on port-specific code. This bug report is intended to represent the Chromium part of the problem and should block 19820.

See the attachment to reproduce the bug.
Comment 1 Cem Kocagil 2011-12-22 19:17:56 PST
Created attachment 120424 [details]
Test case
Comment 2 Cem Kocagil 2011-12-22 19:35:39 PST
Created attachment 120426 [details]
Patch
Comment 3 WebKit Review Bot 2011-12-22 21:17:37 PST
Comment on attachment 120426 [details]
Patch

Attachment 120426 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10939163

New failing tests:
fast/dom/access-key-iframe.html
Comment 4 Cem Kocagil 2011-12-23 00:11:07 PST
Created attachment 120439 [details]
Patch (skips the layout test)
Comment 5 Cem Kocagil 2011-12-23 03:17:04 PST
Created attachment 120449 [details]
Patch

Now with changelog; waiting to be reviewed.
Comment 6 WebKit Review Bot 2011-12-23 03:20:09 PST
Attachment 120449 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Cem Kocagil 2011-12-23 03:35:10 PST
Created attachment 120452 [details]
Patch
Comment 8 Cem Kocagil 2011-12-28 14:38:44 PST
Created attachment 120701 [details]
Patch
Comment 9 Ryosuke Niwa 2012-01-03 13:14:45 PST
Comment on attachment 120701 [details]
Patch

We can't test this?
Comment 10 Ryosuke Niwa 2012-01-03 13:19:56 PST
Comment on attachment 120701 [details]
Patch

I don't think it's right to only change the behavior of Chromium port given that all other major browsers exihit the same behavior and the code is shared in WebCore (handleAccessKey). We should modify handleAccessKey so that it'll go through all frames instead.
Comment 11 Alexey Proskuryakov 2012-01-03 13:38:54 PST
This looks security sensitive.

What guarantees that this doesn't introduce XSS? A frame could dispatch a keyboard event to another frame this way, or it could fool a user into pressing the access key combo, triggering an action in a different origin frame.
Comment 12 Cem Kocagil 2012-01-03 16:54:03 PST
(In reply to comment #11)
> This looks security sensitive.
> 
> What guarantees that this doesn't introduce XSS? A frame could dispatch a keyboard event to another frame this way, or it could fool a user into pressing the access key combo, triggering an action in a different origin frame.

It doesn't look like dispatched events can trigger WebViewImpl::charEvent and my simple tests verified this (I'm not quite sure though). It would be strange if that was the case since it's a method of the webview itself, not the Page or a Frame.

The calls to EventHandler::handleAccessKey does not send events to other frames, it only finds the element corresponding to that accesskey and invokes accessKeyAction on that element.
Comment 13 Alexey Proskuryakov 2012-01-05 16:34:35 PST
Thanks.

I still feel very uneasy (security-wise) about access keys working across multiple documents.