Last Comment Bug 337199 - Key event/text input regressions on Mac (RETURN key events double-processed, ...)
: Key event/text input regressions on Mac (RETURN key events double-processed, ...
Status: RESOLVED FIXED
: fixed1.8.1
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: 1.8 Branch
: PowerPC Mac OS X
: -- normal (vote)
: ---
Assigned To: Mark Mentovai
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on: 339346
Blocks: 332579 336357 337277 337338 337370 338759
  Show dependency treegraph
 
Reported: 2006-05-08 15:07 PDT by Mark Mentovai
Modified: 2006-05-25 20:36 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Checkpoint v1 (61.00 KB, patch)
2006-05-09 20:56 PDT, Mark Mentovai
no flags Details | Diff | Splinter Review
Patch to change how the JEP sends keystrokes to the browser (1.72 KB, patch)
2006-05-18 09:33 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details | Diff | Splinter Review
v2 for trunk, AE TSM handlers restored (30.38 KB, patch)
2006-05-18 13:48 PDT, Mark Mentovai
jaas: review+
shaver: superreview+
shaver: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
v3, review comments addressed, for checkin (30.89 KB, patch)
2006-05-21 11:55 PDT, Mark Mentovai
no flags Details | Diff | Splinter Review
v4, this one's going in (30.87 KB, patch)
2006-05-21 16:51 PDT, Mark Mentovai
no flags Details | Diff | Splinter Review
TSM carbonization (35.71 KB, patch)
2006-05-23 11:22 PDT, Mark Mentovai
no flags Details | Diff | Splinter Review

Description Mark Mentovai 2006-05-08 15:07:45 PDT
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.
Comment 1 Mark Mentovai 2006-05-08 17:26:06 PDT
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.
Comment 2 Håkan Waara 2006-05-09 02:12:54 PDT
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
Comment 3 Mark Mentovai 2006-05-09 05:55:39 PDT
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;
Comment 4 Mark Mentovai 2006-05-09 06:09:35 PDT
Similar problem at bug 337277.
Comment 5 Mark Mentovai 2006-05-09 20:56:10 PDT
Created attachment 221541 [details] [diff] [review]
Checkpoint v1

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.
Comment 6 Hiro 2006-05-09 21:33:10 PDT
(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. 
Comment 7 Hiro 2006-05-10 04:47:49 PDT
(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. )
Comment 8 Mark Mentovai 2006-05-10 06:33:02 PDT
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 9 Mark Mentovai 2006-05-10 07:25:17 PDT
Comment on attachment 221541 [details] [diff] [review]
Checkpoint v1

Command-shift-G is doing find next instead of find previous.
Comment 10 Nicholas Fagerlund 2006-05-17 13:23:15 PDT
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.
Comment 11 Nicholas Fagerlund 2006-05-17 13:25:59 PDT
And my apologies about the mess those Unicode chars made in the comment above; I've never tried typing Japanese into Bugzilla before. >_<
Comment 12 Steven Michaud [:smichaud] (Retired) 2006-05-18 08:19:25 PDT
(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.

Comment 13 Mark Mentovai 2006-05-18 08:40:43 PDT
I've actually got it working with CE handlers throughout in my own tree, but am having problems with ending an inline input session.
Comment 14 Steven Michaud [:smichaud] (Retired) 2006-05-18 08:45:50 PDT
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 :-(
Comment 15 Steven Michaud [:smichaud] (Retired) 2006-05-18 09:33:11 PDT
Created attachment 222508 [details] [diff] [review]
Patch to change how the JEP sends keystrokes to the browser

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".
Comment 16 Steven Michaud [:smichaud] (Retired) 2006-05-18 10:11:26 PDT
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.

Comment 17 Mark Mentovai 2006-05-18 10:42:13 PDT
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.
Comment 18 Steven Michaud [:smichaud] (Retired) 2006-05-18 12:29:06 PDT
> 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.)

Comment 19 Mark Mentovai 2006-05-18 13:06:40 PDT
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.
Comment 20 Steven Michaud [:smichaud] (Retired) 2006-05-18 13:14:11 PDT
Thanks!
Comment 21 Mark Mentovai 2006-05-18 13:43:47 PDT
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.)
Comment 22 Mark Mentovai 2006-05-18 13:48:01 PDT
Created attachment 222537 [details] [diff] [review]
v2 for trunk, AE TSM handlers restored

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.
Comment 23 Mark Mentovai 2006-05-18 13:53:36 PDT
Test build will be 2.0a2 (1.8.1a2) plus patch v2, above.
Comment 24 Mark Mentovai 2006-05-18 14:28:41 PDT
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.
Comment 25 Mark Mentovai 2006-05-18 16:51:57 PDT
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
Comment 26 Mark Mentovai 2006-05-18 17:17:40 PDT
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?)
Comment 27 Hiro 2006-05-18 20:17:19 PDT
(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. 
Comment 28 Steven Michaud [:smichaud] (Retired) 2006-05-18 21:22:09 PDT
(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.)

Comment 29 philippe (part-time) 2006-05-18 21:57:23 PDT
(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.
Comment 30 Katsuhiro MIHARA 2006-05-19 01:12:59 PDT
(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.
Comment 31 Katsuhiro MIHARA 2006-05-19 01:36:27 PDT
(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....
Comment 32 Katsuhiro MIHARA 2006-05-19 02:18:16 PDT
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
Comment 34 Mark Mentovai 2006-05-19 06:22:35 PDT
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.
Comment 35 Hiro 2006-05-19 07:37:36 PDT
(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 36 Mark Mentovai 2006-05-19 07:52:09 PDT
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 37 Josh Aas 2006-05-19 08:14:48 PDT
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 38 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-20 16:07:01 PDT
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!
Comment 39 Katsuhiro MIHARA 2006-05-20 22:53:12 PDT
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?
Comment 40 Mark Mentovai 2006-05-21 09:44:27 PDT
(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.
Comment 41 Mark Mentovai 2006-05-21 11:55:42 PDT
Created attachment 222792 [details] [diff] [review]
v3, review comments addressed, for checkin

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.
Comment 42 Mark Mentovai 2006-05-21 16:51:53 PDT
Created attachment 222816 [details] [diff] [review]
v4, this one's going in

Renamed HandleNKeyEvent to HandleKeyUpDownEvent to better reflect its function; relaxed one assertion in HandleKeyUpDownEvent to accept events without a charcode.
Comment 43 Mark Mentovai 2006-05-21 17:35:16 PDT
Checked in on trunk and MOZILLA_1_8_BRANCH for 1.8.1a3 (Fox 2a3).  Stay tuned for followups.
Comment 44 Mark Mentovai 2006-05-21 17:55:44 PDT
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.
Comment 45 Mark Mentovai 2006-05-21 20:17:35 PDT
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!
Comment 46 Mark Mentovai 2006-05-23 11:22:42 PDT
Created attachment 223068 [details] [diff] [review]
TSM carbonization

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.
Comment 47 Katsuhiro MIHARA 2006-05-24 20:23:32 PDT
(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.
Comment 48 Mark Mentovai 2006-05-24 21:27:18 PDT
(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.
Comment 49 Katsuhiro MIHARA 2006-05-25 20:36:34 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.