We should implement a hack for Google Japanese Input. Google Japanese Input retrieves the first character rect of changing string for showing candidate window but the position is sometimes wrong.

In most cases, Google Japanese Input for Win uses first character of selected clause of composition string for deciding candidate window position. This patch implements a hack for it. If the retrieved character is still not laid out, this returns the caret before the selected clause. I'm not sure if Mozc also has same bug and its TIP name is same as "Google 日本語入力".

Google Japanese Input sometimes uses caret rect for deciding the position of candidate window. I'm not sure why this sometimes occurs, though. This patch returns the last caret offset which is the nearest position from it. I think that ideally, we should return the first character rect of the selected clause. But it's too difficult for us. Therefore, this uses the position instead.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1) > I'm not sure if Mozc also has same bug and its TIP name is same as "Google > 日本語入力". Sorry for the trouble. I believe this issue has been fixed in Mozc 1.15.1785.102 so you don't need to introduce workaround at least for Mozc. https://code.google.com/p/mozc/wiki/ReleaseHistory#2014-05-16_1.15.1785.102 I can confirm that Mozc 1.15.1872.100 + Firefox Nightly 34.0a1 (2014-09-02) are working nicely on Windows 8.1 with all the recommended Windows update as of today. As for Google Japanese Input, unfortunately the latest version is a bit old due to a number of reasons in release logistics. Hopefully the next release will contain the same fix but I cannot provide an ETA. BTW, I'll visit Osaka next weekend to hear you talk at the FxOS event! We can talk in parson as for detailed technical issues.

Yukawa-san, thank you for the information. When we make Aurora TSF-aware in default settings but Google Japanese Input still has this bug, we should include this workaround. So, the ETA is important to us. Anyway, we should talk about this in this weekend! I'm glad to see you!

Comment on attachment 8482777 [details] [diff] [review] part.1 nsTextStore::GetTextExt() should rReturn previous character rect instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first character of selected clause at composing Review of attachment 8482777 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for my delayed response. The workaround logic looks correct. One minor suggestion that may improve the availability of your workaround mechanism. ::: widget/windows/nsTextStore.cpp @@ +1165,5 @@ > #define TIP_NAME_EASY_CHANGJEI \ > (NS_LITERAL_STRING( \ > "\x4E2D\x6587 (\x7E41\x9AD4) - \x6613\x9821\x8F38\x5165\x6CD5")) > +#define TIP_NAME_GOOGLE_JA_INPUT \ > + (NS_LITERAL_STRING("Google \x65E5\x672C\x8A9E\x5165\x529B")) I'm a little concerned that this works only when the display language is Japanese. IIRC, you will receive "Google Japanese Input" instead in other language settings because Google Japanese Input and Mozc are calling ITfInputProcessorProfilesEx::SetLanguageProfileDisplayName. I guess guidProfile returned from ITfInputProcessorProfiles::GetLanguageProfileDescription would be more stable and reliable identifier because it is considered to be locale independent. References: - http://msdn.microsoft.com/en-us/library/windows/desktop/ms629028.aspx - https://code.google.com/p/mozc/source/browse/trunk/src/win32/base/tsf_registrar.cc?r=185#222

Good catch! I'll check it, thank you, Yukawa-san!

> I'm a little concerned that this works only when the display language is Japanese. IIRC, you > will receive "Google Japanese Input" instead in other language settings because Google Japanese > Input and Mozc are calling ITfInputProcessorProfilesEx::SetLanguageProfileDisplayName. I was concerned about this too at writing the patches of Traditional Chinese TIPs. However, I have no way to check it on Traditional Chinese Windows because my MSDN license has already been expired and I've not downloaded it. On the other hand, and similarly, I don't have English Windows 8 for checking about Google Japanese Input's TIP name :-( > I guess guidProfile returned from ITfInputProcessorProfiles::GetLanguageProfileDescription > would be more stable and reliable identifier because it is considered to be locale independent. This sounds better for creating whitelist, but I worry this for creating blacklist because isn't it possible to be changed by each version of ATOK, MS-IME, etc (Especially, each version of ATOK can be installed separately). If so, it's hard to confirm all GUIDs of them (including the future's releases!). I think that number of TIP names could be one or two. I have no idea of the reason why TIP name is needed to be localized to three or more languages. Therefore, I think that this way is a realistic solution: 1. Research two names of a TIP, one is on Windows for its target locale and the other is on Windows for English. 2. Make an enum which includes all TIPs which need our hack. 3. Check both or common name for it at changing TIP and store the enum. If there is a TIP whose name is too unstable on each locale, we should use GUID instead. How do you think, Yukawa-san?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #7) > On the other hand, and similarly, I don't have English Windows 8 for > checking about Google Japanese Input's TIP name :-( You can use "Google Japanese Input" for non-Japanese display name of Google Japanese Input. To be clear, it is controlled by the IDS_TEXTSERVICE_DISPLAYNAME_SYNONYM macro in the following resource file. https://mozc.googlecode.com/svn/trunk/src/win32/tip/tip_resource.rc Also you can find the profile GUIDs of both Google Japanese Input and OSS Mozc here. https://code.google.com/p/mozc/source/browse/trunk/src/win32/base/tsf_profile.cc (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #7) > > I guess guidProfile returned from ITfInputProcessorProfiles::GetLanguageProfileDescription > > would be more stable and reliable identifier because it is considered to be locale independent. > > This sounds better for creating whitelist, but I worry this for creating > blacklist because isn't it possible to be changed by each version of ATOK, > MS-IME, etc (Especially, each version of ATOK can be installed separately). > If so, it's hard to confirm all GUIDs of them (including the future's > releases!). It depends on your purpose. From the viewpoint of QA, applying a workaround code to unreleased versions of a 3rd party application does not always make sense because there is no guarantee that such a workaround can deal with newer versions. Ideally someone should (probably manually) make sure that the workaround logic is still functional with newer versions before it is enabled in the stable release. Anyway, I don't say we shouldn't compare the display name. It depends on the purpose. > I think that number of TIP names could be one or two. I have no idea of the > reason why TIP name is needed to be localized to three or more languages. > Therefore, I think that this way is a realistic solution: > > 1. Research two names of a TIP, one is on Windows for its target locale and > the other is on Windows for English. > 2. Make an enum which includes all TIPs which need our hack. > 3. Check both or common name for it at changing TIP and store the enum. > > If there is a TIP whose name is too unstable on each locale, we should use > GUID instead. How do you think, Yukawa-san? One of the reasons why TIP names should be localized is that they are visible to users. For example, it is visible on the LangBar and "Text Services and Input Languages". However, AFAIK, most of IME developers except Microsoft don't care about localization and tend to support only for the target language of their IMEs. For example, ATOK is only supported in Japanese edition of Windows. So it might be OK to use the display name unless you want to introduce a hack for built-in IMEs. Just FYI, you can query the TIPs that have translatable display name as follows. reg query HKLM\SOFTWARE\Microsoft\CTF\TIP\ /s /v "Display Description" For example, I can see the following entry in my machine. HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\CTF\TIP\{03B5835F-F03C-411B-9CE2-AA23E1171E36}\LanguageProfile\0x00000411\{A76C93D9-5523-4E90-AAFA-4DB112F9AC76} Display Description REG_EXPAND_SZ @%SystemRoot%\system32\input.dll,-5183 The above entry means that the display name of MS-IME Japanese is specified by the string resource ID 5183 of input.dll. So basically this is just a string resource for Windows UI.

Comment on attachment 8482777 [details] [diff] [review] part.1 nsTextStore::GetTextExt() should rReturn previous character rect instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first character of selected clause at composing Review of attachment 8482777 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for my late response. Looks good in general. One small nit about in comments. ::: widget/windows/nsTextStore.cpp @@ +3157,5 @@ > TSFStaticSink::GetInstance()->GetActiveTIPKeyboardDescription(); > + > + // NOTE: TSF (at least on Win 8.1) doesn't return TS_E_NOLAYOUT to the > + // caller even if we return it. It's converted to just E_FAIL. > + // TODO: On Win 9, we need to check this hack is still necessary. nit: Can you update this line based on Windows 10 TP?

Comment on attachment 8482779 [details] [diff] [review] part.2 nsTextStore::GetTextExt() should rReturn previous character rect of modified range instead of TS_E_NOLAYOUT when Google Japanese Input retrieves caret rect during composition Thank you for handling this. Looks good, given that this hack is not going to be used for long time hopefully.

Although, the count of incorrect positioning is decreased, sometimes I see this. Let's take both patches. I confirmed that Google Japanese Input's TIP name is "Google Japanese Input" on Windows 8.1 for English. So, we should check it too.

Comment on attachment 8570402 [details] [diff] [review] part.1 nsTextStore::GetTextExt() should return previous character rect instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first character of selected clause at composing Review of attachment 8570402 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsTextStore.cpp @@ +3169,5 @@ > } > > + // NOTE: TSF (at least on Win 8.1) doesn't return TS_E_NOLAYOUT to the > + // caller even if we return it. It's converted to just E_FAIL. > + // However, this is fixed on Win 10. Could you make these hacks Windows <= 8.1 only? You will need to add IsWin10OrLater() to WindowsVersion.h.

Comment on attachment 8570404 [details] [diff] [review] part.2 nsTextStore::GetTextExt() should return previous character rect of modified range instead of TS_E_NOLAYOUT when Google Japanese Input retrieves caret rect during composition Review of attachment 8570404 [details] [diff] [review]: ----------------------------------------------------------------- Why two separate patches and prefs are needed? Does it make sense to enable only one pref?

(In reply to Masatoshi Kimura [:emk] from comment #13) > Comment on attachment 8570402 [details] [diff] [review] > part.1 nsTextStore::GetTextExt() should return previous character rect > instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first > character of selected clause at composing > > Review of attachment 8570402 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/windows/nsTextStore.cpp > @@ +3169,5 @@ > > } > > > > + // NOTE: TSF (at least on Win 8.1) doesn't return TS_E_NOLAYOUT to the > > + // caller even if we return it. It's converted to just E_FAIL. > > + // However, this is fixed on Win 10. > > Could you make these hacks Windows <= 8.1 only? > You will need to add IsWin10OrLater() to WindowsVersion.h. Yeah, but isn't it too early for now? I think that we should wait the RTM of Win10.

(In reply to Masatoshi Kimura [:emk] from comment #14) > Comment on attachment 8570404 [details] [diff] [review] > part.2 nsTextStore::GetTextExt() should return previous character rect of > modified range instead of TS_E_NOLAYOUT when Google Japanese Input retrieves > caret rect during composition > > Review of attachment 8570404 [details] [diff] [review]: > ----------------------------------------------------------------- > > Why two separate patches and prefs are needed? Does it make sense to enable > only one pref? Yeah, it allows to test if each hack is still needed in the future. BTW, I realized that I should check if both hacks are still necessary with the latest Google Japanese Input. I'll check it soon.

I confirmed that both hacks are still necessary. part.1 prevents to position the candidate window at wrong place when there is only one clause. part.2 adjusts the position of the candidate window after a conversion is completed (this may cause jumping the candidate window, though). # Like this, separated prefs are useful :)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #15) > > Could you make these hacks Windows <= 8.1 only? > > You will need to add IsWin10OrLater() to WindowsVersion.h. > > Yeah, but isn't it too early for now? I think that we should wait the RTM of > Win10. We already added manifest for Windows 10 based on Technical Preview. Also, IsWin10OrLater() is not used to workaround the bug on Windows 10 here. Rather, it will be used to limit the workaround on Windows 8.1 or earlier. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #16) > Yeah, it allows to test if each hack is still needed in the future. BTW, I > realized that I should check if both hacks are still necessary with the > latest Google Japanese Input. I'll check it soon. It is only useful for developers who are debugging future version of Google Japanese Input with Firefox on Windows 8.1 or earlier to verify the bug is fixed (virtually, only you). Moreover, you don't have to flip each pref separately because two bugs will happen in different situations. I can't imagine Google Japanese Input fixing only one bug, especially given that OSS mozc already fixed both bugs. What if Google Japanese Input changed the TIP name without fixing bugs? I think the current prefs are unnecessarily flexible.

(In reply to Masatoshi Kimura [:emk] from comment #18) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > from comment #15) > > > Could you make these hacks Windows <= 8.1 only? > > > You will need to add IsWin10OrLater() to WindowsVersion.h. > > > > Yeah, but isn't it too early for now? I think that we should wait the RTM of > > Win10. > > We already added manifest for Windows 10 based on Technical Preview. Oh, yeah? I don't see new method for Windows 10: http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h > Also, IsWin10OrLater() is not used to workaround the bug on Windows 10 here. > Rather, it will be used to limit the workaround on Windows 8.1 or earlier. Yes, of course, I agree with limiting the Windows versions. My question is, we should do it *today*. > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > from comment #16) > > Yeah, it allows to test if each hack is still needed in the future. BTW, I > > realized that I should check if both hacks are still necessary with the > > latest Google Japanese Input. I'll check it soon. > > It is only useful for developers who are debugging future version of Google > Japanese Input with Firefox on Windows 8.1 or earlier to verify the bug is > fixed (virtually, only you). Absolutely, but I really want that! I won't want to check with adding #if 0/#endif and build my self. > Moreover, you don't have to flip each pref > separately because two bugs will happen in different situations. > I can't imagine Google Japanese Input fixing only one bug, especially given > that OSS mozc already fixed both bugs. Indeed, but even with the fact, we should continue to test each bug of another application (in this case, Google Japanese Input). Therefore, I really want it. I don't like this question, but I ask you. Why do you hate the separated prefs for the hacks? > What if Google Japanese Input changed > the TIP name without fixing bugs? It MIGHT be possible. However, I don't think that it's realistic scenario because some applications may hide their bugs with checking specific TIP names like us. Changing TIP name is very risky for TIP developers but they won't get enough merit.

Making clear my position: I don't like to add any hack, but I understand that users shouldn't feel any inconvenience even with bugs caused not by us. So, I'd like to limit the hacks to work with only Win 8.1 or earlier. However, there is no utility method which checks if it runs on Win10. Therefore, I'm not sure if we should check Win10 now.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #19) > Oh, yeah? I don't see new method for Windows 10: > http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h I said I added a manifest, not a new function. If it already exists, I would not have said "You will need to add...". > Yes, of course, I agree with limiting the Windows versions. My question is, > we should do it *today*. In general, we should write a code against the ideal operating system and the ideal third part applications, then add hacks to *known* broken real software, just like using CSS hacks. > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > > from comment #16) > I don't like this question, but I ask you. Why do you hate > the separated prefs for the hacks? Adding prefs will add complexity and it will increase the test matrix exponentially. Rather, I don't understand why do you insist separate prefs. But OK, nobody else will change this pref anyway.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #20) > However, there is no utility method which checks if it runs on Win10. It's just because the function was not needed before (apparently we didn't need it for Windows 8.1). It's the time to add the function.

(In reply to Masatoshi Kimura [:emk] from comment #21) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > from comment #19) > > Oh, yeah? I don't see new method for Windows 10: > > http://mxr.mozilla.org/mozilla-central/source/mfbt/WindowsVersion.h > > I said I added a manifest, not a new function. If it already exists, I would > not have said "You will need to add...". Oh, sorry. > > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > > > from comment #16) > > I don't like this question, but I ask you. Why do you hate > > the separated prefs for the hacks? > > Adding prefs will add complexity and it will increase the test matrix > exponentially. Rather, I don't understand why do you insist separate prefs. > But OK, nobody else will change this pref anyway. Thanks. I'm really grad to your decision. (In reply to Masatoshi Kimura [:emk] from comment #22) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > from comment #20) > > However, there is no utility method which checks if it runs on Win10. > > It's just because the function was not needed before (apparently we didn't > need it for Windows 8.1). It's the time to add the function. Okay. Although, I'll look for the proper value for the argument. But if you know the value or documents and tell me it, I'm really happy.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #23) > Okay. Although, I'll look for the proper value for the argument. But if you > know the value or documents and tell me it, I'm really happy. return IsWindowsVersionOrLater(0x0a000000ul); (version 10.0 service pack 0.0) will work.

Comment on attachment 8570402 [details] [diff] [review] part.1 nsTextStore::GetTextExt() should return previous character rect instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first character of selected clause at composing Review of attachment 8570402 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/windows/nsTextStore.h @@ +648,5 @@ > private: > nsString mText; > + // mLastCompositionString stores the composition string when the document > + // is locked. This is necessary to compute mMinTextModifiedOffset. > + nsString mLastCompositionString; Can you avoid wasting memory when the hack is not needed? Or is this change needed regardless of the hack?

(In reply to Masatoshi Kimura [:emk] from comment #25) > Comment on attachment 8570402 [details] [diff] [review] > part.1 nsTextStore::GetTextExt() should return previous character rect > instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first > character of selected clause at composing > > Review of attachment 8570402 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: widget/windows/nsTextStore.h > @@ +648,5 @@ > > private: > > nsString mText; > > + // mLastCompositionString stores the composition string when the document > > + // is locked. This is necessary to compute mMinTextModifiedOffset. > > + nsString mLastCompositionString; > > Can you avoid wasting memory when the hack is not needed? Or is this change > needed regardless of the hack? In strict speaking, it's necessary because current nsTextStore::Content::ReplaceTextWith() is wrong. But nobody must be sure if the bug causes actual problem. I don't think that it's problem of wasting memory but actually it might cause memory fragmentation...

I'm really sorry for the fact that my fix is not yet included in Google Japanese Input stable releases. Actually, the latest dev channel release contains the fix, but the stable releases hasn't. https://tools.google.com/dlpage/japaneseinput/eula.html?hl=ja&extra=dev So if the latest dev channel release of Google Japanese Input works well in Firefox w/o the hack in Firefox side, the most reasonable solution is to push the fix to stable channel in Google Japanese Input side as soon as possible. I'm really sorry that I cannot promise when. I'll ping the release manager again to notify about this situation. Regarding the TIP name of Google Japanese Input, it is less likely to change, and my personal feeling is that it is safe to rely on. I'll pay attention when someone tries to change the TIP name in question.

Yukawa-san: No problem. And actually we should've landed the patches after you give me the feedback. So, I'm sorry for the delay. We've given bad UX for your users using Nightly builds. Anyway, I understand the difficulty of releasing product except what releases rapidly. So, don't mind about this issue. This is not your fault and we're very glad you reported the bug of TSF to Microsoft. It makes the bug fixed on Windows 10!

Comment on attachment 8570402 [details] [diff] [review] part.1 nsTextStore::GetTextExt() should return previous character rect instead of TS_E_NOLAYOUT when Google Japanese Input retrieves first character of selected clause at composing r=me assuming the version check will be added.

As far as I tested, IsWindowsVersionOrLater(0x06040000ul) returns true on my Win10 TP environment. IsWindowsVersionOrLater(0x0a000000ul) returns false...

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #30) > Created attachment 8575301 [details] [diff] [review] > Implement IsWin10OrLater() > > As far as I tested, IsWindowsVersionOrLater(0x06040000ul) returns true on my > Win10 TP environment. IsWindowsVersionOrLater(0x0a000000ul) returns false... Please update your preview to build 9926. IsWindowsVersionOrLater(0x0a000000ul) returns true for me.

(In reply to Masatoshi Kimura [:emk] from comment #33) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > from comment #30) > > Created attachment 8575301 [details] [diff] [review] > > Implement IsWin10OrLater() > > > > As far as I tested, IsWindowsVersionOrLater(0x06040000ul) returns true on my > > Win10 TP environment. IsWindowsVersionOrLater(0x0a000000ul) returns false... > > Please update your preview to build 9926. > IsWindowsVersionOrLater(0x0a000000ul) returns true for me. Okay, I confirmed. jimm: If you think the argument should be modified to 0x0a000000ul and it's the only point which should be modified, please give r+, I'll modify it before landing.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #34) > (In reply to Masatoshi Kimura [:emk] from comment #33) > > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) > > from comment #30) > > > Created attachment 8575301 [details] [diff] [review] > > > Implement IsWin10OrLater() > > > > > > As far as I tested, IsWindowsVersionOrLater(0x06040000ul) returns true on my > > > Win10 TP environment. IsWindowsVersionOrLater(0x0a000000ul) returns false... > > > > Please update your preview to build 9926. > > IsWindowsVersionOrLater(0x0a000000ul) returns true for me. > > Okay, I confirmed. > > > jimm: > > If you think the argument should be modified to 0x0a000000ul and it's the > only point which should be modified, please give r+, I'll modify it before > landing. Whatever works with the current testing release, we can update if they change it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb4932dd12e https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c1363842c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/67faef0b6c20

https://hg.mozilla.org/mozilla-central/rev/6fb4932dd12e https://hg.mozilla.org/mozilla-central/rev/a2c1363842c7 https://hg.mozilla.org/mozilla-central/rev/67faef0b6c20

Comment on attachment 8575301 [details] [diff] [review] Implement IsWin10OrLater() [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Required to fix bug 1166867. Users will see bug 1158298 if declined. Fix Landed on Version: 39 Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: nope See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

I'm going to uplift this under bug 1166867 since I think that makes more sense for tracking purposes.

Comment on attachment 8575301 [details] [diff] [review] Implement IsWin10OrLater() Clearing the approval so it doesn't show up in the bug queries.