Bug 48134

Summary: Keyboard events generated using event.initKeyboardEvent() are different from the real key press
Product: WebKit Reporter: Chang Shu <cshu>
Component: Tools / TestsAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, ap, commit-queue, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 46905, 48145    
Attachments:
Description Flags
fix patch
tonikitoo: review-
same patch but less intrusive.
none
fix patch 2
none
fix patch 3 none

Chang Shu
Reported 2010-10-22 08:20:28 PDT
In spatial navigation tests, keyboard events are simulated by calling event.initKeyboardEvent('keydown', true, true, document.defaultView, gExpectedResults[gIndex][0], 0, false, false, false, false, false). However, the keyboardevent object created inside WebCore are not exactly the same as if created by real key press. So instead, we should use eventSender methods.
Attachments
fix patch (23.27 KB, patch)
2010-10-22 08:22 PDT, Chang Shu
tonikitoo: review-
same patch but less intrusive. (1.40 KB, patch)
2010-10-22 08:47 PDT, Antonio Gomes
no flags
fix patch 2 (2.93 KB, patch)
2010-10-22 11:31 PDT, Chang Shu
no flags
fix patch 3 (3.64 KB, patch)
2010-10-22 13:39 PDT, Chang Shu
no flags
Chang Shu
Comment 1 2010-10-22 08:22:40 PDT
Created attachment 71566 [details] fix patch
Antonio Gomes
Comment 2 2010-10-22 08:47:43 PDT
Created attachment 71567 [details] same patch but less intrusive. I talked to yael two weeks ago about it, and even set her a patch for review. it is essentially the same as yours. could we go this way, instead?
Antonio Gomes
Comment 3 2010-10-22 08:48:07 PDT
Chang Shu
Comment 4 2010-10-22 08:53:59 PDT
(In reply to comment #2) > Created an attachment (id=71567) [details] > same patch but less intrusive. > > I talked to yael two weeks ago about it, and even set her a patch for review. it is essentially the same as yours. could we go this way, instead? No problem. :) I can see the effort of making non-DRT mode working, but if it's not 100% accurate, it would cause confusion. I'd rather not to make it work for non-DRT.
Antonio Gomes
Comment 5 2010-10-22 08:56:16 PDT
> I can see the effort of making non-DRT mode working, but if it's not 100% accurate, it would cause confusion. I'd rather not to make it work for non-DRT. Why? :) The other method was not being accurate for me too, so I did not put the patch up for review. <select> related tests were failing on both qt and mac for me. What do you see?
Alexey Proskuryakov
Comment 6 2010-10-22 10:15:36 PDT
Synthesizing events that are exactly like the ones *** This bug has been marked as a duplicate of bug 16735 ***
Alexey Proskuryakov
Comment 7 2010-10-22 10:16:15 PDT
Sorry, hit "Commit" by accident. But see bug 16735 for related info.
Chang Shu
Comment 8 2010-10-22 10:20:36 PDT
(In reply to comment #7) > Sorry, hit "Commit" by accident. But see bug 16735 for related info. It seems the problem has been noticed and has been worked on for a long time. But I don't see an immediate close of the bug. Before WebCore is fixed, I guess we still have to use eventSender.
Chang Shu
Comment 9 2010-10-22 11:31:37 PDT
Created attachment 71580 [details] fix patch 2 In this patch, I use Antonio's suggestion to convert event names in spatial-navigation-utils.js. It has less impact on the current code and also, in the future, when bug 16735 is fixed, we can simply roll back the change to support both DRT and non-DRT mode. The failed multiple-select case will be addressed in a separate bug.
WebKit Commit Bot
Comment 10 2010-10-22 11:57:55 PDT
Comment on attachment 71580 [details] fix patch 2 Rejecting patch 71580 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: ing 21625 test cases. fast/events/spatial-navigation/snav-single-select.html -> timed out Sampling process 67159 for 10 seconds with 10 milliseconds of run time between samples Sampling completed, processing symbols... Sample analysis of process 67159 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt Exiting early after 1 failures. 7696 tests run. 208.48s total testing time 7695 test cases (99%) succeeded 1 test case (<1%) timed out 2 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4721022
Chang Shu
Comment 11 2010-10-22 13:39:55 PDT
Created attachment 71592 [details] fix patch 3
Eric Seidel (no email)
Comment 12 2010-10-22 20:50:26 PDT
Comment on attachment 71580 [details] fix patch 2 Cleared Antonio Gomes's review+ from obsolete attachment 71580 [details] so that this bug does not appear in http://webkit.org/pending-commit.
WebKit Commit Bot
Comment 13 2010-10-25 13:04:31 PDT
Comment on attachment 71592 [details] fix patch 3 Clearing flags on attachment: 71592 Committed r70481: <http://trac.webkit.org/changeset/70481>
WebKit Commit Bot
Comment 14 2010-10-25 13:04:37 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 15 2011-01-27 14:17:36 PST
You probably know this already, but changing events generated using event.initKeyboardEvent() to be like real ones shouldn't be approached lightly. See also: bug 9933 and bug 16735.
Chang Shu
Comment 16 2011-01-27 14:22:24 PST
(In reply to comment #15) > You probably know this already, but changing events generated using event.initKeyboardEvent() to be like real ones shouldn't be approached lightly. See also: bug 9933 and bug 16735. Thanks, Alexey. I created the bug with this title hoping to fix the root cause but instead I just changed the test to use eventSender. Hopefully, someone will fix the above two bugs so we can run the test from browser.
Ademar Reis
Comment 17 2011-01-28 11:15:30 PST
Revision r70481 cherry-picked into qtwebkit-2.1.x with commit 8a14b20 <http://gitorious.org/webkit/qtwebkit/commit/8a14b20>
Note You need to log in before you can comment on or make changes to this bug.