Bug 96727

Summary: EventSendingController::keyDown does not support non-array modifier arguments
Product: WebKit Reporter: Sudarsana Nagineni (babu) <naginenis>
Component: WebKit2Assignee: Sudarsana Nagineni (babu) <naginenis>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric, kenneth, mrobinson, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Sudarsana Nagineni (babu) 2012-09-13 22:31:13 PDT
fast/forms/legend-access-key.html is failing since it uses "altKey" as the modifier argument. Non-array modifier arguments should be handled in EventSendingController::keyDown to solve the issue.
Comment 1 Sudarsana Nagineni (babu) 2012-09-14 04:38:34 PDT
Created attachment 164104 [details]
Patch
Comment 2 WebKit Review Bot 2012-09-14 06:19:14 PDT
Comment on attachment 164104 [details]
Patch

Clearing flags on attachment: 164104

Committed r128602: <http://trac.webkit.org/changeset/128602>
Comment 3 WebKit Review Bot 2012-09-14 06:19:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Alexey Proskuryakov 2012-09-14 09:49:27 PDT
> Non-array modifier arguments should be handled in EventSendingController::keyDown to solve the issue.

Are you saying that DumpRenderTree supports this, so WebKitTestRunner should, too? I don't see that in DumpRenderTree implementation.

I'd like to roll out this patch unless there is a convincing explanation.
Comment 5 Sudarsana Nagineni (babu) 2012-09-14 10:47:58 PDT
(In reply to comment #4)
> > Non-array modifier arguments should be handled in EventSendingController::keyDown to solve the issue.
> 
> Are you saying that DumpRenderTree supports this, so WebKitTestRunner should, too? I don't see that in DumpRenderTree implementation.

This support was added by different ports for DumpRenderTree in EventSender, so I thought it make sense to have it in WKTR too.

http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/EventSender.cpp#L256
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/gtk/EventSender.cpp#L297
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestRunner/EventSender.cpp#L192
Comment 6 Alexey Proskuryakov 2012-09-17 16:01:08 PDT
CC'ing people who were adding this code to ports over the years.

This is just wrong. This was never meant to work with string arguments, and still doesn't in Mac DumpRenderTree. Was everyone just adding this code to accommodate for a typo in a single test?! How did that pass review multiple times?

Please at the very least implement this on Mac too, to regain interoperability of DumpRenderTree implementations.
Comment 7 Eric Seidel (no email) 2012-09-17 16:04:22 PDT
Seems like a reasonable feature to me.  I certainly don't mind supporting this variant.
Comment 8 Sudarsana Nagineni (babu) 2012-10-01 01:35:31 PDT
Implemented this on Mac too, in the bug #97805