Closed Bug 222509 Opened 19 years ago Closed 16 years ago

In OS/2 onkeydown onkeyup do not report the DOM_VK_* constants


(Core :: DOM: UI Events & Focus Handling, defect)

Not set





(Reporter: leetartak, Assigned: mozilla)


(Keywords: fixed1.4.2, fixed1.6, verified1.8.1.8)


(3 files, 8 obsolete files)

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
I will post a fix for this shortly
Reassigning to mkaply
Assignee: events → mkaply
OS: other → OS/2
Attached patch Patch (obsolete) — Splinter Review
Code for first review
Attachment #133444 - Attachment is obsolete: true
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

>+   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...
Attachment #133789 - Flags: review-
Attachment #133789 - Attachment is obsolete: true
Attachment #134916 - Flags: review?(mkaply)
Attached patch More current patch (obsolete) — Splinter Review
We had to rewrite some of the code to handle non alpha better.
Attachment #134916 - Attachment is obsolete: true
Attached patch The real slim shady (obsolete) — Splinter Review
Attachment #137470 - Attachment is obsolete: true
Comment on attachment 137603 [details] [diff] [review]
The real slim shady

r=mkaply, sr=blizzard (platform specific), a=mkaply
Attachment #137603 - Flags: superreview+
Attachment #137603 - Flags: review+
Attachment #137603 - Flags: approval1.6+
Attachment #137603 - Flags: approval1.4.2+
Closed: 19 years ago
Keywords: fixed1.4.2, fixed1.6
Resolution: --- → FIXED
Comment on attachment 134916 [details] [diff] [review]
Made changes proposed by timeless

removing obsolete review request
Attachment #134916 - Flags: review?(mkaply)
I'm reopening this.

This broke too much stuff, so I am backing it out everywhere.
Resolution: FIXED → ---
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.
bummer. This is an icky one.
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...
Here's the bug that caused me to back it out:
Attached patch test patch for 1.8.0 branch (obsolete) — Splinter Review
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 releases and get some testing). I uploaded a DLL with the fix to <>.

The patch surely fixes the problems seen in the JS/Canvas test at <> 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.
Assignee: mozilla → mozilla
Attachment #137603 - Attachment is obsolete: true
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...)
Attachment #222938 - Attachment description: DOM key test results table → DOM key test results table (to be viewed in IBM850 encoding)
Attached patch Patch for review (trunk) (obsolete) — Splinter Review
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).
Attachment #223245 - Flags: review?(mozilla)
Attachment #219523 - Attachment description: test for 1.8.0 branch → test patch for 1.8.0 branch
Attached patch Patch with fixes and cleanup (obsolete) — Splinter Review
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 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)
Attachment #223245 - Flags: review?(mozilla)
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.
Attachment #219523 - Attachment is obsolete: true
Attachment #223245 - Attachment is obsolete: true
Attachment #223246 - Attachment is obsolete: true
Attachment #231243 - Flags: review?(mozilla)
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 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 :)
Attachment #231243 - Flags: review?(mozilla) → review+
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.
Closed: 19 years ago16 years ago
Resolution: --- → FIXED
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
Hmm, why are there no OS/2 people CCed to this bug? The previous warning should have gone to all of you...
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).
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.
OK, so I now landed this on the 1.8 branch.
Keywords: fixed1.8.1.8
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:
   Gecko/20070923 BonEcho/
   Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:
   Gecko/20071003 BonEcho/

Let's hope that no regressions come up any more.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.