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?

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.

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.

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.

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.

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!

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 '&#257;'. 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.

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.

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.

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.

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

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.

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.

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?

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 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...

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

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

In case you need another key test, we did a pretty extensive one: https://bugzilla.mozilla.org/attachment.cgi?id=133441

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 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"

I wanted to select Christian Biesinger to be a superreviewer, but mail cbiesinger@web.de in module owners is refused by Bugzilla.

Comment on attachment 181753 [details] [diff] [review] diff -d -u -20 Fully functional sorry... I'm not a super-reviewer

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*'

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).

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).

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

(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.

> 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 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.

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

Sure, new files would be fine too.

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.

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 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 :) ).

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.

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

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.

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

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?

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.

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.

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.

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).

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.

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.

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

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.

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

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 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 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!

> 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 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.

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

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?

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?

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).

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 on attachment 207749 [details] [diff] [review] Make sure compiles on WinCE could you post a patch that just contains your changes for WINCE?

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.

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

Patch checked in to trunk.

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

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 (?)

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.

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

Bustage fix committed.

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

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

this probably caused Bug 322813

Patch 207541 does not work (result: hang).

Neither does patch 206856 (result: identical hang)

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

(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+&#1492; 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.)

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

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 on attachment 208402 [details] [diff] [review] partial backout patch (leave early Windows detection in nsToolkit) Backed out.

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

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.

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.

Fix checked in to trunk.

Could this have broken FF installer builds on Win98 ? Error Starting Program The FIREFOX.EXE file is linked to missing export SHELL32.DLL:SHGetFolderLocation

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

(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.

(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.

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.

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.

(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.

Patch backed out.

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

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

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

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

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?

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!

(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).

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

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.

(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.)

(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?

(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 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?

> 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

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.

Patch 215299 checked in.

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

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.

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.

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

Please nominated appropriate patch(es) for the 1.8.1 branch

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.

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

(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.

(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...

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?

(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?)

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

(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 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 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.

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).

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...

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.

Minusing per comment 129.