Bug 45014 – [Qt] Number Pad Enter Key Returns Char Code 0 Instead of 13

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jarred Nicholls]

There is a missing case statement in the PlatformKeyboardEventQt.cpp file to handle the Enter button on the number pad.  Because it is not handled, the char code returned in a javascript event is 0 rather than 13.  Attached is an svn diff patch I'd like to submit.

Thanks,
Jarred

[Jarred Nicholls]

Created attachment 66158 [details]
Patch that corrects the bug

[Alexey Proskuryakov]

Does this affect any test results? If not, can you add a test covering this?

[Jarred Nicholls]

(In reply to comment #2)
> Does this affect any test results? If not, can you add a test covering this?

No test are affected.  I will add a test to cover it.  I am also adding an appropriate ChangeLog as that was omitted previously.  Thanks!

[Jarred Nicholls]

Created attachment 66245 [details]
Patch + ChangeLog

[WebKit Review Bot]

Attachment 66245 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jarred Nicholls]

Created attachment 66247 [details]
Patch + ChangeLog

[Jarred Nicholls]

Created attachment 66252 [details]
Manual Test (WebCore/manual-tests/qt/numpad-enter-key.html)

This is a manual test, not committed as a new file, but that could be placed in WebCore/manual-tests/qt/.  It tests the resulting keycode of keypress event
and pops an alert when 13 is discovered.  Prior to the patch, hitting Enter key on Number Pad did not result in an alert.  After the patch it does.

Thanks.

[WebKit Review Bot]

Attachment 66252 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

First off, I cannot reproduce the problem (WebKit trunk, Qt 4.7, Linux) using the manual test. Is this a platform specific problem?

Second, the manual test needs to include instructions on how to use it. Just a textual description in the page body is fine. (What should I press? Should NumLock be on/off? etc.) Also, please avoid the alert() if possible.

Third, please squash the patches into one (and make sure the test is included in the ChangeLog entry.)

[Ariya Hidayat]

Comment on attachment 66252 [details]
Manual Test (WebCore/manual-tests/qt/numpad-enter-key.html)

Clear the review flag, this needs to be in the same patch as the fix.

[Jarred Nicholls]

Thanks Andreas for the info.

I checked WebKit trunk, and there is no fix/case statement for the enter key under the "isKeypad" if branch.  See http://gitorious.org/webkit/webkit/blobs/master/WebCore/platform/qt/PlatformKeyboardEventQt.cpp  If there is no case statement for the return/enter keys, such as is included in my patch, then it is *not* fixed or working...that I can assure you.  I have suffered from this issue since 4.6.0, and finally got around to correcting it myself.

I didn't find a bug report for this on bugs.webkit.org, so I created it (my first one).  However just this morning I did find a bug report on Nokia's site from some time ago: http://bugreports.qt.nokia.com/browse/QTWEBKIT-221  The submitted patch is nearly identical to my patch, only it is missing the Qt::Key_Return case statement.

I too am running WebKit trunk, Qt 4.7, and Linux.  Num Lock is on, and before I compiled my change in, the normalized keycode that is returned is 0.  Afterwards, it is properly 13.

Thanks,
Jarred

[Antonio Gomes]

LGTM too.

Jarred, as said, please make your manual test and the patch in the same patch file, and re-upload.

[Andreas Kling]

Created attachment 67581 [details]
Cleaned up patch

Same patch including cleaned-up manual test.

[Andreas Kling]

Comment on attachment 67581 [details]
Cleaned up patch

Clearing flags on attachment: 67581

Committed r67483: <http://trac.webkit.org/changeset/67483>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.