287179 - Unshifted charCode is generated for keypress event when both Ctrl and Shift are held down

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Dainis Jonitis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050225 Firefox/1.0.1

See keyboard test:
https://bugzilla.mozilla.org/attachment.cgi?id=55849&action=view

Reproducible: Always

Steps to Reproduce:
1. Press 'a'
2. Press Shift+'a'
3. Press Ctrl+Shift+'a'

Actual Results:  
For Ctrl+Shift+'a' combination the character 'a' is generated even if the Shift
key is down.

Expected Results:  
For Ctrl+Shift+'a' combination the character 'A' should be generated.

This problem is not limited only to the alphabet letters. For example pressing
the  Shift+Ctrl+'`' should produce the '~' character (at least on US keyboards).

The problem is in fact that no WM_CHAR message is generated if Ctrl key is held
and as result the Win32 widget code has to emulate the NS_KEY_PRESS event from
information that is available in WM_KEYDOWN. See code at:
http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsWindow.cpp#3450

For special keys in 'switch' the state of mIsShiftDown is not taken into
account. And 'default' which handles letters from a to z is correct only for US
keyboard layout. Let's say you have Russian keybord layout active and you press
Shift+Ctrl+'л'. Instead of capital 'Л' you will get latin 'K' which is situated
on the same key.

Can someone investigate the behaviour on other platforms and possibly in other
browsers? What forthcoming DOM3 events will recomend in this situation?

Comment 1

[Dainis Jonitis]

Attachment: Diff -d -u -20 Quick and dirty fix for US keyboard only

This patch corrects the handling of '`~', '-_' and ';:' combinations and is
valid for US keyboards only. The more generic fix that would take into account
the current keyboard layout would be required.

I used bad example in 'Steps to recreate' :( With Shift+Ctrl+'a' we will get
the capital 'A' which is right. 
But the problem with foreign keyboard layouts and special keys like '`~', ';:',
''"', ',<', '.>', '/?' etc. is still actual.

Comment 2

[Dainis Jonitis]

For correct mapping the 'GetKeyboardState' and 'ToUnicode' Win32 SDK functions
should be used. I will post a patch when it's ready.
But before that I want to know whether it is desired behaviour that if more than
one modifier key is pressed then charCode is always generated according to the
state of Shift key (taking into account the state of CapsLock) and state of
Ctrl, Alt and Meta keys is ignored.

Comment 3

[Dainis Jonitis]

Attachment: diff -d -u -20 Keyboard layout independent fix (not for Win9x!!!)

This fix makes it so that for these keys:
1. VK_0..VK_9
2. VK_A..VK_Z
3. VK_OEM_1 ';:'
4. VK_OEM_PLUS '=+'
5. VK_OEM_COMMA ',<'
6. VK_OEM_MINUS '-_'
7. VK_OEM_PERIOD '.>'
8. VK_OEM_2 '/?'
9. VK_OEM_4 '[{'
10.VK_OEM_5 '\]'
11.VK_OEM_6 ']}'
12.VK_OEM_7 ''"'

the charCode is always generated taking into the account the state of Shift
key. Compared to old code it correctly works with non US-keyboard layouts (like
Russian). It should fix bug 285161. The state of CapsLock is taken into account
for all cases but Alt+[a..z] to make sure that menu accelerators do not depend
on state of CapsLock.

This version is still final, because it uses the 'ToUnicode' function which is
not available on Win95.

Comment 4

[Dainis Jonitis]

Seems that Win95 does have the 'ToUnicode' function after all.
But there is another problem. Calling this function breaks dead key handling (on
any Windows version).

Michael Kaplan from Microsoft talks about this problem in his blog:
http://blogs.msdn.com/michkap/archive/2005/01/19/355870.aspx

I will look for a way how to work around this problem.

Comment 5

[Dainis Jonitis]

Attachment: diff -d -u -20 Keyboard layout independent fix (with hacky dead-key handling)

I prepared quick patch which seems to handle the dead-key problem. Currently
the dead-key is hardcoded to the '`~' (VK = 0xC0) which is used in my Latvian
keyboard layout. I will make code that dynamicaly detects all active deadkeys
and ignores them (seems that it should also listen for keyboard layout switches
during runtime).

I would really appreciate if someone plays with this patch to see if there are
no obvious regressions. The possible problems could be with CapsLock (for
Jewish keyboard layot?) and handling of AltGr key (left Ctrl+Alt) combinations
for German keyboard layouts. Does this run on Win95?

Thanks in advance for any feedback!

Comment 6

[Dainis Jonitis]

Attachment: diff -d -u -20

This patch seems to work pretty fine with deadkeys. I also made a change that
allows to generate more than one character if there was pressed the deadkey and
then a character that does not produce the combined char. For example if your
ded-key is '`' and you press the '`'+'a' in Latvian layout you will get
'ā'. But if you press '`'+'d' you will get two characters '`', 'd'.

I introduced new class 'DeadKeyInfo' that depending on current keyboard layout
detects all dead-keys. In future one can make it more general and rename it to
'KeyboardLayout' and move all keyboard layout dependant global variables
(gKeyboardLayout, gCurrentKeyboardCP) into this class.

Comment 7

[David G King]

Dainis asked me to build this under Cygwin, however my initial attempt failed
with ::BlockInput has not been declared, which happened on line 215 of nsWindow.cpp.

Comment 8

[Dainis Jonitis]

Attachment: Same as above, but compiles on MinGW.

The BlockInput was not declared in MinGW header files. And it is not available
on Win95. Made change to use the stub function on Win95.

David, please try this.

Comment 9

[David G King]

Well, the good news is that fix was good. The bad news is that you've just
exposed a bug in cygwin's gdkprivate-win32.h which has the following :-

#ifndef VM_OEM_PLUS
#define VK_OEM_PLUS 0xBB
#endif

The VM should be a VK. Hence why I now get an error about VK_OEM_PLUS not being
defined.

(In reply to comment #8)
> Created an attachment (id=180025) [edit]
> Try to make it compile with MinGW
> 
> The BlockInput was not declared in MinGW header files. And it is not available
> on Win95. Made change to use the stub function on Win95.
> 
> David, please try this.

Comment 10

[David G King]

On second thought, the commented out include of <winuser.h> could also do it.

Comment 11

[Dainis Jonitis]

I think we should not work around the MinGW wrong definition in Mozilla source
code. It is obvious bug in MinGW include file and it should be fixed in MinGW
itself.

Comment 12

[David G King]

Currently I can't determine if it's a problem with cygwin or with the Mozilla
code. In either case, I'm getting some rest and will look at it again later.

You might as well leave the "r" request as it is though.

Comment 13

[Dainis Jonitis]

Phew! The dead-keys are really pain in the ass!
The ::ToUnicode does not take into account the dead-key if character is pressed
together with Alt. If Alt+Ctrl is used then dead-key is taken into account to
produce correct combined character.

I tried many different things, but the only way to get correct dead-key affected
combined character for Alt+character case is to let them be handled by the
Windows and the charcode to be extracted from WM_CHAR message.

The code to manually generate NS_KEYPRESS for Alt+[a..z] in nsWindow::OnKeyDown
was added in v3.506 for bug 217560 (2004-07-09), although defect is unrelated to
this. The idea is to ignore the CapsLock state for accelerator keys in menus.

In my opinion it is responsibility of upper layer cross platform code to ignore
the case of the accelerator keys. The widget code should not do this at least
because of fact that for many keyboard layouts the CapsLock+character !=
Shift+character. And being unable to enter dead-key combined char in combination
with Alt prevents the possibility to select menu items by keyboard shortcuts if
they happen to use such accelerator keys.

Ere,
Should I prepare yet another patch that handles Alt+[a..z] in OnChar function as
it was before bug 217560?

Comment 14

[David G King]

I was wrong, again....
The problem is that Mozilla et. al. is designed/forced to compile for Windows 95
and above. To do this, functions that aren't applicable to Win95 aren't
available. For example, VK_OEM_PLUS is only available to WinME, Win2K and above
so can't be used in Mozilla.

(In reply to comment #9)
> Well, the good news is that fix was good. The bad news is that you've just
> exposed a bug in cygwin's gdkprivate-win32.h which has the following :-

Comment 15

[Dainis Jonitis]

Comment on attachment 180025 [details] [diff] [review]
Same as above, but compiles on MinGW.

It is all wrong! I tried Czech keyboard layout and AltGr (left Alt+Ctrl) key
combinations do not work (because I unconditionally mask the state of Alt and
Ctrl keys). Seems that we should try to report base character together with Alt
and Ctrl flags in NS_KEYPRESS event only if this combination does not produce
the different character. But then again there is a problem to get it working
together with dead-keys...

Comment 16

[Ere Maijala (slow)]

Let's take a short break here. I'll try to fix bug 285161 first.

Comment 17

[Dainis Jonitis]

Attachment: Work in progress: dead-key happy, international friendly approach

Taking bug, because now I am sure this is possible to fix :)
This patch seems to be fully working for all testcases that I can imagine at
the moment. Not sure about IMEs, but for Latvian, Russian & Czech it works fine
whether dead-keys are used or not.
Because of bad side effects of dead-keys to the ::ToUnicode function I
construct my own lookup tables once new keyboard layout is activated and then
use them instead of ::ToUnicode. This approach was suggested by Microsoft
internationalization guru Michael Kaplan in his blog:
    http://blogs.msdn.com/michkap/archive/2005/01/19/355870.aspx
Seems that same strategy is used by great Microsoft Keyboard Layout Creator:
    http://www.microsoft.com/globaldev/tools/msklc.mspx

I will clean up the patch and run some more tests. Hopefully it will be ready
for review in one week. As always any feedback is very welcome :)

Comment 18

[Mike Kaply [:mkaply]]

In case you need another key test, we did a pretty extensive one:

https://bugzilla.mozilla.org/attachment.cgi?id=133441

Comment 19

[Dainis Jonitis]

Attachment: Work in progress II

Cleaned-up version. Additionally it fixes one more trunk Mozilla error when
removing WM_CHAR/WM_SYSCHAR messages from message queue. Depending from
keyboard layout one key may produce up to 4 UTF-16 characters. Also if dead-key
is followed by character that does not produce the composite character then we
have to remove from message queue both dead-key character and following base
character.

There is at least one more thing to be fixed. The correct Alt/Ctrl/Shift flags
should be passed to NS_KEY_PRESS handler for dead key that did not produced the
one composite character. Currently the same flags are passed as for base
character. Also both Alt and Ctrl flags should be masked out if AltGr
combination with key produces any character.

Comment 20

[David G King]

Comment on attachment 181424 [details] [diff] [review]
Work in progress II

I know "Work in progress", but I just couldn't resist trying a compile.

Got some errors, one of which was
"/mozilla/widget/src/windows/nsWindow.cpp:320: error: ISO C++ fo
rbids declaration of `CompareDeadKeyEntries' with no type"

Comment 21

[Dainis Jonitis]

Attachment: diff -d -u -20 Fully functional

I wanted to select Christian Biesinger to be a superreviewer, but mail
[e-mail] in module owners is refused by Bugzilla.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 181753 [details] [diff] [review]
diff -d -u -20  Fully functional

sorry... I'm not a super-reviewer

Comment 23

[David G King]

Comment on attachment 181753 [details] [diff] [review]
diff -d -u -20  Fully functional

Errors in cygwin/MinGW build :-

e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:336: error: `const
Dea
dKeyTable* KeyboardLayout::AddDeadKeyTable(const DeadKeyEntry*, PRUint32)' is
pr
ivate
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:364: error: within
thi
s context
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:394: error: extra
qual
ification `DeadKeyTable::' on member `GetCompositeChar' ignored

e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:735: error: invalid
co
nversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:743: error: invalid
co
nversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp: In static member
func
tion `static PRBool KeyboardLayout::EnsureDeadKeyActive(PRBool, PRUint8, const
P
RUint8*)':
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:800: error: invalid
co
nversion from `const PRUint8*' to `BYTE*'
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:800: error: invalid
co
nversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp: In member function
`P
RUint32 KeyboardLayout::GetDeadKeyCombinations(PRUint8, const PRUint8*,
PRUint16
, DeadKeyEntry*, PRUint32)':
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:869: error: invalid
co
nversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsWindow.cpp:887: error: invalid
co
nversion from `PRUint16*' to `WCHAR*'

Comment 24

[Dainis Jonitis]

FYI:
The above patch which I asked to review does not compile with MinGW and VS6, but
compiles fine with VS2003 :( Sorry for that!
I will fix all these minor compiler compatibility errors and attach new patch.
There seems to be difference in Platform SDK include files, too. For example old
VS6 winuser.h does not define VM_OEM_x symbols and for ::ToUnicode function 3rd
param lpKeyState requests PBYTE instead of more correct const PBYTE (as in new
Platform SDK).

Comment 25

[David G King]

There is a fundamental problem here. The Platform SDK's are different depending
on the version of Windows OS you are running.

For example, VK_OEM_PLUS, VK_OEM_MINUS aren't available under Windows 95 and I
suspect Windows 98 and Windows 98SE. AFAIK the Mozilla Foundation still supports
those platforms, so you will neeed to define these yourself or change the code
to not use them.


(In reply to comment #24)
> FYI:
> The above patch which I asked to review does not compile with MinGW and VS6, but
> compiles fine with VS2003 :( Sorry for that!
> I will fix all these minor compiler compatibility errors and attach new patch.
> There seems to be difference in Platform SDK include files, too. For example old
> VS6 winuser.h does not define VM_OEM_x symbols and for ::ToUnicode function 3rd
> param lpKeyState requests PBYTE instead of more correct const PBYTE (as in new
> Platform SDK).

Comment 26

[Dainis Jonitis]

Attachment: Same as above, but compiles with VS6, MinGW and older Platform SDK

Comment 27

[Ian Neal]

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

Comment 28

[Stefan [:stefanh]]

(In reply to comment #0)

> Can someone investigate the behaviour on other platforms and possibly in other
> browsers?

There is an issue on mac as well, see bug 287179. Doesn't seem to happen with
a-z keys, though. But with lots of other keys.

Comment 29

[Stefan [:stefanh]]

> There is an issue on mac as well, see bug 287179. Doesn't seem to happen with
> a-z keys, though. But with lots of other keys.

Err, I meant bug 280805.

Comment 30

[Ere Maijala (slow)]

Comment on attachment 181960 [details] [diff] [review]
Same as above, but compiles with VS6, MinGW and older Platform SDK

Sorry for not getting to this before. As can be expected, the patch doesn't apply cleanly anymore. In any case, nsWindow is getting rather big, so I think it would be better to put this stuff into nsToolkit. Ok? And then a quick explanation of the BlockInput stuff would be appreciated.

Comment 31

[Dainis Jonitis]

You meant nsToolkit.cpp? Probably it is even better to add new file nsKeyboardLayout.cpp. Which approach you prefer?

Comment 32

[Ere Maijala (slow)]

Sure, new files would be fine too. 

Comment 33

[Jacek Piskozub]

If you need any help testing this on WindowsME, I can do that. I have a Seamonkey CVS tree I can use to test your patches.

Comment 34

[Dainis Jonitis]

Attachment: Move new code to separate files

Moved all keyboard layout specific code to new files: nsKeyboardLayout.cpp and nsKeyboardLayout.h. Later we can move some more code from nsWindow.cpp to the class KeyboardLayout.

The Win32 ::BlockInput function is available since Win98. It blocks mouse and keyboard input for current thread. I use it to ensure that no user actions will interrupt the calculation of keyboard deadkey state table. Probably everything will work fine even without it, but I wanted to be sure. I've also noticed that Microsoft Keyboard Layout Creator also uses this function. We do not block input in debug builds, because otherwise you won't be able to use mouse or keyboard in your debugger once you stop on breakpoint while BlockInput is in effect.

Comment 35

[Ere Maijala (slow)]

Comment on attachment 205124 [details] [diff] [review]
Move new code to separate files

Looks good to me. Let's get this in finally have keyboard handling problems solved (hopefully :) ).

Comment 36

[Jacek Piskozub]

Patch 205124 does not work for me. I applied it to my Seamonkey tree (English Windows ME with polish keyboard chosen). Now, when I write a Polish letter (for example) RighAlt-A then instead of latin letter A (as happened wrongly before applying the patch), nothing happens. I mean no letters appear when I try to write Polish signs. I tested it in the Mail Compose window as well as the URL bar.

Comment 37

[Ere Maijala (slow)]

Works for me on WinXP SP2 using the Polish (programmers) keyboard. AltGr-A creates a with ogonek (right?).

Comment 38

[Jacek Piskozub]

Right. However the unpatched code also WFM on Windows XP. It's Windows ME that is the problem. 

I frgot to add that I keep Regional settings (English/United Kingdom) and Polish keyboard. This combination makes it impossible to use Polish characters also on some other programs depending on the WinXP of recognizing the regional settings 9whatever it is). An example is an Polish communicator named Gadu-Gadu.

Comment 39

[Dainis Jonitis]

Jacek, now you confused me with your last two comments :)
Does the patch works as expected or not? Or it was your error with settings?
How can unpatched code work on WinXP. AltGr+A produces latin A which is wrong, isn't it?
I recall I was testing some Czech and Russian keyboard layouts under Win95 and had no problems. It actually should not depend from your regional settings - only from current keyboard layout. You may want to verify which characters are assigned to which key combination by using Microsoft Keyboard Layout Creator:
http://www.microsoft.com/globaldev/tools/msklc.mspx

Comment 40

[Jacek Piskozub]

OK. I'll describe in detail what happens when I press RightAlt-A in both WindowsME and XP with different Language settings (Control Panel: Regional Settings) and with different Mozilla versions. What I expect to get is "a with ogonek", namely "ą". Both Windows wersions (ME & XP) are English ones. The keyboard setting is Polish (programmers) - the one using RightAlt for Polish letters, the default among Polish users.

First Windows ME. I test with fresh build of trunk Seamonkey with the patch applied and a nightly build without the patch (obviously) and with Firefox 1.5.

- Windows ME, Language setting: English, _no patch_: I get Latin letter "a" instead of "ą"
- Windows ME, Language setting: Polish, _no patch_: I get the expected "ą" letter (WFM)
- Windows ME, Language setting English, _patch applied_: I get nothing (no leter appears on the screen)
- Windows ME, Language setting Polish, _patch applied_: I get nothing (no leter appears on the screen)

The results with Firefox 1.5 are identical as with the unpatched nightly Seamonkey build.

To sum it up: everything works as long as I do not apply the patch and set-up Language:Polish (which is not my favorite setting as it creates a Language mix-up in the English windows). My favorite English settings gets me wrong letters (latin cousins of the letters I want). The patch screws everything up with any language setting.

Now Window XP. I do not have a patched version of Seamonkey so I use a fresh nightly build of Seamonkey, Firefox 1.5 and Thunderbird 1.5 rc2.

- Windows XP, Regional Options: English (United Kingdom), _no patch_: I get "ą" as expected (WFM)
- Windows XP, Regional Options: Polish, _no patch_: I get "ą" as expected (WFM)

To sum it up: all recent builds of Mozilla.org software work for me without the patch. No problem here. I have not tested a patched version here but it cannot improve anything for me on Windows XP.

Is this clear now?

Comment 41

[Jacek Piskozub]

I believe the actual problem on Windows ME (and probably 95/98) is that Mozilla uses the Language settings instead of Keyboard setting. I believe my tests prove exactly this (wrong) behavior. It must have something to do with changes between 95/98/ME and XP because I know others programs (Gadu-Gadu) which show identical problesms with older windows.

Comment 42

[Jacek Piskozub]

I just received an email message from a user of English Windows 98 that my work-around (setting Language: Polish) enabled using Polish letters with Firefox 1.5.

Comment 43

[Dainis Jonitis]

I once more checked that under WinXP switching both languages and keyboard layouts correctly generate WM_INPUTLANGUAGECHANGE message.
  I have bad suspicion that Win32 function ::ToUnicode is not fully implemented under Win9x. Now I recall that after seeing that in Platform SDK docs the minimal oprating system is stated WinNT 3.1 I've looked at include files and they were WINVER >= 0x400. Also the win95 user32.dll contained the function entrypoint. Probably it explains why MKLC requires Win2k+.
  I have to find Win98 installation somewhere, install VirtualPC and debug the problem. Will not happen very soon because I'm busy at work.

  Ere, Jacek, if you have Win9x and have debugger available can you check if 

      PRUint16 uniChars [5];
      PRInt32 rv = ::ToUnicode (virtualKey, 0, (PBYTE)kbdState, (LPWSTR)uniChars, NS_ARRAY_LENGTH (uniChars), 0);

in KeyboardLayout::LoadLayout () line 455 ever returns rv different from 0. Normally it should be 1 and uniChars [0] should contain the letter code.

Comment 44

[Ere Maijala (slow)]

I have a Win98SE installation but no debugger there. Anyway, I put up a small console app to test it, and it did return 0 for every call to ToUnicode() under Win98 (way different from XP).

Comment 45

[Jacek Piskozub]

Did you install the newest Unidode layer for Windows 9x? It's available from here 
http://www.microsoft.com/downloads/details.aspx?FamilyID=73BA7BD7-ED06-4F0D-80A4-2A7EEAEE17E2&displaylang=en

The unicows.dll (ver. 1.1.3790.0) and unicows.pdb should go to c:\windows\system

Anyway, I had this installed and all my tests (comment #40) come from this configuration.

Comment 46

[Dainis Jonitis]

Attachment: Possible fix for Win9x. For testing only!!

Please test this new version. It uses ::ToAsciiEx on Win9x where ::ToUnicode is just useless stub. I tried it on my WinXP with nsToolkit::mIsNT forced to false and it worked well. With that approach only the characters that can be represented by currently selected code page (depends on selected keyboard layout language) can be displayed. But to my understanding it is the limitation of Win9x keyboard layouts anyway - thus we are not loosing functionality.

Comment 47

[Dainis Jonitis]

Attachment: Possible fix for Win9x. For testing only!!

Doh! Previous patch was missing the new files. And please ignore some small unrelated changes from bug 319957 that are not checked in yet.

Comment 48

[Jacek Piskozub]

Yes, patch 206803 works for me!!! I now can use Polish GreyAlt entered letters ąęśćółżźł & ĄĘŚĆÓŁŻŹŃ in compose window, email address fileds, URL filelds (everywhere I tried).

I believe this patch should be checked in before Thunderbird 1.5 ships. Otherwise many Win9x people with keoybord settings needing GrayAlt and Language set to English (there are many reasonable reasons for such a combination) will have problems. BTW Thunderbird 1.0.* was free from this problem.

Comment 49

[Dainis Jonitis]

Attachment: New code in separate files + works with Win9x

According to Jacek this works fine on older Win9x systems. I am not sure how this will affect DBCS versions of Windows 9x. Anyone interested to test?

This patch intentionally took some code parts from nsWindow and copied it to nsKeyboardLayout.cpp (e.g. LangIDToCP). I am trying to minimize the size of this patch. If everything will be fine then in followup bug I will remove redundant code from nsWindow.cpp and move some more keyboard related things into nsKeyboardLayout.cpp.
I also had to touch the nsToolkit.cpp because the mIsNT and other static variables are initialized only after all static variables of nsWindow.cpp are initialized. Again I was afraid to change the file order in makefile.in to not cause some other problems. Probably we need to hide all these variables in inline functions that ensure that variables are initialized before use.

Isn't this too late for Thunderbird RC2? This is large patch... Anyway, even the users of Win2K+ will get new functionality. With this you can press e.g. Ctrl+Shift+6 and you will get the '^' symbol the same way as if Ctrl is not pressed.

Comment 50

[Ere Maijala (slow)]

Comment on attachment 206856 [details] [diff] [review]
New code in separate files + works with Win9x

It might be good to have a wrapper for ToUnicode/ToAsciiEx that would do the right thing so you wouldn't need if-then-else everywhere. But that's nitpicking and you can do it in the clean-up phase if it's worth it. r=emaijala

Comment 51

[Dainis Jonitis]

Ere, thanks for review.
About having wrapper around API calls. My current feeling is that it's not a good idea. In some places it is sufficient to have only the return value of ToAsciiEx and we do not need to call ::MultiByteToWideChar. For keyboard layouts with many dead-key combinations it is usual to call ::ToUnicode 30000 times and more. Avoiding to call ::MultiByteToWideChar might be important. I will think about it a bit more. 

Who can superreview this code? Darin Fisher, Brian Ryner or someone else?

Comment 52

[Brian Ryner (not reading)]

Comment on attachment 206856 [details] [diff] [review]
New code in separate files + works with Win9x

I'll go ahead and call out that I don't know a whole lot about the Win32 API calls that you're using.  I'll trust Ere's review on that and concentrate on other issues.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsKeyboardLayout.cpp	26 Dec 2005 15:45:11 -0000
>+PRUint8 KeyboardLayout::gVirtualKeyCodes [NUM_OF_KEYS];

Make this |static|.

>+class DeadKeyTable
>+{
>+  friend class KeyboardLayout;
>+
>+  PRUint16 mEntries;
>+  DeadKeyEntry mTable [1];    // KeyboardLayout::AddDeadKeyTable () will allocate as many entries as required.
>+                              // It is the only way to create the new DeadKeyTable instances.

"It is the only way to create new DeadKeyTable instances."

>+  static PRUint32 SizeInBytes (PRUint32 aEntries)
>+  {
>+    return FIELD_OFFSET (DeadKeyTable, mTable) + aEntries * sizeof (DeadKeyEntry);
>+  }

Mozilla convention is to use offsetof().

>+static BOOL WINAPI BlockInputProc (BOOL fBlockIt)
>+  #ifdef NS_DEBUG

NS_DEBUG is deprecated, please use #ifdef DEBUG instead.

>+inline PRUint16 VirtualKey::GetCompositeChar (PRUint8 aShiftState, PRUint16 aBaseChar) const
>+{

Add something like:

NS_ASSERTION(aShiftState < NS_ARRAY_LENGTH(mShiftStates), "invalid index");

here and also to other functions that take an index into the mShiftStates array... just to make debugging easier if this becomes a problem.

>+  return mShiftStates [aShiftState].DeadKey.Table->GetCompositeChar (aBaseChar);
>+}
>+
>+const DeadKeyTable* VirtualKey::MatchingDeadKeyTable (const DeadKeyEntry* aDeadKeyArray, PRUint32 aEntries) const
>+{
>+  if (!mIsDeadKey)
>+    return nsnull;
>+
>+  for (PRUint32 shiftState = 0; shiftState < 16; shiftState++)

Could this 16 be changed to NS_ARRAY_LENGTH(mShiftStates) ?

>+KeyboardLayout::KeyboardLayout ()
>+{
>+  static PRBool virtualKeyCodeTableInitialized = PR_FALSE;
>+  
>+  if (!virtualKeyCodeTableInitialized)
>+  {
>+    for (PRUint32 virtualKey = 0; virtualKey < 256; virtualKey++)
>+    {
>+      PRInt32 vki = GetKeyIndex (virtualKey);
>+      
>+      if (vki >= 0)

Assert that vki < NUM_OF_KEYS

Is there some reason we can't just make gVirtualKeyCodes |const| and initialize it with known values?  It seems like we're doing some predictable work here that the compiler could do instead.

>+PRInt32 KeyboardLayout::GetKeyIndex (PRUint8 aVirtualKey)
>+{
>+  PRInt32 ndx = -1;
>+
>+  if (aVirtualKey >= '0' && aVirtualKey <= '9')                   // 10
>+    ndx = aVirtualKey - '0';
>+  else if (aVirtualKey >= 'A' && aVirtualKey <= 'Z')              // 26
>+    ndx = aVirtualKey - 'A' + 10;
>+  else if (aVirtualKey == VK_SPACE)                               //  1
>+    ndx = 10 + 26;
>+  else if (aVirtualKey >= VK_OEM_1 && aVirtualKey <= VK_OEM_3)    //  7
>+    ndx = aVirtualKey - VK_OEM_1 + 10 + 26 + 1;
>+  else if (aVirtualKey >= VK_OEM_4 && aVirtualKey <= VK_OEM_8)    //  5
>+    ndx = aVirtualKey - VK_OEM_4 + 10 + 26 + 1 + (VK_OEM_3 - VK_OEM_1 + 1);

Ok, it kind of sucks that we need this mapping, but the space savings is non-negligible.  I'd suggest at least using enum values to denote the beginning of each range in our array space.  It would make things more readable and less error-prone than hardcoding the index computation.

>+void KeyboardLayout::OnKeyDown (PRUint8 aVirtualKey)
>+{
>+  mLastVirtualKeyIndex = GetKeyIndex (aVirtualKey);
>+
>+  if (mLastVirtualKeyIndex < 0)
>+  {
>+    // Does not produce any printable characters, but still preserves the dead-key state.
>+    mNumOfChars = 0;
>+  } else
>+  {

I should have mentioned this up higher.  Please following the prevailing style for braces and indentation, see http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual .  I think the Windows widget module is fairly consistent on this.

>+    PRUint8 kbdState [256];

I'll resume reviewing here later.

Comment 53

[Brian Ryner (not reading)]

Comment on attachment 206856 [details] [diff] [review]
New code in separate files + works with Win9x

>+    PRUint8 kbdState [256];
>+
>+    if (::GetKeyboardState ((PBYTE)kbdState))

Keep types consistent.  If kbdState is a win32 native type (BYTE array), then go ahead and use that.

>+void KeyboardLayout::LoadLayout ()
>+{

>+        // Repeat dead-key to deactivate it and get it's character representation.

s/it's/its/

>+  // Now process each dead-key to find all it's base characters and resulting composite characters.

Same.

>+PRUint32 KeyboardLayout::GetDeadKeyCombinations (PRUint8 aDeadKey, const PRUint8* aDeadKeyKbdState,
>+                                                 PRUint16 aShiftStatesWithBaseChars,
>+                                                 DeadKeyEntry* aDeadKeyArray, PRUint32 aMaxEntries)
>+{
>+        // Ensure dead-key is in active state, when it swollows entered character and waits for next pressed key.

"swallows", and maybe "waits for the next pressed key."

>+        // Depending from character that followed dead-key the keyboard driver can produce one composite character
>+        // or dead-key character followed by second character.

I'd rephrase a bit:

"Depending on the character the followed the dead-key, the keyboard driver can produce one composite character, or a dead-key character followed by a second character."

>+PRBool LangIDToCP (WORD aLangID, UINT* aCodePage)

You should make the copy of this function in nsWindow.cpp reusable, rather than copying it.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsKeyboardLayout.h	26 Dec 2005 15:45:12 -0000
>+class KeyboardLayout
>+{
>+  struct DeadKeyTableListEntry
>+  {
>+    DeadKeyTableListEntry* Next;
>+    PRUint8 Data [1];

Change to "next" and "data" (convention is interCaps).

>+  };
>+
>+  enum { NUM_OF_KEYS = 49 };

This would make more sense as a "const int", I think, or if that doesn't work out, a #define.

>--- nsWindow.cpp	23 Dec 2005 20:23:45 -0000	3.601
>+++ nsWindow.cpp	26 Dec 2005 15:45:24 -0000

>+static PRBool dummy = nsToolkit::InitVersionInfo();

I'd rather not call this this way, running methods during static initialization can lead to hard-to-find problems.  It probably won't in this case, but why does this need to run so early?

Looks pretty good otherwise.  I'd like to see a new patch with these comments addressed.  Thanks!

Comment 54

[Dainis Jonitis]

Attachment: review notes addressed (without bracing changes - to allow interdiff)

Comment 55

[Dainis Jonitis]

> Make this |static|.

It already was declared as static in nsKeyboardLayout.h. But now this array is gone anyway.

> "It is the only way to create new DeadKeyTable instances."
done

> Mozilla convention is to use offsetof().
done

> NS_DEBUG is deprecated, please use #ifdef DEBUG instead.
done

>Add something like:
>
>NS_ASSERTION(aShiftState < NS_ARRAY_LENGTH(mShiftStates), "invalid index");
>
>here and also to other functions that take an index into the mShiftStates
>array... just to make debugging easier if this becomes a problem.

I have not put assertions in every function that receives the aShiftState. I've put the assertion in main entrypoints KeyboardLayout::SetShiftState(), VirtualKey::SetNormalChars() and VirtualKey::SetDeadChar(). If something will be wrong these will assert first before calling other functions.

>>+  for (PRUint32 shiftState = 0; shiftState < 16; shiftState++)
>
>Could this 16 be changed to NS_ARRAY_LENGTH(mShiftStates) ?

I prefer to leave it 16 here. It should symbolize the bitmask (2^4). If mShiftStates will have less than 16 entries then assertion will fire.

>>+KeyboardLayout::KeyboardLayout ()
>>+{
>>+  static PRBool virtualKeyCodeTableInitialized = PR_FALSE;
>>+  
>>+  if (!virtualKeyCodeTableInitialized)
>>+  {
>>+    for (PRUint32 virtualKey = 0; virtualKey < 256; virtualKey++)
>>+    {
>>+      PRInt32 vki = GetKeyIndex (virtualKey);
>>+      
>>+      if (vki >= 0)
>
>Assert that vki < NUM_OF_KEYS

This code is now gone. But If you want I can put similar assertion in KeyboardLayout::LoadLayout().

>Is there some reason we can't just make gVirtualKeyCodes |const| and initialize
>it with known values?  It seems like we're doing some predictable work here
>that the compiler could do instead.

This array is not used anymore. The translation of mActiveDeadKeyIndex back to virtual key was the only user of this array. Now I use mActiveDeadKey (virtual key) and when index is required I call GetKeyIndex(mActiveDeadKey). Now GetKeyIndex() is inlined and very very cheap (see below).


>+PRInt32 KeyboardLayout::GetKeyIndex (PRUint8 aVirtualKey)
>+{
>+  PRInt32 ndx = -1;
>+
>+  if (aVirtualKey >= '0' && aVirtualKey <= '9')                   // 10
>+    ndx = aVirtualKey - '0';
>+  else if (aVirtualKey >= 'A' && aVirtualKey <= 'Z')              // 26
>+    ndx = aVirtualKey - 'A' + 10;
>+  else if (aVirtualKey == VK_SPACE)                               //  1
>+    ndx = 10 + 26;
>+  else if (aVirtualKey >= VK_OEM_1 && aVirtualKey <= VK_OEM_3)    //  7
>+    ndx = aVirtualKey - VK_OEM_1 + 10 + 26 + 1;
>+  else if (aVirtualKey >= VK_OEM_4 && aVirtualKey <= VK_OEM_8)    //  5
>+    ndx = aVirtualKey - VK_OEM_4 + 10 + 26 + 1 + (VK_OEM_3 - VK_OEM_1 + 1);

>Ok, it kind of sucks that we need this mapping, but the space savings is
>non-negligible.  I'd suggest at least using enum values to denote the beginning
>of each range in our array space.  It would make things more readable and less
>error-prone than hardcoding the index computation.

I've replaced this code with static constant lookup table with 256 entries. It is even more readable than I thought it will be :). And it allows to the function be inlined and very very fast thatmight be important in tight loops in LoadLayout(). 
  One option was to throw away the first 32 bytes and last 32 bytes of table and use:
return (aVirtualKey >= 0x20 && aVirtualKey < 0xE0) ? xlat [aVirtualKey - 0x20] : -1;

But code amount for range check is comparable with those 64 bytes we can save. I think 256 bytes of data is good tradeof of those speed improvements. And instaead of calling function it is replaced with direct memory access in array (2 machine operations).


>I should have mentioned this up higher.  Please following the prevailing style
>for braces and indentation, see
>http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual .  I think the
>Windows widget module is fairly consistent on this.

Do you mean opening brace on same line as if, for, while? I will post the patch in a moment. I attached this patch to make it easier to compare with previous version.


>+    PRUint8 kbdState [256];
>+
>+    if (::GetKeyboardState ((PBYTE)kbdState))

>Keep types consistent.  If kbdState is a win32 native type (BYTE array), then
>go ahead and use that.

Well, that required to include <windows.h> from nsKeyboardLayout.h. And we still need to use (PBYTE) cast in KeyboardLayout::EnsureDeadKeyActive(), because the old PlatformSDK from Visual Studio 6.0 has ::ToUnicode declared as receiving PBYTE param, instead of more correct "const PBYTE" as in newer Platform SDK's.
done.

>>+        // Repeat dead-key to deactivate it and get it's character >representation.
>
>s/it's/its/
done

>>+  // Now process each dead-key to find all it's base characters and resulting >composite characters.
>
>Same.
done

> "swallows", and maybe "waits for the next pressed key."
done

>I'd rephrase a bit:
done

>>+PRBool LangIDToCP (WORD aLangID, UINT* aCodePage)
>
>You should make the copy of this function in nsWindow.cpp reusable, rather than
>copying it.

As I wrote in Comment #49 I copied code intentionaly to reduce the size of this patch. If this patch will be checked in and there won't be regressions I will open followup bug to move more functionality in KeyboardLayout class from nsWindow. Then there won't be even need to make it publicly visible.
Is it ok to do it in separate bug?

> Change to "next" and "data" (convention is interCaps).
done

>+  };
>+
>+  enum { NUM_OF_KEYS = 49 };
>
> This would make more sense as a "const int", I think, or if that doesn't work
> out, a #define.

Microsoft Visual C++ 6.0 and later versions have different rules how to init constnt class members. That's why I used enum.
Replaced with #define.


>+static PRBool dummy = nsToolkit::InitVersionInfo();
>
>I'd rather not call this this way, running methods during static initialization
>can lead to hard-to-find problems.  It probably won't in this case, but why
>does this need to run so early?

As I said in Comment #49 I tried to minimize changes to code parts that are not related to specific problem, because the patch was alredy too big. I was afraid to change link order to make sure nsToolkit.cpp global variables are initialized before nsWindow.cpp global variables.
If you don't mind I will think about best way how to fix in the same followup bug I talked above. I even think that there should be inline read only functions about all these version flags. 
Is it ok to leave it as is for now?


Will attach other patch with corrected bracing style within an hour.

Comment 56

[Brian Ryner (not reading)]

Comment on attachment 207541 [details] [diff] [review]
review notes addressed (without bracing changes - to allow interdiff)

Looks good, thanks.  I'm ok with doing the code consolidation in a follow-up bug.

Comment 57

[Dainis Jonitis]

Attachment: review notes addressed (with bracing changes - for checkin)

Same as above patch that got superreview, but with requested bracing changes.
It also has one extra NS_ASSERTION that was hinted by review notes to catch situation when someone added new key in xlat array without increasing NUM_OF_KEYS.

Ere, can you please check this in.

P.S.
Before moving code to separate nsKeyboardLayout.* files it was compiling with MS VC++ 6.0 and MinGW. About this patch I can say that it compiles fine with MS VC++ 2003.

Comment 58

[Ere Maijala (slow)]

Comment on attachment 207541 [details] [diff] [review]
review notes addressed (without bracing changes - to allow interdiff)

I dislike the bracing style :) But I try to live with it. I had to find one thing for you to fix still:

+  PRUint16 shiftStatesWithDeadKeys = 0;     // Bitfield with all shift states that has at least one dead-key.

has -> have

r=emaijala

Comment 59

[Dainis Jonitis]

Yeah, I hate this bracing style, too. But that's the price to pay to get superreview :)

Will you change that has->have before checking in or you want me to prepare new patch?

P.S.
Do you know how this will affect the WINCE? Should Doug Terner look at this too?

Comment 60

[Ere Maijala (slow)]

Sure I can do it. I'll check it in later when I have time to watch the tree. For the record, my preferred preferred bracing is:

if ()
{
  ..
}
else
{
  ..
}

I have to cope with other styles too, of course. 

Doug, what do you think? Do we make a mess of WinCE with this patch?

Comment 61

[Doug Turner (:dougt)]

it will - most of the APIs used in KeyboardLayout are not available in WinCE. (in fact, many ce devices do not have a control key)  

thanks for the heads up.  Can you put a #ifndef WINCE around gKbdLayout usage in nsWindows.cpp and in the Makefile.in, move nsKeyboardLayout.cpp to the non-wince section (directly under nsBidiKeyboard.cpp would work find).

Comment 62

[Dainis Jonitis]

Attachment: Make sure compiles on WinCE

Additional #ifndef WINCE to compile on Windows CE. Currently all public functions of KeyboardLayout are just stubs. As I understand if key is not pressed togethe with Alt or Ctrl this code path will never be called on WinCE. If Alt/Ctrl handling will be required we might need to implement some lightweight version of OnKeyDown and GetUnichars.


Do I need new superreview for this?

Comment 63

[Doug Turner (:dougt)]

Comment on attachment 207749 [details] [diff] [review]
Make sure compiles on WinCE

could you post a patch that just contains your changes for WINCE?

Comment 64

[Dainis Jonitis]

Doug, you can look at an interdiff with "review notes addressed (without bracing changes - to allow interdiff)". I moved some code blocks around to reduce the number of #ifdefs. Now all public functions come first, then private ones in #ifndef WINCE block.

Actually you can look at the nsKeyboardLayout.h and nsKeyboardLayout.cpp and search for WINCE. The nsWindow is not affected by WINCE changes. I verified that when I define WINCE for nsKeyboardLayout.cpp the widget code compiles and links fine.

Comment 65

[Doug Turner (:dougt)]

looks good.  I can follow up with any bustage, if required.

Comment 66

[Ere Maijala (slow)]

Patch checked in to trunk.

Comment 67

[David G King]

Getting the following error on my Win32/MinGW/cygwin build that seems to be associated with this bug :-

Building deps for /cygdrive/e/mozilla/source/mozilla/widget/src/windows/nsKeyboa
rdLayout.cpp
/cygdrive/e/mozilla/source/mozilla/build/cygwin-wrapper g++ -mtune=athlon-xp -ma
rch=athlon-xp -mmmx -msse -m3dnow -mno-cygwin -o nsKeyboardLayout.o -c  -D_IMPL_
NS_WIDGET -DMOZ_UNICODE  -DMOZILLA_INTERNAL_API -DOSTYPE=\"WINNT5.1\" -DOSARCH=\
"WINNT\" -DBUILD_ID=0000000000 -I. -I/cygdrive/e/mozilla/source/mozilla/widget/s
rc/windows/../xpwidgets -I/cygdrive/e/mozilla/source/mozilla/widget/src/windows
-I../../../dist/include/xpcom -I../../../dist/include/string -I../../../dist/inc
lude/dom -I../../../dist/include/accessibility -I../../../dist/include/content -
I../../../dist/include/content_xul -I../../../dist/include/gfx -I../../../dist/i
nclude/gfxwin -I../../../dist/include/necko -I../../../dist/include/pref -I../..
/../dist/include/plugin -I../../../dist/include/timer -I../../../dist/include/uc
onv -I../../../dist/include/intl -I../../../dist/include/locale -I../../../dist/
include/webshell -I../../../dist/include/docshell -I../../../dist/include/layout
 -I../../../dist/include/xuldoc -I../../../dist/include/view -I../../../dist/inc
lude/imglib2 -I../../../dist/include/widget -I../../../dist/include -I../../../d
ist/include/nspr    -I../../../dist/sdk/include       -fno-rtti -fno-exceptions
-Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wn
o-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -mms-bitfield
s -pipe  -DDEBUG -D_DEBUG -DDEBUG_David -DTRACING -g -fexceptions   -DX_DISPLAY_
MISSING=1 -DMOZILLA_VERSION=\"1.9a1\" -DMOZILLA_VERSION_U=1.9a1 -DHAVE_SNPRINTF=
1 -D_WINDOWS=1 -D_WIN32=1 -DWIN32=1 -DXP_WIN=1 -DXP_WIN32=1 -DHW_THREADS=1 -DWIN
VER=0x400 -D_WIN32_WINNT=0x400 -DSTDC_HEADERS=1 -DWIN32_LEAN_AND_MEAN=1 -DNO_X11
=1 -D_X86_=1 -DD_INO=d_ino -DSTDC_HEADERS=1 -Duid_t=int -Dgid_t=int -DHAVE_DIREN
T_H=1 -DHAVE_GETOPT_H=1 -DHAVE_MEMORY_H=1 -DHAVE_UNISTD_H=1 -DHAVE_MALLOC_H=1 -D
HAVE_LIBM=1 -DNO_X11=1 -DMMAP_MISSES_WRITES=1 -DHAVE_STRERROR=1 -DHAVE_SNPRINTF=
1 -DHAVE_MEMMOVE=1 -DHAVE_RINT=1 -DVA_COPY=va_copy -DHAVE_VA_COPY=1 -DMOZ_DEFAUL
T_TOOLKIT=\"windows\" -DMOZ_SUITE=1 -DMOZ_BUILD_APP=suite -DMOZ_DISTRIBUTION_ID=
\"org.mozilla\" -DOJI=1 -DIBMBIDI=1 -DMOZ_VIEW_SOURCE=1 -DMOZ_XPINSTALL=1 -DMOZ_
JSLOADER=1 -DNS_PRINTING=1 -DNS_PRINT_PREVIEW=1 -DMOZ_XTF=1 -DMOZ_MATHML=1 -DMOZ
_UPDATE_CHANNEL=default -DMOZ_LOGGING=1 -DDETECT_WEBSHELL_LEAKS=1 -DHAVE___CXA_D
EMANGLE=1 -DMOZ_DEMANGLE_SYMBOLS=1 -DMOZ_USER_DIR=\"Mozilla\" -DMOZ_XUL=1 -DMOZ_
PROFILELOCKING=1 -DMOZ_DLL_SUFFIX=\".dll\" -DJS_THREADSAFE=1 -DMOZ_REFLOW_PERF=1
 -DMOZ_REFLOW_PERF_DSP=1 -DMOZILLA_LOCALE_VERSION=\"1.9a1\" -DMOZILLA_REGION_VER
SION=\"1.9a1\" -DMOZILLA_SKIN_VERSION=\"1.8\"  -D_MOZILLA_CONFIG_H_ -DMOZILLA_CL
IENT /cygdrive/e/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp: In static mem
ber function `static PRUint32 DeadKeyTable::SizeInBytes(PRUint32)':
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:71: warning: i
nvalid access to non-static data member `DeadKeyTable::mTable' of NULL object
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:71: warning: (
perhaps the `offsetof' macro was used incorrectly)
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp: In member fun
ction `void KeyboardLayout::LoadLayout()':
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:342: warning:
cast from pointer to integer of different size
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:361: warning:
comparison between signed and unsigned integer expressions
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:384: error: in
valid conversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:394: error: in
valid conversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp: In member fun
ction `PRUint32 KeyboardLayout::GetDeadKeyCombinations(PRUint8, BYTE*, PRUint16,
 DeadKeyEntry*, PRUint32)':
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:657: error: in
valid conversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:685: error: in
valid conversion from `PRUint16*' to `WCHAR*'
e:/mozilla/source/mozilla/widget/src/windows/nsKeyboardLayout.cpp:755:7: warning
: no newline at end of file
make[6]: *** [nsKeyboardLayout.o] Error 1

Comment 68

[Dainis Jonitis]

Attachment: Try to fix MinGW

David, please test this additional patch that adds some explicit type casts. I am not sure about the last line of patch that contains:
-#endif
\ No newline at end of file
+#endif

If that causes any problem just add new line at the end of file in any text editor. At the moment I do not know how to fix the warnings caused by offsetof() in static function. Can you try to replace 'offsetof' with 'FIELD_OFFSET' and see if it generates the same warning. If not then please send your definition of 'offsetof' macro from stddef.h (?)

Comment 69

[David G King]

That patch deals with the build problems.

As for the offset warnings, I see them all over the place, and haven't really had time to investigate them. In my opinion, don't worry about them.

Thanks for the quick patch.

Comment 70

[Dave Townsend [:mossop]]

For info, roughly the same errors as with MinGW come out with VC8, but that last patch solves it.

Comment 71

[Ere Maijala (slow)]

Bustage fix committed. 

Comment 72

[[e-mail]]

Technically offsetof is illegal in C++ except on C data structures.
But most compilers are clever enough to figure out what you mean ;-)

Comment 73

[Dainis Jonitis]

Jacek,

Thanks for all info. Can you possible check whether patche 207541 still works? If not then please also try 206856

Comment 74

[Peter van der Woude [:Peter6]]

this probably caused Bug 322813

Comment 75

[Jacek Piskozub]

Patch 207541 does not work (result: hang).

Comment 76

[Jacek Piskozub]

Neither does patch 206856 (result: identical hang)

Comment 77

[Eyal Rozenberg]

The fix for this bug may have caused the regression bug 323212. Please look into it.

Comment 78

[Eyal Rozenberg]

(In reply to bug 323212, comment 6)
I see. Well, there are two options. Either Ctrl+ shortcuts should be handled by getting the physical keys pressed rather than the virtual key, or alternatively each shortcut must be duplicated according to the keyboard layouts in use (e.g. for Hebrew all of Ctrl+v, Ctrl+V, Ctrl+ה should trigger paste). 

Knowing the way things are done around here, though, I wouldn't expect an ETA on that before 2010... however, once this behavior hits release builds, numerous users will dump ff/sm/tb  for this reason alone. So until this is sorted out, I suggest that patch for this bug be backed out.

(Of course, I'm not very objective about this matter.)

Comment 79

[Ere Maijala (slow)]

We're going to back out the patch. And no, not as a reaction to comment #78.

Comment 80

[Dainis Jonitis]

Attachment: partial backout patch (leave early Windows detection in nsToolkit)

This removes nsKeyboardLayout.cpp from makefile.in and removes use of KeyboardLayout class from nsWindow.cpp. It preserves the changes in nsToolkit and nsWindow that were related to nsToolkit::InitVersionInfo.

Comment 81

[Ere Maijala (slow)]

Comment on attachment 208402 [details] [diff] [review]
partial backout patch (leave early Windows detection in nsToolkit)

Backed out.

Comment 82

[Serge Baranov]

(In reply to comment #81)
> (From update of attachment 208402 [details] [diff] [review] [edit])
> Backed out. 
> 

It has fixed bug 323212.

Comment 83

[Dainis Jonitis]

Attachment: Fix hang and shortcut key regressions

1. This patch should fix hang that some people were experiencing on Win9x. It was due to uninitialized member variable mDeadKeyTableListHead in KeyboardLayout constructor. Not Win9x specific.
2. It should fix bug 322813. KeyboardLayout functions should use real virtual key, not one that is mapped to DOM key code equivalent.
3. Additionaly handle one more key VK_DECIMAL.
4. It takes more conservative approach when to report real keyboard layout encoded letters and when latin characters A..Z. Compared with initial patch that caused problems for Russian keyboard layout users because all Alt+key and Ctrl+key shortcut keys were not accessible if they were using English version of Mozilla. Now for VK_A..VK_Z if they are pressed together either with Alt or Ctrl (but not both) we report the latin characters A..Z. If Alt+Ctrl+key (AltGr+key) is pressed we read real international character from keyboard layout. That should make users of Polish, Czech and some other European keyboard layouts happy. Also for VK_0..VK_9 we report real characters from keyboard layout. That should not be a big problem, because there are no many Alt+number or Ctrl+number shortcuts. It should allow to read real character when Shift is pressed in combination with Alt or Ctrl.

Comment 84

[Dainis Jonitis]

Attachment: Fix hang and shortcut key regressions II

All comments of previous patch are valid, except that for VK_0..VK_9 if pressed together with Alt or Ctrl (but not both), we also ignore the keyboard layout returned characters and return what EN-US keyboard layout does. This is required, because there are Ctrl+number shortcuts in both Firefox (select tab) and Seamonkey (open Navigator, Mail etc.). And at least Czech keyboard layout requires Shift pressed to get Number. As result Ctrl+number shortcuts do not work as they do not expect Shift pressed.

Comment 85

[Ere Maijala (slow)]

Fix checked in to trunk.

Comment 86

[Peter van der Woude [:Peter6]]

Could this have broken FF installer builds on Win98 ?

Error Starting Program
The FIREFOX.EXE file is linked to missing export SHELL32.DLL:SHGetFolderLocation

Comment 87

[Ere Maijala (slow)]

No, but the patch for bug 267526 did and was backed out.

Comment 88

[u52928]

(In reply to comment #87)
> No, but the patch for bug 267526 did and was backed out.
> 
I'm not exactly sure which patch is causing the problem, but it's still not possible to use either the trunk zip or installer builds on Win98. 

The problem looks exactly like what happened during the "dead key" incident we had about two weeks ago: the computer freezes and this can only be undone by killing FF using the "Close Program" dialogue.

PLEASE reopen this bug; the issue is not resolved, at least on Win98.

Comment 89

[u52928]

(In reply to comment #87)
> No, but the patch for bug 267526 did and was backed out.
> 
I'm not exactly sure which patch is causing the problem, but it's still not possible to use either the trunk zip or installer builds on Win98. 

The problem looks exactly like what happened during the "dead key" incident we had about two weeks ago: the computer freezes and this can only be undone by killing FF using the "Close Program" dialogue.

PLEASE reopen this bug; the issue is not resolved, at least on Win98.

Comment 90

[u52928]

This message appeared on the Forum for the trunk build today (06 Feb):

xavier114fch wrote:
Any people here using Chinese IME? I found a very interesting bug from todays's build. 
I use English version of Windows XP, and have the IME "Chinese (Traditional) - Quick" installed on the system. When I enter Chinese characters in the input boxes (whatever it is an input field, or textarea etc.) and press escape to close the character selection box, the same character will display two times. It's quite annoying as the escape key is often use to dismiss the selection box. Anyone could confirm? 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060205 Firefox/1.6a1 ID:2006020505
------------
Perhaps it's related.

Comment 91

[Ere Maijala (slow)]

Reopening. The SHGetFolderLocation was unrelated, but the others were apparently caused by this. I'm going to back out the patch unless Dainis has a solution in his hand already.

Comment 92

[u52928]

(In reply to comment #91)
> Reopening. The SHGetFolderLocation was unrelated, but the others were
> apparently caused by this. I'm going to back out the patch unless Dainis has > a solution in his hand already.
> 
Thank you. This is obviously a pretty complicated issue. I'm sorry I don't have the knowledge to help work on a solution.  

I will, though, be happy to do any testing as required.

Comment 93

[Ere Maijala (slow)]

Patch backed out.

Comment 94

[Ere Maijala (slow)]

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

Comment 95

[Dainis Jonitis]

Attachment: Standalone Win32 test application (ZIP)

I believe I found the problem why the previous patch was still not working for some Win98SE users. That's because of bug in Win98SE (not sure about other versions of Win9x) ::ToAsciiEx function which on Win98SE returns array of 2 WORD entries, but on Win2k and newer the array of 2 BYTE entries. As result it was corrupting the stack.

Please run the attached Win32 application which is compiled with VS8 debug runtime heap and stack checks. It works fine for my VirtualPC Win98SE.

I am especially interested to hearing back from Joe Greenman :)

Comment 96

[Jacek Piskozub]

Windows ME (no change from previous messages):

LoadLayout
HKL = F0070415h, Code page = 1250
BlockInput ok
VK C0 (1) is dead-key (char=007E)
VK C0 (9) is dead-key (char=007E)
Process dead-keys

VK C0 (1). New dead-key table with 19 entries
 [00] base=0020, combined=007E
 [01] base=0041, combined=0104
 [02] base=0043, combined=0106
 [03] base=0045, combined=0118
 [04] base=004C, combined=0141
 [05] base=004E, combined=0143
 [06] base=004F, combined=00D3
 [07] base=0053, combined=015A
 [08] base=0058, combined=0179
 [09] base=005A, combined=017B
 [10] base=0061, combined=0105
 [11] base=0063, combined=0107
 [12] base=0065, combined=0119
 [13] base=006C, combined=0142
 [14] base=006E, combined=0144
 [15] base=006F, combined=00F3
 [16] base=0073, combined=015B
 [17] base=0078, combined=017A
 [18] base=007A, combined=017C

VK C0 (9). Existing dead-key table with 19 entries
 [00] base=0020, combined=007E
 [01] base=0041, combined=0104
 [02] base=0043, combined=0106
 [03] base=0045, combined=0118
 [04] base=004C, combined=0141
 [05] base=004E, combined=0143
 [06] base=004F, combined=00D3
 [07] base=0053, combined=015A
 [08] base=0058, combined=0179
 [09] base=005A, combined=017B
 [10] base=0061, combined=0105
 [11] base=0063, combined=0107
 [12] base=0065, combined=0119
 [13] base=006C, combined=0142
 [14] base=006E, combined=0144
 [15] base=006F, combined=00F3
 [16] base=0073, combined=015B
 [17] base=0078, combined=017A
 [18] base=007A, combined=017C
Un-BlockInput ok

Comment 97

[Martijn Wargers (dead)]

I was also the last month or getting random lock ups/freezing of Firefox, until recently at the same time the patch of this bug got backed out, see also:
http://forums.mozillazine.org/viewtopic.php?t=379570

I'm using WindowsXP.
This is my output from the Standalone Win32 test application:
http://wargers.org/mozilla/w2m_output.txt

Comment 98

[Hermann Schwab]

Attachment: output from standalone test application on Win98 german version

Comment 99

[WADA:World Anti-bad-Duping Agency]

Bug 327342 for Tb 1.5 looks for me to have relation to this bug, but opener says no problem when Tb 1.0.x, then I'm not uncertain whether this bug has relation or not.
Dainis Jonitis, can Bug 287179 be a cause of Bug 327342 on Tb 1.5?

Comment 100

[Dainis Jonitis]

Attachment: Workaround Win98 ToAsciiEx bug

On Win98SE (and possibly other Win9x versions) the API function ::ToAsciiEx fourth parameter (LPWORD lpChar) returns array with 2 WORD entries in case for dead-key combination. On Win2k and newer the function returns 2 BYTE array that fits into one WORD variable we were pointing to. That should be the right behaviour as returned character codes are guaranteed to be in range of 0..255 (using code pages). To make sure that we do not corrupt the stack, always provide the array with at least two WORD variables.

This should fix the hang problem, although I still have not received the confirmation from Joe Greenman. I am not sure whether the Chinese IME problem was caused by previous patch. I tried to install Chinese simplified IME and see wheteher it causes any problems. Well, I have not noticed anything bad, but on the other hand I have no idea how IME should work and what I've been actually typing, as Chinese is not my strongest point :)
Is there anyone with Chinese/Japan language skills that can help me to verify that attached patch is not causing regressions to IME input. PLEASE!

Comment 101

[Dainis Jonitis]

(In reply to comment #90 about effect on Chinese IME)
It is not related. I tried todays trunk Firefox.
1. Selected "Chinese (Traditional) - Quick" keyboard layout.
2. Pressed e+e+4. As result some hieroglyph appeared.
3. When pressed Esc the IME window was closed and hieroglyph duplicated.
Thus my previous patch was not related to this regression (compared to 1.5.0.1).

Comment 102

[WADA:World Anti-bad-Duping Agency]

CCing to Nakano san.
Nakano san, do you think Dainis's patch will cause some problems on Japanese IME? 

Comment 103

[Dainis Jonitis]

Comment on attachment 212618 [details] [diff] [review]
Workaround Win98 ToAsciiEx bug

No any feedback since last week. Then I guess the only way to verify that code works or not is to check it in trunk.

Comment 104

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #103)
> (From update of attachment 212618 [details] [diff] [review] [edit])
> I guess the only way to verify that code works or not is to check it in trunk.
Is asking test to bug opener of Bug 327342 has meaning?
(We Japanese can do test with Japanese IME, although normal use case only and with limited IMEs.)

Comment 105

[u52928]

(In reply to comment #100)
> This should fix the hang problem, although I still have not received the
> confirmation from Joe Greenman. 

Please excuse me for having taken so long to reply. I'm in the middle of teaching an intensive English course, and it's keeping me pretty busy :-(

Anyway, I've run the test file that Dainis was kind enough to send me. The short answer is that my machine didn't hang at all. When I ran it from Windows, I just saw the DOS window open for a split second, some lines of text zipped down the page, and that was it. My machine didn't hang - or explode - so I guess it's OK. 

Dainis said I should try to run it from the command line, which I did, but I got the message: "This program cannot be run in DOS mode."

In regard to running it from Windows, Dainis said:
"Here are instructions for WinXP. Win98 was similar, but I am not sure that I recall all the details.

1. Open up your command prompt by presing Win+R (Or Start button + "Run..."). In the "Open.." prompt enter "cmd.exe"
2. Go to directory where you unzipped the w2m.exe (I would suggest the "c:\")
3. type: w2m > out.txt"

I tried this, but it doesn't do anything. Dainis is really working **** this, and I'd really like to help. Any ideas?

Comment 106

[[e-mail]]

(In reply to comment #95)
>I believe I found the problem why the previous patch was still not working for
>some Win98SE users. That's because of bug in Win98SE (not sure about other
>versions of Win9x) ::ToAsciiEx function which on Win98SE returns array of 2
>WORD entries, but on Win2k and newer the array of 2 BYTE entries.
According to my documentation (old WinHelp SDK) it points to a DWORD.

Comment 107

[[e-mail]]

Comment on attachment 212618 [details] [diff] [review]
Workaround Win98 ToAsciiEx bug

>-      WORD ascii;
>+      WORD ascii [2];     // Win98SE ToAsciiEx bug: requires two WORD array instead of two BYTE array.
Please rewrite all of these as something like
// ToAsciiEx documentation regression: lpChar points to a doubleword buffer.

>     PRUint16 dummyChars [5];
Should be WORD for consistency.

>       rv = ::ToUnicode (aDeadKey, 0, (PBYTE)aDeadKeyKbdState, (LPWSTR)dummyChars, NS_ARRAY_LENGTH (dummyChars), 0);
Is the (LPWSTR) cast necessary?

Comment 108

[Dainis Jonitis]

> Please rewrite all of these as something like
> // ToAsciiEx documentation regression: lpChar points to a doubleword buffer.

It is not a documentation regression, but change in behaviour starting from Win2k (probably older NT series, too). The Win2k behaviour that is documented in latest Platform SDK is most likely more correct than old Win9x. It is sufficient to have 1 WORD variable to return two byte characters (for dead-key + character that does not pair with dead-key character). And as this is ASCII version that returns character code within active code page then it always fits in one byte. Thus 2 chars = 1 WORD. On Win2k it does _NOT_ require doubleword buffer.

As majority of developers will look in latest documentation, I would assume that existing comment better reflects fact that function behaviour under Win9x does not match the documentation and could be interpreted as an implementation bug.

See also this:
http://www.chiark.greenend.org.uk/~sgtatham/putty/wishlist/win-dead-keys.html


> Is the (LPWSTR) cast necessary?
LPWSTR cast is required for MinGW.

>>     PRUint16 dummyChars [5];
> Should be WORD for consistency.
Can I do it in followup bug? There are many type mismatch warnings when compiling with VS8 (but not with 7.1) and probably MinGW. I would prefer to fix them in one shot. I have also other small things that I should clean-up (like removing of LangIDToCP() from nsWindow, moving all keyboard layout related things to nsKeyboardLayout, cleaner way to ensure that static function for Windows version determination are run before first use no matter what module link order is)

Neil, if you do not object, can you check it in as it is now? Thanks

Comment 109

[[e-mail]]

The point is that your patch didn't work in Windows 9x because you weren't using Windows 9x documentation. That doesn't make it a Windows 9x bug.

Comment 110

[Dainis Jonitis]

Attachment: Same as last patch, but with fixed comment

Comment 111

[Dainis Jonitis]

Patch 215299 checked in.

Comment 112

[Dainis Jonitis]

As bug 331752 removed Win9x support for trunk, it would be nice to have fix for both 287179 and 331752 on 1.8 branch

Comment 113

[Martijn Wargers (dead)]

I did get a lock-up/freezing up of Firefox again with 2006-04-04 trunk build.
I'm on windowsXP. I kind of suspect that this patch has caused this again.

Comment 114

[Marek Stępień [:marcoos, inactive]]

Can this be checked into 1.8 as well? It fixes bug 319308 which makes Caps Lock completely unusable for Polish and barely usable for French users and probably others...

CC'ing Pike.

Comment 115

[Hubert Gajewski]

blocking1.8.1=?

This is a serious bug for many users. On Polish Mozilla Forum we have many duplicate posts about this problem:

http://mozillapl.org/forum/about-19440.html
http://mozillapl.org/forum/about-19461.html
http://mozillapl.org/forum/about-19998.html
http://mozillapl.org/forum/about-20227.html
http://mozillapl.org/forum/about-20689.html
http://mozillapl.org/forum/about-21943.html
http://mozillapl.org/forum/about-23286.html

Comment 116

[Mike Schroepfer]

Please nominated appropriate patch(es) for the 1.8.1 branch

Comment 117

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd also note that marking checked in patches as obsolete makes it very hard for drivers to understand in a triage meeting which patches might be necessary to get the bug fixed on a branch.

Note also that we may reconsider the blocking1.8.1+ status based on the risk of the patches -- it looks like they may be quite risky.

Comment 118

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

I'm fighting with the regression of this bug in bug 339723 :-(

Comment 119

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

(In reply to comment #118)
> I'm fighting with the regression of this bug in bug 339723 :-(

And the patch of bug 339723 is only for WinNT. That breaks Win9x support.

Comment 120

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #117)

> Note also that we may reconsider the blocking1.8.1+ status based on the risk of
> the patches -- it looks like they may be quite risky.


When you will be reconsidering this patch, please not forget about the impact of this bug on localizated releases. We get a lot of complains about this bug. And keeping it for Firefox 2.0 means that Caps-lock is useless for majority of the world in Firefox. I'd call it major bug...

Comment 121

[Darin Fisher]

nakano-san: given that the patch for bug 339723 is WinNT only, what would it take to get a fix suitable for Win9x as well that could be landed for FF2?

Comment 122

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

(In reply to comment #121)
> nakano-san: given that the patch for bug 339723 is WinNT only, what would it
> take to get a fix suitable for Win9x as well that could be landed for FF2?

First, my previous comment has a wrong. It's not WinNT only. It's only after Win2k.

By MSDN, Win2k and WinXP have new virtual key codes for plus sign and minus sign. My patch of bug 339723 is using them. But other Windows don't have the key code. I don't know old key code specs...

Fortunately, I have WinMe and that has keyboard layout switching. I will be able to work for that after bug 339723. But I cannot promise I can fix it. And the patch needs many days for nightly build testing for all keyboard layout users.

But before the working, I need related patch list for this fix. (Is it only dependent bugs? Or are there other regressions?)

Comment 123

[tech]

(In reply to comment #122)
Perhaps you shouldn't bother fixing this bug for these prehistoricals OSes like said in bug 330276

Comment 124

[Masatoshi Kimura [:emk]]

(In reply to comment #123)
> Perhaps you shouldn't bother fixing this bug for these prehistoricals OSes like
> said in bug 330276
We are talking about landing on Fx2 which still supports Win9x.
Bug 330276 dropped support for Win9x on Fx3. Ok?

Comment 125

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 215299 [details] [diff] [review]
Same as last patch, but with fixed comment

Requesting approval1.8.1. 

We really can't ship Fx 2 with a broken Caps Lock...

Comment 126

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 215299 [details] [diff] [review]
Same as last patch, but with fixed comment

Un-requesting, this patch is not compatible with 1.8.1. :(

Sorry for bugspam.

Comment 127

[Benjamin Smedberg]

Dainis, Neil, ere: is it possible to provide a branch patch for this? This looks pretty serious but it's much too risky to take after beta2 (and I'm not even clear on the risks now or the regressions it may have caused on trunk).

Comment 128

[Dainis Jonitis]

Benjamin,

If required I can prepare the required patch for branch. It should have support for Win9x, right?
I won't be able to work on patch before next Monday...

Comment 129

[Benjamin Smedberg]

Since codefreeze is today and it sounds like this patch is too risky to take after beta2, we're going to minus this one. Don't bother making a branch patch.

Comment 130

[Mike Schroepfer]

Minusing per comment 129.