This is required to fix bug 287179. This is cross platform fix for bugs like bug 178110, bug 138475, bug 6135, bug 158679 etc. The idea is to change all consumers of "keypress" to not expect that lowercase "charCode" will be returned by platform specific Widget code, if Alt/Ctrl modifier keys are pressed (as it was done in bug 285161). The platform specific code should return the correct letter case, taking into account the Shift and Caps Lock key states. The consumers should perform case insensitive comparison for "charCode". To distinguish between lower and upper case characters they should explicitly check the "isShift".

The only consumer that was not upper/lower case ready was: /mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp The other files are changed only to make minor cleanup by removing redundant code, fixing broken debug code etc. Please note that I've added the Tab as a valid separator for modifier keys in nsXBLPrototypeHandler::ConstructPrototype (). I hope it is logical to not distinguish between Tab and Space characters.

Comment on attachment 184318 [details] [diff] [review] diff -d -u -15 I am not sure whether I selected correct component and even more confused about who should review/superreview the code. Some changes are in content, other in layout and even in extensions.

I'm not sure that calling ToLowerCase on a PRUint32 is a meaningful operation... In fact, I'm pretty sure that breaks for non-plane-0 characters. That said, I'm not sure I can do a thorough review of this any time soon (within a week or more). bryner, neil, jag might be able to review changes to XBL.

Do you think that I should throw away most significant word by converting to PRUnichar? Or should I interpret it as a nsAString and perform comparison on it to allow non-plane-0 characters?

Comment on attachment 184318 [details] [diff] [review] diff -d -u -15 >- if (!(mKeyMask & cShift)) >- mKeyMask &= ~cShiftMask; This looks as if it might break bug 104371.

Neil, At least with patch for bug 287179 I do not see regression with bug 104371. And even without it everything should work, because both nsHTMLEditor.cpp and nsPlaintextEditor ignore only Alt, Ctrl and Meta modifiers, but accept the Shift modifier. As Boris noticed the case comparison code is not ready for higher plane Unicode characters. The same problem is within these files: 1. nsXBLPrototypeHandler.cpp 2. nsListControlFrame.cpp 3. nsMenuBarFrame.cpp 4. nsMenuPopupFrame.cpp I will prepare new patch that fixes all those consumers. BTW nsImageDocument.cpp and nsIsIndexFrame.cpp do not check the modifier keys when comparing the GetCharCode. Should not they be restricted to case when no any modifier key is pressed?

Erm. Why are we just casting that PRUint32* to PRUnichar*? How is that PRUint32 produced, exactly? That is, higher-plane characters may require more than two PRUnichars to be encoded, but they may be encoded in a single PRUint32 using UTF-32 or some such...

I am casting &PRUint32 (and not *PRUint32) to *PRUnichar in assumption that that PRUint32 is in the same form as PRUnichar strings (but not 0 terminated) and contains one or two UTF16 code points. If it is plane-0 character then higher two bytes will be 0. If it is non plane-0 character then all four bytes are significant. I believe the DOM events specification should say how this PRUint32 is encoded. But actually there are no other alternatives to the one described above. In worst case it can enforce the higher two bytes to be 0 and thus limit to the plane-0 characters, but it will still be compatible with my code, right?

> in assumption that that PRUint32 is in the same form as PRUnichar strings I don't believe that's a correct assumption here.... > If it is plane-0 character then higher two bytes will be 0. Independent of endianness?

> > If it is plane-0 character then higher two bytes will be 0. > > Independent of endianness? At least old code relied on it - it always casted to PRUnichar to throw away the higher two bytes. If everything worked in old version on all platforms then it should work with this patch, too. Example from old nsXBLPrototypeHandler.cpp: mDetail = key[0]; // PRUint32 is initialized with first character of string and later used: if (code != PRUint32(mDetail)) // PRUint32 code compared with PRUnichar If you think that these assumptions are wrong can you suggest some sample code to look at. It was my first attempt to use Mozilla string data types and it was best that I can think of.

> it always casted to PRUnichar to throw away the higher two bytes. Given a PRUint32 n, (PRUnichar)n will throw away the two high bytes. On the other hand, ((PRunichar*)&n)[0] (which is what you are saying is equivalent) will throw away the second two bytes in the memory layout. Those may be the high or low bytes, depending on endianness. In particular, say we are looking at the English character 'A'. The PRUnichar for this would be 0x41. Depending on endianness, the two bytes in it would be [0x0 0x41] or [0x41 0x0]. Now a PRUint32 representing that same 'A' would also be 0x41 _as a number_. The bytes in it would be, depending on endianness, either [0x0 0x00 0x00 0x41] or [0x41 0x0 0x0 0x0]. In the latter case (little-endian), casting that byte array to PRUnichar* gives us 0x41 as the fist PRUnichar and 0x0 as the second one. In the former case (big-endian), you get 0x0 as the FIRST PRUnichar, and 0x41 as the second one. In other words, it looks like the empty string on a big-endian system. It sound like we have no clear idea (as in, no one ever stopped to think about it) of how this PRUint32 encodes a character, so it may make the most sense to just cast it to PRUnichar as the old code did and file a followup bug to sort out what happens with non-plane-0 stuff. :(

Then first version of patch should be ok for this temporary solution. Should I change the review '?' flags back to it again? About this followup bug. In which component should it be? DOM events?

Yeah, if the first patch version does the right thing there, just switch the flags to it... And yes, DOM events.

Then I think the best solution is to add the new GetCharCode (nsAString& aCharCodeString); method to the nsDOMKeyboardEvent class in nsDOMKeyboardEvent.cpp. It will be the same helper method as GetWhich (). It will allow to hide all endianness details within this method. What do you think? If you agree with solution should I still do it in a separate bug?

Exposed on what interface?

Initially I thought not to expose it to any of interfaces, so that callers should explicitly cast nsIDOMKeyEvent* to nsDOMKeyboardEvent* before accessing it. But it could be much more convenient to expose it to the nsIDOMKeyEvent interface to avoid significant changes in callers. nsIDOMKeyEvent.idl does not contain FROZEN keyword. 1. Does it mean that I can add extra readonly attribute string charCode to this file? 2. Should it be the last line to preserve binary compatibility? 3. Should I change the GUID of interface anyway? Sorry for too much questions - it's all new for me...

> so that callers should explicitly cast nsIDOMKeyEvent* to nsDOMKeyboardEvent* > before accessing it. That's really not a good solution, imo. > But it could be much more convenient to expose it to the nsIDOMKeyEvent Unfortunately, that's not a great solution either. Changes to that interface need to be pretty carefully thought about (and shouldn't be happening before 1.9, imo, whereas I got the impression that we want this patch for 1.8). So I'd just file that separate DOM Events bug...

After more investigation seems that GetWhich () is exposed on nsIDOMNSUIEvent interface. I can expose this new function on this interface then, too. It will not have sense for Mouse events, but I can add assert for this case.

The problem is that all the interfaces you're talking about are exposed to content script; adding new things on them should be done with some care.

Comment on attachment 184318 [details] [diff] [review] diff -d -u -15 Back to revision 1 of patch. I will open a followup bug to handle the non-plane-0 characters.

Comment on attachment 184318 [details] [diff] [review] diff -d -u -15 >Index: content/xbl/src/nsXBLPrototypeHandler.cpp > if (mMisc) >+ { Bracing style for this file is: if () { } else { } Please follow that? >+ match = (ToLowerCase(code) == mDetail); How about ToLowerCase(PRUnichar(code)) to make it clear what's going on? sr=bzbarsky with those nits picked.

Dainis, has this landed? Does this need to land for 1.8? If so, this should probably get into 1.8b4 asap....

No, it still waits for review from Neil and is very far in his review queue.

Comment on attachment 185448 [details] [diff] [review] Same as version 1, but with Boris review notes fixed >+ if (mMisc) { > aKeyEvent->GetCharCode(&code); >+ match = (ToLowerCase(PRUnichar(code)) == mDetail); >+ } else { > aKeyEvent->GetKeyCode(&code); >+ match = (code == PRUint32(mDetail)); >+ } > >+ if (!match) > return PR_FALSE; It seems to me that this can be simplified as follows: if (!match) => if (mMisc ? (ToLowerCase(PRUnichar(code)) != mDetail) : (code != PRUint32(mDetail))) => if (mMisc ? (PRUint32(ToLowerCase(PRUnichar(code))) != PRUint32(mDetail)) : (code != PRUint32(mDetail))) => if ((mMisc ? PRUint32(ToLowerCase(PRUnichar(code))) : code) != PRUint32(mDetail)) => if (mMisc) code = ToLowerCase(PRUnichar(code)); if (code != PRUint32(mDetail)) which can then be merged back in to the existing block. r=me with this fixed. I tried this on Windows and was glad to see Caps Lock didn't affect j/J keys for junk mail :-)

Neil, You mean something like this: PRBool nsXBLPrototypeHandler::KeyEventMatched(nsIDOMKeyEvent* aKeyEvent) { if (mDetail == -1) return PR_TRUE; // No filters set up. It's generic. // Get the keycode or charcode of the key event. PRUint32 code; if (mMisc) { aKeyEvent->GetCharCode(&code); code = ToLowerCase(PRUnichar(code)); } else aKeyEvent->GetKeyCode(&code); if (code != PRUint32(mDetail)) return PR_FALSE; return ModifiersMatchMask(aKeyEvent); } Should be ok. Probably even more readable. Can you then please check in these changes? I do not have CVS access rights. If you want new patch then let me know. Thanks in advance!

Neil was not in CC list, thus I decided to prepare new patch. Boris, probably you can check this in? Once this will be checked in, I will open the followup bug that fixes the non plane-0 characters in endian independent way.

I can check this in on trunk tonight or tomorrow; do you want to try to get this into 1.8? Is it safe enough?

It will be great if you check it on trunk. I think it is safe enough for 1.8, but you better ask Neil.

Neil, does that last patch address your comments? And do you think this is 1.8 material?

(In reply to comment #32) >Neil, does that last patch address your comments? Seems to. >And do you think this is 1.8 material? Safe, useful, but not essential.

Fixed on trunk. If people want this on branch, please request approval.

Opened follow-up bug 306627 to fix non 0-plane character problem.

This caused regression bug 306686 (fixed) and regression bug 307423 (needs fixing).

Probably it is worth to add "any" keyword support for "modifiers" for XUL "key", like XBL.

Note that that also involves making the shortcuts display correctly in menus (I guess that means changes to nsMenuX.cpp and nsMenuFrame.cpp).

Can we turn off the warning? NS_WARNING("GetCharCode used for wrong key event; should use onkeypress."); This case is common, a lot of code call it and compare output with 0. Annoying while debugging.

Sure. Go ahead and comment it out.

Done.

ginn.chen@sun.com: you just made a change to /content/ w/o any sign off from a content peer. that's not how things are supposed to be done.

Ginn, please undo the change you made, and do so ASAP. Checking the charcode of a key event other than keypress is a bug, since the number you get is meaningless. So if you're hitting this a lot during debugging, file bugs on the code that does it. That's why we have the warning! Removing the warning because there's a lot of buggy code that triggers it is very very backwards. In the future, do NOT check in changes to content without content peer review. Ever. Dainis saying "go ahead" does not constitute content peer review.

Undo-ed. Sorry, all.

Most of these warnings are not from incorrectly written external callers, but from internall callers that need to get all information about key event (both keycode and charcode), like in: 1. content/xbl/src/nsXBLWindowKeyHandler.cpp, WalkHandlers() 2. extension/typeaheadfind/src/nsTypeAheadFind.cpp, KeyPress() 3.layout/forms/nsTextControlFrame.cpp, DOMEventToNativeKeyEvent()

Then we should fix said internal callers -- they're asking for the charcode in cases when the answer is meaningless.

The warnings in GTK builds are due to bug 309316, for what it's worth.