IME composition doesn't start by typing a character for searching a tab on Panorama

RESOLVED FIXED in Firefox 4.0b10

Status

P2
normal
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: nhirata, Assigned: masayuki)

Tracking

(Blocks: 1 bug, {inputmethod, jp-critical})

Trunk
Firefox 4.0b10
x86
Windows XP
inputmethod, jp-critical
Dependency tree / graph

Details

(Whiteboard: [4b7], URL)

Attachments

(6 attachments, 7 obsolete attachments)

633.17 KB, image/png
Details
9.49 KB, patch
karlt
: review+
smaug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
4.33 KB, patch
smaug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
15.45 KB, patch
masayuki
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
6.01 KB, patch
masayuki
: review+
johnath
: approval2.0+
Details | Diff | Splinter Review
937 bytes, patch
neil
: review+
johnath
: approval2.0+
Details | Diff | Splinter Review
Created attachment 489321 [details]
Screenshot before typing in anything

Mozilla/5.0 (Windows NT 5.1; rv:2.0b7) Gecko/20100101 Firefox/4.0b7

1. click on the tab candy icon
2. notice the IME
3. type

Expected: IME will start off in the IME that the browser is in, and will not change IME when typing
Actual: IME starts off in half width English after the first character it will automatically switch to Japanese.

Note:
1. Tested on VMware XP with Japanese language pack
Summary: Japanese IME starts off with Half-width English IME → Japanese version of Firefox starts off with Half Width English in Tab Candy

Comment 1

8 years ago
Confirmed. on Windows 7 + ATOK2006 and Windows XP + IME2003.

When I type mojira,
The first m is activated SearchBox of Tabcandy and m is inputed the box.
and following ojira are composition.
i.e. m[ojira]
Keywords: inputmethod

Updated

8 years ago
Whiteboard: [4b7]
This is same as bug 483122.

The first key event is probably redirected by js. However, that is wrong approach for IME users. This is UI design issue. Please change the design as:

1. The first key event only opens the editor and focus it.

or

2. The search box should be always visible and focus it when panorama is opened.
blocking2.0: --- → ?
Keywords: jp-critical

Comment 3

8 years ago
This is same ie. first character appears in english and from second input works fine:
Tested in:
-chinese IME,
-and with dead keys for example:in spanish keyboard layout,first input (é) fails.

Updated

8 years ago
Assignee: nobody → aza
Blocks: 608028
Priority: -- → P2
It's possible that bug 597399 will fix this; we were doing some manual key code manipulation, but now we're just using the char code straight out of the event.
(In reply to comment #4)
> It's possible that bug 597399 will fix this; we were doing some manual key code
> manipulation, but now we're just using the char code straight out of the event.

No, I think you don't understand this bug. If we want to fix this bug without the behavior change, we need to send the native key event to IME on all platforms. However, there is no such mechanism.
Created attachment 493182 [details] [diff] [review]
Win-widget patch

Mitcho:

This patch can redirect the native key event to IME on Windows 7. However, it's possible only when keydown event handler moves the focus. However, FAYT and Panorama use keypress event handler for moving focus. Therefore, it doesn't work fine only with this patch.

> data:text/html,<body onkeydown="document.getElementById('p').focus();"><input id="p">
# succeeded on this testcase
So, can somebody change the Panorama's code for this behavior? If so, we can fix this bug without UI redesigning.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/search.js#334

However, there are some i18n problem. The code looks the charCode of the keypress event whether it's alphabet or digit key. On Kana input mode of Japanese IME, the first key can be '-', '^', '\', ';', ':', ']', ',', '.' or '/' on JIS keyboard layout. So, for i18n, any character input should cause starting the search.

I think that special keys which are not used for character input like arrow keys should be in a blacklist and the keydown handler shouldn't start the search at these keyCodes.
Created attachment 493338 [details] [diff] [review]
Linux(gtk2)-widget patch
Created attachment 493342 [details] [diff] [review]
Linux(gtk2)-widget patch v1.1

The approach of this patch is useful for some web applications too. Even if this bug isn't fixed by this way, we should take this patch on trunk.

When keydown event handler moves focus to editor from non-IME usable element, this patch redirect the native key event to IME. Then, the DOM events will fired in the focused editor as following order:

When IME isn't opened:
1. focus
2. keypress
3. keyup

When IM is opened:
1. focus
2. compositionstart

So, the keydown event isn't dispatched on new focused editor.
Attachment #493338 - Attachment is obsolete: true
Attachment #493342 - Flags: review?(karlt)
Attachment #493342 - Flags: review?(Olli.Pettay)
I'll update Win's patch due to some bugs.

I guess that Mac doesn't need any changes. I'll confirm it.

And I should note that the steps of the tests for my patch are:

1. open the URL
2. type a key on <body>
3. then, focus will be moved to editor and the pressed key's character will be inputted. if IME is opened, composition will be started instead of inputting a row character.
Created attachment 493629 [details] [diff] [review]
Patch v2.0

This patch redirects WM_(SYS)KEYDOWN message by SendInput() when gecko receives it during IME disabled but keydown event handler moves focus to IME enabled editor.

SendInput() returns asynchronously. Therefore, we should record the redirected message for preventing unnecessary keydown event dispatching.

However, there are some bugs on current trunk. I fix these bugs by this patch because it makes inconsistent behavior with Linux (gtk2).

1. When a keydown causes starting composition, the WPARAM is VK_PROCESSKEY. But we don't compute a correct virtual keycode by ImmGetVirtualKey().

2. When WPARAM is VK_PROCESSKEY, the key event is consumed by IME, so, we shouldn't send it as keypress event.
Attachment #493182 - Attachment is obsolete: true
Attachment #493629 - Flags: review?(jmathies)

Updated

8 years ago
Attachment #493342 - Flags: review?(Olli.Pettay) → review+
Created attachment 493903 [details] [diff] [review]
Patch for tabview v1.0

this patch can fix this bug with the other patches.
Assignee: aza → masayuki
Status: NEW → ASSIGNED
Created attachment 493904 [details] [diff] [review]
Dispatch correct key events for testing v1.0

EventUtils.js sends wrong key events.

keydown and keyup events shouldn't be set charCode but should be always set keyCode.

keypress event shouldn't be set keyCode if it has charCode. Otherwise, it should be set keyCode.
Attachment #493904 - Flags: review?(Olli.Pettay)
Attachment #493342 - Flags: review?(karlt) → review+
Comment on attachment 493903 [details] [diff] [review]
Patch for tabview v1.0

I confirmed that these patches fixes this bug on Win, Linux (gtk2) and Mac. (Mac doesn't need widget patch.)

We should use keydown event for moving focus to the editor. Then, new widget code sends keypress event or composition events to the new focused editor.

This patch also fixes bug 593904.
Attachment #493903 - Flags: review?(raymond)
Blocks: 593904, 483122
Attachment #493903 - Flags: review?(raymond) → review?(ian)
Created attachment 493918 [details] [diff] [review]
Dispatch correct key events for testing v1.0.1

fix a nit, sorry for the spam.
Attachment #493904 - Attachment is obsolete: true
Attachment #493918 - Flags: review?(Olli.Pettay)
Attachment #493904 - Flags: review?(Olli.Pettay)
Blocks: 615879
No longer blocks: 483122
Summary: Japanese version of Firefox starts off with Half Width English in Tab Candy → Doesn't start IME composition when type a character for searching a tab on Panorama
Summary: Doesn't start IME composition when type a character for searching a tab on Panorama → IME composition doesn't start by typing a character for searching a tab on Panorama
Comment on attachment 493903 [details] [diff] [review]
Patch for tabview v1.0

Does it still work (in English) that the first key press (the one that activates the search box) goes into the search box? Does it do so reliably?
(In reply to comment #16)
> Comment on attachment 493903 [details] [diff] [review]
> Patch for tabview v1.0
> 
> Does it still work (in English) that the first key press (the one that
> activates the search box) goes into the search box? Does it do so reliably?

Yes. If nobody calls preventDefault() in keydown event handler, widget will generate keypress event. Then, the event causes character input on focused editor.
> If nobody calls preventDefault() in keydown event handler, widget will
> generate keypress event.

Sorry, this is incorrect. keypress event is always dispatched except when IME composition is starting by the keydown event or the widget is destroyed by the keydown event handler.

However, if preventDefault() of the keydown event is called, keypress event has prevent-default flag. Then, editor will ignore the keypress event. Therefore, if preventDefault() of keydown event isn't called, keypress event will cause inputting a character on new focused editor.
Comment on attachment 493903 [details] [diff] [review]
Patch for tabview v1.0

Looks good to me, then.
Attachment #493903 - Flags: review?(ian) → review+
jimm, smaug:

would you review the patches?
Comment on attachment 493918 [details] [diff] [review]
Dispatch correct key events for testing v1.0.1

I don't have us keyboard, so can't verify _computeKeyCodeFromChar.
(perhaps I could search for the layout)

But anyway, this is test-only.
Attachment #493918 - Flags: review?(Olli.Pettay) → review+
Created attachment 496781 [details] [diff] [review]
Windows-widget patch v2.1

fix some nits.
Attachment #493629 - Attachment is obsolete: true
Attachment #496781 - Flags: review?(jmathies)
Attachment #493629 - Flags: review?(jmathies)
Blocks: 595533

Comment 23

8 years ago
Comment on attachment 496781 [details] [diff] [review]
Windows-widget patch v2.1


>+  // If this method doesn't call OnKeyDown(), this method must clean up the
>+  // redirected message information itself.
>+  struct AutoForgetRedirectedKeyDownMessage
>+  {
>+    AutoForgetRedirectedKeyDownMessage(nsWindow* aWindow, const MSG &aMsg) :
>+      mCancel(!nsWindow::IsRedirectedKeyDownMessage(aMsg)),
>+      mWindow(aWindow), mMsg(aMsg)
>+    {
>+    }
>+
>+    ~AutoForgetRedirectedKeyDownMessage()
>+    {
>+      if (mCancel) {
>+        return;
>+      }
>+      // Prevent unnecessary keypress event
>+      if (!mWindow->mOnDestroyCalled) {
>+        nsWindow::RemoveNextCharMessage(mWindow->mWnd);
>+      }
>+      // Foreget the redirected message
>+      nsWindow::ForgetRedirectedKeyDownMessage();
>+    }
>+
>+    PRBool mCancel;
>+    nsCOMPtr<nsWindow> mWindow;
>+    const MSG &mMsg;
>+  };

Can we move this out of the method, maybe define it above or move it to the header?

>+
>+    // If IMC wasn't associated to the window but is associated it now,
>+    // that means the keydown event causes moving focus to an editor.

Trying to understand this.. we send the key down message and during this the focus changes to a text editor. Is that correct? If so can we make that clear here, the code that's coming up is confusing without a clear explanation of what we're testing for.
 
>+    // Then, WM_CHAR and WM_SYSCHAR shouldn't cause first character inputting
>+    // if IME is opened.  We should redirect the native keydown message to IME.

We just sent the key down and only do this redirect if the dispatch returns !noDefault? We should make this clear here in the comments. I guess what I'm looking for in this comment of an explanation of the problem we're fixing with this code.
 
>+    // However, if the current focused window isn't ours, we shouldn't redirect
>+    // the message because Windows will send the message to the focused window.
>+    // So, the non-our window shouldn't receive the our hacky message.

Please clean this last comment up.


>+    HWND focusedWnd = ::GetFocus();
>+    if (!noDefault && !aFakeCharMessage && oldIMC && !mOldIMC && focusedWnd &&
>+        !PluginHasFocus()) {
>+      RemoveNextCharMessage(focusedWnd);


What happens if there's no char message pending, I guess this would be passive but we are about to send a char message regardless.
(In reply to comment #23)
> >+
> >+    // If IMC wasn't associated to the window but is associated it now,
> >+    // that means the keydown event causes moving focus to an editor.
> 
> Trying to understand this.. we send the key down message and during this the
> focus changes to a text editor. Is that correct?

Yes.

> >+    // Then, WM_CHAR and WM_SYSCHAR shouldn't cause first character inputting
> >+    // if IME is opened.  We should redirect the native keydown message to IME.
> 
> We just sent the key down and only do this redirect if the dispatch returns
> !noDefault? We should make this clear here in the comments. I guess what I'm
> looking for in this comment of an explanation of the problem we're fixing with
> this code.

I don't think we should prevent the IME behavior when noDefault is TRUE.

Because:

1. Other platforms are not so on current build (and earlier versions too).
2. If an editor has focus, i.e., IM context is associated to the window, the WM_KEYDOWN message comes *after* IME starts composition. If we want to prevent the composition, we need some complicated code for Windows. I don't think that is worth.

# I agree that the behavior may be better for DOM event specification. However, if some application tries to prevent composition at keydown event handler, it doesn't work fine on gtk2 build. Anyway, that should be out of scope of this bug.

> >+    // However, if the current focused window isn't ours, we shouldn't redirect
> >+    // the message because Windows will send the message to the focused window.
> >+    // So, the non-our window shouldn't receive the our hacky message.
> 
> Please clean this last comment up.

If we redirect the message, the keydown message causes two character inputs. One is received by us and it causes focus switching to another application's window. And the new focused window will receive same input message. So, it makes the user be confused.

> >+    HWND focusedWnd = ::GetFocus();
> >+    if (!noDefault && !aFakeCharMessage && oldIMC && !mOldIMC && focusedWnd &&
> >+        !PluginHasFocus()) {
> >+      RemoveNextCharMessage(focusedWnd);
> 
> 
> What happens if there's no char message pending, I guess this would be passive
> but we are about to send a char message regardless.

If char message isn't in the queue, nothing happens. A char message is posted by TranslateMessage API for WM_KEYDOWN. Therefore, if the pressed key causes a character input, the WM_CHAR message must be in the queue here. And OnKeyDown() assumes this. So, I think that there is no problem about this.
Created attachment 497505 [details] [diff] [review]
Windows-widget patch v2.2
Attachment #496781 - Attachment is obsolete: true
Attachment #497505 - Flags: review?(jmathies)
Attachment #496781 - Flags: review?(jmathies)

Comment 26

8 years ago
Comment on attachment 497505 [details] [diff] [review]
Windows-widget patch v2.2

+  // RemoveNextCharMessage() should be called by WM_KEYDOWN or WM_SYSKEYDOWM
+  // message handler.  If there is no WM_(SYS)CHAR message for it, this
+  // method does nothing.
+  // NOTE: WM_(SYS)CHAR message is posted by TranslateMessage() API which is
+  // called in message loop.  So, WM_(SYS)KEYDOWN message should have
+  // WM_(SYS)CHAR message in the queue if the keydown event causes character
+  // input.

Let's move that into the cpp on top of RemoveNextCharMessage.

Sorry for the delay on the reviews!
Attachment #497505 - Flags: review?(jmathies) → review+
Created attachment 497709 [details] [diff] [review]
Windows-widget patch v2.2.1

No problem, thank you for your review!
Attachment #497505 - Attachment is obsolete: true
Attachment #497709 - Flags: review+
Attachment #493342 - Flags: approval2.0?
Attachment #493903 - Flags: approval2.0?
Attachment #493918 - Flags: approval2.0?
Attachment #497709 - Flags: approval2.0?
These patches have some minor risk. If keydown event handler moves focus to an editor, it may cause IME composition starting on the new focused editor unexpectedly.

However, Fx3.6 or earlier Mac build works as so. So, if some web applications will be in trouble by this change, they are being in trouble on Mac build already.

So, actually, these patches don't have serious risk. But they improves the search UI of Panorama. And these changes will be needed for bug 615879.
blocking2.0: ? → -
Decided that this won't block the release, as the search button still works. Sorry!
(In reply to comment #29)
> Decided that this won't block the release, as the search button still works.
> Sorry!

Hmm, then, why is there such feature?
Created attachment 498080 [details] [diff] [review]
Patch for tabview v1.0.1

updating for latest code.
Attachment #493903 - Attachment is obsolete: true
Attachment #498080 - Flags: review+
Attachment #498080 - Flags: approval2.0?
Attachment #493903 - Flags: approval2.0?
Johnathan: ping
Attachment #498080 - Flags: approval2.0? → approval2.0+
I'm going to assume that approval was for all patches that were requested; yes?
(In reply to comment #33)
> I'm going to assume that approval was for all patches that were requested; yes?

I believe so.
Attachment #493342 - Flags: approval2.0? → approval2.0+
Attachment #493918 - Flags: approval2.0? → approval2.0+
Attachment #497709 - Flags: approval2.0? → approval2.0+
Yes, but Jonathan didn't agree to the widget code change for now.
Masayuki and I agreed on IRC that he would follow up with Johnath to see how he felt about the risk in the widget code, just to make sure.
Comment on attachment 497709 [details] [diff] [review]
Windows-widget patch v2.2.1

>+    nsCOMPtr<nsWindow> mWindow;
This is WRONG. Did you mean nsRefPtr<nsWindow>?
(In reply to comment #38)
> Comment on attachment 497709 [details] [diff] [review]
> Windows-widget patch v2.2.1
> 
> >+    nsCOMPtr<nsWindow> mWindow;
> This is WRONG. Did you mean nsRefPtr<nsWindow>?

I'm not sure what is wrong. What's the difference between nsCOMPtr and nsRefPtr? I just need to hold the pointer strongly, so, I think nsRefPtr can work fine for that too.
I found this document.
http://groups.google.com/group/netscape.public.mozilla.xpcom/browse_frm/thread/77258f1cd7d99773/268b949066f2ba4a?hl=de&q=#268b949066f2ba4a

I guess that the WRONG in your comment meant that using nsCOMPtr for non-abstract class breaks the rule even if the class has default constructor. I'll post a follow up patch when I'll get up.
(In reply to comment #40)
> I found this document.
> http://groups.google.com/group/netscape.public.mozilla.xpcom/browse_frm/thread/77258f1cd7d99773/268b949066f2ba4a?hl=de&q=#268b949066f2ba4a
I think that predates nsRefPtr, which was invented to address the problem of automatically maintaining a strong reference to a concrete object, thus allowing nsCOMPtr to be reserved for references to interfaces.

Also the bug was only half your fault because someone botched the interface inheritance on nsBaseWidget so that the QI fails.
Created attachment 504243 [details] [diff] [review]
follow up patch

Okay, let's fix this.
Attachment #504243 - Flags: review?(neil)

Updated

8 years ago
Depends on: 626178
Comment on attachment 504243 [details] [diff] [review]
follow up patch

Thanks.
Attachment #504243 - Flags: review?(neil) → review+
Comment on attachment 504243 [details] [diff] [review]
follow up patch

This fixes a simple mistake of the windows-widget patch. There was no problem on tryserver.
Attachment #504243 - Flags: approval2.0?
Attachment #504243 - Flags: approval2.0? → approval2.0+

Updated

8 years ago
Depends on: 628628

Updated

8 years ago
No longer depends on: 628628
No longer depends on: 629846
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.