582893 - IME isn't disabled when password fields on sheet dialog get focus

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

This is a regression of bug 513952.

nsCocoaIMEHandler::IsFocused() checks [window isMainWindow], however, it should be |[window isMainWindow] || [window isSheet]|.

I'll post the patch.

Comment 1

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

Attachment: Patch v1.0

The main cause is [NSWindow isSheet] isn't called from IsFocus(). And there are two additional issues. One is ::TSMGetActiveDocument() returns *latest* active document. On 10.6, the result of ::TSMGetActiveDocument() is same pointer with the result of [NSInputManager currentInputManager]. [NSInputManager currentInputManager] returns *current* instance. So, we must call it before we call ::TSMGetActiveDocument(), then, we can get the right active document during focus changing.

The other issue is, ResetTimer() quits if mTimer is already created but it cancels the current timer. This is a bug, it should reboot the timer.

Note that this patch has a test. However, it can *not* detect this bug because nsChildView::GetIMEEnabled() returns non-actual state of the system. If we test this regression, we need to add an API to get the actual IME state of the system. But it's not good for now, it should be tested on native IME code testing (bug 460056). But I think the new test may be worth for some other regressions.

Comment 2

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

Attachment: Patch v1.0

Sorry, this is "-U 8 -p".

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 462014 [details] [diff] [review]
Patch v1.0

This looks fine to me.  But I haven't tested it (or compiled it),
because I don't have a testcase I can use "by hand".  Please provide
one, if possible.

(From comment #0)

> One is ::TSMGetActiveDocument() returns *latest* active document. On
> 10.6, the result of ::TSMGetActiveDocument() is same pointer with
> the result of [NSInputManager currentInputManager]. [NSInputManager
> currentInputManager] returns *current* instance.

This is false ... or at least unclear.

But your comment in nsCocoaIMEHandler::GetCurrentTSMDocumentID() is
much better:

+  // First, we must call [NSInputManager currentInputManager] because if
+  // focus is moving, this calling changes the result of
+  // ::TSMGetActiveDocument() from old focused window's to new focused window's.

The gist of it is that (on OS X 10.6.X at least)
::TSMGetActiveDocument() has a bug that prevents it from returning
accurate results unless [NSInputManager currentInputManager] is called
first.

I didn't know this (and without a testcase I'm unable to confirm it).
But it certainly sounds plausible :-)

Comment 4

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

Thank you, Steven.

(In reply to comment #3)
> This looks fine to me.  But I haven't tested it (or compiled it),
> because I don't have a testcase I can use "by hand".  Please provide
> one, if possible.

Paste following code to the input field of _Error Console_.

> var arg1 = new Object(), arg2 = new Object(); Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService).promptPassword(window, "title", "text", arg1, "msg", arg2);

Then, you can see this bug.

> (From comment #0)
> 
> > One is ::TSMGetActiveDocument() returns *latest* active document. On
> > 10.6, the result of ::TSMGetActiveDocument() is same pointer with
> > the result of [NSInputManager currentInputManager]. [NSInputManager
> > currentInputManager] returns *current* instance.
> 
> This is false ... or at least unclear.

Ah, I meant that the TSMDocumentID is an alias of NSInputManager on 10.6. However, the currentInputManger returns an actual *current* input manager, but the ::TSMGetActiveDocument() isn't looking for the actual current input manager itself during moving focus. So, it returns an NSInputManger of previous focused view.

First, I just added [window isSheet] to IsFocused(), but the SyncASCIICapableOnly() didn't work fine. I was confused by that.

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/ce4d646e8a1c

Comment 6

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

Ugh, something was changed on trunk. The new test was now disabled temporary.
http://hg.mozilla.org/mozilla-central/rev/68b886f9b3c3

Comment 7

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

um... I cannot reproduce the failure on my system...

13908 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/widget/tests/test_imestate.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]" nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)" location: "JS frame :: chrome://mochikit/content/chrome/widget/tests/test_imestate.html :: runTestPasswordFieldOnDialog :: line 905" data: no] at :0