866736 - InputScope support for IMM32 with CUAS

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Yohei Yukawa]

Attachment: Use SetInputScopes API to fix this issue

This is a variant of Bug 783882.

Even on IMM32 (more precisely CUAS enabled) Firefox, we can map HTML5 input types to Win32 InputScopes by using SetInputScopes API without having full implementation of TextStore.

Comment 1

[Yohei Yukawa]

Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

Here is the patch I made.
Could you take a look?

Thanks!

Comment 2

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

Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

># HG changeset patch
># User Yohei Yukawa <[e-mail]>
># Date 1367250105 -32400
># Node ID b6378e881634bb84c1c3fad76253d608ae747503
># Parent  05533d50f2f7fa60667b89829bd779d7263df7ca
>Use SetInputScopes to support InputScope on IMM32 Firefox.

Please include "Bug 866736" at the head of description when you do qrefresh or something.

> // static
> void
> IMEHandler::Initialize()
> {
> #ifdef NS_ENABLE_TSF
>   nsTextStore::Initialize();
>   sIsInTSFMode = nsTextStore::IsInTSFMode();
>+  if (!sIsInTSFMode) {
>+    // When full nsTextStore is not available, try to use SetInputScopes API
>+    // to enable at least InputScope. Use GET_MODULE_HANDLE_EX_FLAG_PIN to
>+    // ensure that msctf.dll will not be unloaded.
>+    HMODULE module = nullptr;
>+    if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, L"msctf.dll",
>+                            &module)) {

nit: wrong indent (a space is redundant).

>+// static
>+void IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
>+                                       const nsString& aHTMLInputType) {
>+  if (sIsInTSFMode || !sSetInputScopes || aWindow->Destroyed()) {
>+    return;
>+  }
>+  UINT arraySize = 0;
>+  const InputScope* scopes = nullptr;
>+  // http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html
>+  if (aHTMLInputType.IsEmpty() || aHTMLInputType.EqualsLiteral("text")) {
>+    static const InputScope inputScopes[] = { IS_DEFAULT };
>+    scopes = &inputScopes[0];
>+    arraySize = ARRAYSIZE(inputScopes);

Use ArrayLength().

Thank you, Yukawa-san, I'll update the points and land the patch!

Comment 3

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

Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

https://hg.mozilla.org/integration/mozilla-inbound/rev/40437cd7395b

The other changed points:

>@@ -240,18 +252,26 @@ IMEHandler::GetOpenState(nsWindow* aWind
>   nsIMEContext IMEContext(aWindow->GetWindowHandle());
>   return IMEContext.GetOpenState();
> }
> 
> // static
> void
> IMEHandler::OnDestroyWindow(nsWindow* aWindow)
> {
>+#ifdef NS_ENABLE_TSF
>   // We need to do nothing here for TSF. Just restore the default context
>   // if it's been disassociated.
>+  if (!sIsInTSFMode) {
>+    // MSDN says we need to set IS_DEFAULT to avoid memory leak when we use
>+    // SetInputScopes API. Use an empty string to do this.
>+    nsString empty;
>+    SetInputScopeForIMM32(aWindow, empty);

For temporary empty string, you can use EmptyString(). And if you need to use string variable in stack, you should use nsAutoString which owns 64 bytes in its member. So, it doesn't use heap for short string.

>+// static
>+void IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
>+                                       const nsString& aHTMLInputType) {

The second line should be only the result type.

nsAString should be used for argument instead of nsString.

{ should be in next line.

i.e.,

void
IMEHandler::SetInputScopeForIMM32(nsWindow* aWindow,
                                  const nsAString& aHTMLInputType)
{

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40437cd7395b

Comment 5

[Kent James (:rkent)]

My Thunderbird builds are now failing with this patch:

c:/tb/2-central/src/mozilla/widget/windows/WinIMEHandler.cpp(430) : error C2065: 'IS_SEARCH' : undeclared identifier

I am using VS 2008, so I am guessing that this variable is not in that version?

Comment 6

[Frank Wein [:mcsmurf]]

A similar patch in Bug 783882 added this define to fix this issue in another case:
    1.45 +#define IS_SEARCH static_cast<InputScope>(50)

Bug 783882 Comment 26 suggests that this define is part of a quite new Windows SDK (it says Win8 SDK, not sure if it's really that new). I suggest to add the define here, too.

Comment 7

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

(In reply to Frank Wein [:mcsmurf] from comment #6)
> A similar patch in Bug 783882 added this define to fix this issue in another
> case:
>     1.45 +#define IS_SEARCH static_cast<InputScope>(50)
> 
> Bug 783882 Comment 26 suggests that this define is part of a quite new
> Windows SDK (it says Win8 SDK, not sure if it's really that new). I suggest
> to add the define here, too.

Ah, I forgot that. I'll make a patch soon.

Comment 8

[Yohei Yukawa]

Oops, sorry for the macro definition. Yes, IS_SEARCH is available only in Win8 SDK.

Nakano-san, thank you for landing the patch!

Comment 9

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

Attachment: Fix the bustage without Win8 SDK

Comment 10

[Frank Wein [:mcsmurf]]

Pushed the patch as Masayuki is more or less offline (says his name :): https://hg.mozilla.org/integration/mozilla-inbound/rev/9dbaa660e5e2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dbaa660e5e2

Comment 12

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

Attachment: part.3 Replace CRLF in WinIMEHandler.(h|cpp) with LF

I realized that the first patch includes CRLFs. This patch replaces all of them with LF.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/10abfa7e0154

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/10abfa7e0154