670776 - Caret/Cursor jumps around while entering text in etherpad's rich text editor

Status: RESOLVED WORKSFORME

(Core :: DOM: Editor, defect)

Description

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

STR:
1) load an etherpad (go here http://etherpad.mozilla.com:9000/ and click "create new pad")
2) click in the rich text editor
3) hold the menu key to bring up the softkeyboard
4) type

ER: entered text goes where the caret is
AR: text goes where ever it feels like

The caret seems to have a preference for being at the top of the editor, so placing it at the bottom is the most reliable way to reproduce

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

If the fix is safe, we might consider for Fx7

Comment 2

[(no longer active)]

Is this something to be fixed in the core editor?  Do you have a regression range?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

For some reason we don't get IME selection change events when the caret is positioned.  I'll take this one.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

Comment 5

[Alex Pakhotin (:alexp)]

Comment on attachment 555731 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

I believe someone who really knows this code should review this change to make sure we are doing the right thing. I guess enn or smaug.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

Comment 7

[Neil Deakin]

Can you explain why the first change is needed?

Masayuki Nakano is the better reviewer for the IME change.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

The first change is necessary because the focus is first cleared, which sets the mFocusedContent to null (in Blur).  This causes us to short circuit early in Focus when we have the doc focused.

The second change is necessary because that's just where the editable doc is picked up for focus.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

The patch on Bug 670694 obsoletes the first part of this patch, so only the additional IME call is needed.

Comment 10

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

I don't understand why it's necessary... At removing focus, calling nsIMEStateManager::OnTextStateBlur(presContext, nsnull); should be enough for nsIMEStateManager. Why does CheckIfFocusable() return NULL when an editor has focus?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from comment #10)
> I don't understand why it's necessary... At removing focus, calling
> nsIMEStateManager::OnTextStateBlur(presContext, nsnull); should be enough
> for nsIMEStateManager.

We could make it work, perhaps, but it's necessary with the current logic.  In OnTextStateBlur, we don't try to find an editable doc to focus.  We do that in OnTextStateFocus (really, GetRootEditableNode) when a null element is passed.

> Why does CheckIfFocusable() return NULL when an
> editor has focus?

Because CheckIfFocusable requires a non-null element.  When a document has focus, the element is null.  At one point I tried making that return the document if it was editable, but ran in to some other issue (that I don't remember now).

Comment 12

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

Hmm, I think that the approach is wrong.

I guess that when aContent is NULL but the document is editable, nsIMEStateManager should think that the root element (or the body element?) gets focus. I mean, I think you should make another else if block before the block which you changed.

I.e., you should make second block of following code.

if (CheckIfFocusable() && ...) {

} else if (doc->editable) {

} else {

}

And also we might be able to remove the special code for this case in nsIMEStateManager. E.g., http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsIMEStateManager.cpp#259

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Masayuki Nakano (Mozilla Japan)(offline: 8/31 and 9/1) from comment #12)

Updated patch which takes the recommended approach.  Mostly it's the same code, though, since the special case in nsIMEEventStateManager can't easily be removed (see comment in patch).

Comment 15

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

Comment on attachment 557229 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

Sorry for the delay.

> +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> +             presContext->Document()->IsEditable()) {

I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should be set to a PRBool variable before |if|.  It's checked in all blocks.

> +    SetFocusedContent(nsnull, aFlags);

I'm not sure whether this is necessary.

> +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> +
> +    if (!aWindowRaised)
> +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));

In the if block, aWindow->UpdateCommands() is called before nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't matter or not so.

Therefore, I need Enn's review.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 670776 - fix focus issue with contentEditable documents in iframes and IME (v2)

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Masayuki Nakano (Mozilla Japan) from comment #15)
> Comment on attachment 557229 [details] [diff] [review]
> Bug 670776 - fix focus issue with contentEditable documents in iframes and
> IME
> 
> Sorry for the delay.
> 
> > +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> > +             presContext->Document()->IsEditable()) {
> 
> I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should
> be set to a PRBool variable before |if|.  It's checked in all blocks.
> 
> > +    SetFocusedContent(nsnull, aFlags);
> 
> I'm not sure whether this is necessary.
> 
> > +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> > +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> > +
> > +    if (!aWindowRaised)
> > +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
> 
> In the if block, aWindow->UpdateCommands() is called before
> nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't
> matter or not so.
> 
> Therefore, I need Enn's review.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Masayuki Nakano (Mozilla Japan) from comment #15)
> Comment on attachment 557229 [details] [diff] [review]
> Bug 670776 - fix focus issue with contentEditable documents in iframes and
> IME
> 
> Sorry for the delay.
> 
> > +  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
> > +             presContext->Document()->IsEditable()) {
> 
> I think that |mFocusedWindow == aWindow && mFocusedContent == nsnull| should
> be set to a PRBool variable before |if|.  It's checked in all blocks.

Done.

> 
> > +    SetFocusedContent(nsnull, aFlags);
> 
> I'm not sure whether this is necessary.

That's actually from 670694, which this bug really should depend on (fixed).  We have to set the flags there so that they are correct when we compare them next time (see 670694).

> 
> > +    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
> > +    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
> > +
> > +    if (!aWindowRaised)
> > +      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
> 
> In the if block, aWindow->UpdateCommands() is called before
> nsIMEStateManager::OnTextStateFocus(). I'm not sure whether the order isn't
> matter or not so.
> 
> Therefore, I need Enn's review.

Comment 19

[Neil Deakin]

Comment on attachment 557229 [details] [diff] [review]
Bug 670776 - fix focus issue with contentEditable documents in iframes and IME

>-  }
>-  else {
>+  } else if (mFocusedWindow == aWindow && mFocusedContent == nsnull &&
>+             presContext->Document()->IsEditable()) {
>+    SetFocusedContent(nsnull, aFlags);
>+
>+    // Editable document has focus, let IME know about it.  If we pass
>+    // null content (as we do below), IME knows to inspect the document
>+    // to see if it is editable and Do The Right Thing.  We can't pass
>+    // the document directly, because IME expects a nsIContent*.
>+    nsIMEStateManager::OnTextStateBlur(presContext, nsnull);
>+
>+    PRUint32 reason = GetFocusMoveReason(aFlags);
>+    nsIMEStateManager::OnChangeFocus(presContext, nsnull, reason);
>+    nsIMEStateManager::OnTextStateFocus(presContext, nsnull);
>+
>+    if (!aWindowRaised)
>+      aWindow->UpdateCommands(NS_LITERAL_STRING("focus"));
>+  } else {

You should be able to combine some of these lines with the following else block. The call to UpdateCommands for example could be written only once.


>     // If the window focus event (fired above when aIsNewDocument) caused
>     // the plugin not to be focusable, update the system focus by focusing
>     // the root widget.
>     if (aAdjustWidgets && objectFrameWidget &&
>         mFocusedWindow == aWindow && mFocusedContent == nsnull) {
>       nsIViewManager* vm = presShell->GetViewManager();
>       if (vm) {
>         nsCOMPtr<nsIWidget> widget;
>         vm->GetRootWidget(getter_AddRefs(widget));
>         if (widget)
>           widget->SetFocus(PR_FALSE);
>       }
>     }

I would think that this plugin related code should be called in both cases.

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

Some combination of IME changes have made this work now.