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

Status: VERIFIED FIXED

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

Description

[Lee Tartak]

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

[Lee Tartak]

Attachment: Test case to show problem

Comment 2

[Lee Tartak]

I will post a fix for this shortly

Comment 3

[Lee Tartak]

Reassigning to mkaply

Comment 4

[Lee Tartak]

Attachment: Patch

Code for first review

Comment 5

[Lee Tartak]

Attachment: Allow Shift, Alt, and Control keys through

Comment 6

[timeless]

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

[Lee Tartak]

Attachment: Made changes proposed by timeless

Comment 8

[Mike Kaply [:mkaply]]

Attachment: More current patch

We had to rewrite some of the code to handle non alpha better.

Comment 9

[Mike Kaply [:mkaply]]

Attachment: The real slim shady

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 137603 [details] [diff] [review]
The real slim shady

r=mkaply, sr=blizzard (platform specific), a=mkaply

Comment 11

[Mike Kaply [:mkaply]]

Fixed

Comment 12

[Matthias Versen [:Matti]]

Comment on attachment 134916 [details] [diff] [review]
Made changes proposed by timeless

removing obsolete review request

Comment 13

[Mike Kaply [:mkaply]]

I'm reopening this.

This broke too much stuff, so I am backing it out everywhere.

Comment 14

[Peter Weilbacher]

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

[Mike Kaply [:mkaply]]

bummer. This is an icky one.

Comment 16

[Peter Weilbacher]

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

[Mike Kaply [:mkaply]]

Here's the bug that caused me to back it out:

https://bugzilla.mozilla.org/show_bug.cgi?id=236527

Comment 18

[Peter Weilbacher]

Attachment: 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 19

[Peter Weilbacher]

Note to self: I didn't find any real specs for this, but some links:

http://www.mozilla.org/editor/key-event-spec.html (old Mozilla docs)
http://www.w3.org/TR/1999/WD-DOM-Level-2-19990304/events.html#Level2-Events-UIEvent
http://developer.mozilla.org/en/docs/DOM:event.charCode
http://developer.mozilla.org/en/docs/DOM:event.keyCode
http://www.javascriptkit.com/dhtmltutors/domeventp2-2.shtml

Comment 20

[Peter Weilbacher]

Attachment: 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

[Peter Weilbacher]

Attachment: 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

[Peter Weilbacher]

Attachment: 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

[Peter Weilbacher]

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

[Peter Weilbacher]

Attachment: 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

[Peter Weilbacher]

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

[Mike Kaply [:mkaply]]

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

[Peter Weilbacher]

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

[Peter Weilbacher]

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

[Peter Weilbacher]

Hmm, why are there no OS/2 people CCed to this bug? The previous warning should have gone to all of you...

Comment 30

[Andy Willis (abwillis)]

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

[Mike Kaply [:mkaply]]

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

[Peter Weilbacher]

OK, so I now landed this on the 1.8 branch.

Comment 33

[Peter Weilbacher]

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.