InputScope support for IMM32 with CUAS

RESOLVED FIXED in mozilla23

Status

()

Core
Widget: Win32
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yohei Yukawa, Assigned: Yohei Yukawa)

Tracking

Trunk
mozilla23
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Created attachment 743100 [details] [diff] [review]
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.
(Assignee)

Comment 1

4 years ago
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!
Attachment #743100 - Flags: review?(masayuki)
Assignee: nobody → yukawa
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 743100 [details] [diff] [review]
Use SetInputScopes API to fix this issue

># HG changeset patch
># User Yohei Yukawa <yukawa@google.com>
># 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!
Attachment #743100 - Flags: review?(masayuki) → review+
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)
{
https://hg.mozilla.org/mozilla-central/rev/40437cd7395b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 5

4 years ago
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?
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.
(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.
(Assignee)

Comment 8

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

Nakano-san, thank you for landing the patch!
Created attachment 744364 [details] [diff] [review]
Fix the bustage without Win8 SDK
Attachment #744364 - Flags: review?(jmathies)

Updated

4 years ago
Depends on: 867939

Updated

4 years ago
Attachment #744364 - Flags: review?(jmathies) → review+
Pushed the patch as Masayuki is more or less offline (says his name :): https://hg.mozilla.org/integration/mozilla-inbound/rev/9dbaa660e5e2
https://hg.mozilla.org/mozilla-central/rev/9dbaa660e5e2
Created attachment 768198 [details] [diff] [review]
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.
Attachment #768198 - Flags: review?(jmathies)

Updated

4 years ago
Attachment #768198 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/10abfa7e0154
https://hg.mozilla.org/mozilla-central/rev/10abfa7e0154
You need to log in before you can comment on or make changes to this bug.