Closed Bug 610821 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P2)

Product:
Firefox Graveyard
Component:
Panorama
Platform:
x86
Windows XP
Type:
defect

Tracking

(blocking2.0 -)

Status:
RESOLVED FIXED
Milestone:
Firefox 4.0b10
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: nhirata, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: inputmethod, jp-critical, Whiteboard: [4b7])

Attachments

(6 files, 7 obsolete files)

Screenshot before typing in anything
633.17 KB, image/png
Details
Linux(gtk2)-widget patch v1.1
9.49 KB, patch
karlt
: review+
smaug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
Dispatch correct key events for testing v1.0.1
4.33 KB, patch
smaug
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
Windows-widget patch v2.2.1
15.45 KB, patch
masayuki
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
Patch for tabview v1.0.1
6.01 KB, patch
masayuki
: review+
johnath
: approval2.0+
Details | Diff | Splinter Review
follow up patch
937 bytes, patch
neil
: review+
johnath
: approval2.0+
Details | Diff | Splinter Review
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
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
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
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.
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.
Attached patch Win-widget patch (obsolete) — — Splinter Review
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.
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.
Attached patch Patch v2.0 (obsolete) — — Splinter Review
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)
Attachment #493342 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch for tabview v1.0 (obsolete) — — Splinter Review
this patch can fix this bug with the other patches.
Assignee: aza → masayuki
Status: NEW → ASSIGNED
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)
Attachment #493903 - Flags: review?(raymond) → review?(ian)
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)
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+
Attached patch Windows-widget patch v2.1 (obsolete) — — Splinter Review
fix some nits.
Attachment #493629 - Attachment is obsolete: true
Attachment #496781 - Flags: review?(jmathies)
Attachment #493629 - Flags: review?(jmathies)
Blocks: 595533
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.
Attached patch Windows-widget patch v2.2 (obsolete) — — Splinter Review
Attachment #496781 - Attachment is obsolete: true
Attachment #497505 - Flags: review?(jmathies)
Attachment #496781 - Flags: review?(jmathies)
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+
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?
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.
Attached patch follow up patch — — Splinter Review
Okay, let's fix this.
Attachment #504243 - Flags: review?(neil)
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+
Depends on: 628628
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.

Attachment

General

Creator:
Created:
Updated:
Size: