321468 - [GTK2] Implement nsIKBStateControl Interface (event of accesskeys and shortcut keys are eaten by IME)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P1)

Description

[Alexander McCormmach]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla XULRunner 1.9a1 (CVS HEAD as of about 25 Dec 2005 08:01:17 -0000)

At least with an en-US locale, GTK2 allows Unicode characters to be entered into input fields by hex codepoint value. This is done by typing the hex number (using 0-9, A-F) while holding down the Ctrl and Shift keys. For instance, the copyright sign, ©, may be entered with Ctrl+Shift+A+9. (In fact I just did so in the previous sentence, using this very functionality in Firefox on Bugzilla's "Enter a Bug" page.)

Such key sequences are captured by GTK2's input-method handling code (specifically, gtk_im_context_filter_keypress), called from nsWindow::IMEFilterEvent. Unfortunately, nsWindow::OnKeyPressEvent simply returns TRUE in these cases, and never calls nsCommonWidget::DispatchEvent to send the event down the event stack, so that DOM/JS event handlers can see/cancel it.

This is bad because, while the default handling of such key sequences is nice as a default, XUL application developers (using xulrunner or even from an extension) should be able at least to *see* these events from within JS, and ideally should be able to prevent the IME handling of them using nsIDOMEvent::preventDefault. In general, IME handling of an event should only occur *after* DOM processing, and then only if the event hasn't been canceled.

Reproducible: Always

Steps to Reproduce:
(Using GTK2-based xulrunner or XUL-based app, of course.)

1. Register a keypress event listener on a XUL window containing a text input field. Have the listener send up an alert popup whenever it receives a key event.
2. In the input field, type Ctrl+Shift+..., where ... is any of 0-9, A-F.

Actual Results:  
You don't get any alert popup for the IME-captured events.

Expected Results:  
You should have gotten an alert popup for each key event. IME processing should have occurred only if preventDefault wasn't called on the event object.

I have a patch which allows a platform-specific event pointer and IME-handling function to be attached to an nsKeyEvent. These are called upon, if present, within nsGenericElement::HandleDOMEvent, after the event has been sent through the DOM event stack, but before the "local handling stage." It works and it should maintain platform-neutrality; however, being as this is my first attempt at hacking the Mozilla source, I wouldn't be surprised if there's a better way to fix the problem.

Comment 1

[Alexander McCormmach]

Attachment: patch described in the original bug report

Comment 2

[Alexander McCormmach]

Attachment: same patch as above, only with more context (cvs diff -U6 -p)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Please coordinate with smaug's event dispatching rewrite...

Comment 4

[Alexander McCormmach]

Attachment: screenshot of KeyListener session before applying the proposed patch

KeyListener is a small XULRunner app I wrote which simply listens to the keypress events received by a textbox, reporting out some details of each intercepted event in a table. The default action is allowed to take place after the table is updated (i.e., preventDefault is not called on the event). In the screenshot, the rightwards arrow and therefore symbols were entered using the Ctrl+Shift+[hex codepoint] method (rightwards arrow = 2192, therefore = 2234). Notice how no events are reported in the table for those keypresses.

Comment 5

[Alexander McCormmach]

Attachment: same KeyListener session, but after applying the proposed patch to widget/gtk2

Notice how the program sees and reports on the four keypresses required to produce each of the two special characters using GTK2's Unicode input method.

Comment 6

[Alexander McCormmach]

(In reply to comment #3)
> Please coordinate with smaug's event dispatching rewrite...
> 

Okay, happy to help if I can. Not sure who "smaug" is (<[e-mail]>, I'm assuming) or how to get some of these fixes into the rewrite. Are there progress-tracking bugs in the database for this effort?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Not sure who "smaug" is (<[e-mail]>, I'm assuming)

Yep.

> Are there progress-tracking bugs in the database for this effort?

There are various bugs that smaug's working on fixing, yes.  But I think he's still finishing up the planning stage, so you should talk to him about what changes you want to make to basic event propagation and how they would affect the design.  In particular, the method you're modifying is going to go away, so the code needs to go somewhere else; the question is where.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> 
> There are various bugs that smaug's working on fixing, yes.  But I think he's
> still finishing up the planning stage, so you should talk to him about what
> changes you want to make to basic event propagation and how they would affect
> the design.  In particular, the method you're modifying is going to go away, so
> the code needs to go somewhere else; the question is where.
> 

HandleDOMEvent is going away, but I think (keyEvent->IMECheckFunction)(keyEvent)
could be used in PreHandleEvent or PostHandleEvent (which I'm going to add).
Most probably in PostHandleEvent because that is called after normal DOM 
dispatching

In the current patch I don't quite understand why the IME handling is after capture but before target handling? Is there a reason for this?
Could it be before capture or after bubbling?
Actually "In general, IME handling of an event should only
occur *after* DOM processing, and then only if the event hasn't been canceled."
...so after bubbling.

Comment 9

[Alexander McCormmach]

Attachment: moved IME checking to nsTextEditorKeyListener::KeyPress

Alright, looking over the comments, I realized that the original call to IMECheckFunction is indeed in the wrong place. My intention was to catch un-handled events in the system pass (after capture, local handling, and bubbling had taken place in the client pass), but I see my addition to nsGenericElement::HandleDOMEvent didn't check for which pass it was in. Not sure it even could have at that level in the stack.

This new patch moves the checking to a much more obviously salient part of the codebase, nsTextEditorKeyListener::KeyPress. Works just as well and perhaps sidesteps Smaug's event-dispatch rewrite. How does it look to y'all?

Comment 10

[Alexander McCormmach]

Attachment: screenshot of another KeyListener session just to show things are working after the latest patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Does that work with an HTML editor (eg with Midas/designMode)?

Comment 12

[Alexander McCormmach]

Attachment: KeyListener session when textbox is replaced with editor

Yes, as this screenshot shows, IME handling is correct even if the textbox is replaced with an editor whose contentDocument's designMode property is set to "On".

Comment 13

[Alexander McCormmach]

Has anyone had a chance to confirm this problem, and/or try out the most recent patch (from 12/27)? (In other words, is the status still UNCONFIRMED?)

Comment 14

[Alexander McCormmach]

Thanks, bzbarsky, for confirming this. I see it's been "assigned to" me. I'm not sure what that entails, especially since I don't have the power to resolve this issue by committing a patch or whatever. The most recent proposed patch is pretty satisfying to me, particularly because it only modifies code on either end of the event stack (unlike my original proposal which tinkered inside nsGenericElement). It shouldn't conflict with any rewrite of the event-dispatching code, and could even be a good candidate for backporting to the 1.8 branch. Furthermore, the addition of IMECheckFunction, etc., to nsKeyEvent lays the groundwork for any similar handling on non-GTK2 platforms.

So my question is, what's next? Is there anything more I can do to help this issue along to the point where a patch (whether my most recent one, or a subsequent one) can be committed? Would it be worth me checking out the 1.8 branch and posting the patch for a similar fix against that? 

Thanks for any insight into the resolution process!

Comment 15

[Benjamin Smedberg]

Alexander, the next step is to get the appropriate module owner to review the patch. You need to "edit" the patch attachment and set the review? flag with an appropriate email. In this case I suggest review?[e-mail]. You will probably also need a superreview, but we can worry about that later (I'm not quite sure who else should be reviewing/superreviewing this).

Comment 16

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

I guess some dom/events person should sr this... maybe jst or bz

Comment 17

[Alexander McCormmach]

Comment on attachment 206947 [details] [diff] [review]
moved IME checking to nsTextEditorKeyListener::KeyPress

added review?[e-mail] per :bs suggestion

Comment 18

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #14)
> Thanks, bzbarsky, for confirming this. I see it's been "assigned to" me. I'm
> not sure what that entails

All that means is that you're the one working on it at the moment.  ;)

> So my question is, what's next?

This has been covered pretty well (I'd suggest jst for the sr, myself), but you may also want to take a look at http://www.mozilla.org/hacking/life-cycle.html to get a general idea of how things go.

> Would it be worth me checking out the 1.8 branch and posting the patch for a
> similar fix against that? 

Worth figuring out whether the change is "compatible enough" for branch first, I think.

Comment 19

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

Excuse me, is this a "bug"?
I think that this is not a bug.
When gtk_im_context_filter_keypress returns true, I think that the key press event should not be fired. Because in that case, the IME composition events may be fired. The XUL application should use the IME events.

FYI:
On Windows, in IME transaction, the key events are never fired.
Current code is compatible on other OS's nsWindow/nsWidget.
But your patch isn't so.

I think that you want to disable IME on non editor widget, right?
If so, we should use another approach that is using nsIKBStateControl interface.
See bug 55751.
I have created the XP mechanism for controling IME state on Gecko1.9.
# On current trunk builds for Windows, we cannot use IME on non editor widgets.

I cannot accept current patch.

Comment 20

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

Note that I'm going to vacation from now, I cannot reply until back to home.

Comment 21

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

I'll create new patch next week. Please check it.

Comment 22

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

I'm working on this bug in bugzilla-jp.
I'll attach the patch after testing in JA environment.

Comment 23

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

Attachment: Patch rv1.0

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 212378 [details] [diff] [review]
Patch rv1.0

!!IMEComposingWindow()

Just use "!= nsnull".

Why are you using NULL instead of nsnull?

Please document somewhere which window(s) hold an mIMEData, and when. Also document which window is referenced by mComposingWindow, and when.

Comment 25

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

Attachment: Patch rv1.1

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        // Actual context. This is used for handling the user's inputting.
"This is used for handling the user's input."

+        // Dummy context. This is used for a trick. On some environment, the
+        // status feedback for the user isn't changing dynamic when the IM
+        // conetext lost the focus. Therefore, in a moment, this gets the focus
+        // at changing the IM state. (And losing the focus immediately.)
I'm not really sure what this means. Can you explain in more detail what situation causes the problem, and how you use the dummy context to fix the problem? Once I understand what is going on, I'll fix up the English :-)

+        // The composing window. This refers the composing window from starting
+        // composition to ending the composition.

The "composing window" is the window that was focused when composition started, where the final input will be sent?

+        PRBool             mEnabled;

PRPackedBool

Comment 27

[Masaki Katakai]

Comment on attachment 212754 [details] [diff] [review]
Patch rv1.1


>+    if (focusedIm && focusedIm == window->mIMEData->mContext) {
>+        // Release current IME focus if IME is enabled.
>+        if (window->mIMEData->mEnabled) {
>+            focusedWin->ResetInputStateInternal();

This focusedWin->ResetInputStateInternal() should not be called until bug 327003 is fixed,
because we don't expect reset when the input focus is back to Mozilla from IM candidate widow,
composing text is still there.

bug 327003 should handle two cases,
- input focus to another
- input focus back to mozilla from another

>+            focusedWin->IMELoseFocus();
>+        }

Comment 28

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

(In reply to comment #27)
> (From update of attachment 212754 [details] [diff] [review] [edit])
> 
> >+    if (focusedIm && focusedIm == window->mIMEData->mContext) {
> >+        // Release current IME focus if IME is enabled.
> >+        if (window->mIMEData->mEnabled) {
> >+            focusedWin->ResetInputStateInternal();
> 
> This focusedWin->ResetInputStateInternal() should not be called until bug
> 327003 is fixed,
> because we don't expect reset when the input focus is back to Mozilla from IM
> candidate widow,
> composing text is still there.
> 
> bug 327003 should handle two cases,
> - input focus to another
> - input focus back to mozilla from another
> 
> >+            focusedWin->IMELoseFocus();
> >+        }
> 

Katakai-san, I don't think so. In this point, it may not be related bug 327003. Because |SetIMEEnabled| is called at the all elements getting the focus. *However*, if the IME enabled state is not changing, this returns in early time in this function. So, in this point, the previous IME state is always disabled.

Comment 29

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

Oops, sorry. Please ignore my previous comment.

In this point, the previous IME state is 'enabled', and it is becoming 'disabled'. In this time, we should always reset the input state. Because when |SetIMEEnabled(PR_FALSE)| is called, it may be only setting the focucs to a non-IME-usable element. Please check it.

> Katakai-san, I don't think so. In this point, it may not be related bug 327003.
> Because |SetIMEEnabled| is called at the all elements getting the focus.
> *However*, if the IME enabled state is not changing, this returns in early time
> in this function. So, in this point, the previous IME state is always disabled.

Comment 30

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

Comment on attachment 212754 [details] [diff] [review]
Patch rv1.1

Katakai-san:

See comment 29 and please review this again.

Comment 31

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

Comment on attachment 212754 [details] [diff] [review]
Patch rv1.1

Sorry. I misread the ESM code. It is called when the window is deactivated.

Comment 32

[Masaki Katakai]

Hi Nakano-san,

Try the following operation, we can simulate how IM candidate window works when
it grabs input focus. It seems that those codes are called unexpectedly,

1. start mozilla with web page that has text field e.g. yahoo
2. click on the text field
3. turn IME on
  Now the text field has input focus and IME conversion mode is ON
4. click another app on the desktop
  the input focus will lose the input focus
5. click the title of mozilla window
  focus should back to the text field

At the re-focusing to the text field, those codes are called so reset()
is called unexpectedly. So I mean when focus is back to Mozilla from
others, I wanted to avoid such reset call. 

Can you avoid such call? SetIMEEnabled(PR_FALSE) seems to be called
for another widget, it's OK, but we should not call ResetInputStateInternal()
in this scenario because IME already is turned on.

Comment 33

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

Attachment: Patch rv1.2

Katakai-san:

I'm sorry. I had a mistake. The previous patch is not latest patch. I forgot patch command after removing the |ResetInputStateInternal|.

Comment 34

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

> I forgot
> patch command after removing the |ResetInputStateInternal|.

Oops. Sorry. This comment is wrong. I didn't attach the new patch in this bug...

Comment 35

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

Attachment: Patch rv1.3

Roc:

Please check this patch's comment. I answer to your question in this patch.

Comment 36

[Masaki Katakai]

Comment on attachment 213293 [details] [diff] [review]
Patch rv1.3

> +        // Dummy context. This is used for a trick. On some environment, the
> +        // toolbar of IM doesn't update when the context loses the focus.
> +        // Because they updates the toolbar only when the context *gets* the
> +        // focus. Therefore, we set the focus to this context for force

Nakano-san, what is toolbar? There are some words - IM palette,
IM status window, IM status bar, etc. Which word is widely used?
I think "IM status window" is more common than "toolbar of IM".

mDummyContext is a dummy context and will be used in IMESetFocus()
when mIMEData->mEnabled=false. This mDummyContext IM state is always "off",
so it works to switch conversion mode to OFF on IM status window.

> +        // updating the toolbar. However, this context will lose the focus
> +        // immediately. Because UIM and SCIM are snooping the key events
> +        // for handling the user's input while a context is having the focus.

These comments already in IMESetFocus(), so I don't think
we need them here.

> +        GtkIMContext       *mDummyContext;

> +        // The composing window (or widget). The window that is referred by
> +        // this has the IM transaction. When the transaction is finished,
> +        // this will be set to NULL. So, If this is NULL, there is not IM
> +        // trunsanction.
> +        nsWindow           *mComposingWindow;

This mComposingWindow is set in IMEComposeStart(), when user
starts composition, then unset in IMEComposeEnd() when user
ends the composition. We will keep the widget where the
actual composition is started.

During the composition, we may get some events like ResetInputStateInternal()
and CancelIMECompositionInternal() by changing input focus,
we will use the original widget of mComposingWindow to commit or
reset the composition.

Comment 37

[Masaki Katakai]

Comment on attachment 213293 [details] [diff] [review]
Patch rv1.3

OK for me, but please update comments so that other contributors can understand easily.

Comment 38

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

Attachment: Patch rv1.4

Sorry for the delay. I changed the comment, thanks.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 214187 [details] [diff] [review]
Patch rv1.4

thanks, that's great. sorry about the delay.

Comment 40

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

checked-in, thanks.

Comment 41

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

OK, so Ctrl+Shift+[0-9a-f] now work as shortcut keys? So the warning about them can be removed?

Comment 42

[[e-mail]]

(In reply to comment #41)
>OK, so Ctrl+Shift+[0-9a-f] now work as shortcut keys?
>So the warning about them can be removed?
Sure, if you're willing to break the IME functionality.

Comment 43

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

(In reply to comment #41)
> OK, so Ctrl+Shift+[0-9a-f] now work as shortcut keys? So the warning about them
> can be removed?
> 

Probably, not. Because this patch only affects on out of editors.
I.e., on the editors, nothing was changed by the patch.

Comment 44

[Andrew Schultz]

I'm hitting the assertion from the latest patch anytime I click on anything:

###!!! ASSERTION: CancelIMEComposition is already called!: 'gIMESuppressCommit', file /build/andrew/moz-debug/mozilla/widget/src/gtk2/nsWindow.cpp, line 4953

Comment 45

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

Andrew:

Please file a new bug for any regressions. And please report more details of your environment.

Comment 46

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

I see the assertion anytime I click on anything as well; been meaning for a few days to track down who caused it.

Comment 47

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

I filed bug 330852, that's may my easy mistake. dbaron, could you review it? if you can review now.

Comment 48

[Boris Zbarsky [:bzbarsky]]

This caused crash bug 337036