11956 - [key]Key Press events are not being generated correctly

Status: VERIFIED FIXED

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

Description

[rods (gone)]

The IME event processing code does not generate generate a "key pressed" event
for a "<control>1". It also generates events for when the shift key is pressed
without any other keys being pressed (I think this is incorrect.)

The current IME is ifdef'ed with my old event processing code. I have made the
appropriate changes (in my tree) to have it process all the key up,press,down
combinations correctly for most all key combinations. I coud check in this code
and it would enable the correct processing of events until the IME key events
are working properly.

Comment 1

[buster]

rod, does your fix allow for the case where a keyDown handler consumes the event
and declare that no more processing should occur
(nsEventStatus_eConsumeNoDefault), such that a keyPress event won't be
generated?  I don't know if the consume-no-default is sufficient, maybe there is
some way to set an event flag to cancel further event generation?  nsGUIEvent.h
is almost completely undocumented, so I have no way of knowing what the various
flags mean or what their semantics are.

Comment 2

[rods (gone)]

Is there something I can do to test it? Is there a situation that cancels after
the key down?

Comment 3

[Kathleen :Brade]

actally, it shouldn't consume the event as Steve requests above.  I spoke with
joki last Friday and he didn't think that the down event should cancel the
keypress.

Comment 4

[tague]

i thought the event model we agreed to last time had no cancellation of keydown
events and generated events for every key transition.

can you put you fixes into an attachement so that we can look at them.

Comment 5

[rods (gone)]

The fixes were to my old key event processing code not to the IME code. In a nut
shell I generate a PRESS event for every OnChar and then send a PRESS event
after the key down for al special keys that don't invoke a ystem OnChar.

Here are the diffs:
Index: nsWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v
retrieving revision 3.187
diff -r3.187 nsWindow.cpp
2158c2158
< #if 1
---
> #ifdef DO_IME
2174a2175
>   BOOL doPress = FALSE;
2182a2184
>       doPress = TRUE;
2187c2189,2193
<       asciiKey = 0; // just to be paranoid
---
>       //asciiKey = 0; // just to be paranoid
>       BYTE keyState[256];
>       ::GetKeyboardState(keyState);
>       ::ToAscii(aVirtualKeyCode, aScanCode, keyState, &asciiKey, FALSE);
>       aVirtualKeyCode = asciiKey;
2196c2202,2206
<     return DispatchKeyEvent(NS_KEY_DOWN, asciiKey, aVirtualKeyCode);
---
>     BOOL status = DispatchKeyEvent(NS_KEY_DOWN, asciiKey, aVirtualKeyCode);
>     if (doPress && status) {
>       status = DispatchKeyEvent(NS_KEY_PRESS, asciiKey, aVirtualKeyCode);
>     }
>     return status;
2243c2253
< #if 1
---
> #ifdef DO_IME
2283c2293
<   return DispatchKeyEvent(NS_KEY_DOWN, aVirtualKeyCode, aVirtualKeyCode);
---
>   return DispatchKeyEvent(NS_KEY_PRESS, aVirtualKeyCode, aVirtualKeyCode);
2401c2411
< #if 1
---
> #ifdef DO_IME
2439c2449
<                                       result = OnChar(char_result,0,false);
---
>                                       result = OnChar(char_result,ch,false);
2453c2463
<             if (mIsControlDown && !mIsAltDown &&
---
>             /*if (mIsControlDown && !mIsAltDown &&
2459c2469
<             }
---
>             }*/
2480c2490
<                             if (!mIMEIsComposing)
---
>             if (!mIMEIsComposing) {
2482c2492
<                             else
---
>             } else
Index: nsWindow.h
===================================================================
RCS file: /cvsroot/mozilla/widget/src/windows/nsWindow.h,v
retrieving revision 3.76
diff -r3.76 nsWindow.h
41a42
> //#define DO_IME
170c171
< #if 1
---
> #ifdef DO_IME

Comment 6

[tague]

I don't think that going back to old code is the right thing to do.
This regresses i18n functionality.  If we need to fix this for M9, the fix
should be done in the existing code.

Comment 7

[joki (gone)]

Just to make sure we're all clear and ok on the keydown cancelling keypress
thing.

4.x cancelled keypresses based on keydowns.
The DOM Level 2 spec doesn't say anything about cancellation.

When we last gathered and talked non-cancellation seemed like the right thing
for i18n and applications could work with this.  So unless we feel a strong
need to add this I think we'll probably work without the cancellation.  This
was a rather obscure feature in the 4.x model which I *think* we can get by
without.

Comment 8

[joki (gone)]

so another interesting point was raised in that for ctrl+key we always send the
uppercase charcode to the keypress event but for just key or alt+key we send the
lowercase version unless the shift key is pressed.  This probably needs to
change to a more uniform model where the ctrl+key sends a lower/uppercase
version as well.

Tague, what are i18n implications of something like this?

Comment 9

[tague]

there's no way i'm going to get this done for m10. i realize that it is a
blocker - but there are other bigger blockers that i needed to look at first.

Comment 10

[rods (gone)]

Will it will be done before beta? We have to have it generating non-IME key code
correctly for beta. Actually, we really need it generating non-IME key code in
M10. If not, we should go back to my old code.

Comment 11

[tague]

please work with lyecies and ftang if you aren't happy with the priority this
item is getting.

Comment 12

[Frank Tang]

reassign to ftang per i18ngrp reassignment meeting.

Comment 13

[Frank Tang]

fix in keyEvent_19991004_BRANCH landing

Comment 14

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 15

[Loco]

Updating QA Contact.

Comment 16

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 17

[Madhur Bhatia]

QA contact updated

Comment 18

[luis rincon]

VERIFIED ON BUILD 2001-07-30-08-TRUNK