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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

defect
P1
normal
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: mccormmach, Assigned: masayuki)

Tracking

({inputmethod, intl, jp-critical})

Trunk
mozilla1.9alpha1
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

22.39 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
Reporter

Description

14 years ago
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.
Reporter

Comment 2

14 years ago
Attachment #206787 - Attachment is obsolete: true
Please coordinate with smaug's event dispatching rewrite...
Reporter

Comment 4

14 years ago
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.
Reporter

Comment 5

14 years ago
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.
Reporter

Comment 6

14 years ago
(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 (<smaug@welho.com>, 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?
> Not sure who "smaug" is (<smaug@welho.com>, 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.
(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.

Reporter

Comment 9

14 years ago
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?
Attachment #206809 - Attachment is obsolete: true
Reporter

Comment 10

14 years ago
Attachment #206892 - Attachment is obsolete: true
Attachment #206893 - Attachment is obsolete: true
Does that work with an HTML editor (eg with Midas/designMode)?
Reporter

Comment 12

14 years ago
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".
Reporter

Comment 13

14 years ago
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?)
Assignee: nobody → mccormmach
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 14

14 years ago
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

14 years ago
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?smaug@welho.com. 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).
I guess some dom/events person should sr this... maybe jst or bz
Reporter

Comment 17

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

added review?smaug@welho.com per :bs suggestion
Attachment #206947 - Flags: review?(smaug)
(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.
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.
Note that I'm going to vacation from now, I cannot reply until back to home.
I'll create new patch next week. Please check it.
I'm working on this bug in bugzilla-jp.
I'll attach the patch after testing in JA environment.
Assignee: mccormmach → masayuki
Status: NEW → ASSIGNED
Priority: -- → P1
Summary: Key press events captured by GTK2 input method/IME handlers not delivered to DOM/JS listeners → [GTK2] Implement nsIKBStateControl Interface (event of accesskeys and shortcut keys are eaten by IME)
Target Milestone: --- → mozilla1.9alpha
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Attachment #206947 - Attachment is obsolete: true
Attachment #206958 - Attachment is obsolete: true
Attachment #206964 - Attachment is obsolete: true
Attachment #212378 - Flags: superreview?(roc)
Attachment #212378 - Flags: review?(masaki.katakai)
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.
Posted patch Patch rv1.1 (obsolete) — Splinter Review
Attachment #212378 - Attachment is obsolete: true
Attachment #212754 - Flags: superreview?(roc)
Attachment #212754 - Flags: review?(masaki.katakai)
Attachment #212378 - Flags: superreview?(roc)
Attachment #212378 - Flags: review?(masaki.katakai)
+        // 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

14 years ago
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();
>+        }
Attachment #212754 - Flags: review?(masaki.katakai) → review-
(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.
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 on attachment 212754 [details] [diff] [review]
Patch rv1.1

Katakai-san:

See comment 29 and please review this again.
Attachment #212754 - Flags: review- → review?(masaki.katakai)
Comment on attachment 212754 [details] [diff] [review]
Patch rv1.1

Sorry. I misread the ESM code. It is called when the window is deactivated.
Attachment #212754 - Flags: review?(masaki.katakai) → review-

Comment 32

14 years ago
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.
Posted patch Patch rv1.2 (obsolete) — Splinter Review
Katakai-san:

I'm sorry. I had a mistake. The previous patch is not latest patch. I forgot patch command after removing the |ResetInputStateInternal|.
Attachment #212754 - Attachment is obsolete: true
Attachment #213046 - Flags: superreview?(roc)
Attachment #213046 - Flags: review?(masaki.katakai)
Attachment #212754 - Flags: superreview?(roc)
> I forgot
> patch command after removing the |ResetInputStateInternal|.

Oops. Sorry. This comment is wrong. I didn't attach the new patch in this bug...
Posted patch Patch rv1.3 (obsolete) — Splinter Review
Roc:

Please check this patch's comment. I answer to your question in this patch.
Attachment #213046 - Attachment is obsolete: true
Attachment #213293 - Flags: superreview?(roc)
Attachment #213293 - Flags: review?(masaki.katakai)
Attachment #213046 - Flags: superreview?(roc)
Attachment #213046 - Flags: review?(masaki.katakai)

Comment 36

14 years ago
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

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

OK for me, but please update comments so that other contributors can understand easily.
Attachment #213293 - Flags: review?(masaki.katakai) → review+
Posted patch Patch rv1.4Splinter Review
Sorry for the delay. I changed the comment, thanks.
Attachment #213293 - Attachment is obsolete: true
Attachment #214187 - Flags: superreview?(roc)
Attachment #214187 - Flags: review+
Attachment #213293 - Flags: superreview?(roc)
Comment on attachment 214187 [details] [diff] [review]
Patch rv1.4

thanks, that's great. sorry about the delay.
Attachment #214187 - Flags: superreview?(roc) → superreview+
checked-in, thanks.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
OK, so Ctrl+Shift+[0-9a-f] now work as shortcut keys? So the warning about them can be removed?
(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.
(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

14 years ago
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
Andrew:

Please file a new bug for any regressions. And please report more details of your environment.
I see the assertion anytime I click on anything as well; been meaning for a few days to track down who caused it.
I filed bug 330852, that's may my easy mistake. dbaron, could you review it? if you can review now.
You need to log in before you can comment on or make changes to this bug.