580388 - Android IME text update code doesn't work with awesomebar

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

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

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.

Comment 1

[Joe Drew (not getting mail)]

I presume blocking2.0 isn't necessary considering this blocks fennec 2.0.

Comment 2

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

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.

Comment 3

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

Attachment: Working patch v1

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.

Comment 4

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

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().

Comment 5

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

> I guess that the editor is recreated by the magin
the magic, I meant.

Comment 6

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

(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.

Comment 7

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

> 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.

Comment 8

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

> the causes must fire the OnIMEFocusChange()
the _cases_ must fire the OnIMEFocusChange(), sorry for the spam.

Comment 9

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

Attachment: Working patch v2

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.

Comment 10

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

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.

Comment 11

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

Attachment: Patch v3

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.

Comment 12

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

(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).

Comment 13

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

(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.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Jim, does this bug affect cedar/puppet widgets?  Do we need this resolved before merging cedar-->m-c?

Comment 15

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

Attachment: Patch v4

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.

Comment 16

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

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.

Comment 17

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

Attachment: 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.

Comment 18

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

Attachment: Patch v5.1

(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.

Comment 19

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

Comment on attachment 477814 [details] [diff] [review]
Patch v5.1

Thanks!

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/1ba6bee99c20

Comment 21

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

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.

Comment 22

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

(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.