CreateTextStateManager not called when editor is reframed during focus

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: jchen, Assigned: masayuki)

Tracking

({inputmethod, regression})

unspecified
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

401 bytes, text/html
Details
3.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.47 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.73 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
17.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
Posted patch Patch (obsolete) — Splinter Review
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?
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #676868 - Flags: feedback?(nchen)
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.
Attachment #676868 - Flags: feedback?(nchen) → feedback+
(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!
(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.
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.
Posted file 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.
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().
Attachment #678628 - Attachment is obsolete: true
Attachment #678629 - Attachment is obsolete: true
Attachment #678630 - Attachment is obsolete: true
Attachment #679465 - Flags: review?(bugs)
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.
Attachment #679470 - Flags: review?(bugs)
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.
Attachment #679478 - Flags: review?(bugs)
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.
Attachment #679479 - Flags: review?(bugs)
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.
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.
(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 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);
Attachment #679465 - Flags: review?(bugs) → review+
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).
Attachment #679470 - Flags: review?(bugs) → review+
Attachment #679478 - Flags: review?(bugs) → review+
Attachment #679479 - Flags: review?(bugs) → review+
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.
Attachment #679553 - Flags: review?(bugs) → review-
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.
In fact using (new nsAsyncDOMEvent(...)).RunDOMEventWhenSafe() would be safer.
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.
Attachment #679481 - Flags: review?(ehsan)
Attachment #679481 - Flags: review?(ehsan) → review+
Attachment #679991 - Flags: review?(bugs) → review+
Depends on: 850559
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.