Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Core
Event Handling
VERIFIED FIXED
14 years ago
10 years ago

People

(Reporter: Lee Tartak, Assigned: Peter Weilbacher)

Tracking

({fixed1.4.2, fixed1.6, verified1.8.1.8})

Trunk
x86
OS/2
fixed1.4.2, fixed1.6, verified1.8.1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

14 years ago
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
(Reporter)

Comment 1

14 years ago
Created attachment 133441 [details]
Test case to show problem
(Reporter)

Comment 2

14 years ago
I will post a fix for this shortly
(Reporter)

Comment 3

14 years ago
Reassigning to mkaply
Assignee: events → mkaply

Updated

14 years ago
OS: other → OS/2
(Reporter)

Comment 4

14 years ago
Created attachment 133444 [details] [diff] [review]
Patch

Code for first review
(Reporter)

Comment 5

14 years ago
Created attachment 133789 [details] [diff] [review]
Allow Shift, Alt, and Control keys through
Attachment #133444 - Attachment is obsolete: true

Comment 6

14 years ago
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...
Attachment #133789 - Flags: review-
(Reporter)

Comment 7

14 years ago
Created attachment 134916 [details] [diff] [review]
Made changes proposed by timeless
Attachment #133789 - Attachment is obsolete: true

Updated

14 years ago
Attachment #134916 - Flags: review?(mkaply)

Comment 8

14 years ago
Created attachment 137470 [details] [diff] [review]
More current patch

We had to rewrite some of the code to handle non alpha better.
Attachment #134916 - Attachment is obsolete: true

Comment 9

14 years ago
Created attachment 137603 [details] [diff] [review]
The real slim shady
Attachment #137470 - Attachment is obsolete: true

Comment 10

14 years ago
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+

Comment 11

14 years ago
Fixed
Status: NEW → RESOLVED
Last Resolved: 14 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)

Comment 13

13 years ago
I'm reopening this.

This broke too much stuff, so I am backing it out everywhere.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

12 years ago
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

12 years ago
bummer. This is an icky one.
(Assignee)

Comment 16

12 years ago
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

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

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

Comment 18

11 years ago
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.
Assignee: mozilla → mozilla
Attachment #137603 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
(Assignee)

Comment 19

11 years ago
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
(Assignee)

Comment 20

11 years ago
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...)
(Assignee)

Updated

11 years ago
Attachment #222938 - Attachment description: DOM key test results table → DOM key test results table (to be viewed in IBM850 encoding)
(Assignee)

Comment 21

11 years ago
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).
Attachment #223245 - Flags: review?(mozilla)
(Assignee)

Updated

11 years ago
Attachment #219523 - Attachment description: test for 1.8.0 branch → test patch for 1.8.0 branch
(Assignee)

Comment 22

11 years ago
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.
(Assignee)

Comment 23

11 years ago
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)
(Assignee)

Comment 24

11 years ago
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.
Attachment #219523 - Attachment is obsolete: true
Attachment #223245 - Attachment is obsolete: true
Attachment #223246 - Attachment is obsolete: true
Attachment #231243 - Flags: review?(mozilla)
(Assignee)

Comment 25

11 years ago
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

11 years ago
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+
(Assignee)

Comment 27

11 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 28

10 years ago
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).
(Assignee)

Comment 29

10 years ago
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).

Comment 31

10 years ago
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.
(Assignee)

Comment 32

10 years ago
OK, so I now landed this on the 1.8 branch.
Keywords: fixed1.8.1.8
(Assignee)

Comment 33

10 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.8 → verified1.8.1.8
You need to log in before you can comment on or make changes to this bug.