Bug 332579 comment 22: > Using build from comment #16, and I saw an issue that might be related to > this patch. In Gmail, I clicked on the "Download" link in order to download > the attachment. This does two this: (1) sets the focus on the "Download" > link, and (2) brings up (at least in my case) a dialog asking whether to > save the file or open with an app. 'Save' was already selected, so I just > hit Enter, but then I saw the dialog immediately reappear. > > It looks like the Enter key was being counted twice, first to select the > default button in the dialog, and secondly to reselect the "Download" link. I'm seeing keypress events for the RETURN key coming back from dispatch with a status of nsEventStatus_eIgnore, even though the events are being handled and the proper status seems like it should be nsEventStatus_eConsumeNoDefault. This can cause a double-processing problem for us on the Mac because we accept key events through two channels: a raw key event handler and a TSM event handler. If, for a single keypress, one handler indicates that the event was not handled, the system will give the event to the other. In the case above, I'm seeing the raw key event handler being called. An NS_KEY_PRESS event is dispatched, but the status is nsEventStatus_eIgnore, so the system is told that the event was not handled. The system pushes the event to the TSM handler, which fires off another NS_KEY_PRESS event. This behavior seems to affect the RETURN key, but not other keys.

What's probably happening is that VK_RETURN is special-cased to call some "do default action" function like a mouse click would do. The original event doesn't get preventDefault called on it. This might be the best way to attack the problem, but it might not be possible to fix it everywhere. One idea I've played with is to use the raw key-down event to always generate NS_KEY_DOWN and the TSM event to always generate NS_KEY_PRESS. To do this, we'd need to always tell the system that the raw key-down event was not handled, otherwise we wouldn't get the TSM event. That's a little suspicious, but it's not the worst part: the worst part is that there's no good way for the key-down handler to tell the TSM handler that the NS_KEY_PRESS event should have preventDefault set if the NS_KEY_DOWN event was cancelled.

If it's of any help, here's the bit of code that hard-wires VK_ENTER to the default button on mac os x: http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsButtonBoxFrame.cpp#104

104 #ifndef XP_MACOSX Actually, this line would fix the problem, but only where the problem return keys are directed at XUL nsButtonBoxFrames: 112 *aEventStatus = nsEventStatus_eConsumeNoDefault;

Similar problem at bug 337277.

I spent a lot of time overhauling how native key events are turned into Gecko events today, and the results are very clean. I overhauled nsMacTSMMessagePump to be Carbon Event-based instead of Apple Event-based. Then, I moved all key event processing out of the distinct application-scope raw event and IME handlers, both of which are now disabled, and into a new raw event handler that's attached to each window as a target. The raw event handler accepts Unicode data directly. One problem is that leaving composition mode isn't handled as cleanly as it could be. If you type (with a US keyboard) option-U,B, you would expect to see umlaut,B (try it in a current release version), but instead, you just get the umlaut. I'm also interested in testing from 10.3 users. This patch fixes this bug, bug 337277, bug 337338, and probably a host of others that haven't been discovered or reported yet. It renders a bunch of code dead but doesn't remove it - the final patch here (or a followup) will clean up unreachable codepaths. I'm putting a new test build up at http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg , equivalent to Firefox 1.5.0.3 + bug 332579v8 + this patch. I'm especially interested in getting feedback from CJK users and users of other esoteric input methods.

(In reply to comment #5) > Created an attachment (id=221541) [edit] > I'm especially interested in > getting feedback from CJK users and users of other esoteric input methods. > Japanese IME a problem is reported with bug337370. This patch might be able to be tested tonight.

(In reply to comment #5) > http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg , equivalent to > I tried Japanese IME with this build. But, Japanese was not able to be input. The input switch of English(Ei-suu) and the hiragana(Kana) cannot be done with the keyboard. (When the English input switch key is pushed, space is input. )

Thanks, Hiro. Based on that, it looks like I really need to get the keypresses out of the way of TSM, and make sure the kEventTextInputUpdateActiveInputArea handler gets first crack.

Comment on attachment 221541 [details] [diff] [review] Checkpoint v1 Command-shift-G is doing find next instead of find previous.

I did some testing to help clarify IME behavior on the most recent patched test build you posted in comment #5. First, the part you already know about: 1. Put cursor in search bar. 2. Switch your input mode to Japanese Hiragana. 3. Type "hashiru." Result: Latin characters "hashiru" Expected: Japanese characters 「はしる」; composition mode should still be active (i.e. characters are underlined, waiting for you to either activate kanji/alternate results matching [spacebar] or confirm displayed characters [return]). Here's the new and intriguing bit: 1. Put cursor in search bar. 2. Switch your input mode to Japanese Hiragana. 3. Type "[option-h]ashiru." Result: Japanese characters 「はしる」; composition mode is still active. Expected: Same, because Japanese IME doesn't use opt-h for any special characters (e.g. opt-y) or input-modification commands (e.g. opt-i, opt-k, opt-j, opt-l, opt-z, opt-x, opt-c, opt-a, and opt-s), which means it is treated the same as an unmodified "h." Since unmodified "h" as first character should enter composition mode, so should opt-h. 3a. Test input-modification commands: spacebar, arrow keys, opt-i/k, opt-z/x/c, etc. Result: as expected. 4. Hit return to confirm the current modified input. Result: TWO copies of the input are inserted into the search bar AND a search is immediately triggered with the currently selected search engine. Expected: ONE copy of the input is inserted into the search bar and no search is triggered. Clicking in the blank space to the right of the selected input DOES yield expected behavior, so this doubling behavior is being caused by the return key rather than whatever the generic exit-the-composition-mode signal is. Also, going in a slightly different direction at step 3: 1. Put cursor in search bar. 2. Switch your input mode to Japanese Hiragana. 3a. Type "[option-i]" Result: Nothing. Expected: Same, because opt-i is claimed by the IME as a special command. 3b. Type "[option-y]" Result: ￥ (yen symbol); composition mode is active. Expected: Same, because opt-y is claimed by the IME as a special character. CONCLUSIONS: 1. This patch is allowing the Japanese IME to treat all option-modified letter keys properly. Although using unmodified letters fails to initiate composition mode, the use of unclaimed opt-letters acts as a backdoor into composition mode, since opt-letters not claimed for special commands or characters are treated by the IME as unmodified letters. (So: Unmodified keys are not being allowed to reach the IME, but option-modified keys are, and are being converted to unmodified keys [as necessary] BY the IME.) 2. Once we enter composition mode using the aforementioned backdoor, we can observe the current bad behavior of the return key (doubled input + activating default action for current form field). Er, I hope that at least reveals something interesting. Cheers.

And my apologies about the mess those Unicode chars made in the comment above; I've never tried typing Japanese into Bugzilla before. >_<

(Following up comment #10) I didn't work through all of Nicholas's report, but I've confirmed the first part -- IME doesn't work with Bon Echo Alpha 2 on any of OS X 10.4.6, 10.3.9 or 10.2.8. Even when your input mode is Japanese Hiragana (or some other input mode that uses IME, chosen from the "flags" menu), typing characters results in ordinary input. This happens even when a Java applet hasn't yet been loaded -- i.e. the Java Embedding Plugin isn't involved. I've also (I think) figured out how to fix this problem. Mozilla.org browsers (the "official" versions) use Apple Event handlers to do IME. But getting rid of WaitNextEvent() seems to have stopped those handlers from receiving any Apple events. Mark, you're trying to do the right thing in widget/src/mac/nsMacMessagePump.cpp's DispatchEvent() (the call to AEProcessAppleEvent()). But the handling required for Apple events in a Carbon handler for kEventClassAppleEvent / kEventAppleEvent is very peculiar -- you need to explicitly remove the Carbon event from the queue before calling AEProcessAppleEvent()! For sample code see the following (it's in a section titled "Processing Apple Events With the Carbon Event Model"): http://developer.apple.com/documentation/AppleScript/Conceptual/AppleEvents/dispatch_aes_aepg/chapter_4_section_3.html I've noticed that, in your patch to this bug (337199), you've switched to using Carbon event handlers to do IME. But if that code is in http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg, it's failing in exactly the same way -- IME is simply not happening. I suggest that you go back to using Apple Event handlers for IME, and try augmenting your code in nsMacMessagePump::DispatchEvent() from the sample code that I've referred to.

I've actually got it working with CE handlers throughout in my own tree, but am having problems with ending an inline input session.

Do whatever works :-) But getting rid of WaitNextEvent() was already a pretty radical (and dangerous) thing to do on a "production" branch. And switching IME from Apple event handlers to Carbon event handlers would be even more radical. At the very least, it may require me to change the Java Embedding Plugin. It contains code to deal with browsers that use Carbon event handlers to do IME ... but that code's never been tested :-(

Much to my surprise, I've found a way to change the Java Embedding Plugin to get rid of the character-doubling problem in Bon Echo Alpha 2. Mark, I'm sure you're aware of this, but here's a bit of background for those who aren't aware: The Java Embedding Plugin uses Apple's Cocoa-Carbon interface to integrate the Cocoa-based JVM with Carbon-based Firefox (and Seamonkey and Mozilla). But there's a design flaw in the Carbon event handlers that implement Apple's Cocoa-Carbon interface -- they swallow all keyboard and mouse events! So if the Java Embedding Plugin didn't work around this design flaw, after loading a Java applet you would no longer be able to use the keyboard or mouse outside of that applet. The JEP's workaround is to install another Carbon handler "after" (or "on top of") Apple's handlers, and "redispatch" to the browser those mouse and keyboard events that the browser needs to see. All recent versions of the JEP do this by calling SendEventToEventTargetWithOptions() with "OptionBits" set to "kEventTargetSendToAllHandlers". This sends the event to _all_ handlers installed (for that event) on a given target, including those that would normally be pre-empted by a "later" handler returning "noErr". The key-doubling that people have reported is probably due to key events being sent both to WNETransitionEventHandler() (in widget/src/mac/nsMacMessagePump.cpp) and to some kind of default keystroke handler (for objects that support text input) installed by the OS. For a long time I thought this was the only possible workaround. But now I've found that, if I send key events to the "application" target (instead of to the "user focus" event target as I have been doing), they no longer get swallowed by Apple's Cocoa-Carbon interface handlers! I'm now thinking of including this change in my next JEP release (0.9.5+e, which I _hope_ will come out next week). My "patch" is for those (Mark?) who are willing to recompile the JEP to test my new way to get keyboard events to a Carbon-based browser. It's against JEP 0.9.5+d (the most recent release). Side note: My patch doesn't seem to get rid of the character doubling problem completely. I still see JavaScript alert messages re-appear after having dismissed them by pressing "Return" or "Enter".

Something I forgot to mention: My patch makes makes keyboard input stop working in the browser (after a Java applet has been loaded) when used with http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg -- presumably because that revision stops WNETransitionEventHandler() from handling key events.

Steven, the checkpoint patch here and the v9 test build should already be immune to double-keys with a current JEP. Getting rid of WNE was radical, but the only way we'll know for sure if it's feasible for fox2 is to try to shake the regressions out now.

> the checkpoint patch here and the v9 test build should already be > immune to double-keys with a current JEP They are ... but (of course) they're broken in other ways. How soon might I/we be able to test one of your builds that has IME working (at least partially) via Carbon handlers? (I'm trying to decide when to release JEP 0.9.5+e, and whether or not to include my keystroke-handling patch in it.)

I'm putting together a new patch now and am comparing it to the 1.8.0 behavior. I'll make a universal test build available later today.

Thanks!

Re comment 15, the Carbon event is never on the queue when AEProcessAppleEvent is called. In fact, on the 1.8 branch now, that should be dead code, because the loop is always run by RunApplicationEventLoop, which has its own handler for Apple events. I've left it in the nsMacMessagePump::DispatchEvent because it's not dead on the trunk, where the loop is sometimes run by RunApplicationEventLoop and sometimes by manual cranking of ReceiveNextEvent. (When I use RNE, I have it pull the events from the queue.) So at least all of those bases are covered. Anyway, this afternoon, I managed to fix the remaining (known) bugs here. Most of the key badness was fixed by ensuring proper routing of the events. The trouble I was having with leaving IME sessions properly was fixed by going back to the AE model for unicode input, as Steven suggested. One difference is that any keypress, even those in IME sessions, now produces an NS_KEY_DOWN event. If in IME, there's no NS_KEY_PRESS, of course. In the past, there wouldn't have been any NS_KEY_DOWN events when in IME either, but there would have been NS_KEY_UP events. I don't think that the new behavior is wrong. (But I might be wrong.)

Will do a test build before requesting review. There will be a followup cleanup patch to get rid of dead code like HandleKeyEvent and HandleUpdateInputArea.

Test build will be 2.0a2 (1.8.1a2) plus patch v2, above.

Steven, if you want to work on JEP's handling of IME with Carbon events, we can do that separately once this is substantially wrapped up.

A test build fixing this bug and the three dependencies (337277, 337338, and 337370) is available at: http://jackassofalltrades.com/tmp/firefox-2a2-337199v2.dmg This is equivalent to Firefox/BonEcho 2.0a2 (1.8.1a2) plus the v2 patch from above. Unlike previous test builds, it is not based on the 1.5.0.x (1.8.0.x) series. Bugs or behavioral differences should be compared to BonEcho 2.0a2: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/bonecho/alpha2/mac/en-US/Bon%20Echo%20Alpha%202.dmg Note that the last official 2.0 (1.8.1) build without the updates from bug 332579 was from 0508: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2006-05-08-09-mozilla1.8/firefox-2.0a1.en-US.mac.dmg

I've noted two bad behaviors that seem like they might be caused by this patch or bug 332579, but aren't: - start browser, focus window, press command-L to focus location bar, press TAB to focus search bar, quickly type a search term and press RETURN. expect search to take place in current tab, but a new tab is opened to hold the search. - variation on the above: sometimes, the RETURN key has no effect. (may be dependent on the timing betewen the last letter typed and the RETURN key.) this variant occurs infrequently. I've tested these by backing out this patch and the patch from bug 332579, and the behaviors still occur, so they seem to be regressions caused by some other change. I've even experienced the former variant on win32. (Anyone want to QA it or let me know if there are already open bugs?)

(In reply to comment #24) > Steven, if you want to work on JEP's handling of IME with Carbon events, we can > do that separately once this is substantially wrapped up. > There is bug in JEP and IME.(bug315972) Does this problem improve by this change? Supposing that is not right, the test of a Java applet and IME is impossible. (Because, if bug315972 occurs, all character inputs will become impossible.) Test build of comment#25 is tried excluding the above issue.

(In reply to comment #27) I've never been able to reproduce any of the problems reported at bug 315972, possibly because I don't have an Apple Japanese keyboard (aka the Shift-JIS keyboard). (I understand that this keyboard has a couple of extra keys, used for changing input methods.) And it's possible that these problems can be avoided even with the Shift-JIS keyboard if you only use the "flags" menu to change input methods. (I probably won't be able to fix bug 315972 until someone tells me how/where to get a Apple Shift-JIS keyboard.) (Following up comment #25) In brief tests that I've just done (on OS X 10.3.9, using Apple's English keyboard), I had no problem doing Japanese (Hiragana) and Chinese (Pinyin) IME, either in an applet or in the browser. I even found that my send-keystroke patch (attachment 222508 [details] [diff] [review]) no longer stops keyboard input in the browser after an applet has been loaded. (I still haven't decided whether to include this patch in JEP 0.9.5+e, though.)

(In reply to comment #26) > I've noted two bad behaviors that seem like they might be caused by this patch > or bug 332579, but aren't: > - start browser, focus window, press command-L to focus location bar, press > TAB to focus search bar, quickly type a search term and press RETURN. > expect search to take place in current tab, but a new tab is opened to hold > the search. I can't seem to reproduce that, hittin RETURN opens the search results in the same tab for me. Possibly you have this pref set ? user_pref("browser.search.openintab", true); Not sure about the second problem you mention, maybe my typing is too slow. The problems from bug 315972 (JEP and IME) are still there. BTW Steven (comment #28), whatever way I choose to switch between input methods: keyboard or 'flags' trigger bug 315972. The problems noted in bug 337370 is fixed by this patch, so far. I couldn't reproduce the character duplication either (bug 336357). Typing special characters (bug 337338) works on my side. After an hour surfing around, no problems to report.

(In reply to comment #14) > Do whatever works :-) > > But getting rid of WaitNextEvent() was already a pretty radical (and > dangerous) thing to do on a "production" branch. And switching IME > from Apple event handlers to Carbon event handlers would be even more > radical. I(Katsuhiro MIHARA) wanted to be more radical at Bug 158859. I wrote Carbon Event handlers(like Apple Event handlers) for TSM, but some problems exist and were not accepted by reviewers. https://bugzilla.mozilla.org/attachment.cgi?id=187999 I think that the reason of this bug in checkpoint v1 is removing Apple Event handlers for TSM and not writing Carbon Event handlers for TSM. This bug maybe be fixed by writing Carbon Event handlers. If you want me to merge patches, please wait. Feel free to merge patches.

(In reply to comment #30) > I think that the reason of this bug in checkpoint v1 is removing Apple Event > handlers for TSM and not writing Carbon Event handlers for TSM. > This bug maybe be fixed by writing Carbon Event handlers. I missunderstood checkpoint v1. I read that Carbon Event handlers exist. Wmmm....

At Bug 158859, I remains Apple Event handler for kUnicodeNotFromInputMethod(because I don't understand Key event handlers on Gecko). > In order to avoid interference with input methods, providing a handler for the kEventTextInputUnicodeForKeyEvent event and examining its parameters is the preferred means on Carbon for directly examining keyboard input in general. http://developer.apple.com/documentation/Carbon/Conceptual/UnderstandTextInput_TSM/index.html#//apple_ref/doc/uid/TP40000957 I think we need to implement either Carbon Event handler for kUnicodeNotFromInputMethod or Apple Event handler for kUnicodeNotFromInputMethod. The patch checkpoint v1 remove Apple Event handler for kUnicodeNotFromInputMethod, but doesn't install Carbon Event handler for kEventTextInputUnicodeForKeyEvent. > - err = AEInstallEventHandler(kTextServiceClass, kUnicodeNotFromInputMethod, mKeyboardUPP, (long)this, false); > - NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::InstallTSMAEHandlers: AEInstallEventHandler[FromInputMethod] failed"); > +#if 0 > + // All key events are handled by raw key event handlers > + err = > + ::InstallApplicationEventHandler(sUnicodeForKeyEventHandlerUPP, > + GetEventTypeCount(kUnicodeForKeyEventList), > + kUnicodeForKeyEventList, > + NS_STATIC_CAST(void*, this), > + &mUnicodeForKeyEventHandler); > + NS_ASSERTION(err == noErr, "Could not install UnicodeForKeyEventHandler"); > +#endif

(In reply to comment #32) > http://developer.apple.com/documentation/Carbon/Conceptual/UnderstandTextInput_TSM/index.html#//apple_ref/doc/uid/TP40000957 http://developer.apple.com/documentation/Carbon/Conceptual/UnderstandTextInput_TSM/tinptsm_concepts/chapter_2_section_3.html#//apple_ref/doc/uid/TP30000091-BHJIDAEE

Katsuhiro, that's right, the v1 checkpoint used an experimental and poorly documented way of getting unicode key data directly from the raw key event. It didn't work. Even when I gave up that battle and turned on the kEventTextInputUnicodeForKeyEvent CE handler active, though, there were problems ending IME sessions.

(In reply to comment #27) > Test build of comment#25 is tried excluding the above issue. > IME works. There is no problem. (But JEP is not tried. ...)

Comment on attachment 222537 [details] [diff] [review] v2 for trunk, AE TSM handlers restored All of the feedback so far has been positive, so I'm going for review. Thanks to the testers and other developers who took the time to help out. Reviewer's guide: the main functinal part of this patch is in nsMacEventHandler.

Comment on attachment 222537 [details] [diff] [review] v2 for trunk, AE TSM handlers restored >Index: mozilla/widget/src/mac/nsMacEventHandler.cpp >=================================================================== ... > #ifndef XP_MACOSX > #include <locale> > #endif Whack that please. > // > // create a TSMDocument for this window. We are allocating a TSM document for > // each Mac window > // > mTSMDocument = nsnull; Whack the tabs. Looks fine to me.

Comment on attachment 222537 [details] [diff] [review] v2 for trunk, AE TSM handlers restored If the assertions in HandleNKeyEvent can happen in situations that don't involve changing a fundamental physical constant, it seems like we should be propagating the error somehow, or perhaps indicating that we didn't handle the event? Not sure if that would lead to us looping on a bogus event, though, or if the better response would be to just claim that we'd handled it and have the martian event just disappear. sr=shaver and 181=shaver if we're sure we're doing the right thing in that case. Very excited to see these fixes coming along!

This is not a comment to patch v2. Apple Event data types left in nsMacEventHandler. ex. nsMacEventHandler::InitializeKeyEvent() If this method recieves Carbon events, this should use GetEventParameter(EventRef, kEventParamKeyModifiers, ...) to read modifiers. To adopt Carbon events, some methods should be rewritten. I don't estimate roughly how lines should be written. Mark, I don't read code you didn't attach to Bugzilla. Do you overhaul key event handlers of Gecko on Mac?

(In reply to comment #39) > Mark, I don't read code you didn't attach to Bugzilla. > Do you overhaul key event handlers of Gecko on Mac? Yes, I did that in v1. The only difference between nsMacTSMMessagePump.cpp as modified by patch v1 (attachment 221541 [details] [diff] [review]) and as it properly exists to implement CE TSM handlers is that the proper implementation doesn't include the |#if 0| around the installation of the kEventTextInputUnicodeForKeyEvent handler, as you pointed out in comment 32, and I explained in comment 34. The work involved in converting the AE handlers to CE is already embodied in patch v1, and it seems also in your patches in bug 158859. (I didn't know about the existence of that bug before I did the conversion myself, and I certainly would have looked at your patches had been aware of it.) As you point out, the conversion does involve calls to GetEventParameter. For anyone who cares, I'll post a patch against the future current trunk to convert the AE to CE handlers, once this is checked in.

I'll check this in tonight. The 1.8 branch version is identical except for context in the Makefile diff. Comment 37 is addressed. In reply to comment 38: The only caller of nsMacEventHandler::HandleNKeyEvent is nsMacWindow::KeyEventHandler. KeyEventHandler is installed as a window-scope handler for only the two event kinds that HandleNKeyEvent knows how to cope with. The only way HandleNKeyEvent could be called and find different values for eventKind is if something very terrible happens.

Renamed HandleNKeyEvent to HandleKeyUpDownEvent to better reflect its function; relaxed one assertion in HandleKeyUpDownEvent to accept events without a charcode.

Checked in on trunk and MOZILLA_1_8_BRANCH for 1.8.1a3 (Fox 2a3). Stay tuned for followups.

Bug 338759 for the dead-code removal I alluded to above. I noticed (and filed) bug 338760 while testing this patch. It's not strictly a dependency, but it does affect Japanese text.

I've got a new test build available, equivalent to 1.5.0.4rc3 plus 332579v8 and further key regression fixes from 337199v4 (this patch). This build is available at: http://jackassofalltrades.com/tmp/firefox-1504rc3-332579v8-337199v4.dmg I've prepared this build even though the above patches are now all checked in on the 1.8.1 branch because that branch is only alpha-quality at this point, and I'd like testers who have the time to be able to bang on the event improvements in isolation on a more stable codebase. So, testers, if you have the time, have at it!

As promised in comment 40, this was my first cut at carbonizing the TSM handlers. The problem here is that it doesn't properly handle leaving IME sessions. When you type option-u, b, you expect to see u-umlaut, b, but instead, it just produces u-umlaut and leaves IME. When the focus is not a textarea, the behavior is bad: if you focus the content area and type option-u, command-l, you expect the location bar to be focused, but the command-l keystroke gets swallowed when IME exits.

(In reply to comment #46) I have one question and one comment about current trunk codebase. http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsMacEventHandler.cpp#760 760 void nsMacEventHandler::InitializeKeyEvent(nsKeyEvent& aKeyEvent, 761 EventRecord& aOSEvent, nsWindow* aFocusedWidget, PRUint32 aMessage, 762 PRBool aConvertChar) 763 { 780 aKeyEvent.isAlt = ((aOSEvent.modifiers & optionKey) != 0); 786 if (aMessage == NS_KEY_PRESS 787 && !IsSpecialRaptorKey((aOSEvent.message & keyCodeMask) >> 8) ) 788 { 789 if (aKeyEvent.isControl) 790 { 791 if (aConvertChar) 792 { 793 aKeyEvent.charCode = (aOSEvent.message & charCodeMask); 794 if (aKeyEvent.charCode <= 26) 795 { 796 if (aKeyEvent.isShift) 797 aKeyEvent.charCode += 'A' - 1; 798 else 799 aKeyEvent.charCode += 'a' - 1; 800 } // if (aKeyEvent.charCode <= 26) 801 } 802 aKeyEvent.keyCode = 0; 803 } // if (aKeyEvent.isControl) 804 else // else for if (aKeyEvent.isControl) 805 { 806 if (!aKeyEvent.isMeta) 807 { 808 aKeyEvent.isControl = aKeyEvent.isAlt = aKeyEvent.isMeta = 0; 809 } // if (!aKeyEvent.isMeta) 810 if ((aMessage == NS_KEY_PRESS) && (!aKeyEvent.isControl) && (!aKeyEvent.isMeta)), this method clear Option-Key flag (aKeyEvent.isAlt) at line 808. This may cause misunderstanding Option-u. Are there reasons? And this method can be simplified to only accept NS_KEY_PRESS and aConvertChar == PR_FALSE now.

(In reply to comment #47) > if ((aMessage == NS_KEY_PRESS) && (!aKeyEvent.isControl) && > (!aKeyEvent.isMeta)), this method clear Option-Key flag (aKeyEvent.isAlt) at > line 808. This may cause misunderstanding Option-u. Are there reasons? If the keystroke was supposed to go to an IME session (to begin it or to service one that's already active), the kUpdateActiveInputArea handler would have picked it up and NOT the kUnicodeNotFromInputMethod handler. For an English keyboard layout, we'll never take option-U through HandleUKeyEvent, and we'll never send an NS_KEY_PRESS for it, because it's managed by IME. We'll only hear about it in UnicodeHandleUpdateInputArea. The alt flag is cleared because if we do reach HandleUKeyEvent for a keypress that was produced with option down and command and control up, it's supposed to generate a character (usually a symbol). Leaving the alt flag on in the NS_KEY_PRESS event will prevent that character from being interpreted as normal entered text, and it will instead be seen as alt-weirdsymbolcharacter. The result would be an inability to type any option-keyed characters not entered in an IME session. That's bad, and it's definitely not the way the Mac is supposed to work. I agree that this is not at all clear in the code. > And this method can be simplified to only accept NS_KEY_PRESS and > aConvertChar == PR_FALSE now. Yeah, and we can also avoid clearing flags already known to be clear (above), comment tricky situations (above), and probably do a general tidying-up in InitializeKeyEvent. If you file a new bug and write a patch, I'll review it. If you file a new bug and assign it to me, time permitting, I'll write the patch.

(In reply to comment #48) > If you file a new bug and write a patch, I'll review it. I filed Bug #339221 and wrote a patch (Attachment #223406 [details] [diff]). Please review it.