Closed Bug 1167022 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(5 files, 3 obsolete files)

part.3 Fix test_imestate.html for conforming to the new IME focus notification behavior
5.35 KB, patch
jchen
: review+
Details | Diff | Splinter Review
part.1 Make IMEContentObserver possible to restart to observe editor root node (r=smaug)
7.70 KB, patch
Details | Diff | Splinter Review
part.2 IMEStateManager::UpdateIMEState() should try to restart to observe focused editor when it's reframed (r=smaug)
9.35 KB, patch
jchen
: review+
Details | Diff | Splinter Review
part.4 Get rid of MozIMEFocus(In|Out) events (r=smaug)
17.85 KB, patch
Details | Diff | Splinter Review
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
6.31 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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.
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).
Attachment #8608530 - Flags: review?(bugs)
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.
Attachment #8608531 - Flags: review?(nchen)
Attachment #8608531 - Flags: review?(bugs)
Fix runEditorReframeTests() without MozIMEFocus(In|Out) events because nsITextInputProcessor allows JS to receive notifications to IME.
Attachment #8608532 - Flags: review?(nchen)
Since JS can receive notifications to IME directly, we can get rid of hacky events for this test now.
Attachment #8608533 - Flags: review?(bugs)
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
Attachment #8608530 - Flags: review?(bugs) → review+
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.
Attachment #8608531 - Flags: review?(bugs) → review+
Comment on attachment 8608533 [details] [diff] [review]
part.4 Get rid of MozIMEFocus(In|Out) events

nice
Attachment #8608533 - Flags: review?(bugs) → review+
(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 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')
Attachment #8609238 - Flags: review?(nchen) → review+
Attachment #8608532 - Flags: review?(nchen) → review+
(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!
(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...
Okay, probably the assertion detects my bug. But I'm still not sure the timeouts.
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.
Attachment #8610078 - Flags: review?(bugs)
Attachment #8610078 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: