806996 - CreateTextStateManager not called when editor is reframed during focus

Status: RESOLVED FIXED

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

Description

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

Masayuki-san: nsEditor::PostCreate() can call nsIMEStateManager::OnChangeFocus(), but in this case, nsEditorEventListener::Focus() is not called if the editor already has focus. As a result, CreateTextStateManager() is not called.

See the comment at http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#325

> If the text control gets reframed during focus, Focus() would not be called...

I think this is related to Bug 580388. I see a regression on Android when using Google Instant.

Comment 1

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

Attachment: Patch

Ah, sorry for the mistake.

Test build are going to be available here:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=94a31688ae08

Could you check the patch?

Comment 2

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

Comment on attachment 676868 [details] [diff] [review]
Patch

># HG changeset patch
># Parent 664e0b8340946e6d71ef05244c230819eb4fcf1c
>Bug 806996 nsTextStateManager isn't created when editor is created and it already has focus
>
>     nsEditorEventListener* listener =
>       reinterpret_cast<nsEditorEventListener*> (mEventListener.get());
>     listener->SpellCheckIfNeeded();
>+
>+    nsIDocument* currentDoc = focusedContent->GetCurrentDoc();
>+    NS_ENSURE_TRUE(currentDoc, NS_OK);
>+    nsIMEStateManager::OnFocusInEditor(pc,
>+      currentDoc->HasFlag(NODE_IS_EDITABLE) ? nullptr : focusedContent);


Hmm this doesn't seem to work. I think earlier when we call nsIMEStateManager.OnChangeFocus:

> 317     nsIMEStateManager::OnChangeFocus(pc, focusedContent,
> 318                                      InputContextAction::CAUSE_UNKNOWN);

focusedContent is the same as the old focused content (input element), so when we call OnChangeFocus, the old text state manager is not destroyed, and later when we call OnFocusInEditor, a new text state manager is not created. I think it will work if we do,

> nsIMEStateManager::OnChangeFocus(pc, nullptr, InputContextAction::CAUSE_UNKNOWN)

But I'm not sure if it is the correct way.

Comment 3

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

(In reply to Jim Chen [:jchen :nchen] from comment #2)
> > 317     nsIMEStateManager::OnChangeFocus(pc, focusedContent,
> > 318                                      InputContextAction::CAUSE_UNKNOWN);
> 
> focusedContent is the same as the old focused content (input element), so
> when we call OnChangeFocus, the old text state manager is not destroyed, and
> later when we call OnFocusInEditor, a new text state manager is not created.
> I think it will work if we do,

Ah, probably, you're right. I'll update the patch. Thanks!

Comment 4

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

(In reply to Jim Chen [:jchen :nchen] from comment #0)
> I think this is related to Bug 580388. I see a regression on Android when
> using Google Instant.

Could you explain the STR and the actual/expected behavior? I'm rewriting something related for this. I want to test my patches.

Comment 5

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

Attachment: part.1 Remove nsTextStateManager::mDestroying for simpler code

Comment 6

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

Attachment: part.2 nsTextStateManager::IsManaging() should check if mRootContent is still in the mEditableNode

Comment 7

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

Attachment: part.3 Use nsIMEStateManager::UpdateIMEState() and it should recreate nsTextStateManager instance if it isn't managing the editor actually

Comment 8

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

The sPresContext and sContent must be the editor's focused content when nsEditor::PostCreate() wants to adjust the IME state.

Then, nsIMEStateManager::UpdateIMEState() is the best method.

But nsIMEStateManager doesn't check whether the nsTextStateManager instance is actually managing the editor's root editable node, neither does nsTextStateManager::IsManaging().

So, I think that IsManaging() should check if the observing editable root is still be so in the focused content.

Comment 9

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

Attachment: Test case

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> (In reply to Jim Chen [:jchen :nchen] from comment #0)
> > I think this is related to Bug 580388. I see a regression on Android when
> > using Google Instant.
> 
> Could you explain the STR and the actual/expected behavior? I'm rewriting
> something related for this. I want to test my patches.

I attached a testcase.

If you test on Android using the latest nightly, the expected behavior is no repeating of entered characters, but the actual behavior is repeating of all previously entered characters.

If you test on other platforms, you can log nsIWidget::OnIMETextChange calls. The expected behavior is getting OnIMETextChange calls even after layout reframe. The actual behavior is OnIMETextChange is not called after layout reframe.

Comment 10

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

Attachment: part.1 Remove nsTextStateManager::mDestroying

Let's sort out the nsTextStateManager and IME focus in/out management for testing them.

First, nsTextStateManager is only referred via nsIMEStateManager::sTextStateObserver. So, mDestroying flag doesn't make sense. When nsIMEStateManager wants to destroy the instance, just set nullptr to the sTextStateObserver before calling Destroy().

Comment 11

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

Attachment: part.2 nsTextStateManager::IsManaging() should check if mRootContent is still in the mEditableNode

nsTextStateManager::mRootContent is anonymous <div> of <input> or <textarea>, root element of designMode editor or focused editing host of content editable editor.

nsTextStateManager should check if mRootContent in the document at IsManagin() because the anonymous <div> may be swapped by reframing. This is the root cause of this bug.

Comment 12

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

Attachment: part.3 Make sure to call nsIWidget::OnIMEFocusChange(false) at destroying nsTextStateManager instance only once per calling nsIWidget::OnIMEFocusChange(true)

This is the biggest patch for this bug but this makes IME focus management simple.

If nsTextStateManager is always created while IME has focus (i.e., DOM focus is in editable element), nsIWidget::OnIMEFocusChange(false) is called just once from nsTextStateManager::Destroy(). Then, widget will receive OnIMEFocusChange(false) only once per OnIMEFcosuChange(true).

If we use this approach, we can move the most code of nsTextStateManager::Init() into the constructor. And then, new method nsTextStateManager::ObserveEditableNode() should be called only when nsTextStateManager is created on current trunk build.

So, this patch provides same behavior on all platforms except whether nsTextStateManager observes selection change and mutation events or not. This is very useful for automated tests.

Comment 13

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

Attachment: part.4 Make sure to recreate nsTextStateManager instance in OnFocusInEditor() and UpdateIMEState() if the focused editor isn't being managed

This is second part of actual fix of this bug.

nsIMEStateManager::OnFocusInEditor() and nsIMEStateManager::UpdateIMEState() don't assume that nsTextStateManager instance needs to be recreated when mRootContent is changed.

Make sure to do it if nsTextStateManager::IsManaging() returns false.

Comment 14

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

Attachment: part.5 nsEditor::PostCreate() should call nsIMEStateManager::UpdateIMEState() rather than nsIMEStateManager::OnChangeFocus() when it already has focus

nsEditor::PostCreate() should use nsIMEStateManager::UpdateIMEState() rather tan nsIMEStateManager::OnChangeFocs() when the editor already has focus. I'll request this review to ehsan after smaug agrees the nsIMEStateManager's change.

Comment 15

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

Attachment: part.6 Test if nsIWidget::OnIMEFocusChange() is called by nsTextStateManager properly

This is the automated tests for this bug. However, this still depends on the patch for bug 805766 which has been backed out. I'll update this soon.

Comment 16

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

Attachment: part.6 Test if nsIWidget::OnIMEFocusChange() is called by nsTextStateManager properly

This patch uses custom events if it's during IME test.

Comment 17

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

(In reply to Jim Chen [:jchen :nchen] from comment #9)
> Created attachment 678748 [details]
> Test case
> 
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #4)
> > (In reply to Jim Chen [:jchen :nchen] from comment #0)
> > > I think this is related to Bug 580388. I see a regression on Android when
> > > using Google Instant.
> > 
> > Could you explain the STR and the actual/expected behavior? I'm rewriting
> > something related for this. I want to test my patches.
> 
> I attached a testcase.

Thank you. That helps me very much. And I confirmed that the behavior in the testcase is same as Fx 16 on Android. And I added the test in the part.6.

Comment 18

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

Comment on attachment 679465 [details] [diff] [review]
part.1 Remove nsTextStateManager::mDestroying


>-  sTextStateObserver->mDestroying = true;
>-  sTextStateObserver->Destroy();
>+  nsRefPtr<nsTextStateManager> tsm = sTextStateObserver;
>   NS_RELEASE(sTextStateObserver);

Perhaps 
nsRefPtr<nsTextStateManager> tsm;
tsm.swap(&sTextStateObserver);

Comment 19

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

Comment on attachment 679470 [details] [diff] [review]
part.2 nsTextStateManager::IsManaging() should check if mRootContent is still in the mEditableNode

Uh. mRootContent should always be in document.
nsTextStateManager Looks leak-prone.
It has plenty of strong references, yet it is not cycle collected.
But we can fix that in a followup.
Could you please file a bug to make nsTextStateManager cycle collectable
(or otherwise not so leak-prone).

Comment 20

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

Comment on attachment 679553 [details] [diff] [review]
part.6 Test if nsIWidget::OnIMEFocusChange() is called by nsTextStateManager properly

Could we cache test.IME value in some static bool variable?
This is not very hot code, but making still pref calls feels a bit
bad.

Comment 21

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

Also, make sure dispatching those test events don't cause assertions.
They are fired at unusual time, so I wonder if there is a script blocker on stack.

Comment 22

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

In fact using (new nsAsyncDOMEvent(...)).RunDOMEventWhenSafe() would be safer.

Comment 23

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

Comment on attachment 679481 [details] [diff] [review]
part.5 nsEditor::PostCreate() should call nsIMEStateManager::UpdateIMEState() rather than nsIMEStateManager::OnChangeFocus() when it already has focus

The first change is the actual fix of this bug. The focus is NOT changing at that time. So, only updating IME state does make sense.

However, there are some duplicated code for computing the content parameter of nsIMEStateManager methods. This is bad. So, I'd like to add a new method for that and make them shared by the callers of nsIMEStateManager methods.

Comment 24

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

Attachment: part.6 Test if nsIWidget::OnIMEFocusChange() is called by nsTextStateManager properly

Comment 25

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f66e17df8cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/db12b505016f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89844b9bd7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e3a6eeb3c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/73a92a915ce3
https://hg.mozilla.org/integration/mozilla-inbound/rev/927017027942

Thank you, Smaug and Ehsan.

Comment 26

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/9f66e17df8cf
https://hg.mozilla.org/mozilla-central/rev/db12b505016f
https://hg.mozilla.org/mozilla-central/rev/e89844b9bd7c
https://hg.mozilla.org/mozilla-central/rev/c6e3a6eeb3c1
https://hg.mozilla.org/mozilla-central/rev/73a92a915ce3
https://hg.mozilla.org/mozilla-central/rev/927017027942