Closed Bug 580388 Opened 10 years ago Closed 9 years ago

Android IME text update code doesn't work with awesomebar

Categories

(Core :: Widget: Android, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: inputmethod)

Attachments

(1 file, 5 obsolete files)

For some reason the IME text update code doesn't like the awesomebar on Android.

Normally when a textbox changes, widget gets notifications through nsIWidget::OnIME****. For the awesomebar on Android, we get focus change notifications but nothing more.

We get correct behavior if the awesomebar is focused *before* any page is loaded, including the startup page. We also get correct behavior if the autocomplete popup is completely disabled.

This bug seems to affect Android only. Fennec on GTK doesn't have this bug.
tracking-fennec: --- → 2.0a1+
tracking-fennec: 2.0a1+ → 2.0+
I presume blocking2.0 isn't necessary considering this blocks fennec 2.0.
blocking2.0: ? → ---
Apparently this isn't just an Android bug (but since only Android Fennec uses text updates, this only shows up on Android)

What I found out so far of what happens:
1) Awesomebar textbox gets focus
2) IME code in content hooks up observers to text node inside textbox
3) After a timer fires, the autocomplete controller starts doing its magic
4) However, autocomplete magic causes layout reflow/frame reconstruction
5) During frame reconstruction, old text node in textbox gets destroyed and new text node is created
6) Since the observers are still on the old text node, we don't get any updates from this point on

So work needs to be done on the content IME code.
Attached patch Working patch v1 (obsolete) — Splinter Review
Patch that fixes the bug. Confirmed on Android and desktop.

Masayuki-san: Could you look over the patch and see if it's a good way to fix the bug? Thanks.
Assignee: nobody → jchen
Status: NEW → ASSIGNED
Attachment #460758 - Flags: feedback?(masayuki)
Excuse me, I don't understand well what you to want to do. nsTextStateManager should observe anonymous <div> in <input> element. So, nsTextStateManager should notify the changes even if the text nodes are changed.

I guess that the editor is recreated by the magin (I'm not sure what's doing) during the editor owner (<input>/<textarea>) has focus. So, if this is right, I think you should call nsIMEStateManager::OnTextStateBlur() from nsTextEditorState::UnbindFromFrame() only when the editor owner content has focus.

And I think you should call nsIMEStateManager::OnTextStateFocus() from editor. But maybe, nsEditor::Init() is bad timing. I guess that nsEditor::PostCreate() is better. And you can use nsEditor::HasFocus() for checking whether the editor has focus or not before you call OnTextStateFocus().
> I guess that the editor is recreated by the magin
the magic, I meant.
(In reply to comment #4)

Sorry I should have said root element, not text node.

I'm not sure either why autocomplete code is causing frame reconstruction, but during UnbindFromFrame the editor and anonymous <div> is destroyed, and during BindToFrame, a new editor and <div> is created. After that, nsTextStateManager loses track of the correct <div>.

Maybe I can call nsIMEStateManager::UpdateTextState from nsEditor::PostCreate?

If we call OnTextStateBlur inside UnbindFromFrame, we have to run that in a script runner (AddScriptRunner). Also, calling OnTextStateBlur and OnTextStateFocus would generate extra nsIWidget::OnIMEFocus calls, which we might not want.
> Maybe I can call nsIMEStateManager::UpdateTextState from nsEditor::PostCreate?

Yes, but I don't like the new method. Especially only for a platform and the cause is for "magic".

> If we call OnTextStateBlur inside UnbindFromFrame, we have to run that in a
script runner (AddScriptRunner).

I don't understand, what is a problem?

> Also, calling OnTextStateBlur and OnTextStateFocus would generate extra nsIWidget::OnIMEFocus calls, which we might not want.

I don't think so, the causes must fire the OnIMEFocusChange() because that means the focused editor instance changed.
> the causes must fire the OnIMEFocusChange()
the _cases_ must fire the OnIMEFocusChange(), sorry for the spam.
Attached patch Working patch v2 (obsolete) — Splinter Review
Masayuki-san, here's the new patch following your suggestions. It calls both OnTextStateBlur and OnTextStateFocus in PostCreate; there is less code this way. I have to add the section inside nsTextStateManager::Init because when the editor is created, the selection does not have a range yet.
Attachment #461994 - Flags: review?(masayuki)
Comment on attachment 461994 [details] [diff] [review]
Working patch v2

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -327,16 +327,25 @@ nsTextStateManager::Init(nsIWidget* aWid
>   }
>   NS_ENSURE_TRUE(selCon, NS_ERROR_FAILURE);
> 
>   nsCOMPtr<nsISelection> sel;
>   nsresult rv = selCon->GetSelection(nsISelectionController::SELECTION_NORMAL,
>                                      getter_AddRefs(sel));
>   NS_ENSURE_TRUE(sel, NS_ERROR_UNEXPECTED);
> 
>+  PRInt32 rangeCount = 0;
>+  rv = sel->GetRangeCount(&rangeCount);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (!rangeCount) {
>+    // force the seleciton to have a range
>+    selCon->SelectAll();
>+  }
>+

Why do you need to change the actual selection? I guess that if there is no selection, you can get the selection root by nsINode::GetSelectionRootContent().

FYI: GetRootEditableNode() in nsIMEStateManager.cpp is wrong. I should fix it ASAP...

>diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp
>--- a/editor/libeditor/base/nsEditor.cpp
>+++ b/editor/libeditor/base/nsEditor.cpp
>@@ -300,16 +300,28 @@ nsEditor::PostCreate()
>   // nuke the modification count, so the doc appears unmodified
>   // do this before we notify listeners
>   ResetModificationCount();
>   
>   // update the UI with our state
>   NotifyDocumentListeners(eDocumentCreated);
>   NotifyDocumentListeners(eDocumentStateChanged);
>   
>+  // update nsTextStateManager if we have focus right now
>+  if (HasFocus()) {
>+      nsCOMPtr<nsIPresShell> presShell;
>+      nsCOMPtr<nsIContent> rootContent = do_QueryInterface(GetRoot());
>+
>+      if (NS_SUCCEEDED(GetPresShell(getter_AddRefs(presShell))) &&
>+          rootContent) {
>+        nsPresContext* context = presShell->GetPresContext();
>+        nsIMEStateManager::OnTextStateBlur(context, nsnull);
>+        nsIMEStateManager::OnTextStateFocus(context, rootContent);
>+      }
>+  }
>   return NS_OK;
> }

First, use 2 spaces for indentation.

And GetRoot() doesn't return right content which you want. I think that we should pass the content is current focused content, so,

nsIFocusManager* fm = nsFocusManager::GetFocusManager();
NS_ASSERTION(fm, "fm must not be null here");
nsCOMPtr<nsIDOMElement> focusedElement;
fm->GetFocusedElement(getter_AddRefs(focusedElement));
NS_ASSERTION(focusedElement, "focusedElement is null even though we're focused");
nsCOMPtr<nsIContent> focusedContent = do_QueryInterface(focusedElement);
NS_ASSERTION(focusedContent, "focusedElement doesn't have nsIContent interface");
nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
NS_ASSERTION(ps, "Why there is no presShell even though we're focused");
nsPresContext* pc = ps->GetPresContext();

The block is in HasFocus(), so, the related objects must be valid, so, you need to check by assertion rather than run time check.
Depends on: 544779
tracking-fennec: 2.0+ → 2.0b1+
Attached patch Patch v3 (obsolete) — Splinter Review
Addressed review comments from the last patch.

The HTML editor does not have this bug, and the old patch causes a regression on an HTML editor crashtest, so I added the IsPlaintextEditor() check.
Attachment #460758 - Attachment is obsolete: true
Attachment #461994 - Attachment is obsolete: true
Attachment #469529 - Flags: review?(masayuki)
Attachment #460758 - Flags: feedback?(masayuki)
Attachment #461994 - Flags: review?(masayuki)
(In reply to comment #11)
> The HTML editor does not have this bug, and the old patch causes a regression
> on an HTML editor crashtest, so I added the IsPlaintextEditor() check.

Hmm, we should find the regression cause. I think that this patch also has the bug because IsPalintextEditor() returns TRUE even if the editor instance is an nsHTMLEditor (e.g., plaintext mail editor).
(In reply to comment #12)
> (In reply to comment #11)
> > The HTML editor does not have this bug, and the old patch causes a regression
> > on an HTML editor crashtest, so I added the IsPlaintextEditor() check.
> 
> Hmm, we should find the regression cause. I think that this patch also has the
> bug because IsPalintextEditor() returns TRUE even if the editor instance is an
> nsHTMLEditor (e.g., plaintext mail editor).

OK, the regression is from the focusedContent assertion, because nsHTMLEditor::HasFocus can return true even if there is no focused content. I think we can call nsEditor::HasFocus explicitly instead.
Jim, does this bug affect cedar/puppet widgets?  Do we need this resolved before merging cedar-->m-c?
Attached patch Patch v4 (obsolete) — Splinter Review
Can you review this patch, Masayuki-san?

The regression occurred because nsHTMLEditor::HasFocus can return true even if there is no focused content. I changed the code to check nsEditor::HasFocus specifically.
Attachment #469529 - Attachment is obsolete: true
Attachment #477492 - Flags: review?(masayuki)
Attachment #469529 - Flags: review?(masayuki)
Um, I think that your patch works, but it's wrong...

I think you should call HasFocus(), but you should check whether the focusedContent is null or not rather than NS_ASSERTION.
Attached patch Patch v5 (obsolete) — Splinter Review
(In reply to comment #16)
> Um, I think that your patch works, but it's wrong...
> 
> I think you should call HasFocus(), but you should check whether the
> focusedContent is null or not rather than NS_ASSERTION.

Ok fixed.
Attachment #477492 - Attachment is obsolete: true
Attachment #477812 - Flags: review?(masayuki)
Attachment #477492 - Flags: review?(masayuki)
Attached patch Patch v5.1Splinter Review
(In reply to comment #17)
> Created attachment 477812 [details] [diff] [review]
> Patch v5
> 
> (In reply to comment #16)
> > Um, I think that your patch works, but it's wrong...
> > 
> > I think you should call HasFocus(), but you should check whether the
> > focusedContent is null or not rather than NS_ASSERTION.
> 
> Ok fixed.

Oops should be NS_ASSERTION(fm, ...

Sorry for the spam.
Attachment #477812 - Attachment is obsolete: true
Attachment #477814 - Flags: review?(masayuki)
Attachment #477812 - Flags: review?(masayuki)
Comment on attachment 477814 [details] [diff] [review]
Patch v5.1

Thanks!
Attachment #477814 - Flags: review?(masayuki) → review+
Blocks: 599053
Blocks: 599072
Blocks: 599077
pushed http://hg.mozilla.org/mozilla-central/rev/1ba6bee99c20
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Jim:

Have you found what the awesomebar did? I'm thinking about removing nsIMEStateManager::OnTextStateBlur() from public scope since it's always called with nsIMEStateManager::OnChangeFocus() in nsFocusManager. However, the patch you made in this bug only calls OnTextStateBlur(). So, for this, probably I need additional work for the case. For that, I want to make automated test for this bug. If you have found what was the magic, it definitely helps me.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> Jim:
> 
> Have you found what the awesomebar did? I'm thinking about removing
> nsIMEStateManager::OnTextStateBlur() from public scope since it's always
> called with nsIMEStateManager::OnChangeFocus() in nsFocusManager. However,
> the patch you made in this bug only calls OnTextStateBlur(). So, for this,
> probably I need additional work for the case. For that, I want to make
> automated test for this bug. If you have found what was the magic, it
> definitely helps me.

I did not find the cause, unfortunately. I only know it is related to autocomplete.

But I think you can replace OnTextStateBlur with OnChangeFocus, as long as you call OnTextStateBlur inside OnChangeFocus. In this case, the existing code in OnChangeFocus should not have any side effect.
You need to log in before you can comment on or make changes to this bug.