Last Comment Bug 222509 - In OS/2 onkeydown onkeyup do not report the DOM_VK_* constants
: In OS/2 onkeydown onkeyup do not report the DOM_VK_* constants
Status: VERIFIED FIXED
: fixed1.4.2, fixed1.6, verified1.8.1.8
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: x86 OS/2
-- normal (vote)
: ---
Assigned To: Peter Weilbacher
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-10-16 14:29 PDT by Lee Tartak
Modified: 2007-10-03 06:41 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case to show problem (4.46 KB, text/html)
2003-10-16 14:32 PDT, Lee Tartak
no flags Details
Patch (8.42 KB, patch)
2003-10-16 15:08 PDT, Lee Tartak
no flags Details | Diff | Splinter Review
Allow Shift, Alt, and Control keys through (9.25 KB, patch)
2003-10-21 15:15 PDT, Lee Tartak
timeless: review-
Details | Diff | Splinter Review
Made changes proposed by timeless (9.16 KB, patch)
2003-11-06 09:39 PST, Lee Tartak
no flags Details | Diff | Splinter Review
More current patch (14.25 KB, patch)
2003-12-15 12:57 PST, Mike Kaply [:mkaply]
no flags Details | Diff | Splinter Review
The real slim shady (9.13 KB, patch)
2003-12-17 13:15 PST, Mike Kaply [:mkaply]
mozilla: review+
mozilla: superreview+
mozilla: approval1.4.2+
mozilla: approval1.6+
Details | Diff | Splinter Review
test patch for 1.8.0 branch (22.62 KB, patch)
2006-04-23 12:47 PDT, Peter Weilbacher
no flags Details | Diff | Splinter Review
DOM key test results table (to be viewed in IBM850 encoding) (3.16 KB, text/plain)
2006-05-22 14:20 PDT, Peter Weilbacher
no flags Details
Patch for review (trunk) (13.78 KB, patch)
2006-05-24 17:18 PDT, Peter Weilbacher
no flags Details | Diff | Splinter Review
Patch with fixes and cleanup (56.72 KB, patch)
2006-05-24 17:27 PDT, Peter Weilbacher
no flags Details | Diff | Splinter Review
new patch with -w (34.90 KB, patch)
2006-07-29 11:17 PDT, Peter Weilbacher
mozilla: review+
Details | Diff | Splinter Review

Description User image Lee Tartak 2003-10-16 14:29:13 PDT
User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.4.1) Gecko/20030924
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.4.1) Gecko/20030924

When handling an onkeydown/up request the keycode returned often does not
match the DOM_VK_* constants. For example all the lowercase keys have the
wrong VK, numeric keypad has 0 onkeydown, some keys have to be mod 256 to
get the correct value etc. Works fine on Windows/Linux

Reproducible: Always

Steps to Reproduce:
Run the reference test case

Actual Results:  
On OS/2 the DOM_VK values are wrong.

Expected Results:  
Same as Linux / Windows
Comment 1 User image Lee Tartak 2003-10-16 14:32:37 PDT
Created attachment 133441 [details]
Test case to show problem
Comment 2 User image Lee Tartak 2003-10-16 14:34:05 PDT
I will post a fix for this shortly
Comment 3 User image Lee Tartak 2003-10-16 14:46:43 PDT
Reassigning to mkaply
Comment 4 User image Lee Tartak 2003-10-16 15:08:24 PDT
Created attachment 133444 [details] [diff] [review]
Patch

Code for first review
Comment 5 User image Lee Tartak 2003-10-21 15:15:18 PDT
Created attachment 133789 [details] [diff] [review]
Allow Shift, Alt, and Control keys through
Comment 6 User image timeless 2003-11-05 07:35:50 PST
Comment on attachment 133789 [details] [diff] [review]
Allow Shift, Alt, and Control keys through

>@@ -129,6 +129,9 @@
> static HPOINTER gPtrArray[IDC_COUNT];
> static PRBool gIsTrackPoint = PR_FALSE;
> static PRBool gIsDBCS = PR_FALSE;
>+static CHAR numPadMap[] = {NS_VK_NUMPAD7, 
note that the statics above numPadMap all have a g prefix? please conform
(gNumPadMap).

>+   if (fsFlags & KC_KEYUP)  // On OS/2 the scancode is in the upper byte of
>+   {                        // usChar when KC_KEYUP is set so mask it off
>+     usChar = usChar & 0x00FF;

instead of using 0x00FF all over, could you use a constant?

>+         if( rc >= 'a' && rc <= 'z')   // The DOM_VK are for upper case only so
>+         {                             // if rc is lower case uper case it.  This code
spelling: 'uper' :(

>+                  CHAR4FROMMP( mp1) == 0x37)
some old code used (foo) instead of ( foo) v
>-      if( isNumPadScanCode(CHAR4FROMMP(mp1)) && 
i'm not sure anyone cares, but i know i like ^ better...
Comment 7 User image Lee Tartak 2003-11-06 09:39:49 PST
Created attachment 134916 [details] [diff] [review]
Made changes proposed by timeless
Comment 8 User image Mike Kaply [:mkaply] 2003-12-15 12:57:54 PST
Created attachment 137470 [details] [diff] [review]
More current patch

We had to rewrite some of the code to handle non alpha better.
Comment 9 User image Mike Kaply [:mkaply] 2003-12-17 13:15:48 PST
Created attachment 137603 [details] [diff] [review]
The real slim shady
Comment 10 User image Mike Kaply [:mkaply] 2003-12-17 13:16:21 PST
Comment on attachment 137603 [details] [diff] [review]
The real slim shady

r=mkaply, sr=blizzard (platform specific), a=mkaply
Comment 11 User image Mike Kaply [:mkaply] 2003-12-17 13:17:55 PST
Fixed
Comment 12 User image Matthias Versen [:Matti] 2004-02-28 13:27:06 PST
Comment on attachment 134916 [details] [diff] [review]
Made changes proposed by timeless

removing obsolete review request
Comment 13 User image Mike Kaply [:mkaply] 2004-04-20 08:07:20 PDT
I'm reopening this.

This broke too much stuff, so I am backing it out everywhere.
Comment 14 User image Peter Weilbacher 2005-12-15 07:04:16 PST
Seems like this is more important now then before as there are now many SVG and Canvas samples using JS keycodes and none of them work correctly.
Comment 15 User image Mike Kaply [:mkaply] 2005-12-15 08:03:07 PST
bummer. This is an icky one.
Comment 16 User image Peter Weilbacher 2005-12-15 14:52:57 PST
Do you remember what was broken exactly with the patch or should I just apply it and I will see it immediately? I don't remember anything really bad end Dec 2003 but as that was really before I became involved...
Comment 17 User image Mike Kaply [:mkaply] 2005-12-15 15:06:23 PST
Here's the bug that caused me to back it out:

https://bugzilla.mozilla.org/show_bug.cgi?id=236527
Comment 18 User image Peter Weilbacher 2006-04-23 12:47:20 PDT
Created attachment 219523 [details] [diff] [review]
test patch for 1.8.0 branch

I updated the patch from attachment 137603 [details] [diff] [review] to work with the 1.8.0 branch (this is to put out binaries to go with the Gecko 1.8.0.2 releases and get some testing). I uploaded a DLL with the fix to <http://weilbacher.org/Mozilla/seamonkey-1.0.1.wdgtos2_keycodes.zip>.

The patch surely fixes the problems seen in the JS/Canvas test at <http://developer.mozilla.org/samples/raycaster/RayCaster.html> and does not have the regression of bug 236527. But I am not sure if all the testcases in attachment 133441 [details] are handled correctly. Is there a spec where can I read up on which keycodes should be visible to DOM?

Btw, the nsWindow code is such a mess regarding style that again it was really difficult to keep my hands off that, so this does contain some cleanup.
Comment 20 User image Peter Weilbacher 2006-05-22 14:20:50 PDT
Created attachment 222938 [details]
DOM key test results table (to be viewed in IBM850 encoding)

Using the patch from attachment 219523 [details] [diff] [review] and SeaMonkey from the 1.8.0 branch I made a comparison of the three platforms OS/2, Linux, and Windows. I typed in a few representative keys, and compared the results in the testcase in attachment 133441 [details]. The ASCII table I attach here shows this result. I basically find agreement for the three platforms (or when OS/2 gets a different code then the codes are also different for Windows and Linux).

I am puzzled a bit about the behavior of the Shift, Ctrl, Alt, and AltGr keys. But on Windows keydown is reported while Linux reacts to keyup only. So I guess the even different OS/2 behavior doesn't matter too much.

I read the texts referred to in comment 19 again, and while I still don't understand everything I think with the patch we comply to all that...

From using the test wdgtos2.dll with the patch I didn't notice any drawbacks and also didn't get any negative feedback, so I guess I will go ahead and produce a proper patch for trunk and get it reviewed. (That should have happened today but I wanted to update my CVS tree and ran into 5 different build breaks on the way...)
Comment 21 User image Peter Weilbacher 2006-05-24 17:18:58 PDT
Created attachment 223245 [details] [diff] [review]
Patch for review (trunk)

OK, this is the version for review that only contains changed code. The one I would like to get checked in is the next one (that includes cleanup).
Comment 22 User image Peter Weilbacher 2006-05-24 17:27:57 PDT
Created attachment 223246 [details] [diff] [review]
Patch with fixes and cleanup

As nsWindow is a wild mix of styles that often even make it hard to see in which indentation regime one currently is. So I would like to take the opportunity and at least clean up some parts of the code (mostly the functions I touched in the patch) to use 2 space indentation, no space after opening brackets of function calls, and put the opening { behind the expression. I also removed some old XXX that will never get fixed and a few #if 0 comments that have been there forever and are most likely obsolete. 

nsWindow still compiles afterwards and seems to work as well as before this small cleanup. I hope this still lets us put this patch into 1.8 branch as well.
Comment 23 User image Peter Weilbacher 2006-06-11 15:24:48 PDT
Comment on attachment 223245 [details] [diff] [review]
Patch for review (trunk)

OK, seems like I have some more debugging to do. From the newsgroup:

When NumLock is OFF, attempting to mark text using Shift+keypad keys 
(in any input field) causes a number to be typed instead.  Weirdly, when 
NumLock is ON, Shift+keypad marks text normally. (Alex Taylor, talking about a SeaMonkey with the patch)
Comment 24 User image Peter Weilbacher 2006-07-29 11:17:28 PDT
Created attachment 231243 [details] [diff] [review]
new patch with -w

OK, this new patch improves my previous diffs in that keys on the NumPad are only handled as a number if NumLock is on, so that text marking with Shift-Numpad cursor works.

The patch is with -w so that some indents look weird. I removed all other whitespace cleanup so that only the code changes in ::OnKey, ::ProcessMessage, and WMChar2KeyCode are left. But as seen in attachment 223246 [details] [diff] [review] I want to do some more whitespace cleanup in nsWindow.cpp on checkin if that is OK.
Comment 25 User image Peter Weilbacher 2006-08-12 15:30:12 PDT
Comment on attachment 231243 [details] [diff] [review]
new patch with -w

mkaply, can I make it easier for you somehow to review this? Would it help to also attach the full code of nsWindow::OnKey and WMChar2KeyCode?
Comment 26 User image Mike Kaply [:mkaply] 2007-01-04 07:36:26 PST
Comment on attachment 231243 [details] [diff] [review]
new patch with -w

I think the best thing is to get this patch in and get people testing it. sorry for the delay.

When I made changes to this code, the only way to find bugs was to have people do weird things with their keyboards :)
Comment 27 User image Peter Weilbacher 2007-01-04 11:53:08 PST
Thanks for the review, Mike! Phew, with all that rubbish and different styles in nsWindow this was a beast to clean up and check in. But it's in trunk now.
Comment 28 User image Peter Weilbacher 2007-09-27 14:12:57 PDT
OK, this is another one of the patches in my PmW versions that I would like to finally get into branch. Haven't heard anything bad about it ever since I revised the numpad handling (comment 24), so I think it is ready to go into branch, too. Unless somebody speaks up, I will get it in next week (so in time for 1.8.1.8).
Comment 29 User image Peter Weilbacher 2007-09-27 14:18:24 PDT
Hmm, why are there no OS/2 people CCed to this bug? The previous warning should have gone to all of you...
Comment 30 User image Andy Willis (abwillis) 2007-09-27 14:52:53 PDT
I am not aware of any keyboard oddities since January when it went into trunk so I would think it should be fine for branch considering noone has mentioned a problem with your builds that are keyboard related that I have seen.  I remember the bug from the 2003 time frame but I don't guess I had realized it had been backed out at the time (I recall it mostly from the comment of the real slim shady which I think was a song or something that was big at the time).
Comment 31 User image Mike Kaply [:mkaply] 2007-09-27 14:56:00 PDT
The only reason the original bad behavior got backed out was because of an enterprise customer I think.

I everyone says it works, I say go for it.
Comment 32 User image Peter Weilbacher 2007-10-02 12:00:08 PDT
OK, so I now landed this on the 1.8 branch.
Comment 33 User image Peter Weilbacher 2007-10-03 06:41:37 PDT
Verified as fixed by long usage on trunk without apparent regressions (see e.g. comment 30).

On branch, I just verified this as fixed by comparing the testcase from attachment 133441 [details] in two nightly builds from branch:
   Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.1.8pre)
   Gecko/20070923 BonEcho/2.0.0.8pre
and
   Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.1.8pre)
   Gecko/20071003 BonEcho/2.0.0.8pre

Let's hope that no regressions come up any more.

Note You need to log in before you can comment on or make changes to this bug.