519913 - The IME composition string isn't committed on Mac when the window is deactivating

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

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

I improved the Mac's IME management in bug 513952.

On Mac, the focus lost event is happened after the native widget really lost the focus. But on Mac, we cannot request the IME related commands when the focus is deactive. Therefore, my patch commits the composition only internally if the focus is lost. And after the widget gets the focus, my patch requests IME to discard the composition.

However, if a window is deactivated by the another our window being activated, the composition isn't committed correctly. Probably, the event handling target is wrong.

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6010
> 6010     if (NS_IsEventTargetedAtFocusedWindow(aEvent)) {
> 6011       nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> 6012       if (!fm)
> 6013          return NS_ERROR_FAILURE;
> 6014  
> 6015       nsCOMPtr<nsIDOMWindow> window;
> 6016       fm->GetFocusedWindow(getter_AddRefs(window));

So, perhaps, the composition event is sent to the active window, not the last focused content in the window. We should check whether we are active or not. If we are active, we should send the event to the last focused content in the window.

Comment 1

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

Attachment: Patch v1.0

needs automated tests...

Comment 2

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

Attachment: Patch v1.1

Comment 3

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

Attachment: Patch v1.2

Comment 4

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

Attachment: Patch v1.3

this passes to all tests.
# tryserver machines fails very many times randomly...

Comment 5

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

This bug can be reproduced on Windows too. And also there are some serious problems.

Maybe, the focus refactoring broke the code. The refactoring changed to that all key events and all IME events are not handled when we are deactive. By this change, we cannot use some software keyboards which steals focus from us.

Comment 6

[Martijn Wargers (dead)]

This reminds me of bug 497839, maybe it is related.

Comment 7

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

(In reply to comment #6)
> This reminds me of bug 497839, maybe it is related.

right. It's going to be fixed by the patch too.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Can this patch be put up for review or is it still undergoing active development? In the latter case, is there an ETA?

In the vein of bug 497839, the patch makes Thunderbird mozmill tests pass on my linux box that are otherwise flakey using mozilla-central (likely depending on window positioning and/or me not moving my mouse at all) but are fine on mozilla-1.9.1.  I believe the Thunderbird mozmill tinderbox runners reflect this as well.  While we will likely degrade our tests to stop synthesizing keyboard events in the short term, it would be nice to be able to have realistic testing in the medium term.

Comment 9

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

See bug 528396, I need the new APIs for this patch's test. However, it's not finished yet.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Should this block 1.9.2? Comment 5 is pretty scary.

Comment 11

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

(In reply to comment #10)
> Should this block 1.9.2? Comment 5 is pretty scary.

Yeah, but the patch could be risky...

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

But still, is there something now terribly broken which works on 1.9.1?

Comment 13

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

Yes, e.g., IME composition string cannot be committed automatically by losing focus. This confuses the IME state in gecko (gecko thinks the composition is committed but actually not so).

And Moji-palette of ATOK which is used to input some Unicode characters which cannot be inputted by IME cannot be used. But this is rare case.

Maybe, if there is most critical issue, IME cannot use completely on some Linux system. I've been told that some Window Manager's IME candidate window steals focus from the client application. If such WM is still there, the users cannot use IME. However, this is very critical issue, so, such users should have reported the bug to us already. So, I guess that such environments are not there now.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Ok, so could we fix this and Bug 532422 the safest possible way, which is, I think, to target IME/keyboard events to the previously focused element, if
there are no active Fx windows.
That wouldn't fix the case where an Fx window is deactivated during
composition by moving focus to another Fx window. But that should be pretty
rare case, I hope.

Comment 15

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

(In reply to comment #14)
> That wouldn't fix the case where an Fx window is deactivated during
> composition by moving focus to another Fx window. But that should be pretty
> rare case, I hope.

Yes. You don't need to worry the case.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm curious as to why this was only raised as a blocking issue recently. Did we not think it was a blocker when originally filed?

Also, what's the relationship now between this bug and bug 532422? Straight dupe? Or was this just fixed-by bug 532422?

Comment 17

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

beltzner:

In bug 532422, we fixed the bugs only when Fx is deactive. It's safest choice for 1.9.2 branch and it fixes most important cases (Fennec's problem and some software's problem).

There is only one bug. The IME composition string isn't committed when the window is deactivating by another Fx's window getting focus. But this is rare case, the users can reproduce this bug easily, but they don't do such operation in daily use. And if we fix this, we need more risky change. So, we decided that we don't fix the remain issue in 1.9.2.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

cancelling the 1.9.2?.

Comment 19

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

Attachment: Patch v2.0

This patch locks up key events and IME related events to each top level window. I think that this change doesn't influence most add-ons. And I don't think these (native) input events which are fired on deactive window should be handled on active window.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 442663 [details] [diff] [review]
Patch v2.0


>+nsresult
>+PresShell::GetFocusedContent(nsIContent** aFocusedContent,
>+                             nsPIDOMWindow** aFocusedWindow)
>+{
>+  NS_PRECONDITION(aFocusedContent, "aFocusedContent is null");
>+  NS_PRECONDITION(aFocusedWindow, "aFocusedWindow is null");
>+
>+  nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+  NS_ENSURE_TRUE(fm, NS_ERROR_FAILURE);
>+
>   nsCOMPtr<nsPIDOMWindow> window =
>     do_QueryInterface(mDocument->GetWindow());
>-  NS_ENSURE_TRUE(window, nsnull);
>+  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
> 
>   nsCOMPtr<nsPIDOMWindow> rootWindow = window->GetPrivateRoot();
>-  NS_ENSURE_TRUE(rootWindow, nsnull);
>-  nsPIDOMWindow* focusedWindow;
>-  nsFocusManager::GetFocusedDescendant(rootWindow, PR_TRUE, &focusedWindow);
>-  return focusedWindow;
>+  NS_ENSURE_TRUE(rootWindow, NS_ERROR_FAILURE);
>+
>+  // if there is no focused frame in this window, there isn't anything to
>+  // fire a key event at so just return
this comment is in a strange place.

>+  nsCOMPtr<nsIDOMWindow> focusedWindow;
>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElementForWindow(rootWindow, PR_TRUE,
>+                                 getter_AddRefs(focusedWindow),
>+                                 getter_AddRefs(focusedElement));
>+  nsCOMPtr<nsIContent> focusedContent = do_QueryInterface(focusedElement);
>+  nsCOMPtr<nsPIDOMWindow> focusedPIWindow = do_QueryInterface(focusedWindow);
>+  focusedContent.swap(*aFocusedContent);
>+  focusedPIWindow.swap(*aFocusedWindow);
>+  return NS_OK;
I don't quite understand the changes here.
You change GetFocusedDescendant to GetFocusedElementForWindow.
GetFocusedElementForWindow does actually call GetFocusedDescendant.



>   if (!sDontRetargetEvents) {
>     if (NS_IsEventTargetedAtFocusedWindow(aEvent)) {
>-      nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>-      if (!fm)
>-         return NS_ERROR_FAILURE;
>- 
>-      nsCOMPtr<nsIDOMWindow> window;
>-      fm->GetFocusedWindow(getter_AddRefs(window));
>-
>-      // if there is no focused frame, there isn't anything to fire a key event
>-      // at so just return
>-      nsCOMPtr<nsPIDOMWindow> piWindow = do_QueryInterface(window);
>-
>-      if (!piWindow) {
>-        // When all our windows are deactive, we should use the last focused
>-        // window under our top level window.
>-        piWindow = GetFocusedDOMWindowInOurWindow();
>-      }
>-
>-      if (!piWindow)
>+      // if there is no focused frame in this window, there isn't anything to
>+      // fire a key event at so just return
Also this comment

Could you describe, (and even write a comment somewhere in PresShell::HandleEvent) that how
key events are handled with this patch.

Comment 21

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

> I don't quite understand the changes here.
> You change GetFocusedDescendant to GetFocusedElementForWindow.
> GetFocusedElementForWindow does actually call GetFocusedDescendant.
> 

Ah, I'm sorry. It's not needed change. I'm confused by the change of bug 532422 which wasn't in my plan.

Comment 22

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

Attachment: Patch v2.1

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 443555 [details] [diff] [review]
Patch v2.1

Enn should also review this.

Comment 24

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

Enn, would you review the patch? I pinged you by email, but you have never replied...

Comment 25

[Neil Deakin]

Comment on attachment 443555 [details] [diff] [review]
Patch v2.1

>+  otherWindow =
>+    window.open("about:blank", "_blank", "chrome,width=100,height=100");
>+  ok(otherWindow, "failed to open other window");
>+  if (!otherWindow) {
>+    SimpleTest.finish();
>+    return;
>+  }
>+
>+  otherWindow.addEventListener("focus", startTests, false);
>+  timer = setTimeout(startTests, 1000); // prevents unexpected timeout
>+  otherWindow.focus();

If you use a data url such as 'data:text/plain,test' instead of 'about:blank', you can just use waitForFocus instead of listening to all these events manually.

The main issue I have with this change is that you've now made key events fire at whatever the operating system happens to send them to, rather than what the focus manager thinks is actually focused.

But I'm willing to try this for a bit to see if it doesn't end up causing any problems.

Comment 26

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

Attachment: Patch v2.2

Thank you, Enn. I'll land this patch.

Comment 27

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

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