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

Description Chang Shu 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.
Comment 1 Chang Shu 2010-10-22 08:22:40 PDT
Created attachment 71566 [details]
fix patch
Comment 2 Antonio Gomes 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?
Comment 3 Antonio Gomes 2010-10-22 08:48:07 PDT
Comment on attachment 71566 [details]
fix patch

see https://bugs.webkit.org/show_bug.cgi?id=48134#c2
Comment 4 Chang Shu 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.
Comment 5 Antonio Gomes 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?
Comment 6 Alexey Proskuryakov 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 ***
Comment 7 Alexey Proskuryakov 2010-10-22 10:16:15 PDT
Sorry, hit "Commit" by accident. But see bug 16735 for related info.
Comment 8 Chang Shu 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.
Comment 9 Chang Shu 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.
Comment 10 WebKit Commit Bot 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
Comment 11 Chang Shu 2010-10-22 13:39:55 PDT
Created attachment 71592 [details]
fix patch 3
Comment 12 Eric Seidel (no email) 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.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2010-10-25 13:04:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Alexey Proskuryakov 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.
Comment 16 Chang Shu 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.
Comment 17 Ademar Reis 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>