If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod, regression})

Trunk
mozilla2.0b4
x86
Mac OS X
inputmethod, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Flags: in-testsuite?
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Summary: IME isn't disabled when password fields on sheet dialog gets focus → IME isn't disabled when password fields on sheet dialog get focus
blocking2.0: ? → final+
Created attachment 462013 [details] [diff] [review]
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.
Attachment #462013 - Flags: review?(smichaud)
Created attachment 462014 [details] [diff] [review]
Patch v1.0

Sorry, this is "-U 8 -p".
Attachment #462013 - Attachment is obsolete: true
Attachment #462014 - Flags: review?(smichaud)
Attachment #462013 - Flags: review?(smichaud)
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 :-)
Attachment #462014 - Flags: review?(smichaud) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/ce4d646e8a1c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Ugh, something was changed on trunk. The new test was now disabled temporary.
http://hg.mozilla.org/mozilla-central/rev/68b886f9b3c3
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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.