[Tracking Requested - why for this release]: Steps To Reproduce: 1. Enable e10s autostart and restart browser 2. Open about:home 3. Click SearchBar 4. IME on 5. Type something Actual Results: Typed text appears in about:home's text textarea Expected Results: Typed text should appear in SearchBar which is focused now This problem happens Microsoft Office IME2010. This problem does not happen if intl.tsf.enable = false

This problem also happen with Atok2006 if intl.tsf.enable = true (default in Nightly)....

Windows8.1 and MS IME2012 is also affected.

Sounds like that focus is confused by e10s. Where composition/text events are fired is decided by PresShell with focused document and focused element. I guess that PresShell thinks that chrome document (in parent process) has focus even in this case...

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3) > Sounds like that focus is confused by e10s. Where composition/text events > are fired is decided by PresShell with focused document and focused element. > I guess that PresShell thinks that chrome document (in parent process) has > focus even in this case... Due to bug 835262, even if focus is lost, TSF doesn't receive IME blur notification. So, e10s with TSF doesn't change focus by IMEStateManager::OnChangeFocus() correctly....

Comment on attachment 8519750 [details] [diff] [review] Always send blur on non-Chrome process If TSF is on, NOTIFY_OF_BLUR is notified. PuppetWidget always commit composition string on blur. So I think we doesn't keep composition on blur even if TSF. May I add GetIMEUpdatePreferenceOnCurrentWidget to nsIWidget?

s/If TSF is on, NOTIFY_OF_BLUR is notified/If TSF is on, NOTIFY_OF_BLUR isn't notified/

(In reply to Makoto Kato (:m_kato) from comment #6) > Comment on attachment 8519750 [details] [diff] [review] > Always send blur on non-Chrome process > > If TSF is on, NOTIFY_OF_BLUR is notified. > > PuppetWidget always commit composition string on blur. So I think we > doesn't keep composition on blur even if TSF. Hmm, this is bad... On Windows, if a process becomes busy (i.e., its window becomes white out), focus is lost temporarily. In this case, committing composition is too bad especially for slow machine users... > May I add GetIMEUpdatePreferenceOnCurrentWidget to nsIWidget? I'm still thinking what is the best approach for this. Please wait. # Anyway, we need a workaround for this soon, though.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8) > (In reply to Makoto Kato (:m_kato) from comment #6) > > Comment on attachment 8519750 [details] [diff] [review] > > Always send blur on non-Chrome process > > > > If TSF is on, NOTIFY_OF_BLUR is notified. > > > > PuppetWidget always commit composition string on blur. So I think we > > doesn't keep composition on blur even if TSF. > > Hmm, this is bad... On Windows, if a process becomes busy (i.e., its window > becomes white out), focus is lost temporarily. In this case, committing > composition is too bad especially for slow machine users... Now, PuppetWidget will commit string on blur. So maybe, we remove this code too. Current implementation of blur on chrome process is only calling TabParent::Deactivate(). So content process cannot know whether blur event is for changing focus from content process to chrome process. Maybe, we should add a parameter for Send/RecvDeactivate to recognize focus change to parent process.

FYI: Will bug 1051842 fix a serious focus/SetInputContext() race, that might help this.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > FYI: Will bug 1051842 fix a serious focus/SetInputContext() race, that might > help this. Unfortunately, this issue isn't race. Root cause is that e10s's child process cannot know whether focus lost by nsIWebBrowserFocus::deactive() is by changing window/application or moving focus to content to chrome...

Comment on attachment 8565840 [details] [diff] [review] Need clear forcus to release IMEContetObserver v1.1 Nakano-san, do you have a good idea? nsIWebBrowserFocus::deactive() is same as window lowered. So IME focus isn't lost on TSF.

Comment on attachment 8565840 [details] [diff] [review] Need clear forcus to release IMEContetObserver v1.1 I don't 100% understand of this patch, however, this patch really works fine. Let's ask review to enn too. This is really his area.

Comment on attachment 8565840 [details] [diff] [review] Need clear forcus to release IMEContetObserver v1.1 > nsIWebBrowserFocus::deactive() is same as window lowered No, that means that the child tab is no longer focused. So we can't do this. WindowRaised and WindowLowered are called whenever focus switches to and from a child process tab to chrome, or to another tab, which means that the focus will get reset whenever focus moves away from the tab. For example, clicking a link to open in a new tab should keep the focus on the link in the previous tab. Note that I don't see this bug on Mac, and the focus is working fine. I don't think this is an issue with the focus manager, but may instead be an issue with the windows ime code.

(In reply to Neil Deakin from comment #17) > Comment on attachment 8565840 [details] [diff] [review] > Need clear forcus to release IMEContetObserver v1.1 > > > nsIWebBrowserFocus::deactive() is same as window lowered > > No, that means that the child tab is no longer focused. > > So we can't do this. WindowRaised and WindowLowered are called whenever > focus switches to and from a child process tab to chrome, or to another tab, > which means that the focus will get reset whenever focus moves away from the > tab. For example, clicking a link to open in a new tab should keep the focus > on the link in the previous tab. After bug 835262, TSF doesn't kill focus correctly when WindowLowerd. As behaivour, see bug 835262. It means the following. - blur event is fired by WindowLowerd/Blur(). - IME blur event for widget isn't fired even if WindowLowered with TSF > Note that I don't see this bug on Mac, and the focus is working fine. I > don't think this is an issue with the focus manager, but may instead be an > issue with the windows ime code. This is only TSF, not all IME. After bug 835262, TSF doesn't release IME content observer (in other word, it isn't fired IME blur even if blur event is fired) when application is deactive. Nakano-san, nsIWebBrowserFocus::deactive() also means that focus is changed from content to chrome. But since it uses as as WidnowLowerd logic, IME content observer isn't released by bug 835262. Could I call IMEStateManager::DestroyIMEContentObserver() from nsWebBrowser::Deactivate()? Other TSF application (ex WordPad) commits composition string when window is deactive. But Firefox doesn't commit it on this situation.

(In reply to Makoto Kato (:m_kato) from comment #18) > (In reply to Neil Deakin from comment #17) > > Note that I don't see this bug on Mac, and the focus is working fine. I > > don't think this is an issue with the focus manager, but may instead be an > > issue with the windows ime code. > > This is only TSF, not all IME. After bug 835262, TSF doesn't release IME > content observer (in other word, it isn't fired IME blur even if blur event > is fired) when application is deactive. And also, in the future, B2G's IME might behave so (just a possibility, though). > Nakano-san, nsIWebBrowserFocus::deactive() also means that focus is changed > from content to chrome. But since it uses as as WidnowLowerd logic, IME > content observer isn't released by bug 835262. Sounds bad... > Could I call IMEStateManager::DestroyIMEContentObserver() from > nsWebBrowser::Deactivate()? Basically, the method shouldn't be called from outside of IMEStateManager. I think if you need to do that, you should call it via another method whose name explains when it's called. However, I don't think that composition should be committed when our process is being inactivated. > Other TSF application (ex WordPad) commits > composition string when window is deactive. But Firefox doesn't commit it > on this situation. Yeah, the reason is bug 789708. So, committing composition when our process is busy causes unexpected committing (and IME dictionary leans the unexpected result). It's too bad. If you can just avoid this case, it's okay to commit composition when our process is inactivated, though. You probably can test it with: input.addEventListener("compositionupdate", function (aEvent) { for (var i = 0; i < 100000; i++) { dump("foo"); } }, false);

Comment on attachment 8573130 [details] [diff] [review] Part 1. Add reason when child process is deactivated TSF (Windows IME framework from Vista) has different behavior for "moving focus" and "window lowered". When "window lowered", TSF keeps composition string and don't fire IMEBlur. But When "moving focus", TSF commits and releases composition string and fires IMEBlur. (see comment #18 and comment #19) But child process doesn't know this deactive() on child process is "window lowered" of parent process or "moving focus" from child to parent. So I want to add a flag for this.

Comment on attachment 8573132 [details] [diff] [review] Part 2. When focus is moved from child to parent, I ME blur should be fired even if using TSF This depends on part 1 patch. If not window lowered, we should fire IME blur by IMEStateManager::OnChangeFocus. If I should not modify nsIWebBrowserFocus, I will change idea to use IEMStateManager on TabParent::RecvDeactive directly.

Comment on attachment 8573132 [details] [diff] [review] Part 2. When focus is moved from child to parent, I ME blur should be fired even if using TSF Does this work as you expected when designMode editor has focus? data:text/html,<body onload="document.designMode='on'"></body> When it's in designMode, only the presContext is non-null. So, your patch might fail to notify IMEContentObserver of blur.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (offline: 2/28-3/8) from comment #24) > Comment on attachment 8573132 [details] [diff] [review] > Part 2. When focus is moved from child to parent, I ME blur should be fired > even if using TSF > > Does this work as you expected when designMode editor has focus? > > data:text/html,<body onload="document.designMode='on'"></body> > > When it's in designMode, only the presContext is non-null. So, your patch > might fail to notify IMEContentObserver of blur. Thanks. I will try it If prescontext isn't null, it works correctly. Root causes of this issue is prescontext is null when using TabParent::Deactive()

Comment on attachment 8573130 [details] [diff] [review] Part 1. Add reason when child process is deactivated > >-bool TabChild::RecvDeactivate() >+bool TabChild::RecvDeactivate(const bool& aWindowLowerd) aWindowLowerd -> aWindowLowered (missing an e) >@@ -1607,17 +1610,17 @@ void nsWebBrowser::WindowLowered(nsIWidg > { > #if defined(DEBUG_smaug) > nsCOMPtr<nsIDocument> document = mDocShell->GetDocument(); > nsAutoString documentURI; > document->GetDocumentURI(documentURI); > printf("nsWebBrowser::NS_DEACTIVATE %p %s\n", (void*)this, > NS_ConvertUTF16toUTF8(documentURI).get()); > #endif >- Deactivate(); >+ Deactivate(true); I don't even think nsWebBrowser::WindowLowered is even used any more. -/* void deactivate (); */ -NS_IMETHODIMP nsWebBrowser::Deactivate(void) +/* void deactivate (in boolean aWindowLowered); */ +NS_IMETHODIMP +nsWebBrowser::Deactivate(bool aWindowLowered) { nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID); nsCOMPtr<nsIDOMWindow> window = GetWindow(); if (fm && window) return fm->WindowLowered(window); return NS_OK; You could pass the aWindowLowered flag to nsIFocusManager::WindowLowered and the change in the other patch could be moved into the focus manager instead. /* void deactivate (in boolean aWindowLowered); */ NS_IMETHODIMP nsWebBrowser::Deactivate(bool aWindowLowered) { + if (!aWindowLowered && + XRE_GetProcessType() != GeckoProcessType_Default) { + // Focus is moved from child to parent if called on child. + nsRefPtr<nsPresContext> presContext; + mDocShell->GetPresContext(getter_AddRefs(presContext)); + if (presContext) { + IMEStateManager::OnChangeFocus(presContext, nullptr, + InputContextAction::CAUSE_UNKNOWN); + } + } Isn't IMEStateManager::OnChangeFocus already called during WindowLowered when it calls Blur? At http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1626 ? Do you need the right pres context?

Comment on attachment 8573132 [details] [diff] [review] Part 2. When focus is moved from child to parent, I ME blur should be fired even if using TSF per comment #24, IME blur doesn't send correctly on multiple windows when changing focus to multiple windows.

(In reply to Neil Deakin from comment #26) > Comment on attachment 8573130 [details] [diff] [review] > Part 1. Add reason when child process is deactivated > > > > >-bool TabChild::RecvDeactivate() > >+bool TabChild::RecvDeactivate(const bool& aWindowLowerd) > > aWindowLowerd -> aWindowLowered (missing an e) > > > >@@ -1607,17 +1610,17 @@ void nsWebBrowser::WindowLowered(nsIWidg > > { > > #if defined(DEBUG_smaug) > > nsCOMPtr<nsIDocument> document = mDocShell->GetDocument(); > > nsAutoString documentURI; > > document->GetDocumentURI(documentURI); > > printf("nsWebBrowser::NS_DEACTIVATE %p %s\n", (void*)this, > > NS_ConvertUTF16toUTF8(documentURI).get()); > > #endif > >- Deactivate(); > >+ Deactivate(true); > > I don't even think nsWebBrowser::WindowLowered is even used any more. > > > -/* void deactivate (); */ > -NS_IMETHODIMP nsWebBrowser::Deactivate(void) > +/* void deactivate (in boolean aWindowLowered); */ > +NS_IMETHODIMP > +nsWebBrowser::Deactivate(bool aWindowLowered) > { > nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID); > nsCOMPtr<nsIDOMWindow> window = GetWindow(); > if (fm && window) > return fm->WindowLowered(window); > return NS_OK; > > You could pass the aWindowLowered flag to nsIFocusManager::WindowLowered and > the change in the other patch could be moved into the focus manager instead. > > > /* void deactivate (in boolean aWindowLowered); */ > NS_IMETHODIMP > nsWebBrowser::Deactivate(bool aWindowLowered) > { > + if (!aWindowLowered && > + XRE_GetProcessType() != GeckoProcessType_Default) { > + // Focus is moved from child to parent if called on child. > + nsRefPtr<nsPresContext> presContext; > + mDocShell->GetPresContext(getter_AddRefs(presContext)); > + if (presContext) { > + IMEStateManager::OnChangeFocus(presContext, nullptr, > + InputContextAction::CAUSE_UNKNOWN); > + } > + } > > Isn't IMEStateManager::OnChangeFocus already called during WindowLowered > when it calls Blur? At > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager. > cpp#1626 ? Do you need the right pres context? presContext is null, IME focus isn't reset. But I should use mFocusedWindow's pres context.

Comment on attachment 8623436 [details] [diff] [review] Reset IME focus when moving focus from child to chrome Add flags to nsIFocusManager to reset IME focus

I take this we can fix this bug by hacking IMEStateManager.

I'm testing on tryserver...

Comment on attachment 8626975 [details] [diff] [review] part.1 Active TabParent should be managed by IMEStateManager First of all, our current IME state management in e10s mode is completely wrong. Currently, TabParent manages if which instance is focused or parent process has focus. However, TabParent doesn't receive enough information (events) and have enough hints. I believe that the best class which manages is IMEStateManager. It has enough information and receives enough notification from nsFocusManager, nsEditor and nsXULPopupManager. If we do so, we can make setting input context easier. IMEStateManager should set that only when focused element isn't a container of a child process. Therefore, this patch removes some hacks which checks if parent process's IME state is disabled. # Anyway, I cannot explain my idea with English words. I believe that you will agree with my approach after you check part.2 and part.3.

Comment on attachment 8626976 [details] [diff] [review] part.2 Notify child process's IMEStateManager of that it should stop manageing IME state When a child process loses the rights to receive input events, IMEStateManager in the process shouldn't keep storing *previous* focused content and should inactivate IMEContentObserver. Because if they are kept, when focus is back to the process, it doesn't think that focus has been changed. For example, currently, since IMEContentObserver isn't created and destroyed in such timing, notifying IME of focus/blur is completely broken. That causes IME composition appears previous focused editor if it's different process's.

Oops, I forgot to post newer patch. TabParent::Send*() requires to check their result. So, this patch does |unused << sActiveTabParent->Send*()|.

When menu installs key listeners, IMEStateManager needs to override the IME state of focused content. Therefore, if child process has focus, the state of menu key listener should be shared both parent's and child's IMEStateManager. If the state isn't shared, child process cannot restore proper IME state when menu loses pseudo focus.

Comment on attachment 8626978 [details] [diff] [review] part.4 Make IMEStateManager::sContent StaticRefPtr This is not necessary for this bug but making the managing of sContent safer and easier.

We can say same thing for sActiveIMEContentObserver.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #43) > Created attachment 8627008 [details] [diff] [review] > part.5 Make IMEStateManager::sActiveIMEContentObserver StaticRefPtr > > We can say same thing for sActiveIMEContentObserver. Although, I want StaticRefPtr<T>::swap(nsRefPtr<T>&).

Comment on attachment 8626975 [details] [diff] [review] part.1 Active TabParent should be managed by IMEStateManager It is not quite clear to me why IMEStateManager::OnChangeFocusInternal doesn't call SetIMEState if there is newTabParent. Please explain and add also a comment about that to the code. I think I should r+ this once there is such comment, so that I understand the method correctly.

Comment on attachment 8627006 [details] [diff] [review] part.2 Notify child process's IMEStateManager of that it should stop manageing IME state >+ if (sActiveTabParent && sActiveTabParent != newTabParent) { >+ MOZ_LOG(sISMLog, LogLevel::Debug, >+ ("ISM: IMEStateManager::OnChangeFocusInternal(), notifying previous " >+ "focused child process of parent process gets focus")); This comment isn't quite right, since sActiveTabParent and newTabParent can be coming from the same child process. In fact, do we need to call SendStopIMEStateManagement if sActiveTabParent is dealing with the same process as newTabParent

(In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 8626975 [details] [diff] [review] > part.1 Active TabParent should be managed by IMEStateManager > > It is not quite clear to me why IMEStateManager::OnChangeFocusInternal > doesn't call SetIMEState if there is newTabParent. The reason is, it means that the process has already lost focus. I.e., IME should be handled by newTabParent. However, it might be better to make input context disabled until newTabParent will set proper IME state. > Please explain and add also a comment about that to the code. Sure. Additionally, I'll check if I should set disabled state.

(In reply to Olli Pettay [:smaug] from comment #46) > Comment on attachment 8627006 [details] [diff] [review] > part.2 Notify child process's IMEStateManager of that it should stop > manageing IME state > > >+ if (sActiveTabParent && sActiveTabParent != newTabParent) { > >+ MOZ_LOG(sISMLog, LogLevel::Debug, > >+ ("ISM: IMEStateManager::OnChangeFocusInternal(), notifying previous " > >+ "focused child process of parent process gets focus")); > This comment isn't quite right, since sActiveTabParent and newTabParent can > be coming from the same child process. Why? When sActiveTabParent (i.e., previously active TabParent) and new active TabParent are different or sActiveTabParent is not null but newTabParent is nullptr, I'm understanding as that different child process or its process (i.e., parent process) is getting focus. I misunderstood the TabParent instance creation? Isn't a instance is created per child process? > In fact, do we need to call SendStopIMEStateManagement if sActiveTabParent > is dealing with the same process as newTabParent No, in such case.

As far as I understand and tested, IMEStateManager should set input context "disabled" when focus is actually being changed and new focused content has TabParent. Otherwise, i.e., if the focus is not being changed, the window is becoming active. In such case, it shouldn't set input context because it causes committing composition in the child process.

merged with the new part.1.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #48) > Why? When sActiveTabParent (i.e., previously active TabParent) and new > active TabParent are different or sActiveTabParent is not null but > newTabParent is nullptr, I'm understanding as that different child process > or its process (i.e., parent process) is getting focus. I misunderstood the > TabParent instance creation? Isn't a instance is created per child process? By default e10s has just one child process, and that process has several TabParents/TabChilds, and even when we have multiple child processes, those processes may have several TabParents/TabChilds. ContentParent/Child represent a process, TabParent/TabChild represent a top level browsing context in such process, and there can be multiple top level browsing contexts in a child process.

Thank you for your explanation. So, can I check if two TabParents are for same child process with TabParent::Manager()? http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.h#381 I.e., |sActiveTabParent->Manager() != newTabParent->Manager()| is the right way?

That should work yes.

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/063841343f86 https://hg.mozilla.org/integration/mozilla-inbound/rev/455ce9971513 https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a4bb897d33 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cd83464b5d https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d3bb17eded

https://hg.mozilla.org/mozilla-central/rev/063841343f86 https://hg.mozilla.org/mozilla-central/rev/455ce9971513 https://hg.mozilla.org/mozilla-central/rev/b5a4bb897d33 https://hg.mozilla.org/mozilla-central/rev/f5cd83464b5d https://hg.mozilla.org/mozilla-central/rev/a0d3bb17eded