Closed Bug 519913 Opened 11 years ago Closed 10 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b1

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: access, inputmethod, intl)

Attachments

(1 file, 6 obsolete files)

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.
Attached patch Patch v1.0 (obsolete) — Splinter Review
needs automated tests...
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #404023 - Attachment is obsolete: true
Attached patch Patch v1.2 (obsolete) — Splinter Review
Attachment #404032 - Attachment is obsolete: true
Attached patch Patch v1.3 (obsolete) — Splinter Review
this passes to all tests.
# tryserver machines fails very many times randomly...
Attachment #404087 - Attachment is obsolete: true
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.
Keywords: access
Summary: The IME composition string isn't committed on Mac if the window is deactivated → The IME composition string isn't committed if the window is deactivated
This reminds me of bug 497839, maybe it is related.
(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.
Blocks: 497839
Blocks: 532422
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.
See bug 528396, I need the new APIs for this patch's test. However, it's not finished yet.
Should this block 1.9.2? Comment 5 is pretty scary.
Flags: blocking1.9.2?
(In reply to comment #10)
> Should this block 1.9.2? Comment 5 is pretty scary.

Yeah, but the patch could be risky...
But still, is there something now terribly broken which works on 1.9.1?
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.
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.
(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.
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?
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.
Summary: The IME composition string isn't committed if the window is deactivated → The IME composition string isn't committed on Mac when the window is deactivating
cancelling the 1.9.2?.
Flags: blocking1.9.2?
Status: NEW → ASSIGNED
Attached patch Patch v2.0 (obsolete) — Splinter Review
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.
Attachment #404274 - Attachment is obsolete: true
Attachment #442663 - Flags: review?(Olli.Pettay)
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.
Attachment #442663 - Flags: review?(enndeakin)
Attachment #442663 - Flags: review?(Olli.Pettay)
Attachment #442663 - Flags: review-
> 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.
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #442663 - Attachment is obsolete: true
Attachment #443555 - Flags: review?(Olli.Pettay)
Attachment #442663 - Flags: review?(enndeakin)
Attachment #443555 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 443555 [details] [diff] [review]
Patch v2.1

Enn should also review this.
Attachment #443555 - Flags: review?(enndeakin)
No longer blocks: 569023
Enn, would you review the patch? I pinged you by email, but you have never replied...
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.
Attachment #443555 - Flags: review?(enndeakin) → review+
Attached patch Patch v2.2Splinter Review
Thank you, Enn. I'll land this patch.
Attachment #443555 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/c7f3d1f216d6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Depends on: 573689
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.