1167022 - Shouldn't notify IME of focus/blur when a focused editor is reframed

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

If we notify IME of focus blur, IME may commit existing composition. This is unexpected behavior when focused editor is reframed.

We shouldn't notify IME of focus/blur during reframing.

Comment 1

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

Attachment: part.1 Make IMEContentObserver possible to restart editor root node

First, IMEContentObserver should be able to restart to observe focused editor even after reframed (i.e., root content which is anonymous div element is replaced with the other).

Comment 2

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

Attachment: part.2 IMEStateManager::UpdateIMEState() should try to restart to observe focused editor if it's reframed

When IMEStateManager::UpdateIMEState() is called, it should try to restart observing the content with existing IMEContentObserver. I.e., this shouldn't cause notifying IME of focus/blur at reframing the editor.

Comment 3

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

Attachment: part.3 Fix test_imestate.html for conforming to the new IME focus notification behavior

Fix runEditorReframeTests() without MozIMEFocus(In|Out) events because nsITextInputProcessor allows JS to receive notifications to IME.

Comment 4

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

Attachment: part.4 Get rid of MozIMEFocus(In|Out) events

Since JS can receive notifications to IME directly, we can get rid of hacky events for this test now.

Comment 5

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

Comment on attachment 8608530 [details] [diff] [review]
part.1 Make IMEContentObserver possible to restart editor root node

>+  bool firstInitialization =
>+    !(mRootContent && !mRootContent->IsInComposedDoc());
took some time to understand this


>+  void NotifyIMEOfBlur(bool aPostEvent);
>+  /**
>+   *  UnregisterObservers() unresiters all listeners and observers.
unregisters

Comment 6

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

Comment on attachment 8608531 [details] [diff] [review]
part.2 IMEStateManager::UpdateIMEState() should try to restart to observe focused editor if it's reframed

>+void
>+IMEContentObserver::MaybeRestartToObserve(nsIWidget* aWidget,
>+                                          nsPresContext* aPresContext,
>+                                          nsIContent* aContent,
>+                                          nsIEditor* aEditor)

Could this return bool


>+{
>+  if (GetState() != eState_StoppedObserving ||
>+      !IsObservingContent(aPresContext, aContent)) {
>+    return;
>+  }
>+
>+  Init(aWidget, aPresContext, aContent, aEditor);
so here add
return IsManaging(aPresContext, aContent);



>+  if (sActiveIMEContentObserver && IsIMEObserverNeeded(aNewIMEState)) {
>+    PR_LOG(sISMLog, PR_LOG_DEBUG,
>+      ("ISM:   IMEStateManager::UpdateIMEState(), try to reinitialize the "
>+       "active IMEContentObserver"));
>+    sActiveIMEContentObserver->MaybeRestartToObserve(widget, sPresContext,
>+                                                     aContent, aEditor);
>+    if (!sActiveIMEContentObserver->IsManaging(sPresContext, aContent)) {
Then you could merge MaybeRestartToObserve and IsManaging call here.

Comment 7

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

Comment on attachment 8608533 [details] [diff] [review]
part.4 Get rid of MozIMEFocus(In|Out) events

nice

Comment 8

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

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8608530 [details] [diff] [review]
> part.1 Make IMEContentObserver possible to restart editor root node
> 
> >+  bool firstInitialization =
> >+    !(mRootContent && !mRootContent->IsInComposedDoc());
> took some time to understand this

Sorry, it's just copied from GetState() of part.2 because part.1 and part.2 were in one patch, but I separated it for review. I believe that you can understand it quickly when you reviewed part.2.

Comment 9

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

Attachment: part.1 Make IMEContentObserver possible to restart to observe editor root node (r=smaug)

Comment 10

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

Attachment: part.2 IMEStateManager::UpdateIMEState() should try to restart to observe focused editor when it's reframed (r=smaug)

Comment 11

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

Attachment: part.4 Get rid of MozIMEFocus(In|Out) events (r=smaug)

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8609238 [details] [diff] [review]
part.2 IMEStateManager::UpdateIMEState() should try to restart to observe focused editor when it's reframed (r=smaug)

Review of attachment 8609238 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with some nits

::: dom/events/IMEContentObserver.h
@@ +78,5 @@
> +   * with new node instances.
> +   * @return            Returns true if the instance is managing the content.
> +   *                    Otherwise, false.
> +   */
> +  bool MaybeRestartToObserve(nsIWidget* aWidget,

I think "MaybeRestartToObserve" is not a very clear name because we are doing more than resetting observers; we are calling Init() to re-initialize.

I think "MaybeReinit" or "EnsureIsManaging" would be more clear names.

::: dom/events/IMEStateManager.cpp
@@ +1141,5 @@
>  }
>  
>  // static
>  bool
> +IMEStateManager::IsIMEObserverNeeded(const IMEState& aState)

Maybe remove IMEStateManager::IsIMEObserverNeeded, and just use IMEState::IsEditable?

@@ +1203,3 @@
>      PR_LOG(sISMLog, PR_LOG_DEBUG,
>        ("ISM:   IMEStateManager::CreateIMEContentObserver() doesn't create "
> +       "IMEContentObserver because of it's not necessary"));

"IMEContentObserver because it's not necessary" (don't need the 'of')

Comment 13

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #12)
> Comment on attachment 8609238 [details] [diff] [review]
> part.2 IMEStateManager::UpdateIMEState() should try to restart to observe
> focused editor when it's reframed (r=smaug)
> 
> Review of attachment 8609238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with some nits
> 
> ::: dom/events/IMEContentObserver.h
> @@ +78,5 @@
> > +   * with new node instances.
> > +   * @return            Returns true if the instance is managing the content.
> > +   *                    Otherwise, false.
> > +   */
> > +  bool MaybeRestartToObserve(nsIWidget* aWidget,
> 
> I think "MaybeRestartToObserve" is not a very clear name because we are
> doing more than resetting observers; we are calling Init() to re-initialize.
> 
> I think "MaybeReinit" or "EnsureIsManaging" would be more clear names.

Okay, I'll rename it to MaybeReinitialize().

> ::: dom/events/IMEStateManager.cpp
> @@ +1141,5 @@
> >  }
> >  
> >  // static
> >  bool
> > +IMEStateManager::IsIMEObserverNeeded(const IMEState& aState)
> 
> Maybe remove IMEStateManager::IsIMEObserverNeeded, and just use
> IMEState::IsEditable?

Yeah, it's same at least for now. However, I'd like to wrap with a method named what it does. In the future, if we add some conditions, this method guarantees both CreateIMEContentObserver() and UpdateIMEState() use does same check.

> @@ +1203,3 @@
> >      PR_LOG(sISMLog, PR_LOG_DEBUG,
> >        ("ISM:   IMEStateManager::CreateIMEContentObserver() doesn't create "
> > +       "IMEContentObserver because of it's not necessary"));
> 
> "IMEContentObserver because it's not necessary" (don't need the 'of')

Oops!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00b1c04181d3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc7f8b88a46
https://hg.mozilla.org/integration/mozilla-inbound/rev/b97f76c4be1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/de4013b979a7

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fb82a47beb96 for reftest-e10s bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=10113856&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=10114581&repo=mozilla-inbound

Comment 16

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

(In reply to Phil Ringnalda (:philor) from comment #15)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fb82a47beb96 for
> reftest-e10s bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=10113856&repo=mozilla-
> inbound and
> https://treeherder.mozilla.org/logviewer.html#?job_id=10114581&repo=mozilla-
> inbound

Hmm, that's wired, is it really caused by the patches? The patches don't touch to layout...

Comment 17

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

Okay, probably the assertion detects my bug. But I'm still not sure the timeouts.

Comment 18

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

Attachment: part.5 IMEContentObserver::Init() should stop keeping to initialize in the first call because Init() may be called again during notifying IME of focus due to flushing layout

PuppetWidget queries content at IME-focus notification. Then, ContentEventHandler flushes the layout. Then, nsEditor may actually created *while* IMEContentObserver::Init() sends IME-focus notification. Therefore, IMEStateManager::UpdateIMEState() tries to reinitialize it with the latest contents.

So, IME-focus notification shouldn't be sent after registering as observers.

This patch restores the order of sending notification and registering as observers. And also, if the IMEContentObserver already starts to observe the latest contents after sending IME-focus notification, it doesn't try to register itself as observer again.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a4025d966e
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a6bef031e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d5aa9774c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4bb1e305234
https://hg.mozilla.org/integration/mozilla-inbound/rev/8adf9f289057

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4a4025d966e
https://hg.mozilla.org/mozilla-central/rev/59a6bef031e2
https://hg.mozilla.org/mozilla-central/rev/34d5aa9774c2
https://hg.mozilla.org/mozilla-central/rev/c4bb1e305234
https://hg.mozilla.org/mozilla-central/rev/8adf9f289057