197474 - Event.altKey && Event.ctrlKey not true together unlike IE

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Win32, defect)

Description

[Timothy C. Quinn]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030210

I have a page speed tester I built in IE and I use the shift-control-alt-T to
start it. This works great in IE but not in mozilla. Looks like mozilla cannot
handle these special keys in combination. 
I will attach a test case. The source should show the workings of the bug. This
code works in IE 5.5+ and Moz 5+. Pardon the cross browser js code at the top it
just makes my level 2 code much cleaner.

Reproducible: Always

Steps to Reproduce:
1.Press control key
2.While holding control key, press alt key
3.notice that control key output is now false

Actual Results:  
Event.ctrlKey is true at 1 then when alt key is pressed (2) Event.ctrlKey is
false and Event.altKey is true.

Expected Results:  
Event.ctrlKey && Event.altKeykey should be true when both are pressed

Comment 1

[Timothy C. Quinn]

Attachment: IE/Moz Test case

Please excuse the X-Browser stuff at top. This is my way of normaizing IE's
event stupidity.

Comment 2

[Timothy C. Quinn]

Correction: when control and alt are pressed together, they are both false while
they should both be true. Shift and Alt and Shift with Control works correct
(both true).

Comment 3

[Wladimir Palant]

The problem is in the file /widget/src/windows/nsWindow.cpp, it's this code
(occures several times in the file):

// If the Context Code bit is down and we got a WM_KEY
// it is a key press for character, not accelerator
// see p246 of Programming Windows 95 [Charles Petzold] for details
mIsControlDown = (0 == (KF_ALTDOWN & HIWORD(lParam)))&& IS_VK_DOWN(NS_VK_CONTROL);
mIsAltDown     = (0 == (KF_ALTDOWN & HIWORD(lParam)))&& IS_VK_DOWN(NS_VK_ALT);

I don't know what is said in this book but I cannot find anything on
microsoft.com that says that you have to ignore modifier keys when the context
code bit is set.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sounds like a win32 gfx issue....

Comment 5

[Ere Maijala (slow)]

Accepting...

Comment 6

[Ere Maijala (slow)]

Attachment: Possible patch

Changes in this patch:

- make it so that the alt, control and shift states are given as parameters to
DispatchKeyEvent to avoid messing around with the member variables
- control and alt are never down at the same time in keypress events (in this
case it's a keypress for a character)
- don't set control and alt state in WM_CHAR/WM_SYSCHAR, there is no point in
doing that
- don't use the context bit (it's not valid in WM_CHAR and useless anywhere
else too)

Comment 7

[:deanis74]

OK, there's no way that can be the right patch.  I haven't looked through the
source yet, but is there any chance this is a copy-and-paste error?

mIsControlDown = (0 == (KF_ALTDOWN & HIWORD(lParam)))&& IS_VK_DOWN(NS_VK_CONTROL);
mIsAltDown     = (0 == (KF_ALTDOWN & HIWORD(lParam)))&& IS_VK_DOWN(NS_VK_ALT);

Why are we looking for KF_ALTDOWN to determine if Control is pressed?

Comment 8

[Ere Maijala (slow)]

That's one of the things my patch removes. Did I miss something?

Comment 9

[[e-mail]]

Is the reason that Mozilla doesn't allow you to use Ctrl+Alt+Shift+T is that in
theory a keyboard could map that to a unique character (&#x166 perhaps)?

Comment 10

[[e-mail]]

Actually I just tried both Dean's keyboard test page and Spy++ and I don't get
WM_CHAR for Ctrl+Alt keys except for those which the keyboard driver remaps.

Comment 11

[Ere Maijala (slow)]

Comment on attachment 142063 [details] [diff] [review]
Possible patch

I discovered a problem with this patch. Dean's test page shows that we're now
firing two keypress events for AltGr-2 (@).

Comment 12

[:deanis74]

> - make it so that the alt, control and shift states are given as parameters to
> DispatchKeyEvent to avoid messing around with the member variables

Isn't this going to be confusing?

Comment 13

[:deanis74]

In fact, every time except once you're passing in the member variables.  Can't
you continue to use the member variables only, and move your isAltDown and
isControlDown determination into an NS_KEY_PRESS section in DispatchKeyEvent?

Comment 14

[Ere Maijala (slow)]

>> - make it so that the alt, control and shift states are given as parameters to
>> DispatchKeyEvent to avoid messing around with the member variables

>Isn't this going to be confusing?

No, it's much more clear than messing with the members. There are actually two
places where non-members are passed (IME being the other one). I want
DispatchKeyEvent to not mess around with the parameters but just do what it
says. It is called from quite a few places and there are already enough places
where the  keycodes and shift states are being handled.

Comment 15

[:deanis74]

Then can we get rid of the member variables entirely?

Comment 16

[Ere Maijala (slow)]

Attachment: Patch v2

A revised patch. I've changed the logic of deciding when to synthesize the
keypress event. Now it's done by peeking the message queue for WM_CHAR,
WM_SYSCHAR or WM_DEADCHAR.

Comment 17

[Ere Maijala (slow)]

Attachment: Patch v2 with no whitespace differences (diff -w)

Same patch with -w.

Comment 18

[Ere Maijala (slow)]

Dean: interesting idea. The only thing worrying me would be the IME code and
whether the state read there would be reliable and not causing any trouble. I've
tried to stay away from that because I don't have too much experience in IME.

Comment 19

[:deanis74]

Oh right, I keep forgetting about the IME code.  We definitely wouldn't want to
touch that!  Those are booleans defined in nsBaseWidget.h.  Too bad they weren't
functions, then we could use them inside nsWindow.cpp and expose them to the
outside as well.  They could just check the current keyboard state for each key.

Comment 20

[[e-mail]]

That OnChar processing looks very ugly, it appears to be passing the same
parameter not once, not twice, but three times!

I don't suppose that in the VK_RETURN/VK_BACK case you can remove the WM_CHAR?

I would prefer to stick with the member variables myself.

CC'ing some guys who might know about IME...

Comment 21

[Ere Maijala (slow)]

Comment on attachment 142265 [details] [diff] [review]
Patch v2

Bah, now I have to clean up OnChar too.

Comment 22

[Ere Maijala (slow)]

Attachment: Patch v3

Third shot: cleaned up OnChar and its parameters and moved all the
enter/backspace special handling into OnKeyDown (it will remove the char
message from the queue so the WM_(SYS)CHAR handler doesn't need to do any
tricks).

Comment 23

[Ere Maijala (slow)]

Attachment: Patch v3 with no whitespace diff

Same patch with -w

Comment 24

[:deanis74]

Comment on attachment 142368 [details] [diff] [review]
Patch v3 with no whitespace diff

Does Ctrl+Enter still work properly with this change?  (bug 50255)

Comment 25

[Ere Maijala (slow)]

Yes, it works.

Comment 26

[Ere Maijala (slow)]

Comment on attachment 142367 [details] [diff] [review]
Patch v3

Embarrassing bug in PeekMessage bits..

Comment 27

[Ere Maijala (slow)]

Attachment: Patch v4

Fixed the mistake with PeekMessage calls' parameters and restructured lines
around them a bit. Thanks to Neil for spotting these.

Comment 28

[Ere Maijala (slow)]

Attachment: Patch v4 with no whitespace diff

Again, the same with diff -w.

Comment 29

[[e-mail]]

Comment on attachment 142372 [details] [diff] [review]
Patch v4

>-        if (!isMultiByte)	{ 
>-          if (mLeadByte)  {	// mLeadByte is used for keeping the lead-byte of CJK char 
>+        if (charCode > 0xFF) {  // multibyte character
>+          if (mLeadByte) {	// mLeadByte is used for keeping the lead-byte of CJK char 

isMultiByte used to be (wParam > 0xff) so I doubt replacing !isMultiByte with
charCode > 0xFF is going to be of much use...

Nit: try to remove trailing whitespace on modified lines.

Comment 30

[Ere Maijala (slow)]

Attachment: Patch v5

Yep, thanks for catching that. This patch fixes the evaluation and removes some
trailing spaces.

Comment 31

[Ere Maijala (slow)]

Attachment: Patch v5 without whitespace diff

Same with diff -w

Comment 32

[Wladimir Palant]

You might want to replace the hack to translate the virtual key into an ascii
code in OnKeyDown as well. It seems to me that this code is equivalent to
something like this:

UINT asciiKey = MapVirtualKey(virtualKeyCode, 2) & 0xFF; // remove the top bit
(set for dead keys)
if (asciiKey)
...

Comment 33

[Wladimir Palant]

Sorry, the bit mask 0x7FFF would be probably more appropriate (though
MapVirtualKeyEx() doesn't seem to return a multibyte value on any occasion). The
bit set for dead keys is 0x8000 on Win98 and 0x80000000 on WinXP, this mask will
remove both. The correct form should be then:

UINT asciiKey = MapVirtualKeyEx(virtualKeyCode, 2, gKeyboardLayout) & 0x7FFF;

I checked it with the German and Russian keyboard layout on both Win98 and WinXP
and it seems to work fine.

Comment 34

[[e-mail]]

OK, I think I found a possible issue - Ctrl+Alt+T gives me "T", Ctrl, Alt which
while better than nothing (i.e what we currently get!) on linux it gives me "t",
Ctrl, Alt; "T", Ctrl, Alt, Shift is the same on both platforms of course.

I still don't like the business about the extra parameters...

Other than that, this is great work, it should clear up several bugs :-)

Comment 35

[Ere Maijala (slow)]

Comment on attachment 142382 [details] [diff] [review]
Patch v5

Ok, I'll remove the parameters and go back to messing around with the members.
I'll also look into the other problems.

Comment 36

[Ere Maijala (slow)]

MapVirtualKeyEx sucks for Moz in many ways. One of them is that NS_VK_RETURN is
converted to char code 13, which doesn't work. I won't try to guess how many
similar problems it would cause.

Comment 37

[Ere Maijala (slow)]

Attachment: Patch v6

Removed the extra parameters to DispatchKeyEvent due to overwhelming pressure
:) Also fixed ctrl-alt-lower case letters.

Comment 38

[Ere Maijala (slow)]

Attachment: Patch v6 without whitespace diff

Again the same with diff -w.

Comment 39

[Ere Maijala (slow)]

Attachment: Patch v7

Removed an accidentally left MapVirtualKeyEx and change the code to the way it
was before. Also removed an unnecessary block from WM_KEYDOWN case.

Comment 40

[Ere Maijala (slow)]

Attachment: Patch v7 with no whitespace diff

Same with diff -w

Comment 41

[:deanis74]

Comment on attachment 142468 [details] [diff] [review]
Patch v7

>+    if (PeekMessage(&msg, mWnd, 0, 0, PM_NOREMOVE | PM_NOYIELD) &&
>+        (msg.message == WM_CHAR || msg.message == WM_SYSCHAR)) {

It's probably no use to put in WM_KEYFIRST, WMKEYLAST instead of 0,0 right?

This is funky code.  Nice.

Comment 42

[[e-mail]]

Comment on attachment 142468 [details] [diff] [review]
Patch v7

>+        asciiKey = virtualKeyCode;
>+        // Take the Shift state into account
>+        if (!mIsShiftDown && NS_VK_A <= virtualKeyCode && virtualKeyCode <= NS_VK_Z)
>+          asciiKey += 0x20;      
Nit: remove trailing spaces... anyway, try this (or some variant thereof):
asciiKey = virtualKeyode;
if (!mIsShiftDown)
  asciiKey |= 0x20;

>+            PRBool saveIsAltDown = mIsAltDown;
>+            PRBool saveIsControlDown = mIsControlDown;
I didn't trawl through the code to see if this is necessary so I'll take your
word for it.
>+            mIsAltDown = saveIsAltDown;
>+            mIsControlDown = saveIsControlDown;

It looks as if this patch doesn't quite solve the original problem - there
appears to be a bug in the menu code such that ctrl+alt+shift+t opens the tools
menu :-(

Comment 43

[Ere Maijala (slow)]

> It's probably no use to put in WM_KEYFIRST, WMKEYLAST instead of 0,0 right?

I decided it's better not as it would cause trouble in the unlikely event there
was another WM_CHAR somewhere down the queue.

>   asciiKey |= 0x20;

Done and trailing spaces removed.

> It looks as if this patch doesn't quite solve the original problem - there
> appears to be a bug in the menu code such that ctrl+alt+shift+t opens the
> tools menu :-(

I noticed that too, but it doesn't prevent the page from getting the key press
before the menu is shown. I think it's worth another bug anyway.

Comment 44

[Hidehiro Kozawa]

comment 20:
I tested patch v7.
Alt + kanji and some IME functions with alt or ctrl work well.
(I don't know that this patch causes no problem for IME...)

Comment 45

[Boris Zbarsky [:bzbarsky]]

It'll take me a few days to get to this.... this code is way outside what I know
about.

Comment 46

[Brian Ryner (not reading)]

Comment on attachment 142468 [details] [diff] [review]
Patch v7

Dan, I'm delegating sr= to you on this one :)

If you don't want it, bounce it back to me.

Comment 47

[Dan M]

Comment on attachment 142468 [details] [diff] [review]
Patch v7

Sorry. I don't know this part of the widget code, and I'm not going to have
time to do the patch any justice.

Comment 48

[Ere Maijala (slow)]

Brian, any estimate on when you can sr this?

Comment 49

[Ere Maijala (slow)]

Cc'ing bryner for a comment...

Comment 50

[Brian Ryner (not reading)]

Comment on attachment 142468 [details] [diff] [review]
Patch v7


Would it be better to factor the PeekMessage call out, something like:

MSG msg;
BOOL gotMsg = ::PeekMessage(&msg, mWnd, 0, 0, PM_NOREMOVE | PM_NOYIELD);

if (virtualKeyCode == NS_VK_RETURN || virtualKeyCode == NS_VK_BACK) {
  if (gotMsg && (msg.message == WM_CHAR || msg.message == WM_SYSCHAR)) {
    ::GetMessage(&msg, mWnd, 0, 0);
  }
} else if (gotMsg &&
	   msg.message == WM_CHAR || msg.message == WM_SYSCHAR || msg.message
== WM_DEADCHAR) {
  return result;
}

The rest of the patch looks ok but I'm going to run it through a few tests
before marking sr+.

Comment 51

[Brian Ryner (not reading)]

Comment on attachment 142468 [details] [diff] [review]
Patch v7

While I did find some alarming inconsistencies between platforms wrt key
events, this patch seems to do nothing but make things better.	sr=me.

Comment 52

[Ere Maijala (slow)]

Fix checked in with the second PeekMessage factored out as Brian suggested.

Comment 53

[Aaron Leventhal]

This is the most likely cause of the regression for Ctrl++ and Ctrl+- zooming
under Windows in bug 244334. It fits the regression window.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Is this bug checked in 1.7 branch or trunk only?
If this bug is checked in 1.7 branch,
bug 194559 should be fixed on 1.7 branch too.

Comment 55

[Ere Maijala (slow)]

This is trunk only.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ere:
Thank you for reply.

However, Bug 194559 is major bug for Japanese people.
If this bug is low risk, I hope that this bug and Bug 194559 are fixed
on 1.7 branch...

Comment 57

[Ere Maijala (slow)]

Unfortunately this isn't a low risk fix and it has caused some regressions.
There is still one open, bug 244334. I'm not sure if after fixing that it would
be possible to merge it to the branch.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Ere.
Bug 194559 is major bug for Japanese people,
but the bug have no value of risking.

Comment 59

[[e-mail]]

*** Bug 208302 has been marked as a duplicate of this bug. ***